feat: don't let feeRecipient be bytes32(0)

catalystdao · Apr 30, 2024 · 2fbcf02 · 2fbcf02
1 parent 0011eb4
commit 2fbcf02
Show file tree

Hide file tree

Showing 9 changed files with 14 additions and 9 deletions.
diff --git a/src/IncentivizedMessageEscrow.sol b/src/IncentivizedMessageEscrow.sol
@@ -107,7 +107,7 @@ abstract contract IncentivizedMessageEscrow is IIncentivizedMessageEscrow, Bytes
      * It cannot be set to address 0, instead use a burn address (0xdead) if no-one wants to take responsibility of the Ether.
      */
     constructor(address sendLostGasTo) {
-        if (sendLostGasTo == address(0)) revert SendLostGasToAddress0();
+        if (sendLostGasTo == address(0)) revert SendLostGasToIsZero();
         SEND_LOST_GAS_TO = sendLostGasTo;
     }
 
@@ -344,6 +344,7 @@ abstract contract IncentivizedMessageEscrow is IIncentivizedMessageEscrow, Bytes
         bytes32 feeRecipient
     ) external virtual payable {
         uint256 gasLimit = gasleft();  // uint256 is used here instead of uint48, since there is no advantage to uint48 until after we calculate the difference.
+        if (feeRecipient == bytes32(0)) revert FeeRecipientIsZero();
 
         // Verify that the message is authentic and remove potential context that the messaging protocol added to the message.
         (bytes32 chainIdentifier, bytes memory implementationIdentifier, bytes calldata message) = _verifyPacket(messagingProtocolContext, rawMessage);

diff --git a/src/interfaces/IMessageEscrowErrors.sol b/src/interfaces/IMessageEscrowErrors.sol
@@ -22,5 +22,6 @@ interface IMessageEscrowErrors {
     error NotEnoughGasProvided(uint128 expected, uint128 actual);    // 0x030748b5
     error NotImplementedError(); // 0xd41c17e7
     error RefundGasToIsZero(); // 0x6a1a6afe
-    error SendLostGasToAddress0(); // 0x470337f2
+    error SendLostGasToIsZero(); // 0x470337f2
+    error FeeRecipientIsZero(); // 0x6a1a6afe
 }
diff --git a/test/IncentivizedMessageEscrow/escrowMessage/MessageIdentifier.t.sol b/test/IncentivizedMessageEscrow/escrowMessage/MessageIdentifier.t.sol
@@ -90,6 +90,8 @@ contract MessageIdentifierTest is TestCommon {
         vm.assume(a != b);
         vm.assume(a != address(application));
         vm.assume(b != address(application));
+        vm.assume(a != address(this));
+        vm.assume(b != address(this));
         IncentiveDescription storage incentive = _INCENTIVE;
 
         vm.deal(a, _getTotalIncentive(_INCENTIVE));

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(0x3697d),  // Gas used
+                uint48(0x36996),  // 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(0x8350),  // Gas used
+                uint48(0x8369),  // 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(0xfd07),  // Gas used
+                uint48(0xfd36),  // 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(0x7d66),  // Gas used
+                uint48(0x7d7f),  // Gas used
                 uint64(1),
                 mockAck
             )
@@ -92,6 +92,7 @@ contract processPacketCallTest is TestCommon {
     }
 
     function test_expect_caller(address caller) public {
+        vm.assume(caller != address(this));
         vm.assume(caller != 0x2e234DAe75C793f67A35089C9d99245E1C58470b);
         bytes memory message = _MESSAGE;
         bytes32 feeRecipient = bytes32(uint256(uint160(address(this))));

diff --git a/test/IncentivizedMessageEscrow/reemitAck/AnyAckExploit.t.sol b/test/IncentivizedMessageEscrow/reemitAck/AnyAckExploit.t.sol
@@ -148,7 +148,7 @@ contract ReemitAckAnywhereExploitTest is TestCommon {
         // lets execute the non-legit message
 
         vm.recordLogs();
-        escrow.processPacket(nonLegitMessageContext, nonLegitMessage, bytes32(abi.encode(address(0))));
+        escrow.processPacket(nonLegitMessageContext, nonLegitMessage, bytes32(abi.encode(address(0xdead))));
         Vm.Log[] memory entries = vm.getRecordedLogs();
 
         (, , bytes memory ackMessage) = abi.decode(entries[1].data, (bytes32, bytes, bytes));

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