Merge pull request #20 from immutable/smr-1914-data-length-32

[SMR-1914] data length must be greater than 32
immutable · Nov 6, 2023 · de91564 · de91564
2 parents b6c05a8 + 2759edd
commit de91564
Show file tree

Hide file tree

Showing 8 changed files with 22 additions and 17 deletions.
diff --git a/src/child/ChildERC20Bridge.sol b/src/child/ChildERC20Bridge.sol
@@ -109,20 +109,21 @@ contract ChildERC20Bridge is
         if (!Strings.equal(messageSourceChain, rootChain)) {
             revert InvalidSourceChain();
         }
-
         if (!Strings.equal(sourceAddress, rootERC20BridgeAdaptor)) {
             revert InvalidSourceAddress();
         }
-        if (data.length == 0) {
-            revert InvalidData();
+        if (data.length <= 32) {
+            // Data must always be greater than 32.
+            // 32 bytes for the signature, and at least some information for the payload
+            revert InvalidData("Data too short");
         }
 
         if (bytes32(data[:32]) == MAP_TOKEN_SIG) {
             _mapToken(data);
         } else if (bytes32(data[:32]) == DEPOSIT_SIG) {
             _deposit(data[32:]);
         } else {
-            revert InvalidData();
+            revert InvalidData("Unsupported action signature");
         }
     }
 

diff --git a/src/interfaces/child/IChildERC20Bridge.sol b/src/interfaces/child/IChildERC20Bridge.sol
@@ -73,7 +73,7 @@ interface IChildERC20BridgeErrors {
     /// @notice Error when a message is given to the bridge from an address not the designated bridge adaptor.
     error NotBridgeAdaptor();
     /// @notice Error when the message's payload is not valid.
-    error InvalidData();
+    error InvalidData(string reason);
     /// @notice Error when the message's source chain is not valid.
     error InvalidSourceChain();
     /// @notice Error when the source chain's message sender is not a recognised address.

diff --git a/src/interfaces/root/IRootERC20Bridge.sol b/src/interfaces/root/IRootERC20Bridge.sol
@@ -106,7 +106,7 @@ interface IRootERC20BridgeErrors {
     /// @notice Error when the given child chain bridge adaptor is invalid.
     error InvalidChildERC20BridgeAdaptor();
     /// @notice Error when a message received has invalid data.
-    error InvalidData();
+    error InvalidData(string reason);
     /// @notice Error when a message received has invalid source address.
     error InvalidSourceAddress();
     /// @notice Error when a message received has invalid source chain.

diff --git a/src/root/RootERC20Bridge.sol b/src/root/RootERC20Bridge.sol
@@ -164,14 +164,16 @@ contract RootERC20Bridge is
         if (!Strings.equal(sourceAddress, childBridgeAdaptor)) {
             revert InvalidSourceAddress();
         }
-        if (data.length == 0) {
-            revert InvalidData();
+        if (data.length <= 32) {
+            // Data must always be greater than 32.
+            // 32 bytes for the signature, and at least some information for the payload
+            revert InvalidData("Data too short");
         }
 
         if (bytes32(data[:32]) == WITHDRAW_SIG) {
             _withdraw(data[32:]);
         } else {
-            revert InvalidData();
+            revert InvalidData("Unsupported action signature");
         }
     }
 

diff --git a/test/integration/child/ChildAxelarBridge.t.sol b/test/integration/child/ChildAxelarBridge.t.sol
@@ -83,7 +83,7 @@ contract ChildERC20BridgeIntegrationTest is Test, IChildERC20BridgeEvents, IChil
         bytes32 commandId = bytes32("testCommandId");
         bytes memory payload = abi.encode("invalid payload");
 
-        vm.expectRevert(InvalidData.selector);
+        vm.expectRevert(abi.encodeWithSelector(InvalidData.selector, "Unsupported action signature"));
         childAxelarBridgeAdaptor.execute(commandId, ROOT_CHAIN_NAME, ROOT_ADAPTOR_ADDRESS, payload);
     }
 
@@ -109,7 +109,7 @@ contract ChildERC20BridgeIntegrationTest is Test, IChildERC20BridgeEvents, IChil
         bytes32 commandId = bytes32("testCommandId");
         bytes memory payload = "";
 
-        vm.expectRevert(InvalidData.selector);
+        vm.expectRevert(abi.encodeWithSelector(InvalidData.selector, "Data too short"));
         childAxelarBridgeAdaptor.execute(commandId, ROOT_CHAIN_NAME, ROOT_ADAPTOR_ADDRESS, payload);
     }
 

diff --git a/test/integration/root/withdrawals.t.sol/RootERC20BridgeWithdraw.t.sol b/test/integration/root/withdrawals.t.sol/RootERC20BridgeWithdraw.t.sol
@@ -87,7 +87,7 @@ contract RootERC20BridgeWithdrawIntegrationTest is
         bytes32 commandId = bytes32("testCommandId");
         string memory sourceAddress = rootBridge.childBridgeAdaptor();
 
-        vm.expectRevert(InvalidData.selector);
+        vm.expectRevert(abi.encodeWithSelector(InvalidData.selector, "Data too short"));
         axelarAdaptor.execute(commandId, CHILD_CHAIN_NAME, sourceAddress, data);
     }
 
@@ -97,7 +97,7 @@ contract RootERC20BridgeWithdrawIntegrationTest is
         bytes32 commandId = bytes32("testCommandId");
         string memory sourceAddress = rootBridge.childBridgeAdaptor();
 
-        vm.expectRevert(InvalidData.selector);
+        vm.expectRevert(abi.encodeWithSelector(InvalidData.selector, "Unsupported action signature"));
         axelarAdaptor.execute(commandId, CHILD_CHAIN_NAME, sourceAddress, data);
     }
 

diff --git a/test/unit/child/ChildERC20Bridge.t.sol b/test/unit/child/ChildERC20Bridge.t.sol
@@ -168,15 +168,15 @@ contract ChildERC20BridgeUnitTest is Test, IChildERC20BridgeEvents, IChildERC20B
 
     function test_RevertIf_onMessageReceiveCalledWithDataLengthZero() public {
         bytes memory data = "";
-        vm.expectRevert(InvalidData.selector);
+        vm.expectRevert(abi.encodeWithSelector(InvalidData.selector, "Data too short"));
         childBridge.onMessageReceive(ROOT_CHAIN_NAME, ROOT_BRIDGE_ADAPTOR, data);
     }
 
     function test_RevertIf_onMessageReceiveCalledWithDataInvalid() public {
         bytes memory data =
             abi.encode("FAKEDATA", address(rootToken), rootToken.name(), rootToken.symbol(), rootToken.decimals());
 
-        vm.expectRevert(InvalidData.selector);
+        vm.expectRevert(abi.encodeWithSelector(InvalidData.selector, "Unsupported action signature"));
         childBridge.onMessageReceive(ROOT_CHAIN_NAME, ROOT_BRIDGE_ADAPTOR, data);
     }
 

diff --git a/test/unit/root/withdrawals/RootERC20BridgeWithdraw.t.sol b/test/unit/root/withdrawals/RootERC20BridgeWithdraw.t.sol
@@ -84,14 +84,16 @@ contract RootERC20BridgeWithdrawUnitTest is Test, IRootERC20BridgeEvents, IRootE
         bytes memory data;
 
         vm.prank(address(mockAxelarAdaptor));
-        vm.expectRevert(InvalidData.selector);
+        vm.expectRevert(abi.encodeWithSelector(InvalidData.selector, "Data too short"));
+
         rootBridge.onMessageReceive(CHILD_CHAIN_NAME, CHILD_BRIDGE_ADAPTOR_STRING, data);
     }
 
     function test_RevertsIf_OnMessageReceiveWithInvalidSignature() public {
         bytes memory data = abi.encode(keccak256("RANDOM"), IMX_TOKEN, address(this), address(this), withdrawAmount);
+
         vm.prank(address(mockAxelarAdaptor));
-        vm.expectRevert(InvalidData.selector);
+        vm.expectRevert(abi.encodeWithSelector(InvalidData.selector, "Unsupported action signature"));
         rootBridge.onMessageReceive(CHILD_CHAIN_NAME, CHILD_BRIDGE_ADAPTOR_STRING, data);
     }