From ba5ad34249db587044c71e49e43419379190de77 Mon Sep 17 00:00:00 2001 From: 0xng Date: Tue, 1 Oct 2024 13:48:54 -0300 Subject: [PATCH 1/6] fix: updating rollback specs --- specs/interop/predeploys.md | 63 ++++++++++++++++++++++++------------- 1 file changed, 42 insertions(+), 21 deletions(-) diff --git a/specs/interop/predeploys.md b/specs/interop/predeploys.md index 8d6fa0b49..b75d74807 100644 --- a/specs/interop/predeploys.md +++ b/specs/interop/predeploys.md @@ -289,9 +289,8 @@ as well as domain binding, ie the executing transaction can only be valid on a s ### `relayExpire` Invariants -- Only callable by the `CrossL2Inbox` - The message source MUST be `block.chainid` -- The `Identifier.origin` MUST be `address(L2ToL2CrossDomainMessenger)` +- The `Identifier.origin` and `sender` MUST be `address(L2ToL2CrossDomainMessenger)` - The `expiredMessages` mapping MUST only contain messages that originated in this chain and failed to be relayed on destination. - Already expired messages MUST NOT be relayed. @@ -385,6 +384,9 @@ function relayMessage(ICrossL2Inbox.Identifier calldata _id, bytes calldata _sen successfulMessages[messageHash] = true; emit RelayedMessage(_source, _nonce, messageHash); } else { + if (failedMessages[messageHash].timestamp == 0) { + failedMessages[messageHash] = FailedMessage{timestamp: block.timestamp, sourceChain: _source}; + } emit FailedRelayedMessage(_source, _nonce, messageHash); } } @@ -407,25 +409,35 @@ failed messages and to prevent malicious actors from performing a griefing attac by expiring messages upon arrival. Once the expired message is sent to the source chain, the message on the local chain is set -as successful in the `successfulMessages` mapping to ensure non-replayability and deleted -from `failedMessages`. An initiating message is then emitted to `relayExpire` +as expired in the `successfulMessages` mapping to ensure non-replayability and deleted +from `failedMessages`. An initiating message is then emitted to `relayExpire`. + +`sendExpire` sets the sender of the message as the `L2ToL2CrossDomainMessenger` to add a path +for `relayExpire` to ensure that the message originated in `sendExpire`. There's should be no +other path where a message can have the `L2ToL2CrossDomainMessenger` as `origin` and `sender`. ```solidity function sendExpire(bytes32 _expiredHash) external nonReentrant { - if (successfulMessages[_expiredHash]) revert MessageAlreadyRelayed(); + require(!successfulMessages[_expiredHash]); + require(failedMessages[_expiredHash].timestamp != 0); (uint256 messageTimestamp, uint256 messageSource) = failedMessages[_expiredHash]; - if (block.timestamp < messageTimestamp + EXPIRY_WINDOW) revert ExpiryWindowHasNotEnsued(); + require(block.timestamp >= messageTimestamp + EXPIRY_WINDOW); delete failedMessages[_expiredHash]; successfulMessages[_expiredHash] = true; - bytes memory data = abi.encodeCall( - L2ToL2CrossDomainMessenger.expired, - (_expiredHash, messageSource) - ); - emit SentMessage(data); + uint256 destination = messageSource; + address target; + uint256 nonce = messageNonce(); + address sender = Predeploys.L2_TO_L2_CROSS_DOMAIN_MESSENGER; + + bytes memory message = abi.encode(_expiredHash); + + emit SentMessage(destination, target, nonce, sender, message); + + msgNonce++; } ``` @@ -433,28 +445,37 @@ function sendExpire(bytes32 _expiredHash) external nonReentrant { When relaying an expired message, only message hashes of actual failed messages should be stored, for this we must ensure the origin -of the log, and caller are all expected contracts. +of the log, and sender is the `L2_TO_L2_CROSS_DOMAIN_MESSENGER`. It's also important to ensure only the hashes of messages that were initiated in this chain are accepted. -If all checks have been successful, the message has is stored in the +If all checks have been successful, the message hash is stored in the `expiredMessages` mapping. This enables smart contracts to read from it and check whether a message expired or not, and handle this case accordingly. +An expiry message is relayed by providing the [identifier](./messaging.md#message-identifier) +to a `SentMessage` event and its corresponding [message payload](./messaging.md#message-payload). + ```solidity -function relayExpire(bytes32 _expiredHash, uint256 _messageSource) external { - if (_messageSource != block.chainid) revert IncorrectMessageSource(); - if (expiredMessages[_expiredHash] != 0) revert ExpiredMessageAlreadyRelayed(); - if (msg.sender != Predeploys.CROSS_L2_INBOX) revert ExpiredMessageCallerNotCrossL2Inbox(); +function relayExpire(ICrossL2Inbox.Identifier calldata _id, bytes calldata _sentMessage) external { + require(_id.origin == Predeploys.L2_TO_L2_CROSS_DOMAIN_MESSENGER); - if (CrossL2Inbox(Predeploys.CROSS_L2_INBOX).origin() != Predeploys.L2_TO_L2_CROSS_DOMAIN_MESSENGER) { - revert CrossL2InboxOriginNotL2ToL2CrossDomainMessenger(); - } + CrossL2Inbox(Predeploys.CROSS_L2_INBOX).validateMessage(_id, keccak256(_sentMessage)); + + (uint256 destination, , , address sender, bytes memory message) = + _decodeSentMessagePayload(_sentMessage); + + require(destination == block.chainId); + require(sender == Predeploys.L2_TO_L2_CROSS_DOMAIN_MESSENGER); + + bytes32 expiredHash = abi.decode(_sentMessage); + + require(expiredMessages[expiredHash] == 0); expiredMessages[_expiredHash] = block.timestamp; - emit MessageHashExpired(_expiredHash); + emit MessageHashExpired(_expiredHash, block.timestamp); } ``` From e6ab24fb134403709a0e6530ebe0581ce139bcc6 Mon Sep 17 00:00:00 2001 From: 0xng Date: Tue, 1 Oct 2024 13:50:57 -0300 Subject: [PATCH 2/6] fix: lint --- specs/interop/predeploys.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/specs/interop/predeploys.md b/specs/interop/predeploys.md index b75d74807..4cd76acc7 100644 --- a/specs/interop/predeploys.md +++ b/specs/interop/predeploys.md @@ -413,8 +413,8 @@ as expired in the `successfulMessages` mapping to ensure non-replayability and d from `failedMessages`. An initiating message is then emitted to `relayExpire`. `sendExpire` sets the sender of the message as the `L2ToL2CrossDomainMessenger` to add a path -for `relayExpire` to ensure that the message originated in `sendExpire`. There's should be no -other path where a message can have the `L2ToL2CrossDomainMessenger` as `origin` and `sender`. +for `relayExpire` to ensure that the message originated in `sendExpire`. There's should be no +other path where a message can have the `L2ToL2CrossDomainMessenger` as `origin` and `sender`. ```solidity function sendExpire(bytes32 _expiredHash) external nonReentrant { @@ -454,7 +454,7 @@ If all checks have been successful, the message hash is stored in the `expiredMessages` mapping. This enables smart contracts to read from it and check whether a message expired or not, and handle this case accordingly. -An expiry message is relayed by providing the [identifier](./messaging.md#message-identifier) +An expiry message is relayed by providing the [identifier](./messaging.md#message-identifier) to a `SentMessage` event and its corresponding [message payload](./messaging.md#message-payload). ```solidity From 87838cdbe9fb60b31e72c171272ec7207c3ed153 Mon Sep 17 00:00:00 2001 From: 0xng Date: Tue, 1 Oct 2024 14:05:01 -0300 Subject: [PATCH 3/6] fix: struct construction syntax --- specs/interop/predeploys.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/specs/interop/predeploys.md b/specs/interop/predeploys.md index 4cd76acc7..1d6e16252 100644 --- a/specs/interop/predeploys.md +++ b/specs/interop/predeploys.md @@ -385,7 +385,7 @@ function relayMessage(ICrossL2Inbox.Identifier calldata _id, bytes calldata _sen emit RelayedMessage(_source, _nonce, messageHash); } else { if (failedMessages[messageHash].timestamp == 0) { - failedMessages[messageHash] = FailedMessage{timestamp: block.timestamp, sourceChain: _source}; + failedMessages[messageHash] = FailedMessage({timestamp: block.timestamp, sourceChain: _source}); } emit FailedRelayedMessage(_source, _nonce, messageHash); } From 1a8cf268cf32d379058ce0c74551395dd18a75ce Mon Sep 17 00:00:00 2001 From: 0xng Date: Wed, 9 Oct 2024 18:47:00 -0300 Subject: [PATCH 4/6] feat: adding returnedMessages mapping --- specs/interop/predeploys.md | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/specs/interop/predeploys.md b/specs/interop/predeploys.md index 3cb238c78..655488ee7 100644 --- a/specs/interop/predeploys.md +++ b/specs/interop/predeploys.md @@ -285,10 +285,11 @@ as well as domain binding, ie the executing transaction can only be valid on a s ### `sendExpire` Invariants -- The message MUST have not been successfully relayed +- The message MUST have failed to be relayed and not have succeded in later attemps. - The `EXPIRY_WINDOW` MUST have elapsed since the message first failed to be relayed - The expired message MUST not have been previously sent back to source - The expired message MUST not be relayable after being sent back +- The `returnedMessages` mapping MUST only contain messages that were sent back to their origin chain. ### `relayExpire` Invariants @@ -379,7 +380,7 @@ flowchart LR ``` When relaying a message through the `L2ToL2CrossDomainMessenger`, it is important to require that -the `_destination` equal to `block.chainid` to ensure that the message is only valid on a single +the `_destination` is equal to `block.chainid` to ensure that the message is only valid on a single chain. The hash of the message is used for replay protection. It is important to ensure that the source chain is in the dependency set of the destination chain, otherwise @@ -408,6 +409,8 @@ function relayMessage(ICrossL2Inbox.Identifier calldata _id, bytes calldata _sen // log data (address _sender, bytes memory _message) = abi.decode(_sentMessage[128:], (address,bytes)); + require(returnedMessages[messageHash] != 0); + bool success = SafeCall.call(_target, msg.value, _message); if (success) { @@ -439,8 +442,8 @@ failed messages and to prevent malicious actors from performing a griefing attac by expiring messages upon arrival. Once the expired message is sent to the source chain, the message on the local chain is set -as expired in the `successfulMessages` mapping to ensure non-replayability and deleted -from `failedMessages`. An initiating message is then emitted to `relayExpire`. +as expired in the `returnedMessages` mapping to ensure non-replayability and deleted from `failedMessages`. +An initiating message is then emitted to `relayExpire`. `sendExpire` sets the sender of the message as the `L2ToL2CrossDomainMessenger` to add a path for `relayExpire` to ensure that the message originated in `sendExpire`. There's should be no @@ -456,7 +459,7 @@ function sendExpire(bytes32 _expiredHash) external nonReentrant { require(block.timestamp >= messageTimestamp + EXPIRY_WINDOW); delete failedMessages[_expiredHash]; - successfulMessages[_expiredHash] = true; + returnedMessages[_expiredHash] = block.timestamp; uint256 destination = messageSource; address target; From b2c5a903f7a71379d21e90d03877bb3473ac04a7 Mon Sep 17 00:00:00 2001 From: 0xng Date: Thu, 10 Oct 2024 11:29:05 -0300 Subject: [PATCH 5/6] fix: error check and invariant clarification --- specs/interop/predeploys.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/specs/interop/predeploys.md b/specs/interop/predeploys.md index 655488ee7..40983e0a7 100644 --- a/specs/interop/predeploys.md +++ b/specs/interop/predeploys.md @@ -287,8 +287,8 @@ as well as domain binding, ie the executing transaction can only be valid on a s - The message MUST have failed to be relayed and not have succeded in later attemps. - The `EXPIRY_WINDOW` MUST have elapsed since the message first failed to be relayed -- The expired message MUST not have been previously sent back to source -- The expired message MUST not be relayable after being sent back +- The expired message MUST NOT have been previously sent back to source +- The expired message MUST NOT be relayable after being sent back to the origin chain - The `returnedMessages` mapping MUST only contain messages that were sent back to their origin chain. ### `relayExpire` Invariants @@ -296,7 +296,7 @@ as well as domain binding, ie the executing transaction can only be valid on a s - The message source MUST be `block.chainid` - The `Identifier.origin` and `sender` MUST be `address(L2ToL2CrossDomainMessenger)` - The `expiredMessages` mapping MUST only contain messages that originated in this chain and failed to be relayed on destination. -- Already expired messages MUST NOT be relayed. +- It MUST NOT accept previously-expired messages, that is, messages stored in the `expiredMessages` mapping. ### Message Versioning @@ -409,7 +409,7 @@ function relayMessage(ICrossL2Inbox.Identifier calldata _id, bytes calldata _sen // log data (address _sender, bytes memory _message) = abi.decode(_sentMessage[128:], (address,bytes)); - require(returnedMessages[messageHash] != 0); + require(returnedMessages[messageHash] == 0); bool success = SafeCall.call(_target, msg.value, _message); From a02287d22d8827495ae4fde963071b24ecfe520a Mon Sep 17 00:00:00 2001 From: 0xng Date: Thu, 10 Oct 2024 11:32:53 -0300 Subject: [PATCH 6/6] fix: typo --- specs/interop/predeploys.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/specs/interop/predeploys.md b/specs/interop/predeploys.md index 40983e0a7..d4fdcb567 100644 --- a/specs/interop/predeploys.md +++ b/specs/interop/predeploys.md @@ -446,7 +446,7 @@ as expired in the `returnedMessages` mapping to ensure non-replayability and del An initiating message is then emitted to `relayExpire`. `sendExpire` sets the sender of the message as the `L2ToL2CrossDomainMessenger` to add a path -for `relayExpire` to ensure that the message originated in `sendExpire`. There's should be no +for `relayExpire` to ensure that the message originated in `sendExpire`. There should be no other path where a message can have the `L2ToL2CrossDomainMessenger` as `origin` and `sender`. ```solidity