fix: correct dos attack from foreign and bad escrow

catalystdao · Mar 12, 2024 · 20c27d3 · 20c27d3
1 parent 4abd57c
commit 20c27d3
Show file tree

Hide file tree

Showing 10 changed files with 27 additions and 32 deletions.
diff --git a/src/IncentivizedMessageEscrow.sol b/src/IncentivizedMessageEscrow.sol
@@ -53,7 +53,7 @@ abstract contract IncentivizedMessageEscrow is IIncentivizedMessageEscrow, Bytes
     mapping(bytes32 => IncentiveDescription) _bounty;
 
     /** @notice A hash of the emitted message on receive such that we can emit a similar one. */
-    mapping(bytes32 => bytes32) _messageDelivered;
+    mapping(bytes => mapping(bytes32 => bytes32)) _messageDelivered;
 
     // Maps applications to their escrow implementations.
     mapping(address => mapping(bytes32 => bytes)) public implementationAddress;
@@ -168,8 +168,8 @@ abstract contract IncentivizedMessageEscrow is IIncentivizedMessageEscrow, Bytes
      * @notice Returns a hash of the ack unless there was no ack then it returns bytes32(uint256(1));
      * If the message hasn't been delivered yet it still returns bytes32(0)
      */
-   function messageDelivered(bytes32 messageIdentifier) external view returns(bytes32 hasMessageBeenExecuted) {
-        return _messageDelivered[messageIdentifier];
+   function messageDelivered(bytes calldata sourceImplementationIdentifier, bytes32 messageIdentifier) external view returns(bytes32 hasMessageBeenExecuted) {
+        return _messageDelivered[sourceImplementationIdentifier][messageIdentifier];
    }
 
      /**
@@ -396,9 +396,9 @@ abstract contract IncentivizedMessageEscrow is IIncentivizedMessageEscrow, Bytes
         // The 3 next lines act as a reentry guard, so this call doesn't have to be protected by reentry.
         // We will re-set _messageDelivered[messageIdentifier] again later as the hash of the ack, however, we need re-entry protection
         // so applications don't try to claim incentives multiple times. So, we set it now and change it later.
-        bytes32 messageState = _messageDelivered[messageIdentifier];
+        bytes32 messageState = _messageDelivered[sourceImplementationIdentifier][messageIdentifier];
         if (messageState != bytes32(0)) revert MessageAlreadySpent();
-        _messageDelivered[messageIdentifier] = bytes32(uint256(1));
+        _messageDelivered[sourceImplementationIdentifier][messageIdentifier] = bytes32(uint256(1));
 
         // Deliver message to application.
         // Decode gas limit, application address and sending application.
@@ -434,7 +434,7 @@ abstract contract IncentivizedMessageEscrow is IIncentivizedMessageEscrow, Bytes
             );
 
             // Store a hash of the acknowledgement so we can later retry a potentially invalid ack proof.
-            _messageDelivered[messageIdentifier] = keccak256(receiveAckWithContext);
+            _messageDelivered[sourceImplementationIdentifier][messageIdentifier] = keccak256(receiveAckWithContext);
 
             // Message has been delivered and shouldn't be executed again.
             emit MessageDelivered(messageIdentifier);
@@ -463,7 +463,7 @@ abstract contract IncentivizedMessageEscrow is IIncentivizedMessageEscrow, Bytes
             );
 
             // Store a hash of the acknowledgement so we can later retry a potentially invalid ack proof.
-            _messageDelivered[messageIdentifier] = keccak256(receiveAckWithContext);
+            _messageDelivered[sourceImplementationIdentifier][messageIdentifier] = keccak256(receiveAckWithContext);
 
             // Message has been delivered and shouldn't be executed again.
             emit MessageDelivered(messageIdentifier);
@@ -510,7 +510,7 @@ abstract contract IncentivizedMessageEscrow is IIncentivizedMessageEscrow, Bytes
         );
 
         // Store a hash of the acknowledgement so we can later retry a potentially invalid ack proof.
-        _messageDelivered[messageIdentifier] = keccak256(receiveAckWithContext);
+        _messageDelivered[sourceImplementationIdentifier][messageIdentifier] = keccak256(receiveAckWithContext);
 
         // Why is the messageDelivered event emitted before _sendPacket?
         // Because it lets us pop messageIdentifier from the stack. This avoid a stack limit reached error. 
@@ -923,7 +923,7 @@ abstract contract IncentivizedMessageEscrow is IIncentivizedMessageEscrow, Bytes
         // This makes it ever so slighly easier to retry messages.
         bytes32 messageIdentifier = bytes32(receiveAckWithContext[MESSAGE_IDENTIFIER_START:MESSAGE_IDENTIFIER_END]);
 
-        bytes32 storedAckHash = _messageDelivered[messageIdentifier];
+        bytes32 storedAckHash = _messageDelivered[implementationIdentifier][messageIdentifier];
         // First, check if there is actually an appropiate hash at the message identifier.
         // Then, check if the storedAckHash matches the executed one.
         if (storedAckHash == bytes32(0) || storedAckHash != keccak256(receiveAckWithContext)) revert CannotRetryWrongMessage(storedAckHash, keccak256(receiveAckWithContext));
@@ -976,7 +976,7 @@ abstract contract IncentivizedMessageEscrow is IIncentivizedMessageEscrow, Bytes
         // Get the message identifier from the message.
         bytes32 messageIdentifier = bytes32(message[MESSAGE_IDENTIFIER_START:MESSAGE_IDENTIFIER_END]);
         // Read the status of the package at MessageIdentifier.
-        bytes32 storedAckHash = _messageDelivered[messageIdentifier];
+        bytes32 storedAckHash = _messageDelivered[implementationIdentifier][messageIdentifier];
         // If has already been processed, then don't allow timeouting the message. Instead, it should be retried.
         if (storedAckHash != bytes32(0)) revert MessageAlreadyProcessed();
         // This also protects a relayer that delivered a timedout message.

diff --git a/src/interfaces/IIncentivizedMessageEscrow.sol b/src/interfaces/IIncentivizedMessageEscrow.sol
@@ -10,7 +10,7 @@ import { IMessageEscrowEvents } from "./IMessageEscrowEvents.sol";
 interface IIncentivizedMessageEscrow is IMessageEscrowStructs, IMessageEscrowErrors, IMessageEscrowEvents {
    function bounty(bytes32 messageIdentifier) external view returns(IncentiveDescription memory incentive);
 
-   function messageDelivered(bytes32 messageIdentifier) external view returns(bytes32 hasMessageBeenExecuted);
+   function messageDelivered(bytes calldata sourceImplementationIdentifier, bytes32 messageIdentifier) external view returns(bytes32 hasMessageBeenExecuted);
 
     function increaseBounty(
         bytes32 messageIdentifier,

diff --git a/test/IncentivizedMessageEscrow/ReemitAckMessage.t.sol b/test/IncentivizedMessageEscrow/ReemitAckMessage.t.sol
@@ -12,8 +12,6 @@ contract ReemitAckMessageTest is TestCommon {
     );
 
     function test_reemit_ack(bytes calldata message) public {
-        bytes32 feeRecipient = bytes32(uint256(uint160(address(this))));
-
         bytes32 destinationFeeRecipient = bytes32(uint256(uint160(address(this))));
 
         (, bytes memory messageWithContext) = setupForAck(address(application), message, destinationFeeRecipient);
@@ -24,24 +22,19 @@ contract ReemitAckMessageTest is TestCommon {
         vm.expectEmit();
         emit Message(
             _DESTINATION_IDENTIFIER,
-            abi.encodePacked(address(escrow)),
+            abi.encode(address(escrow)),
             this.memorySlice(messageWithContext, 32)
         );
 
-        escrow.reemitAckMessage(_DESTINATION_IDENTIFIER, abi.encodePacked(address(escrow)), rawMessage);
+        escrow.reemitAckMessage(_DESTINATION_IDENTIFIER, abi.encode(address(escrow)), rawMessage);
     }
 
     // This tests a relayer trying to reemit a message which is slightly changed.
     function test_reemit_ack_wrong_message(bytes calldata message) public {
-        bytes32 feeRecipient = bytes32(uint256(uint160(address(this))));
-
         bytes32 destinationFeeRecipient = bytes32(uint256(uint160(address(this))));
 
         (, bytes memory messageWithContext) = setupForAck(address(application), message, destinationFeeRecipient);
 
-        // Remove the context that setupForAck adds.
-        bytes memory rawMessage = this.memorySlice(messageWithContext, 96);
-
         // Simulate a relayer changing the ack from the application. We add a byte
         // but it could also be removing, flipping or similar.
         bytes memory newMessage = bytes.concat(

diff --git a/test/IncentivizedMessageEscrow/TimeoutMessage.t.sol b/test/IncentivizedMessageEscrow/TimeoutMessage.t.sol
@@ -24,7 +24,7 @@ contract TimeoutMessageTest is TestCommon, Bytes65 {
         vm.expectEmit();
         emit Message(
             _DESTINATION_IDENTIFIER,
-            abi.encodePacked(address(escrow)),
+            abi.encode(address(escrow)),
             bytes.concat(
                 _DESTINATION_IDENTIFIER,
                 _DESTINATION_IDENTIFIER,
@@ -38,7 +38,7 @@ contract TimeoutMessageTest is TestCommon, Bytes65 {
 
         escrow.timeoutMessage(
             _DESTINATION_IDENTIFIER,
-            abi.encodePacked(address(escrow)),
+            abi.encode(address(escrow)),
             block.number,
             rawSubmitMessage
         );
@@ -107,18 +107,20 @@ contract TimeoutMessageTest is TestCommon, Bytes65 {
     function test_message_cannot_be_timeouted_after_exec(bytes calldata message) public {
         bytes32 destinationFeeRecipient = bytes32(uint256(uint160(address(this))));
 
-        (, bytes memory submitMessageWithContext) = setupsubmitMessage(address(application), message);
+        (, bytes memory submitMessageWithContext) = setupsubmitMessage(address(application), message, 2);
 
         // Ready for ack.
         setupprocessPacket(submitMessageWithContext, destinationFeeRecipient);
 
         // Remove the context
         bytes memory rawSubmitMessage = this.memorySlice(submitMessageWithContext, 96);
 
+        vm.warp(3);
+
         vm.expectRevert(abi.encodeWithSignature("MessageAlreadyProcessed()"));
         escrow.timeoutMessage(
             _DESTINATION_IDENTIFIER,
-            abi.encodePacked(address(escrow)),
+            abi.encode(address(escrow)),
             block.number,
             rawSubmitMessage
         );
@@ -144,7 +146,7 @@ contract TimeoutMessageTest is TestCommon, Bytes65 {
         vm.expectRevert(abi.encodeWithSignature("DeadlineNotPassed(uint64,uint64)", uint64(newTimestamp+1), uint64(newTimestamp)));
         escrow.timeoutMessage(
             _DESTINATION_IDENTIFIER,
-            abi.encodePacked(address(escrow)),
+            abi.encode(address(escrow)),
             block.number,
             newRawSubmitMessage
         );
@@ -167,7 +169,7 @@ contract TimeoutMessageTest is TestCommon, Bytes65 {
         vm.expectRevert(abi.encodeWithSignature("DeadlineNotPassed(uint64,uint64)", 0, newTimestamp));
         escrow.timeoutMessage(
             _DESTINATION_IDENTIFIER,
-            abi.encodePacked(address(escrow)),
+            abi.encode(address(escrow)),
             block.number,
             newRawSubmitMessage
         );

diff --git a/test/IncentivizedMessageEscrow/processMessage/GasSpendControl.sol b/test/IncentivizedMessageEscrow/processMessage/GasSpendControl.sol
@@ -58,7 +58,7 @@ contract GasSpendControlTest is TestCommon {
                 messageIdentifier,
                 _DESTINATION_ADDRESS_APPLICATION,
                 destinationFeeRecipient,
-                uint48(0x366fa),  // Gas used
+                uint48(0x368c3),  // Gas used
                 uint64(1),
                 bytes1(0xff),  // This states that the call went wrong.
                 message

diff --git a/test/IncentivizedMessageEscrow/processMessage/NoReceive.t.sol b/test/IncentivizedMessageEscrow/processMessage/NoReceive.t.sol
@@ -54,7 +54,7 @@ contract processPacketNoReceiveTest is TestCommon {
                 messageIdentifier,
                 _DESTINATION_ADDRESS_THIS,
                 feeRecipient,
-                uint48(0x80cd),  // Gas used
+                uint48(0x8296),  // Gas used
                 uint64(1),
                 abi.encodePacked(bytes1(0xff)),
                 message

diff --git a/test/IncentivizedMessageEscrow/processMessage/Reentry.call.t.sol b/test/IncentivizedMessageEscrow/processMessage/Reentry.call.t.sol
@@ -61,7 +61,7 @@ contract CallReentryTest is TestCommon, ICrossChainReceiver {
                 messageIdentifier,
                 _DESTINATION_ADDRESS_APPLICATION,
                 feeRecipient,
-                uint48(0xf965),  // Gas used
+                uint48(0xfc1d),  // Gas used
                 uint64(1),
                 uint8(1)
             )

diff --git a/test/IncentivizedMessageEscrow/processMessage/_handleCall.t.sol b/test/IncentivizedMessageEscrow/processMessage/_handleCall.t.sol
@@ -40,7 +40,7 @@ contract processPacketCallTest is TestCommon {
                 messageIdentifier,
                 _DESTINATION_ADDRESS_APPLICATION,
                 feeRecipient,
-                uint48(0x7af5),  // Gas used
+                uint48(0x7cbe),  // Gas used
                 uint64(1),
                 mockAck
             )

diff --git a/test/OnRecvIncentivizedMockEscrow/processMessage/_handleCall.t.sol b/test/OnRecvIncentivizedMockEscrow/processMessage/_handleCall.t.sol
@@ -45,7 +45,7 @@ contract OnRecvCallTest is TestOnRecvCommon {
                 messageIdentifier,
                 _DESTINATION_ADDRESS_APPLICATION,
                 feeRecipient,
-                uint48(0x6b2a),  // Gas used
+                uint48(0x6cf3),  // Gas used
                 uint64(1),
                 hex"d9b60178cfb2eb98b9ff9136532b6bd80eeae6a2c90a2f96470294981fcfb62b"
             )

diff --git a/test/TestCommon.t.sol b/test/TestCommon.t.sol
@@ -22,7 +22,7 @@ interface ICansubmitMessage is IMessageEscrowStructs{
 contract TestCommon is Test, IMessageEscrowEvents, IMessageEscrowStructs {
 
     uint256 constant GAS_SPENT_ON_SOURCE = 6617;
-    uint256 constant GAS_SPENT_ON_DESTINATION = 31477;
+    uint256 constant GAS_SPENT_ON_DESTINATION = 31934;
 
     bytes32 constant _DESTINATION_IDENTIFIER = bytes32(uint256(0x123123) + uint256(2**255));