From dec0180bad7b2c4420eb08d18004c884c597bb78 Mon Sep 17 00:00:00 2001 From: Benjamin Patch Date: Mon, 6 Nov 2023 08:40:44 +1100 Subject: [PATCH 1/3] Add length <32 check to child bridge --- src/child/ChildERC20Bridge.sol | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/child/ChildERC20Bridge.sol b/src/child/ChildERC20Bridge.sol index 9a9e6212..851de824 100644 --- a/src/child/ChildERC20Bridge.sol +++ b/src/child/ChildERC20Bridge.sol @@ -109,11 +109,12 @@ contract ChildERC20Bridge is if (!Strings.equal(messageSourceChain, rootChain)) { revert InvalidSourceChain(); } - if (!Strings.equal(sourceAddress, rootERC20BridgeAdaptor)) { revert InvalidSourceAddress(); } - if (data.length == 0) { + 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(); } From 1fc0d9770e83004ddec1b83f50882483b28ceb06 Mon Sep 17 00:00:00 2001 From: Benjamin Patch Date: Mon, 6 Nov 2023 08:43:29 +1100 Subject: [PATCH 2/3] Add checks and update tests --- src/child/ChildERC20Bridge.sol | 2 +- src/interfaces/child/IChildERC20Bridge.sol | 2 ++ src/interfaces/root/IRootERC20Bridge.sol | 2 ++ src/root/RootERC20Bridge.sol | 6 ++++-- test/integration/child/ChildAxelarBridge.t.sol | 2 +- .../root/withdrawals.t.sol/RootERC20BridgeWithdraw.t.sol | 2 +- test/unit/child/ChildERC20Bridge.t.sol | 2 +- test/unit/root/withdrawals/RootERC20BridgeWithdraw.t.sol | 2 +- 8 files changed, 13 insertions(+), 7 deletions(-) diff --git a/src/child/ChildERC20Bridge.sol b/src/child/ChildERC20Bridge.sol index 851de824..9037215d 100644 --- a/src/child/ChildERC20Bridge.sol +++ b/src/child/ChildERC20Bridge.sol @@ -115,7 +115,7 @@ contract ChildERC20Bridge is 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(); + revert DataTooShort(); } if (bytes32(data[:32]) == MAP_TOKEN_SIG) { diff --git a/src/interfaces/child/IChildERC20Bridge.sol b/src/interfaces/child/IChildERC20Bridge.sol index 087daa89..62d7cfe7 100644 --- a/src/interfaces/child/IChildERC20Bridge.sol +++ b/src/interfaces/child/IChildERC20Bridge.sol @@ -72,6 +72,8 @@ interface IChildERC20BridgeErrors { error AlreadyMapped(); /// @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 too short. + error DataTooShort(); /// @notice Error when the message's payload is not valid. error InvalidData(); /// @notice Error when the message's source chain is not valid. diff --git a/src/interfaces/root/IRootERC20Bridge.sol b/src/interfaces/root/IRootERC20Bridge.sol index 13bfb1e8..a0dd34dd 100644 --- a/src/interfaces/root/IRootERC20Bridge.sol +++ b/src/interfaces/root/IRootERC20Bridge.sol @@ -105,6 +105,8 @@ interface IRootERC20BridgeErrors { error BalanceInvariantCheckFailed(uint256 actualBalance, uint256 expectedBalance); /// @notice Error when the given child chain bridge adaptor is invalid. error InvalidChildERC20BridgeAdaptor(); + /// @notice Error when the message's payload is too short. + error DataTooShort(); /// @notice Error when a message received has invalid data. error InvalidData(); /// @notice Error when a message received has invalid source address. diff --git a/src/root/RootERC20Bridge.sol b/src/root/RootERC20Bridge.sol index bf72034d..b67a3ddd 100644 --- a/src/root/RootERC20Bridge.sol +++ b/src/root/RootERC20Bridge.sol @@ -164,8 +164,10 @@ 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 DataTooShort(); } if (bytes32(data[:32]) == WITHDRAW_SIG) { diff --git a/test/integration/child/ChildAxelarBridge.t.sol b/test/integration/child/ChildAxelarBridge.t.sol index c73f41a6..cad11ba8 100644 --- a/test/integration/child/ChildAxelarBridge.t.sol +++ b/test/integration/child/ChildAxelarBridge.t.sol @@ -109,7 +109,7 @@ contract ChildERC20BridgeIntegrationTest is Test, IChildERC20BridgeEvents, IChil bytes32 commandId = bytes32("testCommandId"); bytes memory payload = ""; - vm.expectRevert(InvalidData.selector); + vm.expectRevert(DataTooShort.selector); 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 index 70b0d140..319dc104 100644 --- 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(DataTooShort.selector); axelarAdaptor.execute(commandId, CHILD_CHAIN_NAME, sourceAddress, data); } diff --git a/test/unit/child/ChildERC20Bridge.t.sol b/test/unit/child/ChildERC20Bridge.t.sol index 7bb37a26..95d72974 100644 --- a/test/unit/child/ChildERC20Bridge.t.sol +++ b/test/unit/child/ChildERC20Bridge.t.sol @@ -168,7 +168,7 @@ contract ChildERC20BridgeUnitTest is Test, IChildERC20BridgeEvents, IChildERC20B function test_RevertIf_onMessageReceiveCalledWithDataLengthZero() public { bytes memory data = ""; - vm.expectRevert(InvalidData.selector); + vm.expectRevert(DataTooShort.selector); 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 index 5ea3c0e4..9fc1813d 100644 --- a/test/unit/root/withdrawals/RootERC20BridgeWithdraw.t.sol +++ b/test/unit/root/withdrawals/RootERC20BridgeWithdraw.t.sol @@ -84,7 +84,7 @@ contract RootERC20BridgeWithdrawUnitTest is Test, IRootERC20BridgeEvents, IRootE bytes memory data; vm.prank(address(mockAxelarAdaptor)); - vm.expectRevert(InvalidData.selector); + vm.expectRevert(DataTooShort.selector); rootBridge.onMessageReceive(CHILD_CHAIN_NAME, CHILD_BRIDGE_ADAPTOR_STRING, data); } From 2759eddcebb2d7f2186683872fb9a3540a63a778 Mon Sep 17 00:00:00 2001 From: Benjamin Patch Date: Tue, 7 Nov 2023 09:04:26 +1100 Subject: [PATCH 3/3] InvalidData with string reason --- src/child/ChildERC20Bridge.sol | 4 ++-- src/interfaces/child/IChildERC20Bridge.sol | 4 +--- src/interfaces/root/IRootERC20Bridge.sol | 4 +--- src/root/RootERC20Bridge.sol | 4 ++-- test/integration/child/ChildAxelarBridge.t.sol | 4 ++-- .../root/withdrawals.t.sol/RootERC20BridgeWithdraw.t.sol | 4 ++-- test/unit/child/ChildERC20Bridge.t.sol | 4 ++-- test/unit/root/withdrawals/RootERC20BridgeWithdraw.t.sol | 6 ++++-- 8 files changed, 16 insertions(+), 18 deletions(-) diff --git a/src/child/ChildERC20Bridge.sol b/src/child/ChildERC20Bridge.sol index 9037215d..d4eae07a 100644 --- a/src/child/ChildERC20Bridge.sol +++ b/src/child/ChildERC20Bridge.sol @@ -115,7 +115,7 @@ contract ChildERC20Bridge is 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 DataTooShort(); + revert InvalidData("Data too short"); } if (bytes32(data[:32]) == MAP_TOKEN_SIG) { @@ -123,7 +123,7 @@ contract ChildERC20Bridge is } 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 index 62d7cfe7..aaf4195d 100644 --- a/src/interfaces/child/IChildERC20Bridge.sol +++ b/src/interfaces/child/IChildERC20Bridge.sol @@ -72,10 +72,8 @@ interface IChildERC20BridgeErrors { error AlreadyMapped(); /// @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 too short. - error DataTooShort(); /// @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 index a0dd34dd..51d235d0 100644 --- a/src/interfaces/root/IRootERC20Bridge.sol +++ b/src/interfaces/root/IRootERC20Bridge.sol @@ -105,10 +105,8 @@ interface IRootERC20BridgeErrors { error BalanceInvariantCheckFailed(uint256 actualBalance, uint256 expectedBalance); /// @notice Error when the given child chain bridge adaptor is invalid. error InvalidChildERC20BridgeAdaptor(); - /// @notice Error when the message's payload is too short. - error DataTooShort(); /// @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 index b67a3ddd..266777a8 100644 --- a/src/root/RootERC20Bridge.sol +++ b/src/root/RootERC20Bridge.sol @@ -167,13 +167,13 @@ contract RootERC20Bridge is 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 DataTooShort(); + 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 index cad11ba8..bf8e0da2 100644 --- 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(DataTooShort.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 index 319dc104..bedf6d97 100644 --- 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(DataTooShort.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 index 95d72974..72694bca 100644 --- a/test/unit/child/ChildERC20Bridge.t.sol +++ b/test/unit/child/ChildERC20Bridge.t.sol @@ -168,7 +168,7 @@ contract ChildERC20BridgeUnitTest is Test, IChildERC20BridgeEvents, IChildERC20B function test_RevertIf_onMessageReceiveCalledWithDataLengthZero() public { bytes memory data = ""; - vm.expectRevert(DataTooShort.selector); + vm.expectRevert(abi.encodeWithSelector(InvalidData.selector, "Data too short")); childBridge.onMessageReceive(ROOT_CHAIN_NAME, ROOT_BRIDGE_ADAPTOR, data); } @@ -176,7 +176,7 @@ contract ChildERC20BridgeUnitTest is Test, IChildERC20BridgeEvents, IChildERC20B 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 index 9fc1813d..be1999cd 100644 --- 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(DataTooShort.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); }