From 1560f5cd13b50460f46dbe33d85acc1e86e23d6d Mon Sep 17 00:00:00 2001 From: -f Date: Thu, 5 Sep 2024 18:50:41 +0530 Subject: [PATCH 01/47] add fix --- solidity/contracts/isms/hook/OPL2ToL1Ism.sol | 4 ++-- solidity/contracts/mock/MockOptimism.sol | 2 ++ solidity/test/isms/OPL2ToL1Ism.t.sol | 13 +++++++++++++ 3 files changed, 17 insertions(+), 2 deletions(-) diff --git a/solidity/contracts/isms/hook/OPL2ToL1Ism.sol b/solidity/contracts/isms/hook/OPL2ToL1Ism.sol index b333b15cde..ac7071d0cf 100644 --- a/solidity/contracts/isms/hook/OPL2ToL1Ism.sol +++ b/solidity/contracts/isms/hook/OPL2ToL1Ism.sol @@ -66,10 +66,10 @@ contract OPL2ToL1Ism is bytes calldata metadata, bytes calldata message ) external override returns (bool) { - bool verified = isVerified(message); - if (!verified) { + if (!isVerified(message)) { _verifyWithPortalCall(metadata, message); } + require(isVerified(message), "OPL2ToL1Ism: message not verified"); releaseValueToRecipient(message); return true; } diff --git a/solidity/contracts/mock/MockOptimism.sol b/solidity/contracts/mock/MockOptimism.sol index f47a82bf2a..3ac017f1ad 100644 --- a/solidity/contracts/mock/MockOptimism.sol +++ b/solidity/contracts/mock/MockOptimism.sol @@ -58,4 +58,6 @@ contract MockOptimismPortal is IOptimismPortal { ); CallLib.call(call); } + + function verifyMessageId(bytes32 messageId) public payable {} } diff --git a/solidity/test/isms/OPL2ToL1Ism.t.sol b/solidity/test/isms/OPL2ToL1Ism.t.sol index 9466a7c937..cc878535c6 100644 --- a/solidity/test/isms/OPL2ToL1Ism.t.sol +++ b/solidity/test/isms/OPL2ToL1Ism.t.sol @@ -163,6 +163,19 @@ contract OPL2ToL1IsmTest is Test { ism.verify(encodedWithdrawalTx, encodedMessage); } + function test_verify_revertWhen_arbitraryCall() public { + deployAll(); + + bytes memory incorrectMetadata = _encodeFinalizeWithdrawalTx( + address(portal), + 0, + messageId + ); + + vm.expectRevert("OPL2ToL1Ism: message not verified"); + ism.verify(incorrectMetadata, encodedMessage); + } + function test_verify_statefulVerify() public { deployAll(); From 44379e1d68233f5fb6af59b15c8b131005bc2cf7 Mon Sep 17 00:00:00 2001 From: -f Date: Fri, 13 Sep 2024 15:40:27 +0530 Subject: [PATCH 02/47] move to inside --- solidity/contracts/isms/hook/OPL2ToL1Ism.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/solidity/contracts/isms/hook/OPL2ToL1Ism.sol b/solidity/contracts/isms/hook/OPL2ToL1Ism.sol index ac7071d0cf..ef39868611 100644 --- a/solidity/contracts/isms/hook/OPL2ToL1Ism.sol +++ b/solidity/contracts/isms/hook/OPL2ToL1Ism.sol @@ -68,8 +68,8 @@ contract OPL2ToL1Ism is ) external override returns (bool) { if (!isVerified(message)) { _verifyWithPortalCall(metadata, message); + require(isVerified(message), "OPL2ToL1Ism: message not verified"); } - require(isVerified(message), "OPL2ToL1Ism: message not verified"); releaseValueToRecipient(message); return true; } From d680e50d49118885434d9b3c129120d54389c0c3 Mon Sep 17 00:00:00 2001 From: -f Date: Mon, 16 Sep 2024 17:05:38 +0530 Subject: [PATCH 03/47] opl2tol1 --- solidity/test/isms/ExternalBridgeTest.sol | 107 ++++++++++++++++++++ solidity/test/isms/OPL2ToL1Ism.t.sol | 115 ++++------------------ 2 files changed, 125 insertions(+), 97 deletions(-) create mode 100644 solidity/test/isms/ExternalBridgeTest.sol diff --git a/solidity/test/isms/ExternalBridgeTest.sol b/solidity/test/isms/ExternalBridgeTest.sol new file mode 100644 index 0000000000..4155dcb510 --- /dev/null +++ b/solidity/test/isms/ExternalBridgeTest.sol @@ -0,0 +1,107 @@ +// SPDX-License-Identifier: MIT or Apache-2.0 +pragma solidity ^0.8.13; + +import {Test} from "forge-std/Test.sol"; +import {TypeCasts} from "../../contracts/libs/TypeCasts.sol"; +import {StandardHookMetadata} from "../../contracts/hooks/libs/StandardHookMetadata.sol"; +import {TestMailbox} from "../../contracts/test/TestMailbox.sol"; +import {Message} from "../../contracts/libs/Message.sol"; +import {MessageUtils} from "./IsmTestUtils.sol"; +import {TestRecipient} from "../../contracts/test/TestRecipient.sol"; +import {AbstractMessageIdAuthorizedIsm} from "../../contracts/isms/hook/AbstractMessageIdAuthorizedIsm.sol"; +import {AbstractMessageIdAuthHook} from "../../contracts/hooks/libs/AbstractMessageIdAuthHook.sol"; + +abstract contract ExternalBridgeTest is Test { + using TypeCasts for address; + using MessageUtils for bytes; + + uint8 internal constant HYPERLANE_VERSION = 1; + uint32 internal ORIGIN_DOMAIN; + uint32 internal DESTINATION_DOMAIN; + uint256 internal GAS_QUOTE; + TestMailbox internal originMailbox; + TestMailbox internal destinationMailbox; + TestRecipient internal testRecipient; + + AbstractMessageIdAuthHook internal hook; + + bytes internal testMessage = + abi.encodePacked("Hello from the other chain!"); + bytes internal testMetadata = + StandardHookMetadata.overrideRefundAddress(address(this)); + bytes internal encodedMessage; + bytes32 internal messageId; + + function setUp() public virtual { + testRecipient = new TestRecipient(); + encodedMessage = _encodeTestMessage(); + messageId = Message.id(encodedMessage); + } + + function _encodeTestMessage() internal view returns (bytes memory) { + return + MessageUtils.formatMessage( + HYPERLANE_VERSION, + uint32(0), + ORIGIN_DOMAIN, + TypeCasts.addressToBytes32(address(this)), + DESTINATION_DOMAIN, + TypeCasts.addressToBytes32(address(testRecipient)), + testMessage + ); + } + + function _encodeHookData( + bytes32 _messageId + ) internal pure returns (bytes memory) { + return + abi.encodeCall( + AbstractMessageIdAuthorizedIsm.verifyMessageId, + (_messageId) + ); + } + + function testFork_postDispatch_revertWhen_chainIDNotSupported() public { + bytes memory message = MessageUtils.formatMessage( + 0, + uint32(0), + DESTINATION_DOMAIN, + TypeCasts.addressToBytes32(address(this)), + 2, // wrong domain + TypeCasts.addressToBytes32(address(testRecipient)), + testMessage + ); + + destinationMailbox.updateLatestDispatchedId(Message.id(message)); + vm.expectRevert( + "AbstractMessageIdAuthHook: invalid destination domain" + ); + hook.postDispatch(testMetadata, message); + } + + function test_postDispatch_revertWhen_notLastDispatchedMessage() public { + vm.expectRevert( + "AbstractMessageIdAuthHook: message not latest dispatched" + ); + hook.postDispatch(testMetadata, encodedMessage); + } + + function test_postDispatch() public { + bytes memory encodedHookData = _encodeHookData(messageId); + destinationMailbox.updateLatestDispatchedId(messageId); + _expectOriginBridgeCall(encodedHookData); + + hook.postDispatch{value: GAS_QUOTE}(testMetadata, encodedMessage); + } + + function _expectOriginBridgeCall( + bytes memory _encodedHookData + ) internal virtual; + + // function testPostDispatch_RevertWhen_ChainIDNotSupported() public virtual; + // function testPostDispatch_RevertWhen_NotLastDispatchedMessage() public virtual; + // function testVerify_WithValue(uint256 _msgValue) public virtual; + // function testVerify_RevertWhen_InvalidMessage() public virtual; + + // Add more common test cases as needed +} diff --git a/solidity/test/isms/OPL2ToL1Ism.t.sol b/solidity/test/isms/OPL2ToL1Ism.t.sol index 9466a7c937..81758f9518 100644 --- a/solidity/test/isms/OPL2ToL1Ism.t.sol +++ b/solidity/test/isms/OPL2ToL1Ism.t.sol @@ -15,57 +15,45 @@ import {TestRecipient} from "../../contracts/test/TestRecipient.sol"; import {MockOptimismMessenger, MockOptimismPortal} from "../../contracts/mock/MockOptimism.sol"; import {OPL2ToL1Hook} from "../../contracts/hooks/OPL2ToL1Hook.sol"; import {OPL2ToL1Ism} from "../../contracts/isms/hook/OPL2ToL1Ism.sol"; +import {ExternalBridgeTest} from "./ExternalBridgeTest.sol"; -contract OPL2ToL1IsmTest is Test { - uint8 internal constant HYPERLANE_VERSION = 1; - uint32 internal constant MAINNET_DOMAIN = 1; - uint32 internal constant OPTIMISM_DOMAIN = 10; - uint32 internal constant GAS_QUOTE = 120_000; - +contract OPL2ToL1IsmTest is ExternalBridgeTest { address internal constant L2_MESSENGER_ADDRESS = 0x4200000000000000000000000000000000000007; uint256 internal constant MOCK_NONCE = 0; - TestMailbox public l2Mailbox; - TestRecipient internal testRecipient; - bytes internal testMessage = - abi.encodePacked("Hello from the other chain!"); - bytes internal encodedMessage; - bytes internal testMetadata = - StandardHookMetadata.overrideRefundAddress(address(this)); - bytes32 internal messageId; - MockOptimismPortal internal portal; MockOptimismMessenger internal l1Messenger; - OPL2ToL1Hook public hook; OPL2ToL1Ism public ism; /////////////////////////////////////////////////////////////////// /// SETUP /// /////////////////////////////////////////////////////////////////// - function setUp() public { + function setUp() public override { + ORIGIN_DOMAIN = 10; + DESTINATION_DOMAIN = 1; + GAS_QUOTE = 120_000; + super.setUp(); + // Optimism messenger mock setup vm.etch( L2_MESSENGER_ADDRESS, address(new MockOptimismMessenger()).code ); - testRecipient = new TestRecipient(); - - encodedMessage = _encodeTestMessage(); - messageId = Message.id(encodedMessage); + deployAll(); } function deployHook() public { - l2Mailbox = new TestMailbox(OPTIMISM_DOMAIN); + destinationMailbox = new TestMailbox(DESTINATION_DOMAIN); hook = new OPL2ToL1Hook( - address(l2Mailbox), - MAINNET_DOMAIN, + address(destinationMailbox), + DESTINATION_DOMAIN, TypeCasts.addressToBytes32(address(ism)), L2_MESSENGER_ADDRESS, - GAS_QUOTE + uint32(GAS_QUOTE) ); } @@ -85,59 +73,19 @@ contract OPL2ToL1IsmTest is Test { ism.setAuthorizedHook(TypeCasts.addressToBytes32(address(hook))); } - function test_postDispatch() public { - deployAll(); - - bytes memory encodedHookData = abi.encodeCall( - AbstractMessageIdAuthorizedIsm.verifyMessageId, - (messageId) - ); - - l2Mailbox.updateLatestDispatchedId(messageId); - + function _expectOriginBridgeCall( + bytes memory _encodedHookData + ) internal override { vm.expectCall( L2_MESSENGER_ADDRESS, abi.encodeCall( ICrossDomainMessenger.sendMessage, - (address(ism), encodedHookData, GAS_QUOTE) + (address(ism), _encodedHookData, uint32(GAS_QUOTE)) ) ); - - hook.postDispatch{value: GAS_QUOTE}(testMetadata, encodedMessage); - } - - function testFork_postDispatch_revertWhen_chainIDNotSupported() public { - deployAll(); - - bytes memory message = MessageUtils.formatMessage( - 0, - uint32(0), - OPTIMISM_DOMAIN, - TypeCasts.addressToBytes32(address(this)), - 2, // wrong domain - TypeCasts.addressToBytes32(address(testRecipient)), - testMessage - ); - - l2Mailbox.updateLatestDispatchedId(Message.id(message)); - vm.expectRevert( - "AbstractMessageIdAuthHook: invalid destination domain" - ); - hook.postDispatch(testMetadata, message); - } - - function test_postDispatch_revertWhen_notLastDispatchedMessage() public { - deployAll(); - - vm.expectRevert( - "AbstractMessageIdAuthHook: message not latest dispatched" - ); - hook.postDispatch(testMetadata, encodedMessage); } function test_verify_directWithdrawalCall() public { - deployAll(); - bytes memory encodedWithdrawalTx = _encodeFinalizeWithdrawalTx( address(ism), 0, @@ -150,7 +98,6 @@ contract OPL2ToL1IsmTest is Test { function test_verify_directWithdrawalCall_revertsWhen_invalidSender() public { - deployAll(); l1Messenger.setXDomainMessageSender(address(this)); bytes memory encodedWithdrawalTx = _encodeFinalizeWithdrawalTx( @@ -164,8 +111,6 @@ contract OPL2ToL1IsmTest is Test { } function test_verify_statefulVerify() public { - deployAll(); - vm.deal(address(portal), 1 ether); IOptimismPortal.WithdrawalTransaction memory withdrawal = IOptimismPortal.WithdrawalTransaction({ @@ -184,8 +129,6 @@ contract OPL2ToL1IsmTest is Test { } function test_verify_statefulAndDirectWithdrawal() public { - deployAll(); - IOptimismPortal.WithdrawalTransaction memory withdrawal = IOptimismPortal.WithdrawalTransaction({ nonce: MOCK_NONCE, @@ -208,15 +151,11 @@ contract OPL2ToL1IsmTest is Test { } function test_verify_revertsWhen_noStatefulAndDirectWithdrawal() public { - deployAll(); - vm.expectRevert(); ism.verify(new bytes(0), encodedMessage); } function test_verify_revertsWhen_invalidIsm() public { - deployAll(); - bytes memory encodedWithdrawalTx = _encodeFinalizeWithdrawalTx( address(this), 0, @@ -228,8 +167,6 @@ contract OPL2ToL1IsmTest is Test { } function test_verify_revertsWhen_incorrectMessageId() public { - deployAll(); - bytes32 incorrectMessageId = keccak256("incorrect message id"); bytes memory encodedWithdrawalTx = _encodeFinalizeWithdrawalTx( @@ -265,28 +202,12 @@ contract OPL2ToL1IsmTest is Test { /* ============ helper functions ============ */ - function _encodeTestMessage() internal view returns (bytes memory) { - return - MessageUtils.formatMessage( - HYPERLANE_VERSION, - uint32(0), - OPTIMISM_DOMAIN, - TypeCasts.addressToBytes32(address(this)), - MAINNET_DOMAIN, - TypeCasts.addressToBytes32(address(testRecipient)), - testMessage - ); - } - function _encodeMessengerCalldata( address _ism, uint256 _value, bytes32 _messageId ) internal view returns (bytes memory) { - bytes memory encodedHookData = abi.encodeCall( - AbstractMessageIdAuthorizedIsm.verifyMessageId, - (_messageId) - ); + bytes memory encodedHookData = _encodeHookData(_messageId); return abi.encodeCall( From ca26e88232320b42785fcf3d2573df1d11c10b32 Mon Sep 17 00:00:00 2001 From: -f Date: Mon, 16 Sep 2024 17:59:46 +0530 Subject: [PATCH 04/47] arb --- solidity/test/isms/ArbL2ToL1Ism.t.sol | 134 +++++----------------- solidity/test/isms/ExternalBridgeTest.sol | 21 +++- solidity/test/isms/OPL2ToL1Ism.t.sol | 20 +++- 3 files changed, 64 insertions(+), 111 deletions(-) diff --git a/solidity/test/isms/ArbL2ToL1Ism.t.sol b/solidity/test/isms/ArbL2ToL1Ism.t.sol index f34c1a74e7..dc89ae6353 100644 --- a/solidity/test/isms/ArbL2ToL1Ism.t.sol +++ b/solidity/test/isms/ArbL2ToL1Ism.t.sol @@ -13,13 +13,9 @@ import {ArbL2ToL1Hook} from "../../contracts/hooks/ArbL2ToL1Hook.sol"; import {ArbL2ToL1Ism} from "../../contracts/isms/hook/ArbL2ToL1Ism.sol"; import {MockArbBridge, MockArbSys} from "../../contracts/mock/MockArbBridge.sol"; import {TestRecipient} from "../../contracts/test/TestRecipient.sol"; +import {ExternalBridgeTest} from "./ExternalBridgeTest.sol"; -contract ArbL2ToL1IsmTest is Test { - uint8 internal constant HYPERLANE_VERSION = 1; - uint32 internal constant MAINNET_DOMAIN = 1; - uint32 internal constant ARBITRUM_DOMAIN = 42161; - uint256 internal constant GAS_QUOTE = 120_000; - +contract ArbL2ToL1IsmTest is ExternalBridgeTest { uint256 internal constant MOCK_LEAF_INDEX = 40160; uint256 internal constant MOCK_L2_BLOCK = 54220000; uint256 internal constant MOCK_L1_BLOCK = 6098300; @@ -28,26 +24,17 @@ contract ArbL2ToL1IsmTest is Test { 0x0000000000000000000000000000000000000064; MockArbBridge internal arbBridge; - TestMailbox public l2Mailbox; - ArbL2ToL1Hook public hook; - ArbL2ToL1Ism public ism; - - TestRecipient internal testRecipient; - bytes internal testMessage = - abi.encodePacked("Hello from the other chain!"); - bytes internal encodedMessage; - bytes internal testMetadata = - StandardHookMetadata.overrideRefundAddress(address(this)); - bytes32 internal messageId; - - function setUp() public { + + function setUp() public override { + ORIGIN_DOMAIN = 42161; + DESTINATION_DOMAIN = 1; + GAS_QUOTE = 120_000; + super.setUp(); + // Arbitrum bridge mock setup vm.etch(L2_ARBSYS_ADDRESS, address(new MockArbSys()).code); - testRecipient = new TestRecipient(); - - encodedMessage = _encodeTestMessage(); - messageId = Message.id(encodedMessage); + deployAll(); } /////////////////////////////////////////////////////////////////// @@ -55,10 +42,10 @@ contract ArbL2ToL1IsmTest is Test { /////////////////////////////////////////////////////////////////// function deployHook() public { - l2Mailbox = new TestMailbox(ARBITRUM_DOMAIN); + originMailbox = new TestMailbox(ORIGIN_DOMAIN); hook = new ArbL2ToL1Hook( - address(l2Mailbox), - MAINNET_DOMAIN, + address(originMailbox), + DESTINATION_DOMAIN, TypeCasts.addressToBytes32(address(ism)), L2_ARBSYS_ADDRESS, GAS_QUOTE @@ -78,58 +65,19 @@ contract ArbL2ToL1IsmTest is Test { ism.setAuthorizedHook(TypeCasts.addressToBytes32(address(hook))); } - function test_postDispatch() public { - deployAll(); - - bytes memory encodedHookData = abi.encodeCall( - AbstractMessageIdAuthorizedIsm.verifyMessageId, - (messageId) - ); - - l2Mailbox.updateLatestDispatchedId(messageId); - + function _expectOriginBridgeCall( + bytes memory _encodedHookData + ) internal override { vm.expectCall( L2_ARBSYS_ADDRESS, abi.encodeCall( MockArbSys.sendTxToL1, - (address(ism), encodedHookData) + (address(ism), _encodedHookData) ) ); - hook.postDispatch(testMetadata, encodedMessage); - } - - function testFork_postDispatch_revertWhen_chainIDNotSupported() public { - deployAll(); - - bytes memory message = MessageUtils.formatMessage( - 0, - uint32(0), - ARBITRUM_DOMAIN, - TypeCasts.addressToBytes32(address(this)), - 2, // wrong domain - TypeCasts.addressToBytes32(address(testRecipient)), - testMessage - ); - - l2Mailbox.updateLatestDispatchedId(Message.id(message)); - vm.expectRevert( - "AbstractMessageIdAuthHook: invalid destination domain" - ); - hook.postDispatch(testMetadata, message); - } - - function test_postDispatch_revertWhen_notLastDispatchedMessage() public { - deployAll(); - - vm.expectRevert( - "AbstractMessageIdAuthHook: message not latest dispatched" - ); - hook.postDispatch(testMetadata, encodedMessage); } function test_verify_outboxCall() public { - deployAll(); - bytes memory encodedOutboxTxMetadata = _encodeOutboxTx( address(hook), address(ism), @@ -143,14 +91,9 @@ contract ArbL2ToL1IsmTest is Test { assertEq(address(testRecipient).balance, 1 ether); } - function test_verify_statefulVerify() public { - deployAll(); - - bytes memory encodedHookData = abi.encodeCall( - AbstractMessageIdAuthorizedIsm.verifyMessageId, - (messageId) - ); - + function _bridgeDestinationCall( + bytes memory _encodedHookData + ) internal override { arbBridge.setL2ToL1Sender(address(hook)); arbBridge.executeTransaction{value: 1 ether}( new bytes32[](0), @@ -161,17 +104,19 @@ contract ArbL2ToL1IsmTest is Test { MOCK_L1_BLOCK, block.timestamp, 1 ether, - encodedHookData + _encodedHookData ); - - vm.etch(address(arbBridge), new bytes(0)); // this is a way to test that the arbBridge isn't called again - assertTrue(ism.verify(new bytes(0), encodedMessage)); - assertEq(address(testRecipient).balance, 1 ether); } - function test_verify_statefulAndOutbox() public { - deployAll(); + // function test_verify_statefulVerify() public { + // bytes memory encodedHookData = abi.encodeCall(AbstractMessageIdAuthorizedIsm.verifyMessageId, (messageId)); + + // vm.etch(address(arbBridge), new bytes(0)); // this is a way to test that the arbBridge isn't called again + // assertTrue(ism.verify(new bytes(0), encodedMessage)); + // assertEq(address(testRecipient).balance, 1 ether); + // } + function test_verify_statefulAndOutbox() public { bytes memory encodedHookData = abi.encodeCall( AbstractMessageIdAuthorizedIsm.verifyMessageId, (messageId) @@ -203,15 +148,11 @@ contract ArbL2ToL1IsmTest is Test { } function test_verify_revertsWhen_noStatefulOrOutbox() public { - deployAll(); - vm.expectRevert(); ism.verify(new bytes(0), encodedMessage); } function test_verify_revertsWhen_notAuthorizedHook() public { - deployAll(); - bytes memory encodedOutboxTxMetadata = _encodeOutboxTx( address(this), address(ism), @@ -226,8 +167,6 @@ contract ArbL2ToL1IsmTest is Test { } function test_verify_revertsWhen_invalidIsm() public { - deployAll(); - bytes memory encodedOutboxTxMetadata = _encodeOutboxTx( address(hook), address(this), @@ -242,8 +181,6 @@ contract ArbL2ToL1IsmTest is Test { } function test_verify_revertsWhen_incorrectMessageId() public { - deployAll(); - bytes32 incorrectMessageId = keccak256("incorrect message id"); bytes memory encodedOutboxTxMetadata = _encodeOutboxTx( @@ -309,17 +246,4 @@ contract ArbL2ToL1IsmTest is Test { encodedHookData ); } - - function _encodeTestMessage() internal view returns (bytes memory) { - return - MessageUtils.formatMessage( - HYPERLANE_VERSION, - uint32(0), - ARBITRUM_DOMAIN, - TypeCasts.addressToBytes32(address(this)), - MAINNET_DOMAIN, - TypeCasts.addressToBytes32(address(testRecipient)), - testMessage - ); - } } diff --git a/solidity/test/isms/ExternalBridgeTest.sol b/solidity/test/isms/ExternalBridgeTest.sol index 4155dcb510..479f1926c5 100644 --- a/solidity/test/isms/ExternalBridgeTest.sol +++ b/solidity/test/isms/ExternalBridgeTest.sol @@ -24,6 +24,7 @@ abstract contract ExternalBridgeTest is Test { TestRecipient internal testRecipient; AbstractMessageIdAuthHook internal hook; + AbstractMessageIdAuthorizedIsm internal ism; bytes internal testMessage = abi.encodePacked("Hello from the other chain!"); @@ -61,7 +62,7 @@ abstract contract ExternalBridgeTest is Test { ); } - function testFork_postDispatch_revertWhen_chainIDNotSupported() public { + function test_postDispatch_revertWhen_chainIDNotSupported() public { bytes memory message = MessageUtils.formatMessage( 0, uint32(0), @@ -72,7 +73,7 @@ abstract contract ExternalBridgeTest is Test { testMessage ); - destinationMailbox.updateLatestDispatchedId(Message.id(message)); + originMailbox.updateLatestDispatchedId(Message.id(message)); vm.expectRevert( "AbstractMessageIdAuthHook: invalid destination domain" ); @@ -88,16 +89,30 @@ abstract contract ExternalBridgeTest is Test { function test_postDispatch() public { bytes memory encodedHookData = _encodeHookData(messageId); - destinationMailbox.updateLatestDispatchedId(messageId); + originMailbox.updateLatestDispatchedId(messageId); _expectOriginBridgeCall(encodedHookData); hook.postDispatch{value: GAS_QUOTE}(testMetadata, encodedMessage); } + function test_verifyMessageId_asyncCall() public { + bytes memory encodedHookData = abi.encodeCall( + AbstractMessageIdAuthorizedIsm.verifyMessageId, + (messageId) + ); + _bridgeDestinationCall(encodedHookData); + + assertTrue(ism.isVerified(encodedMessage)); + } + function _expectOriginBridgeCall( bytes memory _encodedHookData ) internal virtual; + function _bridgeDestinationCall( + bytes memory _encodedHookData + ) internal virtual; + // function testPostDispatch_RevertWhen_ChainIDNotSupported() public virtual; // function testPostDispatch_RevertWhen_NotLastDispatchedMessage() public virtual; // function testVerify_WithValue(uint256 _msgValue) public virtual; diff --git a/solidity/test/isms/OPL2ToL1Ism.t.sol b/solidity/test/isms/OPL2ToL1Ism.t.sol index 81758f9518..04a5369cfe 100644 --- a/solidity/test/isms/OPL2ToL1Ism.t.sol +++ b/solidity/test/isms/OPL2ToL1Ism.t.sol @@ -25,7 +25,6 @@ contract OPL2ToL1IsmTest is ExternalBridgeTest { MockOptimismPortal internal portal; MockOptimismMessenger internal l1Messenger; - OPL2ToL1Ism public ism; /////////////////////////////////////////////////////////////////// /// SETUP /// @@ -47,9 +46,9 @@ contract OPL2ToL1IsmTest is ExternalBridgeTest { } function deployHook() public { - destinationMailbox = new TestMailbox(DESTINATION_DOMAIN); + originMailbox = new TestMailbox(ORIGIN_DOMAIN); hook = new OPL2ToL1Hook( - address(destinationMailbox), + address(originMailbox), DESTINATION_DOMAIN, TypeCasts.addressToBytes32(address(ism)), L2_MESSENGER_ADDRESS, @@ -202,6 +201,21 @@ contract OPL2ToL1IsmTest is ExternalBridgeTest { /* ============ helper functions ============ */ + function _bridgeDestinationCall( + bytes memory _encodedHookData + ) internal override { + IOptimismPortal.WithdrawalTransaction + memory withdrawal = IOptimismPortal.WithdrawalTransaction({ + nonce: MOCK_NONCE, + sender: L2_MESSENGER_ADDRESS, + target: address(l1Messenger), + value: 0, + gasLimit: uint256(GAS_QUOTE), + data: _encodeMessengerCalldata(address(ism), 0, messageId) + }); + portal.finalizeWithdrawalTransaction(withdrawal); + } + function _encodeMessengerCalldata( address _ism, uint256 _value, From a5c4b623a4f3dcee63224b72cdc318e1fa5329b2 Mon Sep 17 00:00:00 2001 From: -f Date: Mon, 16 Sep 2024 18:02:59 +0530 Subject: [PATCH 05/47] rm revertWhen_invalidMetadata --- solidity/test/isms/ArbL2ToL1Ism.t.sol | 5 ----- solidity/test/isms/ExternalBridgeTest.sol | 5 +++++ solidity/test/isms/OPL2ToL1Ism.t.sol | 5 ----- 3 files changed, 5 insertions(+), 10 deletions(-) diff --git a/solidity/test/isms/ArbL2ToL1Ism.t.sol b/solidity/test/isms/ArbL2ToL1Ism.t.sol index dc89ae6353..ada4971f7f 100644 --- a/solidity/test/isms/ArbL2ToL1Ism.t.sol +++ b/solidity/test/isms/ArbL2ToL1Ism.t.sol @@ -147,11 +147,6 @@ contract ArbL2ToL1IsmTest is ExternalBridgeTest { assertEq(address(testRecipient).balance, 1 ether); } - function test_verify_revertsWhen_noStatefulOrOutbox() public { - vm.expectRevert(); - ism.verify(new bytes(0), encodedMessage); - } - function test_verify_revertsWhen_notAuthorizedHook() public { bytes memory encodedOutboxTxMetadata = _encodeOutboxTx( address(this), diff --git a/solidity/test/isms/ExternalBridgeTest.sol b/solidity/test/isms/ExternalBridgeTest.sol index 479f1926c5..30a840fb2f 100644 --- a/solidity/test/isms/ExternalBridgeTest.sol +++ b/solidity/test/isms/ExternalBridgeTest.sol @@ -105,6 +105,11 @@ abstract contract ExternalBridgeTest is Test { assertTrue(ism.isVerified(encodedMessage)); } + function test_verify_revertWhen_invalidMetadata() public { + vm.expectRevert(); + ism.verify(new bytes(0), encodedMessage); + } + function _expectOriginBridgeCall( bytes memory _encodedHookData ) internal virtual; diff --git a/solidity/test/isms/OPL2ToL1Ism.t.sol b/solidity/test/isms/OPL2ToL1Ism.t.sol index 04a5369cfe..b16d513b6d 100644 --- a/solidity/test/isms/OPL2ToL1Ism.t.sol +++ b/solidity/test/isms/OPL2ToL1Ism.t.sol @@ -149,11 +149,6 @@ contract OPL2ToL1IsmTest is ExternalBridgeTest { assertTrue(ism.verify(encodedWithdrawalTx, encodedMessage)); } - function test_verify_revertsWhen_noStatefulAndDirectWithdrawal() public { - vm.expectRevert(); - ism.verify(new bytes(0), encodedMessage); - } - function test_verify_revertsWhen_invalidIsm() public { bytes memory encodedWithdrawalTx = _encodeFinalizeWithdrawalTx( address(this), From 54b432769f9634b7adf275159f8d8bc020e3b55a Mon Sep 17 00:00:00 2001 From: -f Date: Mon, 16 Sep 2024 23:14:44 +0530 Subject: [PATCH 06/47] msg_value --- solidity/test/isms/ArbL2ToL1Ism.t.sol | 15 ++----- solidity/test/isms/ExternalBridgeTest.sol | 15 ++++++- solidity/test/isms/OPL2ToL1Ism.t.sol | 54 +++++++++-------------- 3 files changed, 38 insertions(+), 46 deletions(-) diff --git a/solidity/test/isms/ArbL2ToL1Ism.t.sol b/solidity/test/isms/ArbL2ToL1Ism.t.sol index ada4971f7f..0754852ce6 100644 --- a/solidity/test/isms/ArbL2ToL1Ism.t.sol +++ b/solidity/test/isms/ArbL2ToL1Ism.t.sol @@ -92,10 +92,11 @@ contract ArbL2ToL1IsmTest is ExternalBridgeTest { } function _bridgeDestinationCall( - bytes memory _encodedHookData + bytes memory _encodedHookData, + uint256 _msgValue ) internal override { arbBridge.setL2ToL1Sender(address(hook)); - arbBridge.executeTransaction{value: 1 ether}( + arbBridge.executeTransaction{value: _msgValue}( new bytes32[](0), MOCK_LEAF_INDEX, address(hook), @@ -103,19 +104,11 @@ contract ArbL2ToL1IsmTest is ExternalBridgeTest { MOCK_L2_BLOCK, MOCK_L1_BLOCK, block.timestamp, - 1 ether, + _msgValue, _encodedHookData ); } - // function test_verify_statefulVerify() public { - // bytes memory encodedHookData = abi.encodeCall(AbstractMessageIdAuthorizedIsm.verifyMessageId, (messageId)); - - // vm.etch(address(arbBridge), new bytes(0)); // this is a way to test that the arbBridge isn't called again - // assertTrue(ism.verify(new bytes(0), encodedMessage)); - // assertEq(address(testRecipient).balance, 1 ether); - // } - function test_verify_statefulAndOutbox() public { bytes memory encodedHookData = abi.encodeCall( AbstractMessageIdAuthorizedIsm.verifyMessageId, diff --git a/solidity/test/isms/ExternalBridgeTest.sol b/solidity/test/isms/ExternalBridgeTest.sol index 30a840fb2f..5b16df7f25 100644 --- a/solidity/test/isms/ExternalBridgeTest.sol +++ b/solidity/test/isms/ExternalBridgeTest.sol @@ -100,7 +100,7 @@ abstract contract ExternalBridgeTest is Test { AbstractMessageIdAuthorizedIsm.verifyMessageId, (messageId) ); - _bridgeDestinationCall(encodedHookData); + _bridgeDestinationCall(encodedHookData, 0); assertTrue(ism.isVerified(encodedMessage)); } @@ -110,14 +110,25 @@ abstract contract ExternalBridgeTest is Test { ism.verify(new bytes(0), encodedMessage); } + function test_verify_msgValueWithAsyncCall() public { + bytes memory encodedHookData = _encodeHookData(messageId); + _bridgeDestinationCall(encodedHookData, 1 ether); + + assertTrue(ism.verify(new bytes(0), encodedMessage)); + assertEq(address(testRecipient).balance, 1 ether); + } + function _expectOriginBridgeCall( bytes memory _encodedHookData ) internal virtual; function _bridgeDestinationCall( - bytes memory _encodedHookData + bytes memory _encodedHookData, + uint256 _msgValue ) internal virtual; + // function _encodeBridgeCall(address _to, uint256 _msgValue, bytes memory _data) internal view returns (bytes memory) + // function testPostDispatch_RevertWhen_ChainIDNotSupported() public virtual; // function testPostDispatch_RevertWhen_NotLastDispatchedMessage() public virtual; // function testVerify_WithValue(uint256 _msgValue) public virtual; diff --git a/solidity/test/isms/OPL2ToL1Ism.t.sol b/solidity/test/isms/OPL2ToL1Ism.t.sol index b16d513b6d..13dc349fb4 100644 --- a/solidity/test/isms/OPL2ToL1Ism.t.sol +++ b/solidity/test/isms/OPL2ToL1Ism.t.sol @@ -72,18 +72,6 @@ contract OPL2ToL1IsmTest is ExternalBridgeTest { ism.setAuthorizedHook(TypeCasts.addressToBytes32(address(hook))); } - function _expectOriginBridgeCall( - bytes memory _encodedHookData - ) internal override { - vm.expectCall( - L2_MESSENGER_ADDRESS, - abi.encodeCall( - ICrossDomainMessenger.sendMessage, - (address(ism), _encodedHookData, uint32(GAS_QUOTE)) - ) - ); - } - function test_verify_directWithdrawalCall() public { bytes memory encodedWithdrawalTx = _encodeFinalizeWithdrawalTx( address(ism), @@ -109,24 +97,6 @@ contract OPL2ToL1IsmTest is ExternalBridgeTest { ism.verify(encodedWithdrawalTx, encodedMessage); } - function test_verify_statefulVerify() public { - vm.deal(address(portal), 1 ether); - IOptimismPortal.WithdrawalTransaction - memory withdrawal = IOptimismPortal.WithdrawalTransaction({ - nonce: MOCK_NONCE, - sender: L2_MESSENGER_ADDRESS, - target: address(l1Messenger), - value: 1 ether, - gasLimit: uint256(GAS_QUOTE), - data: _encodeMessengerCalldata(address(ism), 1 ether, messageId) - }); - portal.finalizeWithdrawalTransaction(withdrawal); - - vm.etch(address(portal), new bytes(0)); // this is a way to test that the portal isn't called again - assertTrue(ism.verify(new bytes(0), encodedMessage)); - assertEq(address(testRecipient).balance, 1 ether); // testing msg.value - } - function test_verify_statefulAndDirectWithdrawal() public { IOptimismPortal.WithdrawalTransaction memory withdrawal = IOptimismPortal.WithdrawalTransaction({ @@ -196,17 +166,35 @@ contract OPL2ToL1IsmTest is ExternalBridgeTest { /* ============ helper functions ============ */ - function _bridgeDestinationCall( + function _expectOriginBridgeCall( bytes memory _encodedHookData ) internal override { + vm.expectCall( + L2_MESSENGER_ADDRESS, + abi.encodeCall( + ICrossDomainMessenger.sendMessage, + (address(ism), _encodedHookData, uint32(GAS_QUOTE)) + ) + ); + } + + function _bridgeDestinationCall( + bytes memory, + /*_encodedHookData*/ uint256 _msgValue + ) internal override { + vm.deal(address(portal), _msgValue); IOptimismPortal.WithdrawalTransaction memory withdrawal = IOptimismPortal.WithdrawalTransaction({ nonce: MOCK_NONCE, sender: L2_MESSENGER_ADDRESS, target: address(l1Messenger), - value: 0, + value: _msgValue, gasLimit: uint256(GAS_QUOTE), - data: _encodeMessengerCalldata(address(ism), 0, messageId) + data: _encodeMessengerCalldata( + address(ism), + _msgValue, + messageId + ) }); portal.finalizeWithdrawalTransaction(withdrawal); } From 66c3bb702824ce8efc9deceb276b5507921760aa Mon Sep 17 00:00:00 2001 From: -f Date: Tue, 17 Sep 2024 02:12:14 +0530 Subject: [PATCH 07/47] direct call --- solidity/test/isms/ArbL2ToL1Ism.t.sol | 23 +++++------ solidity/test/isms/ExternalBridgeTest.sol | 50 ++++++++++++++++------- solidity/test/isms/OPL2ToL1Ism.t.sol | 27 ++++++------ 3 files changed, 60 insertions(+), 40 deletions(-) diff --git a/solidity/test/isms/ArbL2ToL1Ism.t.sol b/solidity/test/isms/ArbL2ToL1Ism.t.sol index 0754852ce6..4b9174d2ac 100644 --- a/solidity/test/isms/ArbL2ToL1Ism.t.sol +++ b/solidity/test/isms/ArbL2ToL1Ism.t.sol @@ -65,7 +65,7 @@ contract ArbL2ToL1IsmTest is ExternalBridgeTest { ism.setAuthorizedHook(TypeCasts.addressToBytes32(address(hook))); } - function _expectOriginBridgeCall( + function _expectOriginExternalBridgeCall( bytes memory _encodedHookData ) internal override { vm.expectCall( @@ -77,21 +77,18 @@ contract ArbL2ToL1IsmTest is ExternalBridgeTest { ); } - function test_verify_outboxCall() public { - bytes memory encodedOutboxTxMetadata = _encodeOutboxTx( - address(hook), - address(ism), - messageId, - 1 ether - ); - - vm.deal(address(arbBridge), 1 ether); + function _encodeExternalDestinationBridgeCall( + address _from, + address _to, + uint256 _msgValue, + bytes32 _messageId + ) internal override returns (bytes memory) { + vm.deal(address(arbBridge), _msgValue); arbBridge.setL2ToL1Sender(address(hook)); - assertTrue(ism.verify(encodedOutboxTxMetadata, encodedMessage)); - assertEq(address(testRecipient).balance, 1 ether); + return _encodeOutboxTx(_from, _to, _messageId, _msgValue); } - function _bridgeDestinationCall( + function _externalBridgeDestinationCall( bytes memory _encodedHookData, uint256 _msgValue ) internal override { diff --git a/solidity/test/isms/ExternalBridgeTest.sol b/solidity/test/isms/ExternalBridgeTest.sol index 5b16df7f25..64ca11ba9d 100644 --- a/solidity/test/isms/ExternalBridgeTest.sol +++ b/solidity/test/isms/ExternalBridgeTest.sol @@ -90,7 +90,7 @@ abstract contract ExternalBridgeTest is Test { function test_postDispatch() public { bytes memory encodedHookData = _encodeHookData(messageId); originMailbox.updateLatestDispatchedId(messageId); - _expectOriginBridgeCall(encodedHookData); + _expectOriginExternalBridgeCall(encodedHookData); hook.postDispatch{value: GAS_QUOTE}(testMetadata, encodedMessage); } @@ -100,39 +100,61 @@ abstract contract ExternalBridgeTest is Test { AbstractMessageIdAuthorizedIsm.verifyMessageId, (messageId) ); - _bridgeDestinationCall(encodedHookData, 0); + _externalBridgeDestinationCall(encodedHookData, 0); assertTrue(ism.isVerified(encodedMessage)); } + function test_verifyMessageId_externalBridgeCall() public { + bytes memory externalCalldata = _encodeExternalDestinationBridgeCall( + address(hook), + address(ism), + 0, + messageId + ); + + ism.verify(externalCalldata, encodedMessage); + assertTrue(ism.isVerified(encodedMessage)); + } + function test_verify_revertWhen_invalidMetadata() public { vm.expectRevert(); ism.verify(new bytes(0), encodedMessage); } - function test_verify_msgValueWithAsyncCall() public { + function test_verify_msgValue_asyncCall() public { bytes memory encodedHookData = _encodeHookData(messageId); - _bridgeDestinationCall(encodedHookData, 1 ether); + _externalBridgeDestinationCall(encodedHookData, 1 ether); assertTrue(ism.verify(new bytes(0), encodedMessage)); assertEq(address(testRecipient).balance, 1 ether); } - function _expectOriginBridgeCall( + function test_verify_msgValue_externalBridgeCall() public { + bytes memory externalCalldata = _encodeExternalDestinationBridgeCall( + address(hook), + address(ism), + 1 ether, + messageId + ); + + ism.verify(externalCalldata, encodedMessage); + assertEq(address(testRecipient).balance, 1 ether); + } + + function _expectOriginExternalBridgeCall( bytes memory _encodedHookData ) internal virtual; - function _bridgeDestinationCall( + function _externalBridgeDestinationCall( bytes memory _encodedHookData, uint256 _msgValue ) internal virtual; - // function _encodeBridgeCall(address _to, uint256 _msgValue, bytes memory _data) internal view returns (bytes memory) - - // function testPostDispatch_RevertWhen_ChainIDNotSupported() public virtual; - // function testPostDispatch_RevertWhen_NotLastDispatchedMessage() public virtual; - // function testVerify_WithValue(uint256 _msgValue) public virtual; - // function testVerify_RevertWhen_InvalidMessage() public virtual; - - // Add more common test cases as needed + function _encodeExternalDestinationBridgeCall( + address _from, + address _to, + uint256 _msgValue, + bytes32 _messageId + ) internal virtual returns (bytes memory); } diff --git a/solidity/test/isms/OPL2ToL1Ism.t.sol b/solidity/test/isms/OPL2ToL1Ism.t.sol index 13dc349fb4..f180c83294 100644 --- a/solidity/test/isms/OPL2ToL1Ism.t.sol +++ b/solidity/test/isms/OPL2ToL1Ism.t.sol @@ -72,16 +72,6 @@ contract OPL2ToL1IsmTest is ExternalBridgeTest { ism.setAuthorizedHook(TypeCasts.addressToBytes32(address(hook))); } - function test_verify_directWithdrawalCall() public { - bytes memory encodedWithdrawalTx = _encodeFinalizeWithdrawalTx( - address(ism), - 0, - messageId - ); - - assertTrue(ism.verify(encodedWithdrawalTx, encodedMessage)); - } - function test_verify_directWithdrawalCall_revertsWhen_invalidSender() public { @@ -166,7 +156,7 @@ contract OPL2ToL1IsmTest is ExternalBridgeTest { /* ============ helper functions ============ */ - function _expectOriginBridgeCall( + function _expectOriginExternalBridgeCall( bytes memory _encodedHookData ) internal override { vm.expectCall( @@ -178,9 +168,20 @@ contract OPL2ToL1IsmTest is ExternalBridgeTest { ); } - function _bridgeDestinationCall( + function _encodeExternalDestinationBridgeCall( + address, + /*_from*/ address _to, + uint256 _msgValue, + bytes32 _messageId + ) internal override returns (bytes memory) { + vm.deal(address(portal), _msgValue); + return _encodeFinalizeWithdrawalTx(_to, _msgValue, _messageId); + } + + function _externalBridgeDestinationCall( bytes memory, - /*_encodedHookData*/ uint256 _msgValue + /*_encodedHookData*/ + uint256 _msgValue ) internal override { vm.deal(address(portal), _msgValue); IOptimismPortal.WithdrawalTransaction From e98382daac63c8195a90e115f9ff924b9c5cc9b8 Mon Sep 17 00:00:00 2001 From: -f Date: Tue, 17 Sep 2024 02:15:24 +0530 Subject: [PATCH 08/47] invalid ism --- solidity/test/isms/ArbL2ToL1Ism.t.sol | 14 -------------- solidity/test/isms/ExternalBridgeTest.sol | 12 ++++++++++++ solidity/test/isms/OPL2ToL1Ism.t.sol | 14 ++------------ 3 files changed, 14 insertions(+), 26 deletions(-) diff --git a/solidity/test/isms/ArbL2ToL1Ism.t.sol b/solidity/test/isms/ArbL2ToL1Ism.t.sol index 4b9174d2ac..1ae1c6a006 100644 --- a/solidity/test/isms/ArbL2ToL1Ism.t.sol +++ b/solidity/test/isms/ArbL2ToL1Ism.t.sol @@ -151,20 +151,6 @@ contract ArbL2ToL1IsmTest is ExternalBridgeTest { ism.verify(encodedOutboxTxMetadata, encodedMessage); } - function test_verify_revertsWhen_invalidIsm() public { - bytes memory encodedOutboxTxMetadata = _encodeOutboxTx( - address(hook), - address(this), - messageId, - 0 - ); - - arbBridge.setL2ToL1Sender(address(hook)); - - vm.expectRevert(); // BridgeCallFailed() - ism.verify(encodedOutboxTxMetadata, encodedMessage); - } - function test_verify_revertsWhen_incorrectMessageId() public { bytes32 incorrectMessageId = keccak256("incorrect message id"); diff --git a/solidity/test/isms/ExternalBridgeTest.sol b/solidity/test/isms/ExternalBridgeTest.sol index 64ca11ba9d..8500b56388 100644 --- a/solidity/test/isms/ExternalBridgeTest.sol +++ b/solidity/test/isms/ExternalBridgeTest.sol @@ -142,6 +142,18 @@ abstract contract ExternalBridgeTest is Test { assertEq(address(testRecipient).balance, 1 ether); } + function test_verify_revertsWhen_invalidIsm() public { + bytes memory externalCalldata = _encodeExternalDestinationBridgeCall( + address(hook), + address(this), + 0, + messageId + ); + + vm.expectRevert(); + ism.verify(externalCalldata, encodedMessage); + } + function _expectOriginExternalBridgeCall( bytes memory _encodedHookData ) internal virtual; diff --git a/solidity/test/isms/OPL2ToL1Ism.t.sol b/solidity/test/isms/OPL2ToL1Ism.t.sol index f180c83294..2c00cddb65 100644 --- a/solidity/test/isms/OPL2ToL1Ism.t.sol +++ b/solidity/test/isms/OPL2ToL1Ism.t.sol @@ -109,17 +109,6 @@ contract OPL2ToL1IsmTest is ExternalBridgeTest { assertTrue(ism.verify(encodedWithdrawalTx, encodedMessage)); } - function test_verify_revertsWhen_invalidIsm() public { - bytes memory encodedWithdrawalTx = _encodeFinalizeWithdrawalTx( - address(this), - 0, - messageId - ); - - vm.expectRevert(); // evmRevert in MockOptimismPortal - ism.verify(encodedWithdrawalTx, encodedMessage); - } - function test_verify_revertsWhen_incorrectMessageId() public { bytes32 incorrectMessageId = keccak256("incorrect message id"); @@ -170,7 +159,8 @@ contract OPL2ToL1IsmTest is ExternalBridgeTest { function _encodeExternalDestinationBridgeCall( address, - /*_from*/ address _to, + /*_from*/ + address _to, uint256 _msgValue, bytes32 _messageId ) internal override returns (bytes memory) { From 56ea53c5d976c2e5ed8c15bcc886c55445bfc2f5 Mon Sep 17 00:00:00 2001 From: -f Date: Tue, 17 Sep 2024 02:18:21 +0530 Subject: [PATCH 09/47] no both branch test --- solidity/test/isms/ArbL2ToL1Ism.t.sol | 31 --------------------------- solidity/test/isms/OPL2ToL1Ism.t.sol | 22 ------------------- 2 files changed, 53 deletions(-) diff --git a/solidity/test/isms/ArbL2ToL1Ism.t.sol b/solidity/test/isms/ArbL2ToL1Ism.t.sol index 1ae1c6a006..8b36b9ab06 100644 --- a/solidity/test/isms/ArbL2ToL1Ism.t.sol +++ b/solidity/test/isms/ArbL2ToL1Ism.t.sol @@ -106,37 +106,6 @@ contract ArbL2ToL1IsmTest is ExternalBridgeTest { ); } - function test_verify_statefulAndOutbox() public { - bytes memory encodedHookData = abi.encodeCall( - AbstractMessageIdAuthorizedIsm.verifyMessageId, - (messageId) - ); - - arbBridge.setL2ToL1Sender(address(hook)); - arbBridge.executeTransaction{value: 1 ether}( - new bytes32[](0), - MOCK_LEAF_INDEX, - address(hook), - address(ism), - MOCK_L2_BLOCK, - MOCK_L1_BLOCK, - block.timestamp, - 1 ether, - encodedHookData - ); - - bytes memory encodedOutboxTxMetadata = _encodeOutboxTx( - address(hook), - address(ism), - messageId, - 1 ether - ); - - vm.etch(address(arbBridge), new bytes(0)); // this is a way to test that the arbBridge isn't called again - assertTrue(ism.verify(encodedOutboxTxMetadata, encodedMessage)); - assertEq(address(testRecipient).balance, 1 ether); - } - function test_verify_revertsWhen_notAuthorizedHook() public { bytes memory encodedOutboxTxMetadata = _encodeOutboxTx( address(this), diff --git a/solidity/test/isms/OPL2ToL1Ism.t.sol b/solidity/test/isms/OPL2ToL1Ism.t.sol index 2c00cddb65..0d204b065f 100644 --- a/solidity/test/isms/OPL2ToL1Ism.t.sol +++ b/solidity/test/isms/OPL2ToL1Ism.t.sol @@ -87,28 +87,6 @@ contract OPL2ToL1IsmTest is ExternalBridgeTest { ism.verify(encodedWithdrawalTx, encodedMessage); } - function test_verify_statefulAndDirectWithdrawal() public { - IOptimismPortal.WithdrawalTransaction - memory withdrawal = IOptimismPortal.WithdrawalTransaction({ - nonce: MOCK_NONCE, - sender: L2_MESSENGER_ADDRESS, - target: address(l1Messenger), - value: 0, - gasLimit: uint256(GAS_QUOTE), - data: _encodeMessengerCalldata(address(ism), 0, messageId) - }); - portal.finalizeWithdrawalTransaction(withdrawal); - - bytes memory encodedWithdrawalTx = _encodeFinalizeWithdrawalTx( - address(ism), - 0, - messageId - ); - - vm.etch(address(portal), new bytes(0)); // this is a way to test that the portal isn't called again - assertTrue(ism.verify(encodedWithdrawalTx, encodedMessage)); - } - function test_verify_revertsWhen_incorrectMessageId() public { bytes32 incorrectMessageId = keccak256("incorrect message id"); From 30982d26af630af8b5a4eac01277cda50e01708c Mon Sep 17 00:00:00 2001 From: -f Date: Tue, 17 Sep 2024 16:28:35 +0530 Subject: [PATCH 10/47] unauth hook --- solidity/test/isms/ArbL2ToL1Ism.t.sol | 21 ++++----------------- solidity/test/isms/ExternalBridgeTest.sol | 21 +++++++++++++++++++++ solidity/test/isms/OPL2ToL1Ism.t.sol | 16 +++------------- 3 files changed, 28 insertions(+), 30 deletions(-) diff --git a/solidity/test/isms/ArbL2ToL1Ism.t.sol b/solidity/test/isms/ArbL2ToL1Ism.t.sol index 8b36b9ab06..fd438c9ca2 100644 --- a/solidity/test/isms/ArbL2ToL1Ism.t.sol +++ b/solidity/test/isms/ArbL2ToL1Ism.t.sol @@ -54,7 +54,6 @@ contract ArbL2ToL1IsmTest is ExternalBridgeTest { function deployIsm() public { arbBridge = new MockArbBridge(); - ism = new ArbL2ToL1Ism(address(arbBridge)); } @@ -62,6 +61,7 @@ contract ArbL2ToL1IsmTest is ExternalBridgeTest { deployIsm(); deployHook(); + arbBridge.setL2ToL1Sender(address(hook)); ism.setAuthorizedHook(TypeCasts.addressToBytes32(address(hook))); } @@ -84,7 +84,6 @@ contract ArbL2ToL1IsmTest is ExternalBridgeTest { bytes32 _messageId ) internal override returns (bytes memory) { vm.deal(address(arbBridge), _msgValue); - arbBridge.setL2ToL1Sender(address(hook)); return _encodeOutboxTx(_from, _to, _messageId, _msgValue); } @@ -92,7 +91,6 @@ contract ArbL2ToL1IsmTest is ExternalBridgeTest { bytes memory _encodedHookData, uint256 _msgValue ) internal override { - arbBridge.setL2ToL1Sender(address(hook)); arbBridge.executeTransaction{value: _msgValue}( new bytes32[](0), MOCK_LEAF_INDEX, @@ -106,18 +104,9 @@ contract ArbL2ToL1IsmTest is ExternalBridgeTest { ); } - function test_verify_revertsWhen_notAuthorizedHook() public { - bytes memory encodedOutboxTxMetadata = _encodeOutboxTx( - address(this), - address(ism), - messageId, - 0 - ); - - arbBridge.setL2ToL1Sender(address(this)); - - vm.expectRevert("ArbL2ToL1Ism: l2Sender != authorizedHook"); - ism.verify(encodedOutboxTxMetadata, encodedMessage); + function _setExternalOriginSender(address _sender) internal override { + unauthorizedHookError = "ArbL2ToL1Ism: l2Sender != authorizedHook"; + arbBridge.setL2ToL1Sender(_sender); } function test_verify_revertsWhen_incorrectMessageId() public { @@ -135,8 +124,6 @@ contract ArbL2ToL1IsmTest is ExternalBridgeTest { (incorrectMessageId) ); - arbBridge.setL2ToL1Sender(address(hook)); - // through outbox call vm.expectRevert("ArbL2ToL1Ism: invalid message id"); ism.verify(encodedOutboxTxMetadata, encodedMessage); diff --git a/solidity/test/isms/ExternalBridgeTest.sol b/solidity/test/isms/ExternalBridgeTest.sol index 8500b56388..b8fa590515 100644 --- a/solidity/test/isms/ExternalBridgeTest.sol +++ b/solidity/test/isms/ExternalBridgeTest.sol @@ -19,6 +19,7 @@ abstract contract ExternalBridgeTest is Test { uint32 internal ORIGIN_DOMAIN; uint32 internal DESTINATION_DOMAIN; uint256 internal GAS_QUOTE; + bytes internal unauthorizedHookError; TestMailbox internal originMailbox; TestMailbox internal destinationMailbox; TestRecipient internal testRecipient; @@ -154,6 +155,24 @@ abstract contract ExternalBridgeTest is Test { ism.verify(externalCalldata, encodedMessage); } + function test_verify_revertsWhen_notAuthorizedHook() public { + bytes memory externalCalldata = _encodeExternalDestinationBridgeCall( + address(this), + address(ism), + 1 ether, + messageId + ); + + _setExternalOriginSender(address(this)); + + vm.expectRevert(unauthorizedHookError); + assertFalse(ism.verify(externalCalldata, encodedMessage)); + + vm.expectRevert(); // evmRevert + _externalBridgeDestinationCall(externalCalldata, 0); + assertEq(ism.isVerified(encodedMessage), false); + } + function _expectOriginExternalBridgeCall( bytes memory _encodedHookData ) internal virtual; @@ -169,4 +188,6 @@ abstract contract ExternalBridgeTest is Test { uint256 _msgValue, bytes32 _messageId ) internal virtual returns (bytes memory); + + function _setExternalOriginSender(address _sender) internal virtual {} } diff --git a/solidity/test/isms/OPL2ToL1Ism.t.sol b/solidity/test/isms/OPL2ToL1Ism.t.sol index 0d204b065f..aa9bbe060c 100644 --- a/solidity/test/isms/OPL2ToL1Ism.t.sol +++ b/solidity/test/isms/OPL2ToL1Ism.t.sol @@ -72,19 +72,9 @@ contract OPL2ToL1IsmTest is ExternalBridgeTest { ism.setAuthorizedHook(TypeCasts.addressToBytes32(address(hook))); } - function test_verify_directWithdrawalCall_revertsWhen_invalidSender() - public - { - l1Messenger.setXDomainMessageSender(address(this)); - - bytes memory encodedWithdrawalTx = _encodeFinalizeWithdrawalTx( - address(ism), - 0, - messageId - ); - - vm.expectRevert(); // evmRevert in MockOptimismPortal - ism.verify(encodedWithdrawalTx, encodedMessage); + function _setExternalOriginSender(address _sender) internal override { + unauthorizedHookError = "AbstractMessageIdAuthorizedIsm: sender is not the hook"; + l1Messenger.setXDomainMessageSender(_sender); } function test_verify_revertsWhen_incorrectMessageId() public { From 88cb33f87325d750d8047205e83043654245d507 Mon Sep 17 00:00:00 2001 From: -f Date: Tue, 17 Sep 2024 17:28:42 +0530 Subject: [PATCH 11/47] invalid messageId --- solidity/test/isms/ArbL2ToL1Ism.t.sol | 42 ++------------------- solidity/test/isms/ExternalBridgeTest.sol | 32 ++++++++++++++-- solidity/test/isms/OPL2ToL1Ism.t.sol | 45 +++-------------------- 3 files changed, 37 insertions(+), 82 deletions(-) diff --git a/solidity/test/isms/ArbL2ToL1Ism.t.sol b/solidity/test/isms/ArbL2ToL1Ism.t.sol index fd438c9ca2..b21b8d0bc2 100644 --- a/solidity/test/isms/ArbL2ToL1Ism.t.sol +++ b/solidity/test/isms/ArbL2ToL1Ism.t.sol @@ -29,6 +29,7 @@ contract ArbL2ToL1IsmTest is ExternalBridgeTest { ORIGIN_DOMAIN = 42161; DESTINATION_DOMAIN = 1; GAS_QUOTE = 120_000; + invalidMessageIdError = "ArbL2ToL1Ism: invalid message id"; super.setUp(); // Arbitrum bridge mock setup @@ -65,6 +66,8 @@ contract ArbL2ToL1IsmTest is ExternalBridgeTest { ism.setAuthorizedHook(TypeCasts.addressToBytes32(address(hook))); } + /* ============ helper functions ============ */ + function _expectOriginExternalBridgeCall( bytes memory _encodedHookData ) internal override { @@ -109,45 +112,6 @@ contract ArbL2ToL1IsmTest is ExternalBridgeTest { arbBridge.setL2ToL1Sender(_sender); } - function test_verify_revertsWhen_incorrectMessageId() public { - bytes32 incorrectMessageId = keccak256("incorrect message id"); - - bytes memory encodedOutboxTxMetadata = _encodeOutboxTx( - address(hook), - address(ism), - incorrectMessageId, - 0 - ); - - bytes memory encodedHookData = abi.encodeCall( - AbstractMessageIdAuthorizedIsm.verifyMessageId, - (incorrectMessageId) - ); - - // through outbox call - vm.expectRevert("ArbL2ToL1Ism: invalid message id"); - ism.verify(encodedOutboxTxMetadata, encodedMessage); - - // through statefulVerify - arbBridge.executeTransaction( - new bytes32[](0), - MOCK_LEAF_INDEX, - address(hook), - address(ism), - MOCK_L2_BLOCK, - MOCK_L1_BLOCK, - block.timestamp, - 0, - encodedHookData - ); - - vm.etch(address(arbBridge), new bytes(0)); // to stop the outbox route - vm.expectRevert(); - assertFalse(ism.verify(new bytes(0), encodedMessage)); - } - - /* ============ helper functions ============ */ - function _encodeOutboxTx( address _hook, address _ism, diff --git a/solidity/test/isms/ExternalBridgeTest.sol b/solidity/test/isms/ExternalBridgeTest.sol index b8fa590515..b22af031a7 100644 --- a/solidity/test/isms/ExternalBridgeTest.sol +++ b/solidity/test/isms/ExternalBridgeTest.sol @@ -19,7 +19,10 @@ abstract contract ExternalBridgeTest is Test { uint32 internal ORIGIN_DOMAIN; uint32 internal DESTINATION_DOMAIN; uint256 internal GAS_QUOTE; + bytes internal unauthorizedHookError; + bytes internal invalidMessageIdError; + TestMailbox internal originMailbox; TestMailbox internal destinationMailbox; TestRecipient internal testRecipient; @@ -138,7 +141,6 @@ abstract contract ExternalBridgeTest is Test { 1 ether, messageId ); - ism.verify(externalCalldata, encodedMessage); assertEq(address(testRecipient).balance, 1 ether); } @@ -159,20 +161,42 @@ abstract contract ExternalBridgeTest is Test { bytes memory externalCalldata = _encodeExternalDestinationBridgeCall( address(this), address(ism), - 1 ether, + 0, messageId ); _setExternalOriginSender(address(this)); + // external call vm.expectRevert(unauthorizedHookError); assertFalse(ism.verify(externalCalldata, encodedMessage)); - vm.expectRevert(); // evmRevert + // async call + vm.expectRevert(); + _externalBridgeDestinationCall(externalCalldata, 0); + assertFalse(ism.isVerified(encodedMessage)); + } + + function test_verify_revertsWhen_incorrectMessageId() public { + bytes32 incorrectMessageId = keccak256("incorrect message id"); + bytes memory externalCalldata = _encodeExternalDestinationBridgeCall( + address(hook), + address(ism), + 0, + incorrectMessageId + ); + + // external call + vm.expectRevert(invalidMessageIdError); + ism.verify(externalCalldata, encodedMessage); + + // async call - native bridges might have try catch block to prevent revert _externalBridgeDestinationCall(externalCalldata, 0); - assertEq(ism.isVerified(encodedMessage), false); + assertFalse(ism.isVerified(testMessage)); } + /* ============ helper functions ============ */ + function _expectOriginExternalBridgeCall( bytes memory _encodedHookData ) internal virtual; diff --git a/solidity/test/isms/OPL2ToL1Ism.t.sol b/solidity/test/isms/OPL2ToL1Ism.t.sol index aa9bbe060c..8e97576719 100644 --- a/solidity/test/isms/OPL2ToL1Ism.t.sol +++ b/solidity/test/isms/OPL2ToL1Ism.t.sol @@ -34,6 +34,7 @@ contract OPL2ToL1IsmTest is ExternalBridgeTest { ORIGIN_DOMAIN = 10; DESTINATION_DOMAIN = 1; GAS_QUOTE = 120_000; + invalidMessageIdError = "OPL2ToL1Ism: invalid message id"; super.setUp(); // Optimism messenger mock setup @@ -72,45 +73,6 @@ contract OPL2ToL1IsmTest is ExternalBridgeTest { ism.setAuthorizedHook(TypeCasts.addressToBytes32(address(hook))); } - function _setExternalOriginSender(address _sender) internal override { - unauthorizedHookError = "AbstractMessageIdAuthorizedIsm: sender is not the hook"; - l1Messenger.setXDomainMessageSender(_sender); - } - - function test_verify_revertsWhen_incorrectMessageId() public { - bytes32 incorrectMessageId = keccak256("incorrect message id"); - - bytes memory encodedWithdrawalTx = _encodeFinalizeWithdrawalTx( - address(this), - 0, - incorrectMessageId - ); - - // through portal call - vm.expectRevert("OPL2ToL1Ism: invalid message id"); - ism.verify(encodedWithdrawalTx, encodedMessage); - - // through statefulVerify - IOptimismPortal.WithdrawalTransaction - memory withdrawal = IOptimismPortal.WithdrawalTransaction({ - nonce: MOCK_NONCE, - sender: L2_MESSENGER_ADDRESS, - target: address(l1Messenger), - value: 0, - gasLimit: uint256(GAS_QUOTE), - data: _encodeMessengerCalldata( - address(ism), - 0, - incorrectMessageId - ) - }); - portal.finalizeWithdrawalTransaction(withdrawal); - - vm.etch(address(portal), new bytes(0)); // to stop the portal route - vm.expectRevert(); // evmRevert() - assertFalse(ism.verify(new bytes(0), encodedMessage)); - } - /* ============ helper functions ============ */ function _expectOriginExternalBridgeCall( @@ -136,6 +98,11 @@ contract OPL2ToL1IsmTest is ExternalBridgeTest { return _encodeFinalizeWithdrawalTx(_to, _msgValue, _messageId); } + function _setExternalOriginSender(address _sender) internal override { + unauthorizedHookError = "AbstractMessageIdAuthorizedIsm: sender is not the hook"; + l1Messenger.setXDomainMessageSender(_sender); + } + function _externalBridgeDestinationCall( bytes memory, /*_encodedHookData*/ From baef8ff5504e61bc3a5b930d1c24cff49663342b Mon Sep 17 00:00:00 2001 From: -f Date: Tue, 17 Sep 2024 19:33:50 +0530 Subject: [PATCH 12/47] 5164 --- solidity/test/isms/ERC5164ISM.t.sol | 156 +++++++--------------- solidity/test/isms/ExternalBridgeTest.sol | 77 ++++++----- 2 files changed, 89 insertions(+), 144 deletions(-) diff --git a/solidity/test/isms/ERC5164ISM.t.sol b/solidity/test/isms/ERC5164ISM.t.sol index c94052e255..773d67f87b 100644 --- a/solidity/test/isms/ERC5164ISM.t.sol +++ b/solidity/test/isms/ERC5164ISM.t.sol @@ -1,7 +1,7 @@ // SPDX-License-Identifier: MIT or Apache-2.0 pragma solidity ^0.8.13; -import {Test} from "forge-std/Test.sol"; +import "forge-std/console.sol"; import {LibBit} from "../../contracts/libs/LibBit.sol"; import {Message} from "../../contracts/libs/Message.sol"; @@ -17,8 +17,9 @@ import {ERC5164Ism} from "../../contracts/isms/hook/ERC5164Ism.sol"; import {TestMailbox} from "../../contracts/test/TestMailbox.sol"; import {TestRecipient} from "../../contracts/test/TestRecipient.sol"; import {MockMessageDispatcher, MockMessageExecutor} from "../../contracts/mock/MockERC5164.sol"; +import {ExternalBridgeTest} from "./ExternalBridgeTest.sol"; -contract ERC5164IsmTest is Test { +contract ERC5164IsmTest is ExternalBridgeTest { using LibBit for uint256; using TypeCasts for address; using Message for bytes; @@ -27,23 +28,8 @@ contract ERC5164IsmTest is Test { IMessageDispatcher internal dispatcher; MockMessageExecutor internal executor; - ERC5164Hook internal hook; - ERC5164Ism internal ism; - TestMailbox internal originMailbox; - TestRecipient internal testRecipient; - - uint32 internal constant TEST1_DOMAIN = 1; - uint32 internal constant TEST2_DOMAIN = 2; - - uint8 internal constant VERSION = 0; - bytes internal testMessage = - abi.encodePacked("Hello from the other chain!"); address internal alice = address(0x1); - // req for most tests - bytes encodedMessage = _encodeTestMessage(0, address(testRecipient)); - bytes32 messageId = encodedMessage.id(); - event MessageDispatched( bytes32 indexed messageId, address indexed from, @@ -56,15 +42,18 @@ contract ERC5164IsmTest is Test { /// SETUP /// /////////////////////////////////////////////////////////////////// - function setUp() public { + function setUp() public override { + ORIGIN_DOMAIN = 1; + DESTINATION_DOMAIN = 2; + super.setUp(); + dispatcher = new MockMessageDispatcher(); executor = new MockMessageExecutor(); - testRecipient = new TestRecipient(); - originMailbox = new TestMailbox(TEST1_DOMAIN); + originMailbox = new TestMailbox(ORIGIN_DOMAIN); ism = new ERC5164Ism(address(executor)); hook = new ERC5164Hook( address(originMailbox), - TEST2_DOMAIN, + DESTINATION_DOMAIN, address(ism).addressToBytes32(), address(dispatcher) ); @@ -100,7 +89,7 @@ contract ERC5164IsmTest is Test { vm.expectRevert("AbstractMessageIdAuthHook: invalid ISM"); hook = new ERC5164Hook( address(originMailbox), - TEST2_DOMAIN, + DESTINATION_DOMAIN, address(0).addressToBytes32(), address(dispatcher) ); @@ -108,53 +97,28 @@ contract ERC5164IsmTest is Test { vm.expectRevert("ERC5164Hook: invalid dispatcher"); hook = new ERC5164Hook( address(originMailbox), - TEST2_DOMAIN, + DESTINATION_DOMAIN, address(ism).addressToBytes32(), address(0) ); } - function test_postDispatch() public { - bytes memory encodedHookData = abi.encodeCall( - AbstractMessageIdAuthorizedIsm.verifyMessageId, - (messageId) - ); - originMailbox.updateLatestDispatchedId(messageId); + function testTypes() public view { + assertEq(hook.hookType(), uint8(IPostDispatchHook.Types.ID_AUTH_ISM)); + assertEq(ism.moduleType(), uint8(IInterchainSecurityModule.Types.NULL)); + } - // note: not checking for messageId since this is implementation dependent on each vendor + function _expectOriginExternalBridgeCall( + bytes memory _encodedHookData + ) internal override { vm.expectEmit(false, true, true, true, address(dispatcher)); emit MessageDispatched( messageId, address(hook), - TEST2_DOMAIN, + DESTINATION_DOMAIN, address(ism), - encodedHookData + _encodedHookData ); - - hook.postDispatch(bytes(""), encodedMessage); - } - - function testTypes() public { - assertEq(hook.hookType(), uint8(IPostDispatchHook.Types.ID_AUTH_ISM)); - assertEq(ism.moduleType(), uint8(IInterchainSecurityModule.Types.NULL)); - } - - function test_postDispatch_RevertWhen_ChainIDNotSupported() public { - encodedMessage = MessageUtils.formatMessage( - VERSION, - 0, - TEST1_DOMAIN, - TypeCasts.addressToBytes32(address(this)), - 3, // unsupported chain id - TypeCasts.addressToBytes32(address(testRecipient)), - testMessage - ); - originMailbox.updateLatestDispatchedId(Message.id(encodedMessage)); - - vm.expectRevert( - "AbstractMessageIdAuthHook: invalid destination domain" - ); - hook.postDispatch(bytes(""), encodedMessage); } function test_postDispatch_RevertWhen_msgValueNotAllowed() public payable { @@ -164,69 +128,51 @@ contract ERC5164IsmTest is Test { hook.postDispatch{value: 1}(bytes(""), encodedMessage); } - /* ============ ISM.verifyMessageId ============ */ - - function test_verifyMessageId() public { - vm.startPrank(address(executor)); - - ism.verifyMessageId(messageId); - assertTrue(ism.verifiedMessages(messageId).isBitSet(255)); - - vm.stopPrank(); - } - - function test_verifyMessageId_RevertWhen_NotAuthorized() public { - vm.startPrank(alice); + // override to omit direct external bridge call + function test_verify_revertsWhen_notAuthorizedHook() public override { + vm.prank(alice); - // needs to be called by the authorized hook contract on Ethereum vm.expectRevert( "AbstractMessageIdAuthorizedIsm: sender is not the hook" ); ism.verifyMessageId(messageId); + assertFalse(ism.isVerified(encodedMessage)); + } - vm.stopPrank(); + // override to assert false instead of revert + function test_verify_revertWhen_invalidMetadata() public override { + assertFalse(ism.verify(new bytes(0), encodedMessage)); } - /* ============ ISM.verify ============ */ + // SKIP - duplicate of test_verify_revertWhen_invalidMetadata + function test_verify_revertsWhen_incorrectMessageId() public override {} - function test_verify() public { - vm.startPrank(address(executor)); + function test_verify_revertsWhen_invalidIsm() public override {} - ism.verifyMessageId(messageId); + // SKIP - 5164 ism does not support msg.value + function test_verify_msgValue_asyncCall() public override {} - bool verified = ism.verify(new bytes(0), encodedMessage); - assertTrue(verified); + function test_verify_msgValue_externalBridgeCall() public override {} - vm.stopPrank(); - } - - function test_verify_RevertWhen_InvalidMessage() public { - vm.startPrank(address(executor)); + /* ============ helper functions ============ */ + function _externalBridgeDestinationCall( + bytes memory _encodedHookData, + uint256 _msgValue + ) internal override { + vm.prank(address(executor)); ism.verifyMessageId(messageId); - - bytes memory invalidMessage = _encodeTestMessage(0, address(this)); - bool verified = ism.verify(new bytes(0), invalidMessage); - assertFalse(verified); - - vm.stopPrank(); } - /* ============ helper functions ============ */ - - function _encodeTestMessage( - uint32 _msgCount, - address _receipient - ) internal view returns (bytes memory) { - return - MessageUtils.formatMessage( - VERSION, - _msgCount, - TEST1_DOMAIN, - TypeCasts.addressToBytes32(address(this)), - TEST2_DOMAIN, - TypeCasts.addressToBytes32(_receipient), - testMessage - ); + function _encodeExternalDestinationBridgeCall( + address _from, + address _to, + uint256 _msgValue, + bytes32 _messageId + ) internal override returns (bytes memory) { + if (_from == address(hook)) { + vm.prank(address(executor)); + ism.verifyMessageId{value: _msgValue}(messageId); + } } } diff --git a/solidity/test/isms/ExternalBridgeTest.sol b/solidity/test/isms/ExternalBridgeTest.sol index b22af031a7..201325f59c 100644 --- a/solidity/test/isms/ExternalBridgeTest.sol +++ b/solidity/test/isms/ExternalBridgeTest.sol @@ -24,7 +24,6 @@ abstract contract ExternalBridgeTest is Test { bytes internal invalidMessageIdError; TestMailbox internal originMailbox; - TestMailbox internal destinationMailbox; TestRecipient internal testRecipient; AbstractMessageIdAuthHook internal hook; @@ -43,27 +42,12 @@ abstract contract ExternalBridgeTest is Test { messageId = Message.id(encodedMessage); } - function _encodeTestMessage() internal view returns (bytes memory) { - return - MessageUtils.formatMessage( - HYPERLANE_VERSION, - uint32(0), - ORIGIN_DOMAIN, - TypeCasts.addressToBytes32(address(this)), - DESTINATION_DOMAIN, - TypeCasts.addressToBytes32(address(testRecipient)), - testMessage - ); - } + function test_postDispatch() public { + bytes memory encodedHookData = _encodeHookData(messageId); + originMailbox.updateLatestDispatchedId(messageId); + _expectOriginExternalBridgeCall(encodedHookData); - function _encodeHookData( - bytes32 _messageId - ) internal pure returns (bytes memory) { - return - abi.encodeCall( - AbstractMessageIdAuthorizedIsm.verifyMessageId, - (_messageId) - ); + hook.postDispatch{value: GAS_QUOTE}(testMetadata, encodedMessage); } function test_postDispatch_revertWhen_chainIDNotSupported() public { @@ -72,7 +56,7 @@ abstract contract ExternalBridgeTest is Test { uint32(0), DESTINATION_DOMAIN, TypeCasts.addressToBytes32(address(this)), - 2, // wrong domain + 3, // wrong domain TypeCasts.addressToBytes32(address(testRecipient)), testMessage ); @@ -91,14 +75,6 @@ abstract contract ExternalBridgeTest is Test { hook.postDispatch(testMetadata, encodedMessage); } - function test_postDispatch() public { - bytes memory encodedHookData = _encodeHookData(messageId); - originMailbox.updateLatestDispatchedId(messageId); - _expectOriginExternalBridgeCall(encodedHookData); - - hook.postDispatch{value: GAS_QUOTE}(testMetadata, encodedMessage); - } - function test_verifyMessageId_asyncCall() public { bytes memory encodedHookData = abi.encodeCall( AbstractMessageIdAuthorizedIsm.verifyMessageId, @@ -121,12 +97,12 @@ abstract contract ExternalBridgeTest is Test { assertTrue(ism.isVerified(encodedMessage)); } - function test_verify_revertWhen_invalidMetadata() public { + function test_verify_revertWhen_invalidMetadata() public virtual { vm.expectRevert(); ism.verify(new bytes(0), encodedMessage); } - function test_verify_msgValue_asyncCall() public { + function test_verify_msgValue_asyncCall() public virtual { bytes memory encodedHookData = _encodeHookData(messageId); _externalBridgeDestinationCall(encodedHookData, 1 ether); @@ -134,7 +110,7 @@ abstract contract ExternalBridgeTest is Test { assertEq(address(testRecipient).balance, 1 ether); } - function test_verify_msgValue_externalBridgeCall() public { + function test_verify_msgValue_externalBridgeCall() public virtual { bytes memory externalCalldata = _encodeExternalDestinationBridgeCall( address(hook), address(ism), @@ -145,7 +121,7 @@ abstract contract ExternalBridgeTest is Test { assertEq(address(testRecipient).balance, 1 ether); } - function test_verify_revertsWhen_invalidIsm() public { + function test_verify_revertsWhen_invalidIsm() public virtual { bytes memory externalCalldata = _encodeExternalDestinationBridgeCall( address(hook), address(this), @@ -154,10 +130,12 @@ abstract contract ExternalBridgeTest is Test { ); vm.expectRevert(); - ism.verify(externalCalldata, encodedMessage); + assertFalse(ism.verify(externalCalldata, encodedMessage)); } - function test_verify_revertsWhen_notAuthorizedHook() public { + function test_verify_revertsWhen_notAuthorizedHook() public virtual { + _setExternalOriginSender(address(this)); + bytes memory externalCalldata = _encodeExternalDestinationBridgeCall( address(this), address(ism), @@ -165,8 +143,6 @@ abstract contract ExternalBridgeTest is Test { messageId ); - _setExternalOriginSender(address(this)); - // external call vm.expectRevert(unauthorizedHookError); assertFalse(ism.verify(externalCalldata, encodedMessage)); @@ -177,7 +153,7 @@ abstract contract ExternalBridgeTest is Test { assertFalse(ism.isVerified(encodedMessage)); } - function test_verify_revertsWhen_incorrectMessageId() public { + function test_verify_revertsWhen_incorrectMessageId() public virtual { bytes32 incorrectMessageId = keccak256("incorrect message id"); bytes memory externalCalldata = _encodeExternalDestinationBridgeCall( address(hook), @@ -197,6 +173,29 @@ abstract contract ExternalBridgeTest is Test { /* ============ helper functions ============ */ + function _encodeTestMessage() internal view returns (bytes memory) { + return + MessageUtils.formatMessage( + HYPERLANE_VERSION, + uint32(0), + ORIGIN_DOMAIN, + TypeCasts.addressToBytes32(address(this)), + DESTINATION_DOMAIN, + TypeCasts.addressToBytes32(address(testRecipient)), + testMessage + ); + } + + function _encodeHookData( + bytes32 _messageId + ) internal pure returns (bytes memory) { + return + abi.encodeCall( + AbstractMessageIdAuthorizedIsm.verifyMessageId, + (_messageId) + ); + } + function _expectOriginExternalBridgeCall( bytes memory _encodedHookData ) internal virtual; From 96848359e7dd210e19f22fb56b8aab0fe6649eed Mon Sep 17 00:00:00 2001 From: -f Date: Tue, 17 Sep 2024 21:00:09 +0530 Subject: [PATCH 13/47] try catch --- solidity/test/isms/ExternalBridgeTest.sol | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/solidity/test/isms/ExternalBridgeTest.sol b/solidity/test/isms/ExternalBridgeTest.sol index 201325f59c..f629a112fe 100644 --- a/solidity/test/isms/ExternalBridgeTest.sol +++ b/solidity/test/isms/ExternalBridgeTest.sol @@ -167,10 +167,20 @@ abstract contract ExternalBridgeTest is Test { ism.verify(externalCalldata, encodedMessage); // async call - native bridges might have try catch block to prevent revert - _externalBridgeDestinationCall(externalCalldata, 0); + try + this.externalBridgeDestinationCallWrapper(externalCalldata, 0) + {} catch {} assertFalse(ism.isVerified(testMessage)); } + // try catch block to prevent revert + function externalBridgeDestinationCallWrapper( + bytes memory _encodedHookData, + uint256 _msgValue + ) external { + _externalBridgeDestinationCall(_encodedHookData, _msgValue); + } + /* ============ helper functions ============ */ function _encodeTestMessage() internal view returns (bytes memory) { From 6c31fbf5c29794fd9820faf689284f2e88e5fd2d Mon Sep 17 00:00:00 2001 From: -f Date: Tue, 17 Sep 2024 21:03:58 +0530 Subject: [PATCH 14/47] formatting --- solidity/test/isms/ERC5164ISM.t.sol | 2 -- solidity/test/isms/ExternalBridgeTest.sol | 6 ++++++ 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/solidity/test/isms/ERC5164ISM.t.sol b/solidity/test/isms/ERC5164ISM.t.sol index 773d67f87b..2de776d2df 100644 --- a/solidity/test/isms/ERC5164ISM.t.sol +++ b/solidity/test/isms/ERC5164ISM.t.sol @@ -1,8 +1,6 @@ // SPDX-License-Identifier: MIT or Apache-2.0 pragma solidity ^0.8.13; -import "forge-std/console.sol"; - import {LibBit} from "../../contracts/libs/LibBit.sol"; import {Message} from "../../contracts/libs/Message.sol"; import {MessageUtils} from "./IsmTestUtils.sol"; diff --git a/solidity/test/isms/ExternalBridgeTest.sol b/solidity/test/isms/ExternalBridgeTest.sol index f629a112fe..74a7e7eeaf 100644 --- a/solidity/test/isms/ExternalBridgeTest.sol +++ b/solidity/test/isms/ExternalBridgeTest.sol @@ -42,6 +42,8 @@ abstract contract ExternalBridgeTest is Test { messageId = Message.id(encodedMessage); } + /* ============ Hook.postDispatch ============ */ + function test_postDispatch() public { bytes memory encodedHookData = _encodeHookData(messageId); originMailbox.updateLatestDispatchedId(messageId); @@ -75,6 +77,8 @@ abstract contract ExternalBridgeTest is Test { hook.postDispatch(testMetadata, encodedMessage); } + /* ============ ISM.verifyMessageId ============ */ + function test_verifyMessageId_asyncCall() public { bytes memory encodedHookData = abi.encodeCall( AbstractMessageIdAuthorizedIsm.verifyMessageId, @@ -97,6 +101,8 @@ abstract contract ExternalBridgeTest is Test { assertTrue(ism.isVerified(encodedMessage)); } + /* ============ ISM.verify ============ */ + function test_verify_revertWhen_invalidMetadata() public virtual { vm.expectRevert(); ism.verify(new bytes(0), encodedMessage); From f64611d51d8eeac5b21a6a0acc0f72c30589150b Mon Sep 17 00:00:00 2001 From: -f Date: Wed, 18 Sep 2024 19:19:24 +0530 Subject: [PATCH 15/47] opstack --- solidity/test/isms/ArbL2ToL1Ism.t.sol | 6 +- solidity/test/isms/ERC5164ISM.t.sol | 7 +- solidity/test/isms/ExternalBridgeTest.sol | 66 ++- solidity/test/isms/OPL2ToL1Ism.t.sol | 3 - solidity/test/isms/OPStackIsm.t.sol | 521 ++++------------------ 5 files changed, 141 insertions(+), 462 deletions(-) diff --git a/solidity/test/isms/ArbL2ToL1Ism.t.sol b/solidity/test/isms/ArbL2ToL1Ism.t.sol index b21b8d0bc2..d3f198818a 100644 --- a/solidity/test/isms/ArbL2ToL1Ism.t.sol +++ b/solidity/test/isms/ArbL2ToL1Ism.t.sol @@ -1,8 +1,6 @@ // SPDX-License-Identifier: MIT or Apache-2.0 pragma solidity ^0.8.13; -import {Test} from "forge-std/Test.sol"; - import {TypeCasts} from "../../contracts/libs/TypeCasts.sol"; import {MessageUtils} from "./IsmTestUtils.sol"; import {TestMailbox} from "../../contracts/test/TestMailbox.sol"; @@ -29,7 +27,6 @@ contract ArbL2ToL1IsmTest is ExternalBridgeTest { ORIGIN_DOMAIN = 42161; DESTINATION_DOMAIN = 1; GAS_QUOTE = 120_000; - invalidMessageIdError = "ArbL2ToL1Ism: invalid message id"; super.setUp(); // Arbitrum bridge mock setup @@ -94,7 +91,8 @@ contract ArbL2ToL1IsmTest is ExternalBridgeTest { bytes memory _encodedHookData, uint256 _msgValue ) internal override { - arbBridge.executeTransaction{value: _msgValue}( + vm.deal(address(arbBridge), _msgValue); + arbBridge.executeTransaction( new bytes32[](0), MOCK_LEAF_INDEX, address(hook), diff --git a/solidity/test/isms/ERC5164ISM.t.sol b/solidity/test/isms/ERC5164ISM.t.sol index 2de776d2df..f00ef792f8 100644 --- a/solidity/test/isms/ERC5164ISM.t.sol +++ b/solidity/test/isms/ERC5164ISM.t.sol @@ -137,11 +137,6 @@ contract ERC5164IsmTest is ExternalBridgeTest { assertFalse(ism.isVerified(encodedMessage)); } - // override to assert false instead of revert - function test_verify_revertWhen_invalidMetadata() public override { - assertFalse(ism.verify(new bytes(0), encodedMessage)); - } - // SKIP - duplicate of test_verify_revertWhen_invalidMetadata function test_verify_revertsWhen_incorrectMessageId() public override {} @@ -152,6 +147,8 @@ contract ERC5164IsmTest is ExternalBridgeTest { function test_verify_msgValue_externalBridgeCall() public override {} + function test_verify_valueAlreadyClaimed(uint256) public override {} + /* ============ helper functions ============ */ function _externalBridgeDestinationCall( diff --git a/solidity/test/isms/ExternalBridgeTest.sol b/solidity/test/isms/ExternalBridgeTest.sol index 74a7e7eeaf..021a83f4d4 100644 --- a/solidity/test/isms/ExternalBridgeTest.sol +++ b/solidity/test/isms/ExternalBridgeTest.sol @@ -21,7 +21,6 @@ abstract contract ExternalBridgeTest is Test { uint256 internal GAS_QUOTE; bytes internal unauthorizedHookError; - bytes internal invalidMessageIdError; TestMailbox internal originMailbox; TestRecipient internal testRecipient; @@ -77,6 +76,18 @@ abstract contract ExternalBridgeTest is Test { hook.postDispatch(testMetadata, encodedMessage); } + function test_postDispatch_revertWhen_tooMuchValue() public { + vm.deal(address(this), uint256(2 ** 255 + 1)); + bytes memory excessValueMetadata = StandardHookMetadata + .overrideMsgValue(uint256(2 ** 255 + 1)); + + originMailbox.updateLatestDispatchedId(messageId); + vm.expectRevert( + "AbstractMessageIdAuthHook: msgValue must be less than 2 ** 255" + ); + hook.postDispatch(excessValueMetadata, encodedMessage); + } + /* ============ ISM.verifyMessageId ============ */ function test_verifyMessageId_asyncCall() public { @@ -89,7 +100,7 @@ abstract contract ExternalBridgeTest is Test { assertTrue(ism.isVerified(encodedMessage)); } - function test_verifyMessageId_externalBridgeCall() public { + function test_verifyMessageId_externalBridgeCall() public virtual { bytes memory externalCalldata = _encodeExternalDestinationBridgeCall( address(hook), address(ism), @@ -104,8 +115,11 @@ abstract contract ExternalBridgeTest is Test { /* ============ ISM.verify ============ */ function test_verify_revertWhen_invalidMetadata() public virtual { - vm.expectRevert(); - ism.verify(new bytes(0), encodedMessage); + bool isValid; + try ism.verify(new bytes(0), encodedMessage) returns (bool _isValid) { + isValid = _isValid; + } catch {} + assertFalse(isValid); } function test_verify_msgValue_asyncCall() public virtual { @@ -153,7 +167,7 @@ abstract contract ExternalBridgeTest is Test { vm.expectRevert(unauthorizedHookError); assertFalse(ism.verify(externalCalldata, encodedMessage)); - // async call + // async call vm.expectRevert(NotCrossChainCall.selector); vm.expectRevert(); _externalBridgeDestinationCall(externalCalldata, 0); assertFalse(ism.isVerified(encodedMessage)); @@ -169,8 +183,13 @@ abstract contract ExternalBridgeTest is Test { ); // external call - vm.expectRevert(invalidMessageIdError); - ism.verify(externalCalldata, encodedMessage); + bool isValid; + try ism.verify(externalCalldata, encodedMessage) returns ( + bool _isValid + ) { + isValid = _isValid; + } catch {} + assertFalse(isValid); // async call - native bridges might have try catch block to prevent revert try @@ -179,12 +198,25 @@ abstract contract ExternalBridgeTest is Test { assertFalse(ism.isVerified(testMessage)); } - // try catch block to prevent revert - function externalBridgeDestinationCallWrapper( - bytes memory _encodedHookData, - uint256 _msgValue - ) external { - _externalBridgeDestinationCall(_encodedHookData, _msgValue); + /// forge-config: default.fuzz.runs = 10 + function test_verify_valueAlreadyClaimed(uint256 _msgValue) public virtual { + _msgValue = bound(_msgValue, 0, 2 ** 254); + _externalBridgeDestinationCall(_encodeHookData(messageId), _msgValue); + + bool verified = ism.verify(new bytes(0), encodedMessage); + assertTrue(verified); + assertEq(address(ism).balance, 0); + assertEq(address(testRecipient).balance, _msgValue); + + // send more value to the ISM + vm.deal(address(ism), _msgValue); + + // verified still true + verified = ism.verify(new bytes(0), encodedMessage); + assertTrue(verified); + // value which was already sent + assertEq(address(ism).balance, _msgValue); + assertEq(address(testRecipient).balance, _msgValue); } /* ============ helper functions ============ */ @@ -212,6 +244,14 @@ abstract contract ExternalBridgeTest is Test { ); } + // try catch block to prevent revert + function externalBridgeDestinationCallWrapper( + bytes memory _encodedHookData, + uint256 _msgValue + ) external { + _externalBridgeDestinationCall(_encodedHookData, _msgValue); + } + function _expectOriginExternalBridgeCall( bytes memory _encodedHookData ) internal virtual; diff --git a/solidity/test/isms/OPL2ToL1Ism.t.sol b/solidity/test/isms/OPL2ToL1Ism.t.sol index 8e97576719..df564699c4 100644 --- a/solidity/test/isms/OPL2ToL1Ism.t.sol +++ b/solidity/test/isms/OPL2ToL1Ism.t.sol @@ -1,8 +1,6 @@ // SPDX-License-Identifier: MIT or Apache-2.0 pragma solidity ^0.8.13; -import {Test} from "forge-std/Test.sol"; - import {TypeCasts} from "../../contracts/libs/TypeCasts.sol"; import {Message} from "../../contracts/libs/Message.sol"; import {MessageUtils} from "./IsmTestUtils.sol"; @@ -34,7 +32,6 @@ contract OPL2ToL1IsmTest is ExternalBridgeTest { ORIGIN_DOMAIN = 10; DESTINATION_DOMAIN = 1; GAS_QUOTE = 120_000; - invalidMessageIdError = "OPL2ToL1Ism: invalid message id"; super.setUp(); // Optimism messenger mock setup diff --git a/solidity/test/isms/OPStackIsm.t.sol b/solidity/test/isms/OPStackIsm.t.sol index 9862717ad3..1b70d402fb 100644 --- a/solidity/test/isms/OPStackIsm.t.sol +++ b/solidity/test/isms/OPStackIsm.t.sol @@ -1,13 +1,14 @@ // SPDX-License-Identifier: MIT or Apache-2.0 pragma solidity ^0.8.13; -import {Test} from "forge-std/Test.sol"; +import "forge-std/console.sol"; import {LibBit} from "../../contracts/libs/LibBit.sol"; import {TypeCasts} from "../../contracts/libs/TypeCasts.sol"; import {StandardHookMetadata} from "../../contracts/hooks/libs/StandardHookMetadata.sol"; import {StandardHookMetadata} from "../../contracts/hooks/libs/StandardHookMetadata.sol"; import {AbstractMessageIdAuthorizedIsm} from "../../contracts/isms/hook/AbstractMessageIdAuthorizedIsm.sol"; +import {MockOptimismMessenger} from "../../contracts/mock/MockOptimism.sol"; import {TestMailbox} from "../../contracts/test/TestMailbox.sol"; import {Message} from "../../contracts/libs/Message.sol"; import {MessageUtils} from "./IsmTestUtils.sol"; @@ -18,16 +19,14 @@ import {TestRecipient} from "../../contracts/test/TestRecipient.sol"; import {NotCrossChainCall} from "@openzeppelin/contracts/crosschain/errors.sol"; import {AddressAliasHelper} from "@eth-optimism/contracts/standards/AddressAliasHelper.sol"; -import {ICrossDomainMessenger, IL2CrossDomainMessenger} from "../../contracts/interfaces/optimism/ICrossDomainMessenger.sol"; +import {ICrossDomainMessenger} from "../../contracts/interfaces/optimism/ICrossDomainMessenger.sol"; +import {ExternalBridgeTest} from "./ExternalBridgeTest.sol"; -contract OPStackIsmTest is Test { +contract OPStackIsmTest is ExternalBridgeTest { using LibBit for uint256; using TypeCasts for address; using MessageUtils for bytes; - uint256 internal mainnetFork; - uint256 internal optimismFork; - address internal constant L1_MESSENGER_ADDRESS = 0x25ace71c97B33Cc4729CF772ae268934F7ab5fA1; address internal constant L1_CANNONICAL_CHAIN = @@ -36,36 +35,12 @@ contract OPStackIsmTest is Test { 0x4200000000000000000000000000000000000007; uint8 internal constant OPTIMISM_VERSION = 0; - uint8 internal constant HYPERLANE_VERSION = 1; uint256 internal constant DEFAULT_GAS_LIMIT = 1_920_000; address internal alice = address(0x1); - ICrossDomainMessenger internal l1Messenger; - IL2CrossDomainMessenger internal l2Messenger; - TestMailbox internal l1Mailbox; - OPStackIsm internal opISM; - OPStackHook internal opHook; - - TestRecipient internal testRecipient; - bytes internal testMessage = - abi.encodePacked("Hello from the other chain!"); - bytes internal testMetadata = - StandardHookMetadata.overrideRefundAddress(address(this)); - - bytes internal encodedMessage; - bytes32 internal messageId; - - uint32 internal constant MAINNET_DOMAIN = 1; - uint32 internal constant OPTIMISM_DOMAIN = 10; - - event SentMessage( - address indexed target, - address sender, - bytes message, - uint256 messageNonce, - uint256 gasLimit - ); + MockOptimismMessenger internal l1Messenger; + MockOptimismMessenger internal l2Messenger; event RelayedMessage(bytes32 indexed msgHash); @@ -73,469 +48,141 @@ contract OPStackIsmTest is Test { event ReceivedMessage(bytes32 indexed messageId); - function setUp() public { - // block numbers to fork from, chain data is cached to ../../forge-cache/ - mainnetFork = vm.createFork(vm.rpcUrl("mainnet"), 18_992_500); - optimismFork = vm.createFork(vm.rpcUrl("optimism"), 114_696_811); + function setUp() public override { + ORIGIN_DOMAIN = 1; + DESTINATION_DOMAIN = 10; + GAS_QUOTE = 1_920_000; // optimism subsidized gas limit + super.setUp(); - testRecipient = new TestRecipient(); + vm.etch( + L1_MESSENGER_ADDRESS, + address(new MockOptimismMessenger()).code + ); + vm.etch( + L2_MESSENGER_ADDRESS, + address(new MockOptimismMessenger()).code + ); + l1Messenger = MockOptimismMessenger(L1_MESSENGER_ADDRESS); + l2Messenger = MockOptimismMessenger(L2_MESSENGER_ADDRESS); - encodedMessage = _encodeTestMessage(); - messageId = Message.id(encodedMessage); + deployAll(); } /////////////////////////////////////////////////////////////////// /// SETUP /// /////////////////////////////////////////////////////////////////// - function deployOptimismHook() public { - vm.selectFork(mainnetFork); - - l1Messenger = ICrossDomainMessenger(L1_MESSENGER_ADDRESS); - l1Mailbox = new TestMailbox(MAINNET_DOMAIN); - - opHook = new OPStackHook( - address(l1Mailbox), - OPTIMISM_DOMAIN, - TypeCasts.addressToBytes32(address(opISM)), + function deployHook() public { + originMailbox = new TestMailbox(ORIGIN_DOMAIN); + hook = new OPStackHook( + address(originMailbox), + DESTINATION_DOMAIN, + TypeCasts.addressToBytes32(address(ism)), L1_MESSENGER_ADDRESS ); - - vm.makePersistent(address(opHook)); } - function deployOPStackIsm() public { - vm.selectFork(optimismFork); - - l2Messenger = IL2CrossDomainMessenger(L2_MESSENGER_ADDRESS); - opISM = new OPStackIsm(L2_MESSENGER_ADDRESS); - - vm.makePersistent(address(opISM)); + function deployIsm() public { + ism = new OPStackIsm(L2_MESSENGER_ADDRESS); } function deployAll() public { - deployOPStackIsm(); - deployOptimismHook(); + deployIsm(); + deployHook(); - vm.selectFork(optimismFork); - - opISM.setAuthorizedHook(TypeCasts.addressToBytes32(address(opHook))); + ism.setAuthorizedHook(TypeCasts.addressToBytes32(address(hook))); + l2Messenger.setXDomainMessageSender(address(hook)); // for sending value - vm.deal( - AddressAliasHelper.applyL1ToL2Alias(L1_MESSENGER_ADDRESS), - 2 ** 255 - ); + // vm.deal(AddressAliasHelper.applyL1ToL2Alias(L1_MESSENGER_ADDRESS), 2 ** 255); } - /////////////////////////////////////////////////////////////////// - /// FORK TESTS /// - /////////////////////////////////////////////////////////////////// - /* ============ hook.quoteDispatch ============ */ - function testFork_quoteDispatch() public { - deployAll(); - - vm.selectFork(mainnetFork); - - assertEq(opHook.quoteDispatch(testMetadata, encodedMessage), 0); + function test_quoteDispatch() public view { + assertEq(hook.quoteDispatch(testMetadata, encodedMessage), 0); } /* ============ hook.postDispatch ============ */ - function testFork_postDispatch() public { - deployAll(); - - vm.selectFork(mainnetFork); - - bytes memory encodedHookData = abi.encodeCall( - AbstractMessageIdAuthorizedIsm.verifyMessageId, - (messageId) - ); - - uint40 testNonce = 123; - l1Mailbox.updateLatestDispatchedId(messageId); - - vm.expectEmit(true, true, true, false, L1_MESSENGER_ADDRESS); - emit SentMessage( - address(opISM), - address(opHook), - encodedHookData, - testNonce, - DEFAULT_GAS_LIMIT + function _expectOriginExternalBridgeCall( + bytes memory _encodedHookData + ) internal override { + vm.expectCall( + L1_MESSENGER_ADDRESS, + abi.encodeCall( + ICrossDomainMessenger.sendMessage, + (address(ism), _encodedHookData, uint32(GAS_QUOTE)) + ) ); - opHook.postDispatch(testMetadata, encodedMessage); } - function testFork_postDispatch_RevertWhen_ChainIDNotSupported() public { - deployAll(); - - vm.selectFork(mainnetFork); - - bytes memory message = MessageUtils.formatMessage( - OPTIMISM_VERSION, - uint32(0), - MAINNET_DOMAIN, - TypeCasts.addressToBytes32(address(this)), - 11, // wrong domain - TypeCasts.addressToBytes32(address(testRecipient)), - testMessage - ); - - l1Mailbox.updateLatestDispatchedId(Message.id(message)); - vm.expectRevert( - "AbstractMessageIdAuthHook: invalid destination domain" + function _externalBridgeDestinationCall( + bytes memory _encodedHookData, + uint256 _msgValue + ) internal override { + vm.deal(L2_MESSENGER_ADDRESS, _msgValue); + l2Messenger.relayMessage( + 0, + address(hook), + address(ism), + _msgValue, + uint32(GAS_QUOTE), + _encodedHookData ); - opHook.postDispatch(testMetadata, message); } - function testFork_postDispatch_RevertWhen_TooMuchValue() public { - deployAll(); - - vm.selectFork(mainnetFork); - - vm.deal(address(this), uint256(2 ** 255 + 1)); - bytes memory excessValueMetadata = StandardHookMetadata - .overrideMsgValue(uint256(2 ** 255 + 1)); - - l1Mailbox.updateLatestDispatchedId(messageId); - vm.expectRevert( - "AbstractMessageIdAuthHook: msgValue must be less than 2 ** 255" - ); - opHook.postDispatch(excessValueMetadata, encodedMessage); + function _encodeExternalDestinationBridgeCall( + address _from, + address _to, + uint256 _msgValue, + bytes32 _messageId + ) internal pure override returns (bytes memory) { + return new bytes(0); } - function testFork_postDispatch_RevertWhen_NotLastDispatchedMessage() - public - { - deployAll(); + // SKIP - no external bridge call + function test_verifyMessageId_externalBridgeCall() public override {} - vm.selectFork(mainnetFork); + function test_verify_msgValue_externalBridgeCall() public override {} - vm.expectRevert( - "AbstractMessageIdAuthHook: message not latest dispatched" - ); - opHook.postDispatch(testMetadata, encodedMessage); - } + function test_verify_revertsWhen_invalidIsm() public override {} /* ============ ISM.verifyMessageId ============ */ - function testFork_verifyMessageId() public { - deployAll(); - - vm.selectFork(optimismFork); - - bytes memory encodedHookData = abi.encodeCall( - AbstractMessageIdAuthorizedIsm.verifyMessageId, - (messageId) - ); - - (uint240 nonce, uint16 version) = decodeVersionedNonce( - l2Messenger.messageNonce() - ); - uint256 versionedNonce = encodeVersionedNonce(nonce + 1, version); - - bytes32 versionedHash = hashCrossDomainMessageV1( - versionedNonce, - address(opHook), - address(opISM), - 0, - DEFAULT_GAS_LIMIT, - encodedHookData - ); - - vm.startPrank( - AddressAliasHelper.applyL1ToL2Alias(L1_MESSENGER_ADDRESS) - ); - - vm.expectEmit(true, false, false, false, address(opISM)); - emit ReceivedMessage(messageId); - - vm.expectEmit(true, false, false, false, L2_MESSENGER_ADDRESS); - emit RelayedMessage(versionedHash); - - l2Messenger.relayMessage( - versionedNonce, - address(opHook), - address(opISM), - 0, - DEFAULT_GAS_LIMIT, - encodedHookData - ); - - assertTrue(opISM.verifiedMessages(messageId).isBitSet(255)); - vm.stopPrank(); - } - - function testFork_verifyMessageId_RevertWhen_NotAuthorized() public { - deployAll(); - - vm.selectFork(optimismFork); - + function test_verify_revertsWhen_notAuthorizedHook() public override { // needs to be called by the canonical messenger on Optimism vm.expectRevert(NotCrossChainCall.selector); - opISM.verifyMessageId(messageId); - - // set the xDomainMessageSender storage slot as alice - bytes32 key = bytes32(uint256(204)); - bytes32 value = TypeCasts.addressToBytes32(alice); - vm.store(address(l2Messenger), key, value); + ism.verifyMessageId(messageId); vm.startPrank(L2_MESSENGER_ADDRESS); + _setExternalOriginSender(address(this)); // needs to be called by the authorized hook contract on Ethereum vm.expectRevert( "AbstractMessageIdAuthorizedIsm: sender is not the hook" ); - opISM.verifyMessageId(messageId); + ism.verifyMessageId(messageId); } - /* ============ ISM.verify ============ */ - - function testFork_verify() public { - deployAll(); - - vm.selectFork(optimismFork); - - orchestrateRelayMessage(0, messageId); - - bool verified = opISM.verify(new bytes(0), encodedMessage); - assertTrue(verified); + function _setExternalOriginSender(address _sender) internal override { + l2Messenger.setXDomainMessageSender(_sender); } - /// forge-config: default.fuzz.runs = 10 - function testFork_verify_WithValue(uint256 _msgValue) public { - _msgValue = bound(_msgValue, 0, 2 ** 254); - deployAll(); - - orchestrateRelayMessage(_msgValue, messageId); - - bool verified = opISM.verify(new bytes(0), encodedMessage); - assertTrue(verified); - - assertEq(address(opISM).balance, 0); - assertEq(address(testRecipient).balance, _msgValue); - } - - /// forge-config: default.fuzz.runs = 10 - function testFork_verify_valueAlreadyClaimed(uint256 _msgValue) public { - _msgValue = bound(_msgValue, 0, 2 ** 254); - deployAll(); - - orchestrateRelayMessage(_msgValue, messageId); - - bool verified = opISM.verify(new bytes(0), encodedMessage); - assertTrue(verified); - - assertEq(address(opISM).balance, 0); - assertEq(address(testRecipient).balance, _msgValue); - - // send more value to the ISM - vm.deal(address(opISM), _msgValue); - - verified = opISM.verify(new bytes(0), encodedMessage); - // verified still true - assertTrue(verified); - - assertEq(address(opISM).balance, _msgValue); - // value which was already sent - assertEq(address(testRecipient).balance, _msgValue); - } - - function testFork_verify_tooMuchValue() public { - deployAll(); + /* ============ ISM.verify ============ */ + function test_verify_tooMuchValue() public { uint256 _msgValue = 2 ** 255 + 1; - vm.expectEmit(false, false, false, false, address(l2Messenger)); - emit FailedRelayedMessage(messageId); - orchestrateRelayMessage(_msgValue, messageId); - - bool verified = opISM.verify(new bytes(0), encodedMessage); - assertFalse(verified); - - assertEq(address(opISM).balance, 0); - assertEq(address(testRecipient).balance, 0); - } - - // sending over invalid message - function testFork_verify_RevertWhen_HyperlaneInvalidMessage() public { - deployAll(); - - orchestrateRelayMessage(0, messageId); - - bytes memory invalidMessage = MessageUtils.formatMessage( - HYPERLANE_VERSION, - uint8(0), - MAINNET_DOMAIN, - TypeCasts.addressToBytes32(address(this)), - OPTIMISM_DOMAIN, - TypeCasts.addressToBytes32(address(this)), // wrong recipient - testMessage + vm.expectRevert( + "AbstractMessageIdAuthorizedIsm: msg.value must be less than 2^255" ); - bool verified = opISM.verify(new bytes(0), invalidMessage); - assertFalse(verified); - } + _externalBridgeDestinationCall(_encodeHookData(messageId), _msgValue); - // invalid messageID in postDispatch - function testFork_verify_RevertWhen_InvalidOptimismMessageID() public { - deployAll(); - vm.selectFork(optimismFork); - - bytes memory invalidMessage = MessageUtils.formatMessage( - HYPERLANE_VERSION, - uint8(0), - MAINNET_DOMAIN, - TypeCasts.addressToBytes32(address(this)), - OPTIMISM_DOMAIN, - TypeCasts.addressToBytes32(address(this)), - testMessage - ); - bytes32 _messageId = Message.id(invalidMessage); - orchestrateRelayMessage(0, _messageId); + assertFalse(ism.isVerified(encodedMessage)); - bool verified = opISM.verify(new bytes(0), encodedMessage); - assertFalse(verified); + assertEq(address(ism).balance, 0); + assertEq(address(testRecipient).balance, 0); } /* ============ helper functions ============ */ - - function _encodeTestMessage() internal view returns (bytes memory) { - return - MessageUtils.formatMessage( - HYPERLANE_VERSION, - uint32(0), - MAINNET_DOMAIN, - TypeCasts.addressToBytes32(address(this)), - OPTIMISM_DOMAIN, - TypeCasts.addressToBytes32(address(testRecipient)), - testMessage - ); - } - - /// @dev from eth-optimism/contracts-bedrock/contracts/libraries/Hashing.sol - /// @notice Hashes a cross domain message based on the V1 (current) encoding. - /// @param _nonce Message nonce. - /// @param _sender Address of the sender of the message. - /// @param _target Address of the target of the message. - /// @param _value ETH value to send to the target. - /// @param _gasLimit Gas limit to use for the message. - /// @param _data Data to send with the message. - /// @return Hashed cross domain message. - function hashCrossDomainMessageV1( - uint256 _nonce, - address _sender, - address _target, - uint256 _value, - uint256 _gasLimit, - bytes memory _data - ) internal pure returns (bytes32) { - return - keccak256( - encodeCrossDomainMessageV1( - _nonce, - _sender, - _target, - _value, - _gasLimit, - _data - ) - ); - } - - /// @dev from eth-optimism/contracts-bedrock/contracts/libraries/Encoding.sol - /// @notice Encodes a cross domain message based on the V1 (current) encoding. - /// @param _nonce Message nonce. - /// @param _sender Address of the sender of the message. - /// @param _target Address of the target of the message. - /// @param _value ETH value to send to the target. - /// @param _gasLimit Gas limit to use for the message. - /// @param _data Data to send with the message. - /// @return Encoded cross domain message. - function encodeCrossDomainMessageV1( - uint256 _nonce, - address _sender, - address _target, - uint256 _value, - uint256 _gasLimit, - bytes memory _data - ) internal pure returns (bytes memory) { - return - abi.encodeWithSignature( - "relayMessage(uint256,address,address,uint256,uint256,bytes)", - _nonce, - _sender, - _target, - _value, - _gasLimit, - _data - ); - } - - /// @dev from eth-optimism/contracts-bedrock/contracts/libraries/Encoding.sol - /// @notice Adds a version number into the first two bytes of a message nonce. - /// @param _nonce Message nonce to encode into. - /// @param _version Version number to encode into the message nonce. - /// @return Message nonce with version encoded into the first two bytes. - function encodeVersionedNonce( - uint240 _nonce, - uint16 _version - ) internal pure returns (uint256) { - uint256 nonce; - assembly { - nonce := or(shl(240, _version), _nonce) - } - return nonce; - } - - /// @dev from eth-optimism/contracts-bedrock/contracts/libraries/Encoding.sol - /// @notice Pulls the version out of a version-encoded nonce. - /// @param _nonce Message nonce with version encoded into the first two bytes. - /// @return Nonce without encoded version. - /// @return Version of the message. - function decodeVersionedNonce( - uint256 _nonce - ) internal pure returns (uint240, uint16) { - uint240 nonce; - uint16 version; - assembly { - nonce := and( - _nonce, - 0x0000ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff - ) - version := shr(240, _nonce) - } - return (nonce, version); - } - - function orchestrateRelayMessage( - uint256 _msgValue, - bytes32 _messageId - ) internal { - vm.selectFork(optimismFork); - - bytes memory encodedHookData = abi.encodeCall( - AbstractMessageIdAuthorizedIsm.verifyMessageId, - (_messageId) - ); - - (uint240 nonce, uint16 version) = decodeVersionedNonce( - l2Messenger.messageNonce() - ); - uint256 versionedNonce = encodeVersionedNonce(nonce + 1, version); - - vm.deal( - AddressAliasHelper.applyL1ToL2Alias(L1_MESSENGER_ADDRESS), - 2 ** 256 - 1 - ); - vm.prank(AddressAliasHelper.applyL1ToL2Alias(L1_MESSENGER_ADDRESS)); - l2Messenger.relayMessage{value: _msgValue}( - versionedNonce, - address(opHook), - address(opISM), - _msgValue, - DEFAULT_GAS_LIMIT, - encodedHookData - ); - } } From ac3613e132d21ca580dbbb56306e2c407e63001b Mon Sep 17 00:00:00 2001 From: -f Date: Wed, 18 Sep 2024 20:47:05 +0530 Subject: [PATCH 16/47] minor edits --- solidity/test/isms/ArbL2ToL1Ism.t.sol | 7 ++----- solidity/test/isms/ERC5164ISM.t.sol | 5 +---- solidity/test/isms/ExternalBridgeTest.sol | 18 +++++------------- solidity/test/isms/OPL2ToL1Ism.t.sol | 7 ++----- solidity/test/isms/OPStackIsm.t.sol | 4 +--- 5 files changed, 11 insertions(+), 30 deletions(-) diff --git a/solidity/test/isms/ArbL2ToL1Ism.t.sol b/solidity/test/isms/ArbL2ToL1Ism.t.sol index d3f198818a..51ccd62ac0 100644 --- a/solidity/test/isms/ArbL2ToL1Ism.t.sol +++ b/solidity/test/isms/ArbL2ToL1Ism.t.sol @@ -24,15 +24,12 @@ contract ArbL2ToL1IsmTest is ExternalBridgeTest { MockArbBridge internal arbBridge; function setUp() public override { - ORIGIN_DOMAIN = 42161; - DESTINATION_DOMAIN = 1; - GAS_QUOTE = 120_000; - super.setUp(); - // Arbitrum bridge mock setup + GAS_QUOTE = 120_000; vm.etch(L2_ARBSYS_ADDRESS, address(new MockArbSys()).code); deployAll(); + super.setUp(); } /////////////////////////////////////////////////////////////////// diff --git a/solidity/test/isms/ERC5164ISM.t.sol b/solidity/test/isms/ERC5164ISM.t.sol index f00ef792f8..df147246a0 100644 --- a/solidity/test/isms/ERC5164ISM.t.sol +++ b/solidity/test/isms/ERC5164ISM.t.sol @@ -41,10 +41,6 @@ contract ERC5164IsmTest is ExternalBridgeTest { /////////////////////////////////////////////////////////////////// function setUp() public override { - ORIGIN_DOMAIN = 1; - DESTINATION_DOMAIN = 2; - super.setUp(); - dispatcher = new MockMessageDispatcher(); executor = new MockMessageExecutor(); originMailbox = new TestMailbox(ORIGIN_DOMAIN); @@ -56,6 +52,7 @@ contract ERC5164IsmTest is ExternalBridgeTest { address(dispatcher) ); ism.setAuthorizedHook(TypeCasts.addressToBytes32(address(hook))); + super.setUp(); } /////////////////////////////////////////////////////////////////// diff --git a/solidity/test/isms/ExternalBridgeTest.sol b/solidity/test/isms/ExternalBridgeTest.sol index 021a83f4d4..8ef14d19e2 100644 --- a/solidity/test/isms/ExternalBridgeTest.sol +++ b/solidity/test/isms/ExternalBridgeTest.sol @@ -16,8 +16,8 @@ abstract contract ExternalBridgeTest is Test { using MessageUtils for bytes; uint8 internal constant HYPERLANE_VERSION = 1; - uint32 internal ORIGIN_DOMAIN; - uint32 internal DESTINATION_DOMAIN; + uint32 internal constant ORIGIN_DOMAIN = 1; + uint32 internal constant DESTINATION_DOMAIN = 2; uint256 internal GAS_QUOTE; bytes internal unauthorizedHookError; @@ -52,13 +52,9 @@ abstract contract ExternalBridgeTest is Test { } function test_postDispatch_revertWhen_chainIDNotSupported() public { - bytes memory message = MessageUtils.formatMessage( - 0, - uint32(0), - DESTINATION_DOMAIN, + bytes memory message = originMailbox.buildOutboundMessage( + 3, TypeCasts.addressToBytes32(address(this)), - 3, // wrong domain - TypeCasts.addressToBytes32(address(testRecipient)), testMessage ); @@ -223,11 +219,7 @@ abstract contract ExternalBridgeTest is Test { function _encodeTestMessage() internal view returns (bytes memory) { return - MessageUtils.formatMessage( - HYPERLANE_VERSION, - uint32(0), - ORIGIN_DOMAIN, - TypeCasts.addressToBytes32(address(this)), + originMailbox.buildOutboundMessage( DESTINATION_DOMAIN, TypeCasts.addressToBytes32(address(testRecipient)), testMessage diff --git a/solidity/test/isms/OPL2ToL1Ism.t.sol b/solidity/test/isms/OPL2ToL1Ism.t.sol index df564699c4..795469ed8a 100644 --- a/solidity/test/isms/OPL2ToL1Ism.t.sol +++ b/solidity/test/isms/OPL2ToL1Ism.t.sol @@ -29,18 +29,15 @@ contract OPL2ToL1IsmTest is ExternalBridgeTest { /////////////////////////////////////////////////////////////////// function setUp() public override { - ORIGIN_DOMAIN = 10; - DESTINATION_DOMAIN = 1; - GAS_QUOTE = 120_000; - super.setUp(); - // Optimism messenger mock setup + GAS_QUOTE = 120_000; vm.etch( L2_MESSENGER_ADDRESS, address(new MockOptimismMessenger()).code ); deployAll(); + super.setUp(); } function deployHook() public { diff --git a/solidity/test/isms/OPStackIsm.t.sol b/solidity/test/isms/OPStackIsm.t.sol index 1b70d402fb..0b059e9d65 100644 --- a/solidity/test/isms/OPStackIsm.t.sol +++ b/solidity/test/isms/OPStackIsm.t.sol @@ -49,10 +49,7 @@ contract OPStackIsmTest is ExternalBridgeTest { event ReceivedMessage(bytes32 indexed messageId); function setUp() public override { - ORIGIN_DOMAIN = 1; - DESTINATION_DOMAIN = 10; GAS_QUOTE = 1_920_000; // optimism subsidized gas limit - super.setUp(); vm.etch( L1_MESSENGER_ADDRESS, @@ -66,6 +63,7 @@ contract OPStackIsmTest is ExternalBridgeTest { l2Messenger = MockOptimismMessenger(L2_MESSENGER_ADDRESS); deployAll(); + super.setUp(); } /////////////////////////////////////////////////////////////////// From 4d293ea5b4d2221e67b882ebe9b61fa1939c802c Mon Sep 17 00:00:00 2001 From: -f Date: Wed, 18 Sep 2024 21:01:51 +0530 Subject: [PATCH 17/47] minor fixes --- solidity/test/isms/ArbL2ToL1Ism.t.sol | 6 ++++-- solidity/test/isms/ExternalBridgeTest.sol | 21 ++++++++++++--------- solidity/test/isms/OPL2ToL1Ism.t.sol | 6 ++++-- solidity/test/isms/OPStackIsm.t.sol | 5 ++++- 4 files changed, 24 insertions(+), 14 deletions(-) diff --git a/solidity/test/isms/ArbL2ToL1Ism.t.sol b/solidity/test/isms/ArbL2ToL1Ism.t.sol index 51ccd62ac0..a251cfda4d 100644 --- a/solidity/test/isms/ArbL2ToL1Ism.t.sol +++ b/solidity/test/isms/ArbL2ToL1Ism.t.sol @@ -102,9 +102,11 @@ contract ArbL2ToL1IsmTest is ExternalBridgeTest { ); } - function _setExternalOriginSender(address _sender) internal override { - unauthorizedHookError = "ArbL2ToL1Ism: l2Sender != authorizedHook"; + function _setExternalOriginSender( + address _sender + ) internal override returns (bytes memory unauthorizedHookErrorMsg) { arbBridge.setL2ToL1Sender(_sender); + return "ArbL2ToL1Ism: l2Sender != authorizedHook"; } function _encodeOutboxTx( diff --git a/solidity/test/isms/ExternalBridgeTest.sol b/solidity/test/isms/ExternalBridgeTest.sol index 8ef14d19e2..624c08581d 100644 --- a/solidity/test/isms/ExternalBridgeTest.sol +++ b/solidity/test/isms/ExternalBridgeTest.sol @@ -18,10 +18,9 @@ abstract contract ExternalBridgeTest is Test { uint8 internal constant HYPERLANE_VERSION = 1; uint32 internal constant ORIGIN_DOMAIN = 1; uint32 internal constant DESTINATION_DOMAIN = 2; + uint256 internal constant MAX_MSG_VALUE = 2 ** 255 - 1; uint256 internal GAS_QUOTE; - bytes internal unauthorizedHookError; - TestMailbox internal originMailbox; TestRecipient internal testRecipient; @@ -73,7 +72,7 @@ abstract contract ExternalBridgeTest is Test { } function test_postDispatch_revertWhen_tooMuchValue() public { - vm.deal(address(this), uint256(2 ** 255 + 1)); + vm.deal(address(this), uint256(MAX_MSG_VALUE + 1)); bytes memory excessValueMetadata = StandardHookMetadata .overrideMsgValue(uint256(2 ** 255 + 1)); @@ -104,7 +103,7 @@ abstract contract ExternalBridgeTest is Test { messageId ); - ism.verify(externalCalldata, encodedMessage); + assertTrue(ism.verify(externalCalldata, encodedMessage)); assertTrue(ism.isVerified(encodedMessage)); } @@ -150,7 +149,9 @@ abstract contract ExternalBridgeTest is Test { } function test_verify_revertsWhen_notAuthorizedHook() public virtual { - _setExternalOriginSender(address(this)); + bytes memory unauthorizedHookErrorMsg = _setExternalOriginSender( + address(this) + ); bytes memory externalCalldata = _encodeExternalDestinationBridgeCall( address(this), @@ -160,7 +161,7 @@ abstract contract ExternalBridgeTest is Test { ); // external call - vm.expectRevert(unauthorizedHookError); + vm.expectRevert(unauthorizedHookErrorMsg); assertFalse(ism.verify(externalCalldata, encodedMessage)); // async call vm.expectRevert(NotCrossChainCall.selector); @@ -196,7 +197,7 @@ abstract contract ExternalBridgeTest is Test { /// forge-config: default.fuzz.runs = 10 function test_verify_valueAlreadyClaimed(uint256 _msgValue) public virtual { - _msgValue = bound(_msgValue, 0, 2 ** 254); + _msgValue = bound(_msgValue, 0, MAX_MSG_VALUE); _externalBridgeDestinationCall(_encodeHookData(messageId), _msgValue); bool verified = ism.verify(new bytes(0), encodedMessage); @@ -236,7 +237,7 @@ abstract contract ExternalBridgeTest is Test { ); } - // try catch block to prevent revert + // wrapper function needed for _externalBridgeDestinationCall because try catch cannot call an internal function function externalBridgeDestinationCallWrapper( bytes memory _encodedHookData, uint256 _msgValue @@ -260,5 +261,7 @@ abstract contract ExternalBridgeTest is Test { bytes32 _messageId ) internal virtual returns (bytes memory); - function _setExternalOriginSender(address _sender) internal virtual {} + function _setExternalOriginSender( + address _sender + ) internal virtual returns (bytes memory) {} } diff --git a/solidity/test/isms/OPL2ToL1Ism.t.sol b/solidity/test/isms/OPL2ToL1Ism.t.sol index 795469ed8a..d829e0211e 100644 --- a/solidity/test/isms/OPL2ToL1Ism.t.sol +++ b/solidity/test/isms/OPL2ToL1Ism.t.sol @@ -92,9 +92,11 @@ contract OPL2ToL1IsmTest is ExternalBridgeTest { return _encodeFinalizeWithdrawalTx(_to, _msgValue, _messageId); } - function _setExternalOriginSender(address _sender) internal override { - unauthorizedHookError = "AbstractMessageIdAuthorizedIsm: sender is not the hook"; + function _setExternalOriginSender( + address _sender + ) internal override returns (bytes memory) { l1Messenger.setXDomainMessageSender(_sender); + return "AbstractMessageIdAuthorizedIsm: sender is not the hook"; } function _externalBridgeDestinationCall( diff --git a/solidity/test/isms/OPStackIsm.t.sol b/solidity/test/isms/OPStackIsm.t.sol index 0b059e9d65..d87d19ff7f 100644 --- a/solidity/test/isms/OPStackIsm.t.sol +++ b/solidity/test/isms/OPStackIsm.t.sol @@ -162,8 +162,11 @@ contract OPStackIsmTest is ExternalBridgeTest { ism.verifyMessageId(messageId); } - function _setExternalOriginSender(address _sender) internal override { + function _setExternalOriginSender( + address _sender + ) internal override returns (bytes memory) { l2Messenger.setXDomainMessageSender(_sender); + return ""; } /* ============ ISM.verify ============ */ From fb66306fc74a81f2eb3a42fe75317ac63306635c Mon Sep 17 00:00:00 2001 From: -f Date: Wed, 18 Sep 2024 21:43:05 +0530 Subject: [PATCH 18/47] override inconsistent tests --- solidity/test/isms/ERC5164ISM.t.sol | 6 ++++- solidity/test/isms/ExternalBridgeTest.sol | 27 ++++++++++++----------- solidity/test/isms/OPStackIsm.t.sol | 17 ++++++++------ 3 files changed, 29 insertions(+), 21 deletions(-) diff --git a/solidity/test/isms/ERC5164ISM.t.sol b/solidity/test/isms/ERC5164ISM.t.sol index df147246a0..75c79fb827 100644 --- a/solidity/test/isms/ERC5164ISM.t.sol +++ b/solidity/test/isms/ERC5164ISM.t.sol @@ -116,7 +116,11 @@ contract ERC5164IsmTest is ExternalBridgeTest { ); } - function test_postDispatch_RevertWhen_msgValueNotAllowed() public payable { + function test_verify_revertWhen_invalidMetadata() public override { + assertFalse(ism.verify(new bytes(0), encodedMessage)); + } + + function test_postDispatch_revertWhen_msgValueNotAllowed() public payable { originMailbox.updateLatestDispatchedId(messageId); vm.expectRevert("ERC5164Hook: no value allowed"); diff --git a/solidity/test/isms/ExternalBridgeTest.sol b/solidity/test/isms/ExternalBridgeTest.sol index 624c08581d..8db043fcf0 100644 --- a/solidity/test/isms/ExternalBridgeTest.sol +++ b/solidity/test/isms/ExternalBridgeTest.sol @@ -40,6 +40,12 @@ abstract contract ExternalBridgeTest is Test { messageId = Message.id(encodedMessage); } + /* ============ hook.quoteDispatch ============ */ + + function test_quoteDispatch() public view { + assertEq(hook.quoteDispatch(testMetadata, encodedMessage), GAS_QUOTE); + } + /* ============ Hook.postDispatch ============ */ function test_postDispatch() public { @@ -110,11 +116,8 @@ abstract contract ExternalBridgeTest is Test { /* ============ ISM.verify ============ */ function test_verify_revertWhen_invalidMetadata() public virtual { - bool isValid; - try ism.verify(new bytes(0), encodedMessage) returns (bool _isValid) { - isValid = _isValid; - } catch {} - assertFalse(isValid); + vm.expectRevert(); + assertFalse(ism.verify(new bytes(0), encodedMessage)); } function test_verify_msgValue_asyncCall() public virtual { @@ -180,17 +183,15 @@ abstract contract ExternalBridgeTest is Test { ); // external call - bool isValid; - try ism.verify(externalCalldata, encodedMessage) returns ( - bool _isValid - ) { - isValid = _isValid; - } catch {} - assertFalse(isValid); + vm.expectRevert(); + assertFalse(ism.verify(externalCalldata, encodedMessage)); // async call - native bridges might have try catch block to prevent revert try - this.externalBridgeDestinationCallWrapper(externalCalldata, 0) + this.externalBridgeDestinationCallWrapper( + _encodeHookData(incorrectMessageId), + 0 + ) {} catch {} assertFalse(ism.isVerified(testMessage)); } diff --git a/solidity/test/isms/OPStackIsm.t.sol b/solidity/test/isms/OPStackIsm.t.sol index d87d19ff7f..6694b7ab8a 100644 --- a/solidity/test/isms/OPStackIsm.t.sol +++ b/solidity/test/isms/OPStackIsm.t.sol @@ -49,7 +49,7 @@ contract OPStackIsmTest is ExternalBridgeTest { event ReceivedMessage(bytes32 indexed messageId); function setUp() public override { - GAS_QUOTE = 1_920_000; // optimism subsidized gas limit + GAS_QUOTE = 0; vm.etch( L1_MESSENGER_ADDRESS, @@ -90,17 +90,20 @@ contract OPStackIsmTest is ExternalBridgeTest { ism.setAuthorizedHook(TypeCasts.addressToBytes32(address(hook))); l2Messenger.setXDomainMessageSender(address(hook)); - // for sending value - // vm.deal(AddressAliasHelper.applyL1ToL2Alias(L1_MESSENGER_ADDRESS), 2 ** 255); } - /* ============ hook.quoteDispatch ============ */ + function test_verify_revertWhen_invalidMetadata() public override { + assertFalse(ism.verify(new bytes(0), encodedMessage)); + } + + function test_verify_revertsWhen_incorrectMessageId() public override { + bytes32 incorrectMessageId = keccak256("incorrect message id"); - function test_quoteDispatch() public view { - assertEq(hook.quoteDispatch(testMetadata, encodedMessage), 0); + _externalBridgeDestinationCall(_encodeHookData(incorrectMessageId), 0); + assertFalse(ism.isVerified(testMessage)); } - /* ============ hook.postDispatch ============ */ + /* ============ helper functions ============ */ function _expectOriginExternalBridgeCall( bytes memory _encodedHookData From fb089e2ac1386213445a554c9f20abc19dfee159 Mon Sep 17 00:00:00 2001 From: -f Date: Fri, 20 Sep 2024 15:53:00 +0530 Subject: [PATCH 19/47] opstack change quote --- solidity/test/isms/OPStackIsm.t.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/solidity/test/isms/OPStackIsm.t.sol b/solidity/test/isms/OPStackIsm.t.sol index 6694b7ab8a..45c818ec3c 100644 --- a/solidity/test/isms/OPStackIsm.t.sol +++ b/solidity/test/isms/OPStackIsm.t.sol @@ -112,7 +112,7 @@ contract OPStackIsmTest is ExternalBridgeTest { L1_MESSENGER_ADDRESS, abi.encodeCall( ICrossDomainMessenger.sendMessage, - (address(ism), _encodedHookData, uint32(GAS_QUOTE)) + (address(ism), _encodedHookData, uint32(DEFAULT_GAS_LIMIT)) ) ); } From e6c5b10d598c87208098bbb2a8c9b8c447683665 Mon Sep 17 00:00:00 2001 From: -f Date: Fri, 20 Sep 2024 16:00:59 +0530 Subject: [PATCH 20/47] fix tests --- solidity/contracts/isms/hook/OPL2ToL1Ism.sol | 2 +- solidity/test/isms/ERC5164ISM.t.sol | 2 ++ solidity/test/isms/ExternalBridgeTest.sol | 2 +- solidity/test/isms/OPStackIsm.t.sol | 10 ++++++---- 4 files changed, 10 insertions(+), 6 deletions(-) diff --git a/solidity/contracts/isms/hook/OPL2ToL1Ism.sol b/solidity/contracts/isms/hook/OPL2ToL1Ism.sol index cc30c023ce..ef39868611 100644 --- a/solidity/contracts/isms/hook/OPL2ToL1Ism.sol +++ b/solidity/contracts/isms/hook/OPL2ToL1Ism.sol @@ -68,7 +68,7 @@ contract OPL2ToL1Ism is ) external override returns (bool) { if (!isVerified(message)) { _verifyWithPortalCall(metadata, message); - return isVerified(message); + require(isVerified(message), "OPL2ToL1Ism: message not verified"); } releaseValueToRecipient(message); return true; diff --git a/solidity/test/isms/ERC5164ISM.t.sol b/solidity/test/isms/ERC5164ISM.t.sol index 75c79fb827..8063979f49 100644 --- a/solidity/test/isms/ERC5164ISM.t.sol +++ b/solidity/test/isms/ERC5164ISM.t.sol @@ -150,6 +150,8 @@ contract ERC5164IsmTest is ExternalBridgeTest { function test_verify_valueAlreadyClaimed(uint256) public override {} + function test_verify_false_arbitraryCall() public override {} + /* ============ helper functions ============ */ function _externalBridgeDestinationCall( diff --git a/solidity/test/isms/ExternalBridgeTest.sol b/solidity/test/isms/ExternalBridgeTest.sol index 4096d6f7ad..1fa4090650 100644 --- a/solidity/test/isms/ExternalBridgeTest.sol +++ b/solidity/test/isms/ExternalBridgeTest.sol @@ -217,7 +217,7 @@ abstract contract ExternalBridgeTest is Test { assertEq(address(testRecipient).balance, _msgValue); } - function test_verify_false_arbitraryCall() public { + function test_verify_false_arbitraryCall() public virtual { bytes memory incorrectCalldata = _encodeExternalDestinationBridgeCall( address(hook), address(this), diff --git a/solidity/test/isms/OPStackIsm.t.sol b/solidity/test/isms/OPStackIsm.t.sol index 45c818ec3c..3230e59b82 100644 --- a/solidity/test/isms/OPStackIsm.t.sol +++ b/solidity/test/isms/OPStackIsm.t.sol @@ -133,10 +133,10 @@ contract OPStackIsmTest is ExternalBridgeTest { } function _encodeExternalDestinationBridgeCall( - address _from, - address _to, - uint256 _msgValue, - bytes32 _messageId + address /*_from*/, + address /*_to*/, + uint256 /*_msgValue*/, + bytes32 /*_messageId*/ ) internal pure override returns (bytes memory) { return new bytes(0); } @@ -148,6 +148,8 @@ contract OPStackIsmTest is ExternalBridgeTest { function test_verify_revertsWhen_invalidIsm() public override {} + function test_verify_false_arbitraryCall() public override {} + /* ============ ISM.verifyMessageId ============ */ function test_verify_revertsWhen_notAuthorizedHook() public override { From 08dda88c91969cf1979cf7003b5ec83fd73efd0d Mon Sep 17 00:00:00 2001 From: -f Date: Fri, 20 Sep 2024 16:04:03 +0530 Subject: [PATCH 21/47] remove reduntant arbitrary call --- solidity/contracts/mock/MockOptimism.sol | 2 -- 1 file changed, 2 deletions(-) diff --git a/solidity/contracts/mock/MockOptimism.sol b/solidity/contracts/mock/MockOptimism.sol index 3ac017f1ad..f47a82bf2a 100644 --- a/solidity/contracts/mock/MockOptimism.sol +++ b/solidity/contracts/mock/MockOptimism.sol @@ -58,6 +58,4 @@ contract MockOptimismPortal is IOptimismPortal { ); CallLib.call(call); } - - function verifyMessageId(bytes32 messageId) public payable {} } From 37ecb2703151bb0d5372cbac661fbd4d163c50e0 Mon Sep 17 00:00:00 2001 From: -f Date: Fri, 20 Sep 2024 16:06:30 +0530 Subject: [PATCH 22/47] rm --- solidity/test/isms/OPL2ToL1Ism.t.sol | 9 --------- 1 file changed, 9 deletions(-) diff --git a/solidity/test/isms/OPL2ToL1Ism.t.sol b/solidity/test/isms/OPL2ToL1Ism.t.sol index c4c78afd4b..d829e0211e 100644 --- a/solidity/test/isms/OPL2ToL1Ism.t.sol +++ b/solidity/test/isms/OPL2ToL1Ism.t.sol @@ -99,15 +99,6 @@ contract OPL2ToL1IsmTest is ExternalBridgeTest { return "AbstractMessageIdAuthorizedIsm: sender is not the hook"; } - // function test_verify_revertWhen_arbitraryCall() public { - // deployAll(); - - // bytes memory incorrectMetadata = _encodeFinalizeWithdrawalTx(address(portal), 0, messageId); - - // vm.expectRevert("OPL2ToL1Ism: message not verified"); - // ism.verify(incorrectMetadata, encodedMessage); - // } - function _externalBridgeDestinationCall( bytes memory, /*_encodedHookData*/ From 1dd0fc3be80b9e9985846ff4f1a55ffdec25e8ee Mon Sep 17 00:00:00 2001 From: -f Date: Fri, 20 Sep 2024 16:36:15 +0530 Subject: [PATCH 23/47] changeset --- .changeset/itchy-singers-hang.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/itchy-singers-hang.md diff --git a/.changeset/itchy-singers-hang.md b/.changeset/itchy-singers-hang.md new file mode 100644 index 0000000000..97096ff1a9 --- /dev/null +++ b/.changeset/itchy-singers-hang.md @@ -0,0 +1,5 @@ +--- +'@hyperlane-xyz/core': patch +--- + +Patched OPL2ToL1Ism to check for correct messageId for external call in verify From c99eb24d8941975a9e346720c09c1f61d9d1a977 Mon Sep 17 00:00:00 2001 From: -f Date: Sat, 21 Sep 2024 01:35:01 +0530 Subject: [PATCH 24/47] add verifyMessageId --- .../hooks/libs/AbstractMessageIdAuthHook.sol | 2 +- .../hook/AbstractMessageIdAuthorizedIsm.sol | 27 +++++++--------- solidity/contracts/isms/hook/ArbL2ToL1Ism.sol | 7 +++- .../isms/hook/layer-zero/LayerZeroV2Ism.sol | 8 ++++- solidity/contracts/libs/OPL2ToL1Metadata.sol | 6 ++-- solidity/test/isms/ArbL2ToL1Ism.t.sol | 5 +-- solidity/test/isms/ERC5164ISM.t.sol | 6 ++-- solidity/test/isms/ExternalBridgeTest.sol | 32 +++++++++++-------- solidity/test/isms/OPL2ToL1Ism.t.sol | 2 +- solidity/test/isms/OPStackIsm.t.sol | 15 ++++++--- solidity/test/isms/PolygonPosIsm.t.sol | 12 +++---- .../test/isms/layer-zero/LayerZeroV2Ism.t.sol | 10 +++--- 12 files changed, 74 insertions(+), 58 deletions(-) diff --git a/solidity/contracts/hooks/libs/AbstractMessageIdAuthHook.sol b/solidity/contracts/hooks/libs/AbstractMessageIdAuthHook.sol index a9e3a41e94..2c1e8206d3 100644 --- a/solidity/contracts/hooks/libs/AbstractMessageIdAuthHook.sol +++ b/solidity/contracts/hooks/libs/AbstractMessageIdAuthHook.sol @@ -84,7 +84,7 @@ abstract contract AbstractMessageIdAuthHook is ); bytes memory payload = abi.encodeCall( AbstractMessageIdAuthorizedIsm.verifyMessageId, - id + (id, metadata.msgValue(0)) ); _sendMessageId(metadata, payload); } diff --git a/solidity/contracts/isms/hook/AbstractMessageIdAuthorizedIsm.sol b/solidity/contracts/isms/hook/AbstractMessageIdAuthorizedIsm.sol index a8b8baee6c..88cfed97a6 100644 --- a/solidity/contracts/isms/hook/AbstractMessageIdAuthorizedIsm.sol +++ b/solidity/contracts/isms/hook/AbstractMessageIdAuthorizedIsm.sol @@ -38,12 +38,9 @@ abstract contract AbstractMessageIdAuthorizedIsm is using Message for bytes; // ============ Public Storage ============ - /// @notice Maps messageId to whether or not the message has been verified - /// first bit is boolean for verification - /// rest of bits is the amount to send to the recipient - /// @dev bc of the bit packing, we can only send up to 2^255 wei - /// @dev the first bit is reserved for verification and the rest 255 bits are for the msg.value - mapping(bytes32 => uint256) public verifiedMessages; + /// @notice Maps messageId to value to whether or not the message has been verified + mapping(bytes32 => bool) public verifiedMessages; + mapping(bytes32 => uint256) public messageValues; /// @notice Index of verification bit in verifiedMessages uint256 public constant VERIFIED_MASK_INDEX = 255; /// @notice address for the authorized hook @@ -90,11 +87,9 @@ abstract contract AbstractMessageIdAuthorizedIsm is */ function releaseValueToRecipient(bytes calldata message) public { bytes32 messageId = message.id(); - uint256 _msgValue = verifiedMessages[messageId].clearBit( - VERIFIED_MASK_INDEX - ); + uint256 _msgValue = messageValues[messageId]; if (_msgValue > 0) { - verifiedMessages[messageId] -= _msgValue; + messageValues[messageId] = 0; payable(message.recipientAddress()).sendValue(_msgValue); } } @@ -104,9 +99,7 @@ abstract contract AbstractMessageIdAuthorizedIsm is * @param message Message to check. */ function isVerified(bytes calldata message) public view returns (bool) { - bytes32 messageId = message.id(); - // check for the first bit (used for verification) - return verifiedMessages[messageId].isBitSet(VERIFIED_MASK_INDEX); + return verifiedMessages[message.id()]; } /** @@ -114,7 +107,10 @@ abstract contract AbstractMessageIdAuthorizedIsm is * @dev Only callable by the authorized hook. * @param messageId Hyperlane Id of the message. */ - function verifyMessageId(bytes32 messageId) public payable virtual { + function verifyMessageId( + bytes32 messageId, + uint256 msgValue + ) public payable virtual { require( _isAuthorized(), "AbstractMessageIdAuthorizedIsm: sender is not the hook" @@ -124,7 +120,8 @@ abstract contract AbstractMessageIdAuthorizedIsm is "AbstractMessageIdAuthorizedIsm: msg.value must be less than 2^255" ); - verifiedMessages[messageId] = msg.value.setBit(VERIFIED_MASK_INDEX); + verifiedMessages[messageId] = true; + messageValues[messageId] = msgValue; emit ReceivedMessage(messageId); } diff --git a/solidity/contracts/isms/hook/ArbL2ToL1Ism.sol b/solidity/contracts/isms/hook/ArbL2ToL1Ism.sol index a7bd714473..0b93978b78 100644 --- a/solidity/contracts/isms/hook/ArbL2ToL1Ism.sol +++ b/solidity/contracts/isms/hook/ArbL2ToL1Ism.sol @@ -41,6 +41,8 @@ contract ArbL2ToL1Ism is // module type for the ISM uint8 public constant moduleType = uint8(IInterchainSecurityModule.Types.ARB_L2_TO_L1); + // length of the call data for the arbOutbox.executeTransaction function + uint8 public constant CALLDATA_LENGTH = 68; // arbitrum nitro contract on L1 to forward verification IOutbox public arbOutbox; @@ -110,7 +112,10 @@ contract ArbL2ToL1Ism is "ArbL2ToL1Ism: l2Sender != authorizedHook" ); // this data is an abi encoded call of verifyMessageId(bytes32 messageId) - require(data.length == 36, "ArbL2ToL1Ism: invalid data length"); + require( + data.length == CALLDATA_LENGTH, + "ArbL2ToL1Ism: invalid data length" + ); bytes32 messageId = message.id(); bytes32 convertedBytes; assembly { diff --git a/solidity/contracts/isms/hook/layer-zero/LayerZeroV2Ism.sol b/solidity/contracts/isms/hook/layer-zero/LayerZeroV2Ism.sol index 1388f32cf8..9e114da560 100644 --- a/solidity/contracts/isms/hook/layer-zero/LayerZeroV2Ism.sol +++ b/solidity/contracts/isms/hook/layer-zero/LayerZeroV2Ism.sol @@ -66,7 +66,7 @@ contract LayerZeroV2Ism is AbstractMessageIdAuthorizedIsm { address, bytes calldata ) external payable { - verifyMessageId(_messageId(_lzMessage)); + verifyMessageId(_messageId(_lzMessage), _msgValue(_lzMessage)); } // ============ Internal function ============ @@ -82,6 +82,12 @@ contract LayerZeroV2Ism is AbstractMessageIdAuthorizedIsm { return bytes32(_message[FUNC_SELECTOR_OFFSET:]); } + function _msgValue( + bytes calldata _message + ) internal pure returns (uint256) { + return uint256(bytes32(_message[FUNC_SELECTOR_OFFSET + 32:])); + } + /** * @notice Validates criteria to verify a message * @dev this is called by AbstractMessageIdAuthorizedIsm.verifyMessageId diff --git a/solidity/contracts/libs/OPL2ToL1Metadata.sol b/solidity/contracts/libs/OPL2ToL1Metadata.sol index 704ce3d4d9..753550fec3 100644 --- a/solidity/contracts/libs/OPL2ToL1Metadata.sol +++ b/solidity/contracts/libs/OPL2ToL1Metadata.sol @@ -7,7 +7,7 @@ pragma solidity >=0.8.0; */ library OPL2ToL1Metadata { // bottom offset to the start of message id in the metadata - uint256 private constant MESSAGE_ID_OFFSET = 88; + uint256 private constant MESSAGE_ID_OFFSET = 120; // from IOptimismPortal.WithdrawalTransaction // Σ { // nonce = 32 bytes @@ -31,9 +31,9 @@ library OPL2ToL1Metadata { // _data // OFFSET = 32 bytes // LENGTH = 32 bytes - // PADDING + verifyMessageId = 64 bytes + // PADDING + verifyMessageId = 96 bytes // } = 292 bytes - uint256 private constant MESSENGER_CALLDATA_LENGTH = 292; + uint256 private constant MESSENGER_CALLDATA_LENGTH = 324; /** * @notice Returns the message ID. diff --git a/solidity/test/isms/ArbL2ToL1Ism.t.sol b/solidity/test/isms/ArbL2ToL1Ism.t.sol index a251cfda4d..d4b6d14e61 100644 --- a/solidity/test/isms/ArbL2ToL1Ism.t.sol +++ b/solidity/test/isms/ArbL2ToL1Ism.t.sol @@ -115,10 +115,7 @@ contract ArbL2ToL1IsmTest is ExternalBridgeTest { bytes32 _messageId, uint256 _value ) internal view returns (bytes memory) { - bytes memory encodedHookData = abi.encodeCall( - AbstractMessageIdAuthorizedIsm.verifyMessageId, - (_messageId) - ); + bytes memory encodedHookData = _encodeHookData(_messageId, _value); bytes32[] memory proof = new bytes32[](16); return diff --git a/solidity/test/isms/ERC5164ISM.t.sol b/solidity/test/isms/ERC5164ISM.t.sol index 75c79fb827..b87abf362e 100644 --- a/solidity/test/isms/ERC5164ISM.t.sol +++ b/solidity/test/isms/ERC5164ISM.t.sol @@ -134,7 +134,7 @@ contract ERC5164IsmTest is ExternalBridgeTest { vm.expectRevert( "AbstractMessageIdAuthorizedIsm: sender is not the hook" ); - ism.verifyMessageId(messageId); + ism.verifyMessageId(messageId, 0); assertFalse(ism.isVerified(encodedMessage)); } @@ -157,7 +157,7 @@ contract ERC5164IsmTest is ExternalBridgeTest { uint256 _msgValue ) internal override { vm.prank(address(executor)); - ism.verifyMessageId(messageId); + ism.verifyMessageId(messageId, _msgValue); } function _encodeExternalDestinationBridgeCall( @@ -168,7 +168,7 @@ contract ERC5164IsmTest is ExternalBridgeTest { ) internal override returns (bytes memory) { if (_from == address(hook)) { vm.prank(address(executor)); - ism.verifyMessageId{value: _msgValue}(messageId); + ism.verifyMessageId{value: _msgValue}(messageId, _msgValue); } } } diff --git a/solidity/test/isms/ExternalBridgeTest.sol b/solidity/test/isms/ExternalBridgeTest.sol index 8db043fcf0..5cd1b3d676 100644 --- a/solidity/test/isms/ExternalBridgeTest.sol +++ b/solidity/test/isms/ExternalBridgeTest.sol @@ -18,6 +18,7 @@ abstract contract ExternalBridgeTest is Test { uint8 internal constant HYPERLANE_VERSION = 1; uint32 internal constant ORIGIN_DOMAIN = 1; uint32 internal constant DESTINATION_DOMAIN = 2; + uint256 internal constant MSG_VALUE = 1 ether; uint256 internal constant MAX_MSG_VALUE = 2 ** 255 - 1; uint256 internal GAS_QUOTE; @@ -49,7 +50,7 @@ abstract contract ExternalBridgeTest is Test { /* ============ Hook.postDispatch ============ */ function test_postDispatch() public { - bytes memory encodedHookData = _encodeHookData(messageId); + bytes memory encodedHookData = _encodeHookData(messageId, 0); originMailbox.updateLatestDispatchedId(messageId); _expectOriginExternalBridgeCall(encodedHookData); @@ -92,10 +93,7 @@ abstract contract ExternalBridgeTest is Test { /* ============ ISM.verifyMessageId ============ */ function test_verifyMessageId_asyncCall() public { - bytes memory encodedHookData = abi.encodeCall( - AbstractMessageIdAuthorizedIsm.verifyMessageId, - (messageId) - ); + bytes memory encodedHookData = _encodeHookData(messageId, 0); _externalBridgeDestinationCall(encodedHookData, 0); assertTrue(ism.isVerified(encodedMessage)); @@ -121,11 +119,11 @@ abstract contract ExternalBridgeTest is Test { } function test_verify_msgValue_asyncCall() public virtual { - bytes memory encodedHookData = _encodeHookData(messageId); - _externalBridgeDestinationCall(encodedHookData, 1 ether); + bytes memory encodedHookData = _encodeHookData(messageId, MSG_VALUE); + _externalBridgeDestinationCall(encodedHookData, MSG_VALUE); assertTrue(ism.verify(new bytes(0), encodedMessage)); - assertEq(address(testRecipient).balance, 1 ether); + assertEq(address(testRecipient).balance, MSG_VALUE); } function test_verify_msgValue_externalBridgeCall() public virtual { @@ -178,7 +176,7 @@ abstract contract ExternalBridgeTest is Test { bytes memory externalCalldata = _encodeExternalDestinationBridgeCall( address(hook), address(ism), - 0, + MSG_VALUE, incorrectMessageId ); @@ -189,17 +187,21 @@ abstract contract ExternalBridgeTest is Test { // async call - native bridges might have try catch block to prevent revert try this.externalBridgeDestinationCallWrapper( - _encodeHookData(incorrectMessageId), + _encodeHookData(incorrectMessageId, MSG_VALUE), 0 ) {} catch {} assertFalse(ism.isVerified(testMessage)); + assertEq(address(testRecipient).balance, 0); } /// forge-config: default.fuzz.runs = 10 function test_verify_valueAlreadyClaimed(uint256 _msgValue) public virtual { _msgValue = bound(_msgValue, 0, MAX_MSG_VALUE); - _externalBridgeDestinationCall(_encodeHookData(messageId), _msgValue); + _externalBridgeDestinationCall( + _encodeHookData(messageId, _msgValue), + _msgValue + ); bool verified = ism.verify(new bytes(0), encodedMessage); assertTrue(verified); @@ -217,6 +219,9 @@ abstract contract ExternalBridgeTest is Test { assertEq(address(testRecipient).balance, _msgValue); } + // add a test where where ALICE calls postDispatch with msg.value and then BOB replays the + // function + /* ============ helper functions ============ */ function _encodeTestMessage() internal view returns (bytes memory) { @@ -229,12 +234,13 @@ abstract contract ExternalBridgeTest is Test { } function _encodeHookData( - bytes32 _messageId + bytes32 _messageId, + uint256 _msgValue ) internal pure returns (bytes memory) { return abi.encodeCall( AbstractMessageIdAuthorizedIsm.verifyMessageId, - (_messageId) + (_messageId, _msgValue) ); } diff --git a/solidity/test/isms/OPL2ToL1Ism.t.sol b/solidity/test/isms/OPL2ToL1Ism.t.sol index d829e0211e..2cf4acd564 100644 --- a/solidity/test/isms/OPL2ToL1Ism.t.sol +++ b/solidity/test/isms/OPL2ToL1Ism.t.sol @@ -126,7 +126,7 @@ contract OPL2ToL1IsmTest is ExternalBridgeTest { uint256 _value, bytes32 _messageId ) internal view returns (bytes memory) { - bytes memory encodedHookData = _encodeHookData(_messageId); + bytes memory encodedHookData = _encodeHookData(_messageId, _value); return abi.encodeCall( diff --git a/solidity/test/isms/OPStackIsm.t.sol b/solidity/test/isms/OPStackIsm.t.sol index 45c818ec3c..2239182f06 100644 --- a/solidity/test/isms/OPStackIsm.t.sol +++ b/solidity/test/isms/OPStackIsm.t.sol @@ -99,8 +99,12 @@ contract OPStackIsmTest is ExternalBridgeTest { function test_verify_revertsWhen_incorrectMessageId() public override { bytes32 incorrectMessageId = keccak256("incorrect message id"); - _externalBridgeDestinationCall(_encodeHookData(incorrectMessageId), 0); + _externalBridgeDestinationCall( + _encodeHookData(incorrectMessageId, MSG_VALUE), + 0 + ); assertFalse(ism.isVerified(testMessage)); + assertEq(address(testRecipient).balance, 0); } /* ============ helper functions ============ */ @@ -153,7 +157,7 @@ contract OPStackIsmTest is ExternalBridgeTest { function test_verify_revertsWhen_notAuthorizedHook() public override { // needs to be called by the canonical messenger on Optimism vm.expectRevert(NotCrossChainCall.selector); - ism.verifyMessageId(messageId); + ism.verifyMessageId(messageId, 0); vm.startPrank(L2_MESSENGER_ADDRESS); _setExternalOriginSender(address(this)); @@ -162,7 +166,7 @@ contract OPStackIsmTest is ExternalBridgeTest { vm.expectRevert( "AbstractMessageIdAuthorizedIsm: sender is not the hook" ); - ism.verifyMessageId(messageId); + ism.verifyMessageId(messageId, 0); } function _setExternalOriginSender( @@ -180,7 +184,10 @@ contract OPStackIsmTest is ExternalBridgeTest { vm.expectRevert( "AbstractMessageIdAuthorizedIsm: msg.value must be less than 2^255" ); - _externalBridgeDestinationCall(_encodeHookData(messageId), _msgValue); + _externalBridgeDestinationCall( + _encodeHookData(messageId, _msgValue), + _msgValue + ); assertFalse(ism.isVerified(encodedMessage)); diff --git a/solidity/test/isms/PolygonPosIsm.t.sol b/solidity/test/isms/PolygonPosIsm.t.sol index fd26afea29..a5d038988b 100644 --- a/solidity/test/isms/PolygonPosIsm.t.sol +++ b/solidity/test/isms/PolygonPosIsm.t.sol @@ -156,7 +156,7 @@ contract PolygonPosIsmTest is Test { bytes memory encodedHookData = abi.encodeCall( AbstractMessageIdAuthorizedIsm.verifyMessageId, - (messageId) + (messageId, 0) ); l1Mailbox.updateLatestDispatchedId(messageId); @@ -237,7 +237,7 @@ contract PolygonPosIsmTest is Test { bytes memory encodedHookData = abi.encodeCall( AbstractMessageIdAuthorizedIsm.verifyMessageId, - (messageId) + (messageId, 0) ); vm.startPrank(POLYGON_CROSSCHAIN_SYSTEM_ADDR); @@ -255,7 +255,7 @@ contract PolygonPosIsmTest is Test { ) ); - assertTrue(polygonPosISM.verifiedMessages(messageId).isBitSet(255)); + assertTrue(polygonPosISM.isVerified(encodedMessage)); vm.stopPrank(); } @@ -266,7 +266,7 @@ contract PolygonPosIsmTest is Test { // needs to be called by the fxchild on Polygon vm.expectRevert(NotCrossChainCall.selector); - polygonPosISM.verifyMessageId(messageId); + polygonPosISM.verifyMessageId(messageId, 0); vm.startPrank(MAINNET_FX_CHILD); @@ -274,7 +274,7 @@ contract PolygonPosIsmTest is Test { vm.expectRevert( "AbstractMessageIdAuthorizedIsm: sender is not the hook" ); - polygonPosISM.verifyMessageId(messageId); + polygonPosISM.verifyMessageId(messageId, 0); } /* ============ ISM.verify ============ */ @@ -350,7 +350,7 @@ contract PolygonPosIsmTest is Test { bytes memory encodedHookData = abi.encodeCall( AbstractMessageIdAuthorizedIsm.verifyMessageId, - (_messageId) + (_messageId, 0) ); vm.prank(POLYGON_CROSSCHAIN_SYSTEM_ADDR); diff --git a/solidity/test/isms/layer-zero/LayerZeroV2Ism.t.sol b/solidity/test/isms/layer-zero/LayerZeroV2Ism.t.sol index 0d778a4d92..f4b61d3d43 100644 --- a/solidity/test/isms/layer-zero/LayerZeroV2Ism.t.sol +++ b/solidity/test/isms/layer-zero/LayerZeroV2Ism.t.sol @@ -24,7 +24,7 @@ contract LayerZeroV2IsmTest is Test { return abi.encodeCall( AbstractMessageIdAuthorizedIsm.verifyMessageId, - (_messageId) + (_messageId, 0) ); } @@ -134,7 +134,7 @@ contract LayerZeroV2IsmTest is Test { } function testLzV2Ism_verifyMessageId_SetsCorrectMessageId( - bytes32 messageId + bytes memory _message ) public { lZIsm.setAuthorizedHook(hook.addressToBytes32()); vm.startPrank(endpoint); @@ -147,16 +147,14 @@ contract LayerZeroV2IsmTest is Test { ) = _makeLzParameters( hook, bytes32(""), - _encodedFunctionCall(messageId), + _encodedFunctionCall(_message.id()), makeAddr("executor"), bytes("") ); lZIsm.lzReceive(origin, guid, message, executor, extraData); vm.stopPrank(); - bool messageIdVerified = lZIsm.verifiedMessages(messageId).isBitSet( - lZIsm.VERIFIED_MASK_INDEX() - ); + bool messageIdVerified = lZIsm.isVerified(_message); assertTrue(messageIdVerified); } } From 97b7bc8a9a2692aaf997c61b06d7ed3c73c83d7d Mon Sep 17 00:00:00 2001 From: -f Date: Sat, 21 Sep 2024 02:08:13 +0530 Subject: [PATCH 25/47] add test --- .../hook/AbstractMessageIdAuthorizedIsm.sol | 2 +- solidity/test/isms/ExternalBridgeTest.sol | 33 +++++++++++++++++-- 2 files changed, 32 insertions(+), 3 deletions(-) diff --git a/solidity/contracts/isms/hook/AbstractMessageIdAuthorizedIsm.sol b/solidity/contracts/isms/hook/AbstractMessageIdAuthorizedIsm.sol index 88cfed97a6..a5d570d3ef 100644 --- a/solidity/contracts/isms/hook/AbstractMessageIdAuthorizedIsm.sol +++ b/solidity/contracts/isms/hook/AbstractMessageIdAuthorizedIsm.sol @@ -121,7 +121,7 @@ abstract contract AbstractMessageIdAuthorizedIsm is ); verifiedMessages[messageId] = true; - messageValues[messageId] = msgValue; + messageValues[messageId] += msgValue; emit ReceivedMessage(messageId); } diff --git a/solidity/test/isms/ExternalBridgeTest.sol b/solidity/test/isms/ExternalBridgeTest.sol index 5cd1b3d676..3767a614ac 100644 --- a/solidity/test/isms/ExternalBridgeTest.sol +++ b/solidity/test/isms/ExternalBridgeTest.sol @@ -219,8 +219,37 @@ abstract contract ExternalBridgeTest is Test { assertEq(address(testRecipient).balance, _msgValue); } - // add a test where where ALICE calls postDispatch with msg.value and then BOB replays the - // function + function test_verify_msgValue_frontrun() public virtual { + bytes memory aliceEncodedHookData = _encodeHookData( + messageId, + MSG_VALUE + ); + bytes memory bobEncodedHookData = _encodeHookData(messageId, 0); + + _externalBridgeDestinationCall(bobEncodedHookData, 0); + + assertTrue(ism.verify(new bytes(0), encodedMessage)); + assertEq(address(testRecipient).balance, 0); + + _externalBridgeDestinationCall(aliceEncodedHookData, MSG_VALUE); + + assertTrue(ism.verify(new bytes(0), encodedMessage)); + assertEq(address(testRecipient).balance, MSG_VALUE); + } + + function test_verify_override_msgValue() public virtual { + bytes memory aliceEncodedHookData = _encodeHookData( + messageId, + MSG_VALUE + ); + bytes memory bobEncodedHookData = _encodeHookData(messageId, 0); + + _externalBridgeDestinationCall(aliceEncodedHookData, MSG_VALUE); + _externalBridgeDestinationCall(bobEncodedHookData, 0); + + assertTrue(ism.verify(new bytes(0), encodedMessage)); + assertEq(address(testRecipient).balance, MSG_VALUE); + } /* ============ helper functions ============ */ From bef347033fafd0f51ab1092ba965bf757b8cce02 Mon Sep 17 00:00:00 2001 From: -f Date: Sat, 21 Sep 2024 02:31:05 +0530 Subject: [PATCH 26/47] add to current value --- .../hooks/libs/AbstractMessageIdAuthHook.sol | 2 +- .../hook/AbstractMessageIdAuthorizedIsm.sol | 27 ++++++++++--------- solidity/contracts/isms/hook/ArbL2ToL1Ism.sol | 7 +---- .../isms/hook/layer-zero/LayerZeroV2Ism.sol | 8 +----- solidity/test/isms/ArbL2ToL1Ism.t.sol | 5 +++- solidity/test/isms/PolygonPosIsm.t.sol | 12 ++++----- .../test/isms/layer-zero/LayerZeroV2Ism.t.sol | 10 ++++--- 7 files changed, 34 insertions(+), 37 deletions(-) diff --git a/solidity/contracts/hooks/libs/AbstractMessageIdAuthHook.sol b/solidity/contracts/hooks/libs/AbstractMessageIdAuthHook.sol index 2c1e8206d3..a9e3a41e94 100644 --- a/solidity/contracts/hooks/libs/AbstractMessageIdAuthHook.sol +++ b/solidity/contracts/hooks/libs/AbstractMessageIdAuthHook.sol @@ -84,7 +84,7 @@ abstract contract AbstractMessageIdAuthHook is ); bytes memory payload = abi.encodeCall( AbstractMessageIdAuthorizedIsm.verifyMessageId, - (id, metadata.msgValue(0)) + id ); _sendMessageId(metadata, payload); } diff --git a/solidity/contracts/isms/hook/AbstractMessageIdAuthorizedIsm.sol b/solidity/contracts/isms/hook/AbstractMessageIdAuthorizedIsm.sol index a5d570d3ef..a8b8baee6c 100644 --- a/solidity/contracts/isms/hook/AbstractMessageIdAuthorizedIsm.sol +++ b/solidity/contracts/isms/hook/AbstractMessageIdAuthorizedIsm.sol @@ -38,9 +38,12 @@ abstract contract AbstractMessageIdAuthorizedIsm is using Message for bytes; // ============ Public Storage ============ - /// @notice Maps messageId to value to whether or not the message has been verified - mapping(bytes32 => bool) public verifiedMessages; - mapping(bytes32 => uint256) public messageValues; + /// @notice Maps messageId to whether or not the message has been verified + /// first bit is boolean for verification + /// rest of bits is the amount to send to the recipient + /// @dev bc of the bit packing, we can only send up to 2^255 wei + /// @dev the first bit is reserved for verification and the rest 255 bits are for the msg.value + mapping(bytes32 => uint256) public verifiedMessages; /// @notice Index of verification bit in verifiedMessages uint256 public constant VERIFIED_MASK_INDEX = 255; /// @notice address for the authorized hook @@ -87,9 +90,11 @@ abstract contract AbstractMessageIdAuthorizedIsm is */ function releaseValueToRecipient(bytes calldata message) public { bytes32 messageId = message.id(); - uint256 _msgValue = messageValues[messageId]; + uint256 _msgValue = verifiedMessages[messageId].clearBit( + VERIFIED_MASK_INDEX + ); if (_msgValue > 0) { - messageValues[messageId] = 0; + verifiedMessages[messageId] -= _msgValue; payable(message.recipientAddress()).sendValue(_msgValue); } } @@ -99,7 +104,9 @@ abstract contract AbstractMessageIdAuthorizedIsm is * @param message Message to check. */ function isVerified(bytes calldata message) public view returns (bool) { - return verifiedMessages[message.id()]; + bytes32 messageId = message.id(); + // check for the first bit (used for verification) + return verifiedMessages[messageId].isBitSet(VERIFIED_MASK_INDEX); } /** @@ -107,10 +114,7 @@ abstract contract AbstractMessageIdAuthorizedIsm is * @dev Only callable by the authorized hook. * @param messageId Hyperlane Id of the message. */ - function verifyMessageId( - bytes32 messageId, - uint256 msgValue - ) public payable virtual { + function verifyMessageId(bytes32 messageId) public payable virtual { require( _isAuthorized(), "AbstractMessageIdAuthorizedIsm: sender is not the hook" @@ -120,8 +124,7 @@ abstract contract AbstractMessageIdAuthorizedIsm is "AbstractMessageIdAuthorizedIsm: msg.value must be less than 2^255" ); - verifiedMessages[messageId] = true; - messageValues[messageId] += msgValue; + verifiedMessages[messageId] = msg.value.setBit(VERIFIED_MASK_INDEX); emit ReceivedMessage(messageId); } diff --git a/solidity/contracts/isms/hook/ArbL2ToL1Ism.sol b/solidity/contracts/isms/hook/ArbL2ToL1Ism.sol index 0b93978b78..a7bd714473 100644 --- a/solidity/contracts/isms/hook/ArbL2ToL1Ism.sol +++ b/solidity/contracts/isms/hook/ArbL2ToL1Ism.sol @@ -41,8 +41,6 @@ contract ArbL2ToL1Ism is // module type for the ISM uint8 public constant moduleType = uint8(IInterchainSecurityModule.Types.ARB_L2_TO_L1); - // length of the call data for the arbOutbox.executeTransaction function - uint8 public constant CALLDATA_LENGTH = 68; // arbitrum nitro contract on L1 to forward verification IOutbox public arbOutbox; @@ -112,10 +110,7 @@ contract ArbL2ToL1Ism is "ArbL2ToL1Ism: l2Sender != authorizedHook" ); // this data is an abi encoded call of verifyMessageId(bytes32 messageId) - require( - data.length == CALLDATA_LENGTH, - "ArbL2ToL1Ism: invalid data length" - ); + require(data.length == 36, "ArbL2ToL1Ism: invalid data length"); bytes32 messageId = message.id(); bytes32 convertedBytes; assembly { diff --git a/solidity/contracts/isms/hook/layer-zero/LayerZeroV2Ism.sol b/solidity/contracts/isms/hook/layer-zero/LayerZeroV2Ism.sol index 9e114da560..1388f32cf8 100644 --- a/solidity/contracts/isms/hook/layer-zero/LayerZeroV2Ism.sol +++ b/solidity/contracts/isms/hook/layer-zero/LayerZeroV2Ism.sol @@ -66,7 +66,7 @@ contract LayerZeroV2Ism is AbstractMessageIdAuthorizedIsm { address, bytes calldata ) external payable { - verifyMessageId(_messageId(_lzMessage), _msgValue(_lzMessage)); + verifyMessageId(_messageId(_lzMessage)); } // ============ Internal function ============ @@ -82,12 +82,6 @@ contract LayerZeroV2Ism is AbstractMessageIdAuthorizedIsm { return bytes32(_message[FUNC_SELECTOR_OFFSET:]); } - function _msgValue( - bytes calldata _message - ) internal pure returns (uint256) { - return uint256(bytes32(_message[FUNC_SELECTOR_OFFSET + 32:])); - } - /** * @notice Validates criteria to verify a message * @dev this is called by AbstractMessageIdAuthorizedIsm.verifyMessageId diff --git a/solidity/test/isms/ArbL2ToL1Ism.t.sol b/solidity/test/isms/ArbL2ToL1Ism.t.sol index d4b6d14e61..a251cfda4d 100644 --- a/solidity/test/isms/ArbL2ToL1Ism.t.sol +++ b/solidity/test/isms/ArbL2ToL1Ism.t.sol @@ -115,7 +115,10 @@ contract ArbL2ToL1IsmTest is ExternalBridgeTest { bytes32 _messageId, uint256 _value ) internal view returns (bytes memory) { - bytes memory encodedHookData = _encodeHookData(_messageId, _value); + bytes memory encodedHookData = abi.encodeCall( + AbstractMessageIdAuthorizedIsm.verifyMessageId, + (_messageId) + ); bytes32[] memory proof = new bytes32[](16); return diff --git a/solidity/test/isms/PolygonPosIsm.t.sol b/solidity/test/isms/PolygonPosIsm.t.sol index a5d038988b..fd26afea29 100644 --- a/solidity/test/isms/PolygonPosIsm.t.sol +++ b/solidity/test/isms/PolygonPosIsm.t.sol @@ -156,7 +156,7 @@ contract PolygonPosIsmTest is Test { bytes memory encodedHookData = abi.encodeCall( AbstractMessageIdAuthorizedIsm.verifyMessageId, - (messageId, 0) + (messageId) ); l1Mailbox.updateLatestDispatchedId(messageId); @@ -237,7 +237,7 @@ contract PolygonPosIsmTest is Test { bytes memory encodedHookData = abi.encodeCall( AbstractMessageIdAuthorizedIsm.verifyMessageId, - (messageId, 0) + (messageId) ); vm.startPrank(POLYGON_CROSSCHAIN_SYSTEM_ADDR); @@ -255,7 +255,7 @@ contract PolygonPosIsmTest is Test { ) ); - assertTrue(polygonPosISM.isVerified(encodedMessage)); + assertTrue(polygonPosISM.verifiedMessages(messageId).isBitSet(255)); vm.stopPrank(); } @@ -266,7 +266,7 @@ contract PolygonPosIsmTest is Test { // needs to be called by the fxchild on Polygon vm.expectRevert(NotCrossChainCall.selector); - polygonPosISM.verifyMessageId(messageId, 0); + polygonPosISM.verifyMessageId(messageId); vm.startPrank(MAINNET_FX_CHILD); @@ -274,7 +274,7 @@ contract PolygonPosIsmTest is Test { vm.expectRevert( "AbstractMessageIdAuthorizedIsm: sender is not the hook" ); - polygonPosISM.verifyMessageId(messageId, 0); + polygonPosISM.verifyMessageId(messageId); } /* ============ ISM.verify ============ */ @@ -350,7 +350,7 @@ contract PolygonPosIsmTest is Test { bytes memory encodedHookData = abi.encodeCall( AbstractMessageIdAuthorizedIsm.verifyMessageId, - (_messageId, 0) + (_messageId) ); vm.prank(POLYGON_CROSSCHAIN_SYSTEM_ADDR); diff --git a/solidity/test/isms/layer-zero/LayerZeroV2Ism.t.sol b/solidity/test/isms/layer-zero/LayerZeroV2Ism.t.sol index f4b61d3d43..0d778a4d92 100644 --- a/solidity/test/isms/layer-zero/LayerZeroV2Ism.t.sol +++ b/solidity/test/isms/layer-zero/LayerZeroV2Ism.t.sol @@ -24,7 +24,7 @@ contract LayerZeroV2IsmTest is Test { return abi.encodeCall( AbstractMessageIdAuthorizedIsm.verifyMessageId, - (_messageId, 0) + (_messageId) ); } @@ -134,7 +134,7 @@ contract LayerZeroV2IsmTest is Test { } function testLzV2Ism_verifyMessageId_SetsCorrectMessageId( - bytes memory _message + bytes32 messageId ) public { lZIsm.setAuthorizedHook(hook.addressToBytes32()); vm.startPrank(endpoint); @@ -147,14 +147,16 @@ contract LayerZeroV2IsmTest is Test { ) = _makeLzParameters( hook, bytes32(""), - _encodedFunctionCall(_message.id()), + _encodedFunctionCall(messageId), makeAddr("executor"), bytes("") ); lZIsm.lzReceive(origin, guid, message, executor, extraData); vm.stopPrank(); - bool messageIdVerified = lZIsm.isVerified(_message); + bool messageIdVerified = lZIsm.verifiedMessages(messageId).isBitSet( + lZIsm.VERIFIED_MASK_INDEX() + ); assertTrue(messageIdVerified); } } From 541be433dd1ded61dc4fa5296417bb1a556f0da7 Mon Sep 17 00:00:00 2001 From: -f Date: Sat, 21 Sep 2024 02:32:17 +0530 Subject: [PATCH 27/47] revert --- .../hook/AbstractMessageIdAuthorizedIsm.sol | 7 ++++- solidity/contracts/libs/OPL2ToL1Metadata.sol | 6 ++-- solidity/test/isms/ExternalBridgeTest.sol | 28 ++----------------- 3 files changed, 12 insertions(+), 29 deletions(-) diff --git a/solidity/contracts/isms/hook/AbstractMessageIdAuthorizedIsm.sol b/solidity/contracts/isms/hook/AbstractMessageIdAuthorizedIsm.sol index a8b8baee6c..fb84fcd74f 100644 --- a/solidity/contracts/isms/hook/AbstractMessageIdAuthorizedIsm.sol +++ b/solidity/contracts/isms/hook/AbstractMessageIdAuthorizedIsm.sol @@ -123,8 +123,13 @@ abstract contract AbstractMessageIdAuthorizedIsm is msg.value < 2 ** VERIFIED_MASK_INDEX, "AbstractMessageIdAuthorizedIsm: msg.value must be less than 2^255" ); + uint256 currentValue = verifiedMessages[messageId].clearBit( + VERIFIED_MASK_INDEX + ); - verifiedMessages[messageId] = msg.value.setBit(VERIFIED_MASK_INDEX); + verifiedMessages[messageId] = (currentValue + msg.value).setBit( + VERIFIED_MASK_INDEX + ); emit ReceivedMessage(messageId); } diff --git a/solidity/contracts/libs/OPL2ToL1Metadata.sol b/solidity/contracts/libs/OPL2ToL1Metadata.sol index 753550fec3..704ce3d4d9 100644 --- a/solidity/contracts/libs/OPL2ToL1Metadata.sol +++ b/solidity/contracts/libs/OPL2ToL1Metadata.sol @@ -7,7 +7,7 @@ pragma solidity >=0.8.0; */ library OPL2ToL1Metadata { // bottom offset to the start of message id in the metadata - uint256 private constant MESSAGE_ID_OFFSET = 120; + uint256 private constant MESSAGE_ID_OFFSET = 88; // from IOptimismPortal.WithdrawalTransaction // Σ { // nonce = 32 bytes @@ -31,9 +31,9 @@ library OPL2ToL1Metadata { // _data // OFFSET = 32 bytes // LENGTH = 32 bytes - // PADDING + verifyMessageId = 96 bytes + // PADDING + verifyMessageId = 64 bytes // } = 292 bytes - uint256 private constant MESSENGER_CALLDATA_LENGTH = 324; + uint256 private constant MESSENGER_CALLDATA_LENGTH = 292; /** * @notice Returns the message ID. diff --git a/solidity/test/isms/ExternalBridgeTest.sol b/solidity/test/isms/ExternalBridgeTest.sol index b86a99879e..b1f744c15b 100644 --- a/solidity/test/isms/ExternalBridgeTest.sol +++ b/solidity/test/isms/ExternalBridgeTest.sol @@ -218,33 +218,11 @@ abstract contract ExternalBridgeTest is Test { assertEq(address(testRecipient).balance, _msgValue); } - function test_verify_msgValue_frontrun() public virtual { - bytes memory aliceEncodedHookData = _encodeHookData( - messageId, - MSG_VALUE - ); - bytes memory bobEncodedHookData = _encodeHookData(messageId, 0); - - _externalBridgeDestinationCall(bobEncodedHookData, 0); - - assertTrue(ism.verify(new bytes(0), encodedMessage)); - assertEq(address(testRecipient).balance, 0); - - _externalBridgeDestinationCall(aliceEncodedHookData, MSG_VALUE); - - assertTrue(ism.verify(new bytes(0), encodedMessage)); - assertEq(address(testRecipient).balance, MSG_VALUE); - } - function test_verify_override_msgValue() public virtual { - bytes memory aliceEncodedHookData = _encodeHookData( - messageId, - MSG_VALUE - ); - bytes memory bobEncodedHookData = _encodeHookData(messageId, 0); + bytes memory encodedHookData = _encodeHookData(messageId); - _externalBridgeDestinationCall(aliceEncodedHookData, MSG_VALUE); - _externalBridgeDestinationCall(bobEncodedHookData, 0); + _externalBridgeDestinationCall(encodedHookData, MSG_VALUE); + _externalBridgeDestinationCall(encodedHookData, 0); assertTrue(ism.verify(new bytes(0), encodedMessage)); assertEq(address(testRecipient).balance, MSG_VALUE); From 4233c0ad000cc6bb735ef73413bd83625b7bed39 Mon Sep 17 00:00:00 2001 From: -f Date: Sat, 21 Sep 2024 02:35:14 +0530 Subject: [PATCH 28/47] magic --- solidity/test/isms/ExternalBridgeTest.sol | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/solidity/test/isms/ExternalBridgeTest.sol b/solidity/test/isms/ExternalBridgeTest.sol index b1f744c15b..e82fc3ea71 100644 --- a/solidity/test/isms/ExternalBridgeTest.sol +++ b/solidity/test/isms/ExternalBridgeTest.sol @@ -133,11 +133,11 @@ abstract contract ExternalBridgeTest is Test { bytes memory externalCalldata = _encodeExternalDestinationBridgeCall( address(hook), address(ism), - 1 ether, + MSG_VALUE, messageId ); ism.verify(externalCalldata, encodedMessage); - assertEq(address(testRecipient).balance, 1 ether); + assertEq(address(testRecipient).balance, MSG_VALUE); } function test_verify_revertsWhen_invalidIsm() public virtual { From 41a407d0bf0eda418fa9e9f269519592410bef8d Mon Sep 17 00:00:00 2001 From: -f Date: Sat, 21 Sep 2024 02:36:20 +0530 Subject: [PATCH 29/47] 5164 --- solidity/test/isms/ERC5164ISM.t.sol | 2 ++ 1 file changed, 2 insertions(+) diff --git a/solidity/test/isms/ERC5164ISM.t.sol b/solidity/test/isms/ERC5164ISM.t.sol index 75c79fb827..687f3db8c8 100644 --- a/solidity/test/isms/ERC5164ISM.t.sol +++ b/solidity/test/isms/ERC5164ISM.t.sol @@ -150,6 +150,8 @@ contract ERC5164IsmTest is ExternalBridgeTest { function test_verify_valueAlreadyClaimed(uint256) public override {} + function test_verify_override_msgValue() public override {} + /* ============ helper functions ============ */ function _externalBridgeDestinationCall( From 5610c336f8fd0ebd6bdaf6b4346498baf0df0804 Mon Sep 17 00:00:00 2001 From: -f Date: Sat, 21 Sep 2024 02:38:42 +0530 Subject: [PATCH 30/47] changeset --- .changeset/green-suits-dance.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/green-suits-dance.md diff --git a/.changeset/green-suits-dance.md b/.changeset/green-suits-dance.md new file mode 100644 index 0000000000..a09a7745e7 --- /dev/null +++ b/.changeset/green-suits-dance.md @@ -0,0 +1,5 @@ +--- +'@hyperlane-xyz/core': patch +--- + +Adding msg.value instead of updating in AuthorizedHookIsm From ef780c67dc8febbe052d3c7c2859aa2c4ba0d646 Mon Sep 17 00:00:00 2001 From: -f Date: Mon, 23 Sep 2024 15:48:33 +0530 Subject: [PATCH 31/47] check sufficient fees and return extra --- solidity/contracts/hooks/OPL2ToL1Hook.sol | 4 --- .../hooks/libs/AbstractMessageIdAuthHook.sol | 12 +++++++ .../hooks/layerzero/LayerZeroV2Hook.t.sol | 32 +++++++---------- solidity/test/isms/ERC5164ISM.t.sol | 2 ++ solidity/test/isms/ExternalBridgeTest.sol | 36 ++++++++++++++++++- 5 files changed, 62 insertions(+), 24 deletions(-) diff --git a/solidity/contracts/hooks/OPL2ToL1Hook.sol b/solidity/contracts/hooks/OPL2ToL1Hook.sol index 7d0fcf36a4..f132e9cac7 100644 --- a/solidity/contracts/hooks/OPL2ToL1Hook.sol +++ b/solidity/contracts/hooks/OPL2ToL1Hook.sol @@ -71,10 +71,6 @@ contract OPL2ToL1Hook is AbstractMessageIdAuthHook { bytes calldata metadata, bytes memory payload ) internal override { - require( - msg.value >= metadata.msgValue(0) + GAS_QUOTE, - "OPL2ToL1Hook: insufficient msg.value" - ); l2Messenger.sendMessage{value: metadata.msgValue(0)}( TypeCasts.bytes32ToAddress(ism), payload, diff --git a/solidity/contracts/hooks/libs/AbstractMessageIdAuthHook.sol b/solidity/contracts/hooks/libs/AbstractMessageIdAuthHook.sol index a9e3a41e94..408bb552ad 100644 --- a/solidity/contracts/hooks/libs/AbstractMessageIdAuthHook.sol +++ b/solidity/contracts/hooks/libs/AbstractMessageIdAuthHook.sol @@ -22,6 +22,9 @@ import {Message} from "../../libs/Message.sol"; import {StandardHookMetadata} from "./StandardHookMetadata.sol"; import {MailboxClient} from "../../client/MailboxClient.sol"; +// ============ External Imports ============ +import {Address} from "@openzeppelin/contracts/utils/Address.sol"; + /** * @title AbstractMessageIdAuthHook * @notice Message hook to inform an Abstract Message ID ISM of messages published through @@ -31,6 +34,7 @@ abstract contract AbstractMessageIdAuthHook is AbstractPostDispatchHook, MailboxClient { + using Address for address payable; using StandardHookMetadata for bytes; using Message for bytes; @@ -82,11 +86,19 @@ abstract contract AbstractMessageIdAuthHook is metadata.msgValue(0) < 2 ** 255, "AbstractMessageIdAuthHook: msgValue must be less than 2 ** 255" ); + bytes memory payload = abi.encodeCall( AbstractMessageIdAuthorizedIsm.verifyMessageId, id ); _sendMessageId(metadata, payload); + + uint256 _overpayment = msg.value - _quoteDispatch(metadata, message); + if (_overpayment > 0) { + address _refundAddress = metadata.refundAddress(msg.sender); + require(_refundAddress != address(0), "no refund address"); + payable(_refundAddress).sendValue(_overpayment); + } } /** diff --git a/solidity/test/hooks/layerzero/LayerZeroV2Hook.t.sol b/solidity/test/hooks/layerzero/LayerZeroV2Hook.t.sol index 94225d2876..f1a6d08189 100644 --- a/solidity/test/hooks/layerzero/LayerZeroV2Hook.t.sol +++ b/solidity/test/hooks/layerzero/LayerZeroV2Hook.t.sol @@ -165,25 +165,19 @@ contract LayerZeroV2HookTest is Test { ); } - function testLzV2Hook_PostDispatch_refundExtraFee(uint256 balance) public { - ( - uint256 nativeFee, - bytes memory metadata - ) = testLzV2Hook_QuoteDispatch_returnsFeeAmount(); - vm.assume(balance > nativeFee); - - uint256 extraValue = balance - nativeFee; - vm.deal(address(this), balance); - - mailbox.dispatch{value: balance}( - HYPERLANE_DEST_DOMAIN, - address(crossChainCounterApp).addressToBytes32(), - "Hello World!", - metadata, - hook - ); - assertEq(address(alice).balance, extraValue); - } + // TODO: fix double refund for LayerZeroV2Hook + // function testLzV2Hook_PostDispatch_refundExtraFee(uint256 balance) public { + // (uint256 nativeFee, bytes memory metadata) = testLzV2Hook_QuoteDispatch_returnsFeeAmount(); + // vm.assume(balance > nativeFee); + + // uint256 extraValue = balance - nativeFee; + // vm.deal(address(this), balance); + + // mailbox.dispatch{value: balance}( + // HYPERLANE_DEST_DOMAIN, address(crossChainCounterApp).addressToBytes32(), "Hello World!", metadata, hook + // ); + // assertEq(address(alice).balance, extraValue); + // } function testLzV2Hook_PostDispatch_executesCrossChain() public { ( diff --git a/solidity/test/isms/ERC5164ISM.t.sol b/solidity/test/isms/ERC5164ISM.t.sol index 75c79fb827..18e7d79d99 100644 --- a/solidity/test/isms/ERC5164ISM.t.sol +++ b/solidity/test/isms/ERC5164ISM.t.sol @@ -150,6 +150,8 @@ contract ERC5164IsmTest is ExternalBridgeTest { function test_verify_valueAlreadyClaimed(uint256) public override {} + function testFuzz_postDispatch_refundsExtraValue(uint256) public override {} + /* ============ helper functions ============ */ function _externalBridgeDestinationCall( diff --git a/solidity/test/isms/ExternalBridgeTest.sol b/solidity/test/isms/ExternalBridgeTest.sol index 8db043fcf0..600f27fd4b 100644 --- a/solidity/test/isms/ExternalBridgeTest.sol +++ b/solidity/test/isms/ExternalBridgeTest.sol @@ -53,7 +53,8 @@ abstract contract ExternalBridgeTest is Test { originMailbox.updateLatestDispatchedId(messageId); _expectOriginExternalBridgeCall(encodedHookData); - hook.postDispatch{value: GAS_QUOTE}(testMetadata, encodedMessage); + uint256 quote = hook.quoteDispatch(testMetadata, encodedMessage); + hook.postDispatch{value: quote}(testMetadata, encodedMessage); } function test_postDispatch_revertWhen_chainIDNotSupported() public { @@ -89,6 +90,37 @@ abstract contract ExternalBridgeTest is Test { hook.postDispatch(excessValueMetadata, encodedMessage); } + function testFuzz_postDispatch_refundsExtraValue( + uint256 extraValue + ) public virtual { + vm.assume(extraValue < MAX_MSG_VALUE); + vm.deal(address(this), address(this).balance + extraValue); + uint256 valueBefore = address(this).balance; + + bytes memory encodedHookData = _encodeHookData(messageId); + originMailbox.updateLatestDispatchedId(messageId); + _expectOriginExternalBridgeCall(encodedHookData); + + uint256 quote = hook.quoteDispatch(testMetadata, encodedMessage); + hook.postDispatch{value: quote + extraValue}( + testMetadata, + encodedMessage + ); + + assertEq(address(this).balance, valueBefore - quote); + } + + function test_postDispatch_revertWhen_insufficientValue() public { + bytes memory encodedHookData = _encodeHookData(messageId); + originMailbox.updateLatestDispatchedId(messageId); + _expectOriginExternalBridgeCall(encodedHookData); + + uint256 quote = hook.quoteDispatch(testMetadata, encodedMessage); + + vm.expectRevert(); //arithmetic underflow + hook.postDispatch{value: quote - 1}(testMetadata, encodedMessage); + } + /* ============ ISM.verifyMessageId ============ */ function test_verifyMessageId_asyncCall() public { @@ -265,4 +297,6 @@ abstract contract ExternalBridgeTest is Test { function _setExternalOriginSender( address _sender ) internal virtual returns (bytes memory) {} + + receive() external payable {} } From a6731b0b55337a7bde5f122254393d504acdbd16 Mon Sep 17 00:00:00 2001 From: -f Date: Mon, 23 Sep 2024 16:05:35 +0530 Subject: [PATCH 32/47] changeset --- .changeset/metal-doors-float.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/metal-doors-float.md diff --git a/.changeset/metal-doors-float.md b/.changeset/metal-doors-float.md new file mode 100644 index 0000000000..05963c8871 --- /dev/null +++ b/.changeset/metal-doors-float.md @@ -0,0 +1,5 @@ +--- +'@hyperlane-xyz/core': patch +--- + +Added check for msg.value > quote for AuthorizedHook From 8510b6ac4f79ec5873faeda8354ecf684f1e770c Mon Sep 17 00:00:00 2001 From: -f Date: Tue, 24 Sep 2024 22:50:34 +0530 Subject: [PATCH 33/47] add childhook --- solidity/contracts/hooks/ArbL2ToL1Hook.sol | 12 +++++-- solidity/contracts/hooks/OPL2ToL1Hook.sol | 32 +++++++++++++------ solidity/contracts/hooks/OPStackHook.sol | 11 ++++--- solidity/contracts/hooks/PolygonPosHook.sol | 9 +++++- .../hooks/aggregation/ERC5164Hook.sol | 11 ++++++- .../hooks/layer-zero/LayerZeroV2Hook.sol | 8 ++++- .../hooks/libs/AbstractMessageIdAuthHook.sol | 17 +++++----- solidity/test/isms/OPL2ToL1Ism.t.sol | 9 ++++-- 8 files changed, 79 insertions(+), 30 deletions(-) diff --git a/solidity/contracts/hooks/ArbL2ToL1Hook.sol b/solidity/contracts/hooks/ArbL2ToL1Hook.sol index 488dabc1ee..43060f809d 100644 --- a/solidity/contracts/hooks/ArbL2ToL1Hook.sol +++ b/solidity/contracts/hooks/ArbL2ToL1Hook.sol @@ -14,9 +14,10 @@ pragma solidity >=0.8.0; @@@@@@@@@ @@@@@@@@*/ // ============ Internal Imports ============ +import {Message} from "../libs/Message.sol"; import {AbstractPostDispatchHook} from "./libs/AbstractMessageIdAuthHook.sol"; import {AbstractMessageIdAuthHook} from "./libs/AbstractMessageIdAuthHook.sol"; -import {Mailbox} from "../Mailbox.sol"; +import {AbstractMessageIdAuthorizedIsm} from "../isms/hook/AbstractMessageIdAuthorizedIsm.sol"; import {StandardHookMetadata} from "./libs/StandardHookMetadata.sol"; import {Message} from "../libs/Message.sol"; import {TypeCasts} from "../libs/TypeCasts.sol"; @@ -35,6 +36,7 @@ import {ArbSys} from "@arbitrum/nitro-contracts/src/precompiles/ArbSys.sol"; */ contract ArbL2ToL1Hook is AbstractMessageIdAuthHook { using StandardHookMetadata for bytes; + using Message for bytes; // ============ Constants ============ @@ -72,8 +74,14 @@ contract ArbL2ToL1Hook is AbstractMessageIdAuthHook { /// @inheritdoc AbstractMessageIdAuthHook function _sendMessageId( bytes calldata metadata, - bytes memory payload + bytes calldata message ) internal override { + bytes32 id = message.id(); + bytes memory payload = abi.encodeCall( + AbstractMessageIdAuthorizedIsm.verifyMessageId, + id + ); + arbSys.sendTxToL1{value: metadata.msgValue(0)}( TypeCasts.bytes32ToAddress(ism), payload diff --git a/solidity/contracts/hooks/OPL2ToL1Hook.sol b/solidity/contracts/hooks/OPL2ToL1Hook.sol index f132e9cac7..2f58ef7294 100644 --- a/solidity/contracts/hooks/OPL2ToL1Hook.sol +++ b/solidity/contracts/hooks/OPL2ToL1Hook.sol @@ -14,10 +14,13 @@ pragma solidity >=0.8.0; @@@@@@@@@ @@@@@@@@*/ // ============ Internal Imports ============ +import {Message} from "../libs/Message.sol"; import {AbstractPostDispatchHook, AbstractMessageIdAuthHook} from "./libs/AbstractMessageIdAuthHook.sol"; +import {AbstractMessageIdAuthorizedIsm} from "../isms/hook/AbstractMessageIdAuthorizedIsm.sol"; import {StandardHookMetadata} from "./libs/StandardHookMetadata.sol"; import {TypeCasts} from "../libs/TypeCasts.sol"; import {IPostDispatchHook} from "../interfaces/hooks/IPostDispatchHook.sol"; +import {InterchainGasPaymaster} from "./igp/InterchainGasPaymaster.sol"; // ============ External Imports ============ import {ICrossDomainMessenger} from "../interfaces/optimism/ICrossDomainMessenger.sol"; @@ -30,13 +33,16 @@ import {ICrossDomainMessenger} from "../interfaces/optimism/ICrossDomainMessenge */ contract OPL2ToL1Hook is AbstractMessageIdAuthHook { using StandardHookMetadata for bytes; + using Message for bytes; // ============ Constants ============ // precompile contract on L2 for sending messages to L1 ICrossDomainMessenger public immutable l2Messenger; - // Immutable quote amount - uint32 public immutable GAS_QUOTE; + // child hook to call first + AbstractPostDispatchHook public immutable childHook; + // Minimum gas limit that the message can be executed with - OP specific + uint32 public constant MIN_GAS_LIMIT = 300_000; // ============ Constructor ============ @@ -45,10 +51,10 @@ contract OPL2ToL1Hook is AbstractMessageIdAuthHook { uint32 _destinationDomain, bytes32 _ism, address _l2Messenger, - uint32 _gasQuote + address _childHook ) AbstractMessageIdAuthHook(_mailbox, _destinationDomain, _ism) { - GAS_QUOTE = _gasQuote; l2Messenger = ICrossDomainMessenger(_l2Messenger); + childHook = AbstractPostDispatchHook(_childHook); } /// @inheritdoc IPostDispatchHook @@ -58,10 +64,11 @@ contract OPL2ToL1Hook is AbstractMessageIdAuthHook { /// @inheritdoc AbstractPostDispatchHook function _quoteDispatch( - bytes calldata, - bytes calldata + bytes calldata metadata, + bytes calldata message ) internal view override returns (uint256) { - return GAS_QUOTE; + return + metadata.msgValue(0) + childHook.quoteDispatch(metadata, message); } // ============ Internal functions ============ @@ -69,12 +76,19 @@ contract OPL2ToL1Hook is AbstractMessageIdAuthHook { /// @inheritdoc AbstractMessageIdAuthHook function _sendMessageId( bytes calldata metadata, - bytes memory payload + bytes calldata message ) internal override { + bytes32 id = message.id(); + bytes memory payload = abi.encodeCall( + AbstractMessageIdAuthorizedIsm.verifyMessageId, + id + ); + + childHook.postDispatch(metadata, message); l2Messenger.sendMessage{value: metadata.msgValue(0)}( TypeCasts.bytes32ToAddress(ism), payload, - GAS_QUOTE + MIN_GAS_LIMIT ); } } diff --git a/solidity/contracts/hooks/OPStackHook.sol b/solidity/contracts/hooks/OPStackHook.sol index a0a33dabcd..0fb050b063 100644 --- a/solidity/contracts/hooks/OPStackHook.sol +++ b/solidity/contracts/hooks/OPStackHook.sol @@ -18,6 +18,7 @@ import {AbstractMessageIdAuthHook} from "./libs/AbstractMessageIdAuthHook.sol"; import {StandardHookMetadata} from "./libs/StandardHookMetadata.sol"; import {TypeCasts} from "../libs/TypeCasts.sol"; import {Message} from "../libs/Message.sol"; +import {AbstractMessageIdAuthorizedIsm} from "../isms/hook/AbstractMessageIdAuthorizedIsm.sol"; import {IPostDispatchHook} from "../interfaces/hooks/IPostDispatchHook.sol"; // ============ External Imports ============ @@ -32,6 +33,7 @@ import {Address} from "@openzeppelin/contracts/utils/Address.sol"; */ contract OPStackHook is AbstractMessageIdAuthHook { using StandardHookMetadata for bytes; + using Message for bytes; // ============ Constants ============ @@ -69,12 +71,13 @@ contract OPStackHook is AbstractMessageIdAuthHook { /// @inheritdoc AbstractMessageIdAuthHook function _sendMessageId( bytes calldata metadata, - bytes memory payload + bytes calldata message ) internal override { - require( - metadata.msgValue(0) < 2 ** 255, - "OPStackHook: msgValue must be less than 2 ** 255" + bytes memory payload = abi.encodeCall( + AbstractMessageIdAuthorizedIsm.verifyMessageId, + message.id() ); + l1Messenger.sendMessage{value: metadata.msgValue(0)}( TypeCasts.bytes32ToAddress(ism), payload, diff --git a/solidity/contracts/hooks/PolygonPosHook.sol b/solidity/contracts/hooks/PolygonPosHook.sol index 6831f9a332..7c4d17cccd 100644 --- a/solidity/contracts/hooks/PolygonPosHook.sol +++ b/solidity/contracts/hooks/PolygonPosHook.sol @@ -19,6 +19,7 @@ import {StandardHookMetadata} from "./libs/StandardHookMetadata.sol"; import {TypeCasts} from "../libs/TypeCasts.sol"; import {Message} from "../libs/Message.sol"; import {IPostDispatchHook} from "../interfaces/hooks/IPostDispatchHook.sol"; +import {AbstractMessageIdAuthorizedIsm} from "../isms/hook/AbstractMessageIdAuthorizedIsm.sol"; // ============ External Imports ============ import {FxBaseRootTunnel} from "fx-portal/contracts/tunnel/FxBaseRootTunnel.sol"; @@ -31,6 +32,7 @@ import {Address} from "@openzeppelin/contracts/utils/Address.sol"; */ contract PolygonPosHook is AbstractMessageIdAuthHook, FxBaseRootTunnel { using StandardHookMetadata for bytes; + using Message for bytes; // ============ Constructor ============ @@ -65,13 +67,18 @@ contract PolygonPosHook is AbstractMessageIdAuthHook, FxBaseRootTunnel { /// @inheritdoc AbstractMessageIdAuthHook function _sendMessageId( bytes calldata metadata, - bytes memory payload + bytes calldata message ) internal override { require( metadata.msgValue(0) == 0, "PolygonPosHook: does not support msgValue" ); require(msg.value == 0, "PolygonPosHook: does not support msgValue"); + + bytes memory payload = abi.encodeCall( + AbstractMessageIdAuthorizedIsm.verifyMessageId, + message.id() + ); _sendMessageToChild(payload); } diff --git a/solidity/contracts/hooks/aggregation/ERC5164Hook.sol b/solidity/contracts/hooks/aggregation/ERC5164Hook.sol index 8cfc80f06a..ece1bf80d8 100644 --- a/solidity/contracts/hooks/aggregation/ERC5164Hook.sol +++ b/solidity/contracts/hooks/aggregation/ERC5164Hook.sol @@ -15,9 +15,11 @@ pragma solidity >=0.8.0; // ============ Internal Imports ============ import {TypeCasts} from "../../libs/TypeCasts.sol"; +import {Message} from "../../libs/Message.sol"; import {IPostDispatchHook} from "../../interfaces/hooks/IPostDispatchHook.sol"; import {IMessageDispatcher} from "../../interfaces/hooks/IMessageDispatcher.sol"; import {AbstractMessageIdAuthHook} from "../libs/AbstractMessageIdAuthHook.sol"; +import {AbstractMessageIdAuthorizedIsm} from "../../isms/hook/AbstractMessageIdAuthorizedIsm.sol"; // ============ External Imports ============ import {Address} from "@openzeppelin/contracts/utils/Address.sol"; @@ -28,6 +30,8 @@ import {Address} from "@openzeppelin/contracts/utils/Address.sol"; * any of the 5164 adapters. */ contract ERC5164Hook is AbstractMessageIdAuthHook { + using Message for bytes; + IMessageDispatcher public immutable dispatcher; constructor( @@ -55,9 +59,14 @@ contract ERC5164Hook is AbstractMessageIdAuthHook { function _sendMessageId( bytes calldata, /* metadata */ - bytes memory payload + bytes calldata message ) internal override { require(msg.value == 0, "ERC5164Hook: no value allowed"); + + bytes memory payload = abi.encodeCall( + AbstractMessageIdAuthorizedIsm.verifyMessageId, + message.id() + ); dispatcher.dispatchMessage( destinationDomain, TypeCasts.bytes32ToAddress(ism), diff --git a/solidity/contracts/hooks/layer-zero/LayerZeroV2Hook.sol b/solidity/contracts/hooks/layer-zero/LayerZeroV2Hook.sol index 244fea5c76..a93079ff59 100644 --- a/solidity/contracts/hooks/layer-zero/LayerZeroV2Hook.sol +++ b/solidity/contracts/hooks/layer-zero/LayerZeroV2Hook.sol @@ -18,6 +18,7 @@ import {TypeCasts} from "../../libs/TypeCasts.sol"; import {Indexed} from "../../libs/Indexed.sol"; import {IPostDispatchHook} from "../../interfaces/hooks/IPostDispatchHook.sol"; import {AbstractMessageIdAuthHook} from "../libs/AbstractMessageIdAuthHook.sol"; +import {AbstractMessageIdAuthorizedIsm} from "../../isms/hook/AbstractMessageIdAuthorizedIsm.sol"; import {StandardHookMetadata} from "../libs/StandardHookMetadata.sol"; struct LayerZeroV2Metadata { @@ -55,8 +56,13 @@ contract LayerZeroV2Hook is AbstractMessageIdAuthHook { /// @inheritdoc AbstractMessageIdAuthHook function _sendMessageId( bytes calldata metadata, - bytes memory payload + bytes calldata message ) internal override { + bytes memory payload = abi.encodeCall( + AbstractMessageIdAuthorizedIsm.verifyMessageId, + message.id() + ); + bytes calldata lZMetadata = metadata.getCustomMetadata(); ( uint32 eid, diff --git a/solidity/contracts/hooks/libs/AbstractMessageIdAuthHook.sol b/solidity/contracts/hooks/libs/AbstractMessageIdAuthHook.sol index 408bb552ad..49c76e708f 100644 --- a/solidity/contracts/hooks/libs/AbstractMessageIdAuthHook.sol +++ b/solidity/contracts/hooks/libs/AbstractMessageIdAuthHook.sol @@ -72,7 +72,7 @@ abstract contract AbstractMessageIdAuthHook is function _postDispatch( bytes calldata metadata, bytes calldata message - ) internal override { + ) internal virtual override { bytes32 id = message.id(); require( _isLatestDispatched(id), @@ -87,16 +87,15 @@ abstract contract AbstractMessageIdAuthHook is "AbstractMessageIdAuthHook: msgValue must be less than 2 ** 255" ); - bytes memory payload = abi.encodeCall( - AbstractMessageIdAuthorizedIsm.verifyMessageId, - id - ); - _sendMessageId(metadata, payload); + _sendMessageId(metadata, message); uint256 _overpayment = msg.value - _quoteDispatch(metadata, message); if (_overpayment > 0) { address _refundAddress = metadata.refundAddress(msg.sender); - require(_refundAddress != address(0), "no refund address"); + require( + _refundAddress != address(0), + "AbstractPostDispatchHook: no refund address" + ); payable(_refundAddress).sendValue(_overpayment); } } @@ -104,10 +103,10 @@ abstract contract AbstractMessageIdAuthHook is /** * @notice Send a message to the ISM. * @param metadata The metadata for the hook caller - * @param payload The payload for call to the ISM + * @param message The message to send to the ISM */ function _sendMessageId( bytes calldata metadata, - bytes memory payload + bytes calldata message ) internal virtual; } diff --git a/solidity/test/isms/OPL2ToL1Ism.t.sol b/solidity/test/isms/OPL2ToL1Ism.t.sol index d829e0211e..c5f9be93e4 100644 --- a/solidity/test/isms/OPL2ToL1Ism.t.sol +++ b/solidity/test/isms/OPL2ToL1Ism.t.sol @@ -14,6 +14,7 @@ import {MockOptimismMessenger, MockOptimismPortal} from "../../contracts/mock/Mo import {OPL2ToL1Hook} from "../../contracts/hooks/OPL2ToL1Hook.sol"; import {OPL2ToL1Ism} from "../../contracts/isms/hook/OPL2ToL1Ism.sol"; import {ExternalBridgeTest} from "./ExternalBridgeTest.sol"; +import {TestInterchainGasPaymaster} from "../../contracts/test/TestInterchainGasPaymaster.sol"; contract OPL2ToL1IsmTest is ExternalBridgeTest { address internal constant L2_MESSENGER_ADDRESS = @@ -21,6 +22,7 @@ contract OPL2ToL1IsmTest is ExternalBridgeTest { uint256 internal constant MOCK_NONCE = 0; + TestInterchainGasPaymaster internal mockOverheadIgp; MockOptimismPortal internal portal; MockOptimismMessenger internal l1Messenger; @@ -30,7 +32,7 @@ contract OPL2ToL1IsmTest is ExternalBridgeTest { function setUp() public override { // Optimism messenger mock setup - GAS_QUOTE = 120_000; + // GAS_QUOTE = 300_000; vm.etch( L2_MESSENGER_ADDRESS, address(new MockOptimismMessenger()).code @@ -42,12 +44,13 @@ contract OPL2ToL1IsmTest is ExternalBridgeTest { function deployHook() public { originMailbox = new TestMailbox(ORIGIN_DOMAIN); + mockOverheadIgp = new TestInterchainGasPaymaster(); hook = new OPL2ToL1Hook( address(originMailbox), DESTINATION_DOMAIN, TypeCasts.addressToBytes32(address(ism)), L2_MESSENGER_ADDRESS, - uint32(GAS_QUOTE) + address(mockOverheadIgp) ); } @@ -76,7 +79,7 @@ contract OPL2ToL1IsmTest is ExternalBridgeTest { L2_MESSENGER_ADDRESS, abi.encodeCall( ICrossDomainMessenger.sendMessage, - (address(ism), _encodedHookData, uint32(GAS_QUOTE)) + (address(ism), _encodedHookData, uint32(300_000)) ) ); } From a1a850ec2b1c7633d1d54b78d6d584ea48a0fced Mon Sep 17 00:00:00 2001 From: -f Date: Tue, 24 Sep 2024 23:23:36 +0530 Subject: [PATCH 34/47] add tests --- solidity/contracts/hooks/ArbL2ToL1Hook.sol | 23 +++--- solidity/contracts/hooks/OPL2ToL1Hook.sol | 7 +- solidity/script/DeployArbHook.s.sol | 84 ---------------------- solidity/test/isms/ArbL2ToL1Ism.t.sol | 20 +++++- solidity/test/isms/OPL2ToL1Ism.t.sol | 14 ++++ 5 files changed, 50 insertions(+), 98 deletions(-) delete mode 100644 solidity/script/DeployArbHook.s.sol diff --git a/solidity/contracts/hooks/ArbL2ToL1Hook.sol b/solidity/contracts/hooks/ArbL2ToL1Hook.sol index 43060f809d..8d1b863931 100644 --- a/solidity/contracts/hooks/ArbL2ToL1Hook.sol +++ b/solidity/contracts/hooks/ArbL2ToL1Hook.sol @@ -42,8 +42,8 @@ contract ArbL2ToL1Hook is AbstractMessageIdAuthHook { // precompile contract on L2 for sending messages to L1 ArbSys public immutable arbSys; - // Immutable quote amount - uint256 public immutable GAS_QUOTE; + // child hook to call first + AbstractPostDispatchHook public immutable childHook; // ============ Constructor ============ @@ -52,21 +52,24 @@ contract ArbL2ToL1Hook is AbstractMessageIdAuthHook { uint32 _destinationDomain, bytes32 _ism, address _arbSys, - uint256 _gasQuote + address _childHook ) AbstractMessageIdAuthHook(_mailbox, _destinationDomain, _ism) { arbSys = ArbSys(_arbSys); - GAS_QUOTE = _gasQuote; + childHook = AbstractPostDispatchHook(_childHook); } + /// @inheritdoc IPostDispatchHook function hookType() external pure override returns (uint8) { return uint8(IPostDispatchHook.Types.ARB_L2_TO_L1); } + /// @inheritdoc AbstractPostDispatchHook function _quoteDispatch( - bytes calldata, - bytes calldata + bytes calldata metadata, + bytes calldata message ) internal view override returns (uint256) { - return GAS_QUOTE; + return + metadata.msgValue(0) + childHook.quoteDispatch(metadata, message); } // ============ Internal functions ============ @@ -76,12 +79,14 @@ contract ArbL2ToL1Hook is AbstractMessageIdAuthHook { bytes calldata metadata, bytes calldata message ) internal override { - bytes32 id = message.id(); bytes memory payload = abi.encodeCall( AbstractMessageIdAuthorizedIsm.verifyMessageId, - id + message.id() ); + childHook.postDispatch{ + value: childHook.quoteDispatch(metadata, message) + }(metadata, message); arbSys.sendTxToL1{value: metadata.msgValue(0)}( TypeCasts.bytes32ToAddress(ism), payload diff --git a/solidity/contracts/hooks/OPL2ToL1Hook.sol b/solidity/contracts/hooks/OPL2ToL1Hook.sol index 2f58ef7294..ab779b73c0 100644 --- a/solidity/contracts/hooks/OPL2ToL1Hook.sol +++ b/solidity/contracts/hooks/OPL2ToL1Hook.sol @@ -78,13 +78,14 @@ contract OPL2ToL1Hook is AbstractMessageIdAuthHook { bytes calldata metadata, bytes calldata message ) internal override { - bytes32 id = message.id(); bytes memory payload = abi.encodeCall( AbstractMessageIdAuthorizedIsm.verifyMessageId, - id + message.id() ); - childHook.postDispatch(metadata, message); + childHook.postDispatch{ + value: childHook.quoteDispatch(metadata, message) + }(metadata, message); l2Messenger.sendMessage{value: metadata.msgValue(0)}( TypeCasts.bytes32ToAddress(ism), payload, diff --git a/solidity/script/DeployArbHook.s.sol b/solidity/script/DeployArbHook.s.sol deleted file mode 100644 index 89f35f8d44..0000000000 --- a/solidity/script/DeployArbHook.s.sol +++ /dev/null @@ -1,84 +0,0 @@ -// SPDX-License-Identifier: MIT OR Apache-2.0 -pragma solidity >=0.8.0; - -import "forge-std/Script.sol"; - -import {Mailbox} from "../../contracts/Mailbox.sol"; -import {TypeCasts} from "../../contracts/libs/TypeCasts.sol"; -import {ArbL2ToL1Hook} from "../../contracts/hooks/ArbL2ToL1Hook.sol"; -import {ArbL2ToL1Ism} from "../../contracts/isms/hook/ArbL2ToL1Ism.sol"; -import {TestRecipient} from "../../contracts/test/TestRecipient.sol"; -import {TestIsm} from "../../contracts/test/TestIsm.sol"; - -contract DeployArbHook is Script { - uint256 deployerPrivateKey; - - ArbL2ToL1Hook hook; - ArbL2ToL1Ism ism; - - uint32 constant L1_DOMAIN = 11155111; - address constant L1_MAILBOX = 0xfFAEF09B3cd11D9b20d1a19bECca54EEC2884766; - address constant L1_BRIDGE = 0x38f918D0E9F1b721EDaA41302E399fa1B79333a9; - address constant L1_ISM = 0x096A1c034c7Ad113B6dB786b7BA852cB67025458; // placeholder - bytes32 TEST_RECIPIENT = - 0x000000000000000000000000155b1cd2f7cbc58d403b9be341fab6cd77425175; // placeholder - - address constant ARBSYS = 0x0000000000000000000000000000000000000064; - address constant L2_MAILBOX = 0x598facE78a4302f11E3de0bee1894Da0b2Cb71F8; - address constant L2_HOOK = 0xd9d99AC1C645563576b8Df22cBebFC23FB60Ec73; // placeholder - - function deployIsm() external { - deployerPrivateKey = vm.envUint("DEPLOYER_PRIVATE_KEY"); - - vm.startBroadcast(deployerPrivateKey); - - ism = new ArbL2ToL1Ism(L1_BRIDGE); - - TestRecipient testRecipient = new TestRecipient(); - testRecipient.setInterchainSecurityModule(address(ism)); - - vm.stopBroadcast(); - } - - function deployHook() external { - deployerPrivateKey = vm.envUint("DEPLOYER_PRIVATE_KEY"); - - vm.startBroadcast(deployerPrivateKey); - - hook = new ArbL2ToL1Hook( - L2_MAILBOX, - L1_DOMAIN, - TypeCasts.addressToBytes32(L1_ISM), - ARBSYS, - 200_000 // estimated gas amount used for verify - ); - - vm.stopBroadcast(); - } - - function deployTestRecipient() external { - deployerPrivateKey = vm.envUint("DEPLOYER_PRIVATE_KEY"); - - vm.startBroadcast(deployerPrivateKey); - - TestIsm noopIsm = new TestIsm(); - noopIsm.setVerify(true); - TestRecipient testRecipient = new TestRecipient(); - testRecipient.setInterchainSecurityModule(address(noopIsm)); - - console.log("TestRecipient address: %s", address(testRecipient)); - - vm.stopBroadcast(); - } - - function setAuthorizedHook() external { - deployerPrivateKey = vm.envUint("DEPLOYER_PRIVATE_KEY"); - - vm.startBroadcast(deployerPrivateKey); - - ism = ArbL2ToL1Ism(L1_ISM); - ism.setAuthorizedHook(TypeCasts.addressToBytes32(L2_HOOK)); - - vm.stopBroadcast(); - } -} diff --git a/solidity/test/isms/ArbL2ToL1Ism.t.sol b/solidity/test/isms/ArbL2ToL1Ism.t.sol index a251cfda4d..c44665cb5e 100644 --- a/solidity/test/isms/ArbL2ToL1Ism.t.sol +++ b/solidity/test/isms/ArbL2ToL1Ism.t.sol @@ -12,6 +12,7 @@ import {ArbL2ToL1Ism} from "../../contracts/isms/hook/ArbL2ToL1Ism.sol"; import {MockArbBridge, MockArbSys} from "../../contracts/mock/MockArbBridge.sol"; import {TestRecipient} from "../../contracts/test/TestRecipient.sol"; import {ExternalBridgeTest} from "./ExternalBridgeTest.sol"; +import {TestInterchainGasPaymaster} from "../../contracts/test/TestInterchainGasPaymaster.sol"; contract ArbL2ToL1IsmTest is ExternalBridgeTest { uint256 internal constant MOCK_LEAF_INDEX = 40160; @@ -22,10 +23,10 @@ contract ArbL2ToL1IsmTest is ExternalBridgeTest { 0x0000000000000000000000000000000000000064; MockArbBridge internal arbBridge; + TestInterchainGasPaymaster internal mockOverheadIgp; function setUp() public override { // Arbitrum bridge mock setup - GAS_QUOTE = 120_000; vm.etch(L2_ARBSYS_ADDRESS, address(new MockArbSys()).code); deployAll(); @@ -38,12 +39,13 @@ contract ArbL2ToL1IsmTest is ExternalBridgeTest { function deployHook() public { originMailbox = new TestMailbox(ORIGIN_DOMAIN); + mockOverheadIgp = new TestInterchainGasPaymaster(); hook = new ArbL2ToL1Hook( address(originMailbox), DESTINATION_DOMAIN, TypeCasts.addressToBytes32(address(ism)), L2_ARBSYS_ADDRESS, - GAS_QUOTE + address(mockOverheadIgp) ); } @@ -60,6 +62,20 @@ contract ArbL2ToL1IsmTest is ExternalBridgeTest { ism.setAuthorizedHook(TypeCasts.addressToBytes32(address(hook))); } + function test_postDispatch_childHook() public { + bytes memory encodedHookData = _encodeHookData(messageId); + originMailbox.updateLatestDispatchedId(messageId); + _expectOriginExternalBridgeCall(encodedHookData); + + bytes memory igpMetadata = StandardHookMetadata.overrideGasLimit( + 78_000 + ); + + uint256 quote = hook.quoteDispatch(igpMetadata, encodedMessage); + assertEq(quote, mockOverheadIgp.quoteGasPayment(ORIGIN_DOMAIN, 78_000)); + hook.postDispatch{value: quote}(igpMetadata, encodedMessage); + } + /* ============ helper functions ============ */ function _expectOriginExternalBridgeCall( diff --git a/solidity/test/isms/OPL2ToL1Ism.t.sol b/solidity/test/isms/OPL2ToL1Ism.t.sol index c5f9be93e4..7287ba0e11 100644 --- a/solidity/test/isms/OPL2ToL1Ism.t.sol +++ b/solidity/test/isms/OPL2ToL1Ism.t.sol @@ -70,6 +70,20 @@ contract OPL2ToL1IsmTest is ExternalBridgeTest { ism.setAuthorizedHook(TypeCasts.addressToBytes32(address(hook))); } + function test_postDispatch_childHook() public { + bytes memory encodedHookData = _encodeHookData(messageId); + originMailbox.updateLatestDispatchedId(messageId); + _expectOriginExternalBridgeCall(encodedHookData); + + bytes memory igpMetadata = StandardHookMetadata.overrideGasLimit( + 78_000 + ); + + uint256 quote = hook.quoteDispatch(igpMetadata, encodedMessage); + assertEq(quote, mockOverheadIgp.quoteGasPayment(ORIGIN_DOMAIN, 78_000)); + hook.postDispatch{value: quote}(igpMetadata, encodedMessage); + } + /* ============ helper functions ============ */ function _expectOriginExternalBridgeCall( From 7ba121dd7a2e4a8db9c14283cf1693a622ad9766 Mon Sep 17 00:00:00 2001 From: -f Date: Wed, 25 Sep 2024 14:25:23 +0530 Subject: [PATCH 35/47] inter --- .../hook/AbstractMessageIdAuthorizedIsm.sol | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/solidity/contracts/isms/hook/AbstractMessageIdAuthorizedIsm.sol b/solidity/contracts/isms/hook/AbstractMessageIdAuthorizedIsm.sol index fb84fcd74f..969e20966e 100644 --- a/solidity/contracts/isms/hook/AbstractMessageIdAuthorizedIsm.sol +++ b/solidity/contracts/isms/hook/AbstractMessageIdAuthorizedIsm.sol @@ -52,7 +52,7 @@ abstract contract AbstractMessageIdAuthorizedIsm is // ============ Events ============ /// @notice Emitted when a message is received from the external bridge - event ReceivedMessage(bytes32 indexed messageId); + event ReceivedMessage(bytes32 indexed messageId, uint256 msgValue); // ============ Initializer ============ @@ -114,23 +114,21 @@ abstract contract AbstractMessageIdAuthorizedIsm is * @dev Only callable by the authorized hook. * @param messageId Hyperlane Id of the message. */ - function verifyMessageId(bytes32 messageId) public payable virtual { + function verifyMessageId( + bytes32 messageId, + uint256 msgValue + ) public payable virtual { require( _isAuthorized(), "AbstractMessageIdAuthorizedIsm: sender is not the hook" ); require( - msg.value < 2 ** VERIFIED_MASK_INDEX, + msg.value < 2 ** VERIFIED_MASK_INDEX && msg.value == msgValue, "AbstractMessageIdAuthorizedIsm: msg.value must be less than 2^255" ); - uint256 currentValue = verifiedMessages[messageId].clearBit( - VERIFIED_MASK_INDEX - ); - verifiedMessages[messageId] = (currentValue + msg.value).setBit( - VERIFIED_MASK_INDEX - ); - emit ReceivedMessage(messageId); + verifiedMessages[messageId] = msg.value.setBit(VERIFIED_MASK_INDEX); + emit ReceivedMessage(messageId, msgValue); } // ============ Internal Functions ============ From cc69d2e8f7e65f163ec9943febaa3fa46bf5e37a Mon Sep 17 00:00:00 2001 From: -f Date: Wed, 25 Sep 2024 14:31:44 +0530 Subject: [PATCH 36/47] hook encode fix --- solidity/contracts/hooks/ArbL2ToL1Hook.sol | 2 +- solidity/contracts/hooks/OPL2ToL1Hook.sol | 2 +- solidity/contracts/hooks/OPStackHook.sol | 2 +- solidity/contracts/hooks/PolygonPosHook.sol | 2 +- solidity/contracts/hooks/aggregation/ERC5164Hook.sol | 2 +- solidity/contracts/hooks/layer-zero/LayerZeroV2Hook.sol | 2 +- 6 files changed, 6 insertions(+), 6 deletions(-) diff --git a/solidity/contracts/hooks/ArbL2ToL1Hook.sol b/solidity/contracts/hooks/ArbL2ToL1Hook.sol index 8d1b863931..989428cdcc 100644 --- a/solidity/contracts/hooks/ArbL2ToL1Hook.sol +++ b/solidity/contracts/hooks/ArbL2ToL1Hook.sol @@ -81,7 +81,7 @@ contract ArbL2ToL1Hook is AbstractMessageIdAuthHook { ) internal override { bytes memory payload = abi.encodeCall( AbstractMessageIdAuthorizedIsm.verifyMessageId, - message.id() + (message.id(), metadata.msgValue(0)) ); childHook.postDispatch{ diff --git a/solidity/contracts/hooks/OPL2ToL1Hook.sol b/solidity/contracts/hooks/OPL2ToL1Hook.sol index ab779b73c0..6106c85fdf 100644 --- a/solidity/contracts/hooks/OPL2ToL1Hook.sol +++ b/solidity/contracts/hooks/OPL2ToL1Hook.sol @@ -80,7 +80,7 @@ contract OPL2ToL1Hook is AbstractMessageIdAuthHook { ) internal override { bytes memory payload = abi.encodeCall( AbstractMessageIdAuthorizedIsm.verifyMessageId, - message.id() + (message.id(), metadata.msgValue(0)) ); childHook.postDispatch{ diff --git a/solidity/contracts/hooks/OPStackHook.sol b/solidity/contracts/hooks/OPStackHook.sol index 0fb050b063..ae7911ac6a 100644 --- a/solidity/contracts/hooks/OPStackHook.sol +++ b/solidity/contracts/hooks/OPStackHook.sol @@ -75,7 +75,7 @@ contract OPStackHook is AbstractMessageIdAuthHook { ) internal override { bytes memory payload = abi.encodeCall( AbstractMessageIdAuthorizedIsm.verifyMessageId, - message.id() + (message.id(), metadata.msgValue(0)) ); l1Messenger.sendMessage{value: metadata.msgValue(0)}( diff --git a/solidity/contracts/hooks/PolygonPosHook.sol b/solidity/contracts/hooks/PolygonPosHook.sol index 7c4d17cccd..15e972d35f 100644 --- a/solidity/contracts/hooks/PolygonPosHook.sol +++ b/solidity/contracts/hooks/PolygonPosHook.sol @@ -77,7 +77,7 @@ contract PolygonPosHook is AbstractMessageIdAuthHook, FxBaseRootTunnel { bytes memory payload = abi.encodeCall( AbstractMessageIdAuthorizedIsm.verifyMessageId, - message.id() + (message.id(), metadata.msgValue(0)) ); _sendMessageToChild(payload); } diff --git a/solidity/contracts/hooks/aggregation/ERC5164Hook.sol b/solidity/contracts/hooks/aggregation/ERC5164Hook.sol index ece1bf80d8..eb9564e548 100644 --- a/solidity/contracts/hooks/aggregation/ERC5164Hook.sol +++ b/solidity/contracts/hooks/aggregation/ERC5164Hook.sol @@ -65,7 +65,7 @@ contract ERC5164Hook is AbstractMessageIdAuthHook { bytes memory payload = abi.encodeCall( AbstractMessageIdAuthorizedIsm.verifyMessageId, - message.id() + (message.id(), 0) ); dispatcher.dispatchMessage( destinationDomain, diff --git a/solidity/contracts/hooks/layer-zero/LayerZeroV2Hook.sol b/solidity/contracts/hooks/layer-zero/LayerZeroV2Hook.sol index a93079ff59..2d88514a4c 100644 --- a/solidity/contracts/hooks/layer-zero/LayerZeroV2Hook.sol +++ b/solidity/contracts/hooks/layer-zero/LayerZeroV2Hook.sol @@ -60,7 +60,7 @@ contract LayerZeroV2Hook is AbstractMessageIdAuthHook { ) internal override { bytes memory payload = abi.encodeCall( AbstractMessageIdAuthorizedIsm.verifyMessageId, - message.id() + (message.id(), metadata.msgValue(0)) ); bytes calldata lZMetadata = metadata.getCustomMetadata(); From a0aaf2335e4dffdb9601db1c4ac9af7d75487b4f Mon Sep 17 00:00:00 2001 From: -f Date: Wed, 25 Sep 2024 14:36:12 +0530 Subject: [PATCH 37/47] add msgvalue to quote --- solidity/contracts/hooks/OPStackHook.sol | 4 ++-- solidity/contracts/hooks/PolygonPosHook.sol | 2 +- solidity/contracts/hooks/layer-zero/LayerZeroV2Hook.sol | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/solidity/contracts/hooks/OPStackHook.sol b/solidity/contracts/hooks/OPStackHook.sol index 0fb050b063..0ddb713d3a 100644 --- a/solidity/contracts/hooks/OPStackHook.sol +++ b/solidity/contracts/hooks/OPStackHook.sol @@ -62,10 +62,10 @@ contract OPStackHook is AbstractMessageIdAuthHook { // ============ Internal functions ============ function _quoteDispatch( - bytes calldata, + bytes calldata metadata, bytes calldata ) internal pure override returns (uint256) { - return 0; // gas subsidized by the L2 + return metadata.msgValue(0); // gas subsidized by the L2 } /// @inheritdoc AbstractMessageIdAuthHook diff --git a/solidity/contracts/hooks/PolygonPosHook.sol b/solidity/contracts/hooks/PolygonPosHook.sol index 7c4d17cccd..e656a15157 100644 --- a/solidity/contracts/hooks/PolygonPosHook.sol +++ b/solidity/contracts/hooks/PolygonPosHook.sol @@ -61,7 +61,7 @@ contract PolygonPosHook is AbstractMessageIdAuthHook, FxBaseRootTunnel { bytes calldata, bytes calldata ) internal pure override returns (uint256) { - return 0; + return metadata.msgValue(0); } /// @inheritdoc AbstractMessageIdAuthHook diff --git a/solidity/contracts/hooks/layer-zero/LayerZeroV2Hook.sol b/solidity/contracts/hooks/layer-zero/LayerZeroV2Hook.sol index a93079ff59..4d7d73d5cf 100644 --- a/solidity/contracts/hooks/layer-zero/LayerZeroV2Hook.sol +++ b/solidity/contracts/hooks/layer-zero/LayerZeroV2Hook.sol @@ -102,7 +102,7 @@ contract LayerZeroV2Hook is AbstractMessageIdAuthHook { message.senderAddress() ); - return msgFee.nativeFee; + return metadata.msgValue(0) + msgFee.nativeFee; } /** From 43eba79e8936629b91f5c79a0411e646daca0abb Mon Sep 17 00:00:00 2001 From: -f Date: Wed, 25 Sep 2024 15:19:21 +0530 Subject: [PATCH 38/47] revert --- .changeset/metal-doors-float.md | 5 ----- solidity/contracts/hooks/ArbL2ToL1Hook.sol | 2 +- solidity/contracts/hooks/OPL2ToL1Hook.sol | 2 +- solidity/contracts/hooks/OPStackHook.sol | 2 +- solidity/contracts/hooks/PolygonPosHook.sol | 4 ++-- solidity/contracts/hooks/aggregation/ERC5164Hook.sol | 2 +- .../contracts/hooks/layer-zero/LayerZeroV2Hook.sol | 2 +- .../isms/hook/AbstractMessageIdAuthorizedIsm.sol | 11 ++++------- solidity/test/isms/ERC5164ISM.t.sol | 2 -- solidity/test/isms/ExternalBridgeTest.sol | 10 ---------- 10 files changed, 11 insertions(+), 31 deletions(-) delete mode 100644 .changeset/metal-doors-float.md diff --git a/.changeset/metal-doors-float.md b/.changeset/metal-doors-float.md deleted file mode 100644 index 05963c8871..0000000000 --- a/.changeset/metal-doors-float.md +++ /dev/null @@ -1,5 +0,0 @@ ---- -'@hyperlane-xyz/core': patch ---- - -Added check for msg.value > quote for AuthorizedHook diff --git a/solidity/contracts/hooks/ArbL2ToL1Hook.sol b/solidity/contracts/hooks/ArbL2ToL1Hook.sol index 989428cdcc..8d1b863931 100644 --- a/solidity/contracts/hooks/ArbL2ToL1Hook.sol +++ b/solidity/contracts/hooks/ArbL2ToL1Hook.sol @@ -81,7 +81,7 @@ contract ArbL2ToL1Hook is AbstractMessageIdAuthHook { ) internal override { bytes memory payload = abi.encodeCall( AbstractMessageIdAuthorizedIsm.verifyMessageId, - (message.id(), metadata.msgValue(0)) + message.id() ); childHook.postDispatch{ diff --git a/solidity/contracts/hooks/OPL2ToL1Hook.sol b/solidity/contracts/hooks/OPL2ToL1Hook.sol index 6106c85fdf..ab779b73c0 100644 --- a/solidity/contracts/hooks/OPL2ToL1Hook.sol +++ b/solidity/contracts/hooks/OPL2ToL1Hook.sol @@ -80,7 +80,7 @@ contract OPL2ToL1Hook is AbstractMessageIdAuthHook { ) internal override { bytes memory payload = abi.encodeCall( AbstractMessageIdAuthorizedIsm.verifyMessageId, - (message.id(), metadata.msgValue(0)) + message.id() ); childHook.postDispatch{ diff --git a/solidity/contracts/hooks/OPStackHook.sol b/solidity/contracts/hooks/OPStackHook.sol index cc915cde8c..0ddb713d3a 100644 --- a/solidity/contracts/hooks/OPStackHook.sol +++ b/solidity/contracts/hooks/OPStackHook.sol @@ -75,7 +75,7 @@ contract OPStackHook is AbstractMessageIdAuthHook { ) internal override { bytes memory payload = abi.encodeCall( AbstractMessageIdAuthorizedIsm.verifyMessageId, - (message.id(), metadata.msgValue(0)) + message.id() ); l1Messenger.sendMessage{value: metadata.msgValue(0)}( diff --git a/solidity/contracts/hooks/PolygonPosHook.sol b/solidity/contracts/hooks/PolygonPosHook.sol index 43967280ce..2959adc3d2 100644 --- a/solidity/contracts/hooks/PolygonPosHook.sol +++ b/solidity/contracts/hooks/PolygonPosHook.sol @@ -58,7 +58,7 @@ contract PolygonPosHook is AbstractMessageIdAuthHook, FxBaseRootTunnel { // ============ Internal functions ============ function _quoteDispatch( - bytes calldata, + bytes calldata metadata, bytes calldata ) internal pure override returns (uint256) { return metadata.msgValue(0); @@ -77,7 +77,7 @@ contract PolygonPosHook is AbstractMessageIdAuthHook, FxBaseRootTunnel { bytes memory payload = abi.encodeCall( AbstractMessageIdAuthorizedIsm.verifyMessageId, - (message.id(), metadata.msgValue(0)) + message.id() ); _sendMessageToChild(payload); } diff --git a/solidity/contracts/hooks/aggregation/ERC5164Hook.sol b/solidity/contracts/hooks/aggregation/ERC5164Hook.sol index eb9564e548..ece1bf80d8 100644 --- a/solidity/contracts/hooks/aggregation/ERC5164Hook.sol +++ b/solidity/contracts/hooks/aggregation/ERC5164Hook.sol @@ -65,7 +65,7 @@ contract ERC5164Hook is AbstractMessageIdAuthHook { bytes memory payload = abi.encodeCall( AbstractMessageIdAuthorizedIsm.verifyMessageId, - (message.id(), 0) + message.id() ); dispatcher.dispatchMessage( destinationDomain, diff --git a/solidity/contracts/hooks/layer-zero/LayerZeroV2Hook.sol b/solidity/contracts/hooks/layer-zero/LayerZeroV2Hook.sol index b2c74e0efe..4d7d73d5cf 100644 --- a/solidity/contracts/hooks/layer-zero/LayerZeroV2Hook.sol +++ b/solidity/contracts/hooks/layer-zero/LayerZeroV2Hook.sol @@ -60,7 +60,7 @@ contract LayerZeroV2Hook is AbstractMessageIdAuthHook { ) internal override { bytes memory payload = abi.encodeCall( AbstractMessageIdAuthorizedIsm.verifyMessageId, - (message.id(), metadata.msgValue(0)) + message.id() ); bytes calldata lZMetadata = metadata.getCustomMetadata(); diff --git a/solidity/contracts/isms/hook/AbstractMessageIdAuthorizedIsm.sol b/solidity/contracts/isms/hook/AbstractMessageIdAuthorizedIsm.sol index 969e20966e..a8b8baee6c 100644 --- a/solidity/contracts/isms/hook/AbstractMessageIdAuthorizedIsm.sol +++ b/solidity/contracts/isms/hook/AbstractMessageIdAuthorizedIsm.sol @@ -52,7 +52,7 @@ abstract contract AbstractMessageIdAuthorizedIsm is // ============ Events ============ /// @notice Emitted when a message is received from the external bridge - event ReceivedMessage(bytes32 indexed messageId, uint256 msgValue); + event ReceivedMessage(bytes32 indexed messageId); // ============ Initializer ============ @@ -114,21 +114,18 @@ abstract contract AbstractMessageIdAuthorizedIsm is * @dev Only callable by the authorized hook. * @param messageId Hyperlane Id of the message. */ - function verifyMessageId( - bytes32 messageId, - uint256 msgValue - ) public payable virtual { + function verifyMessageId(bytes32 messageId) public payable virtual { require( _isAuthorized(), "AbstractMessageIdAuthorizedIsm: sender is not the hook" ); require( - msg.value < 2 ** VERIFIED_MASK_INDEX && msg.value == msgValue, + msg.value < 2 ** VERIFIED_MASK_INDEX, "AbstractMessageIdAuthorizedIsm: msg.value must be less than 2^255" ); verifiedMessages[messageId] = msg.value.setBit(VERIFIED_MASK_INDEX); - emit ReceivedMessage(messageId, msgValue); + emit ReceivedMessage(messageId); } // ============ Internal Functions ============ diff --git a/solidity/test/isms/ERC5164ISM.t.sol b/solidity/test/isms/ERC5164ISM.t.sol index 2070735cd5..18e7d79d99 100644 --- a/solidity/test/isms/ERC5164ISM.t.sol +++ b/solidity/test/isms/ERC5164ISM.t.sol @@ -150,8 +150,6 @@ contract ERC5164IsmTest is ExternalBridgeTest { function test_verify_valueAlreadyClaimed(uint256) public override {} - function test_verify_override_msgValue() public override {} - function testFuzz_postDispatch_refundsExtraValue(uint256) public override {} /* ============ helper functions ============ */ diff --git a/solidity/test/isms/ExternalBridgeTest.sol b/solidity/test/isms/ExternalBridgeTest.sol index f66a61eb21..df0f2bc3cd 100644 --- a/solidity/test/isms/ExternalBridgeTest.sol +++ b/solidity/test/isms/ExternalBridgeTest.sol @@ -250,16 +250,6 @@ abstract contract ExternalBridgeTest is Test { assertEq(address(testRecipient).balance, _msgValue); } - function test_verify_override_msgValue() public virtual { - bytes memory encodedHookData = _encodeHookData(messageId); - - _externalBridgeDestinationCall(encodedHookData, MSG_VALUE); - _externalBridgeDestinationCall(encodedHookData, 0); - - assertTrue(ism.verify(new bytes(0), encodedMessage)); - assertEq(address(testRecipient).balance, MSG_VALUE); - } - /* ============ helper functions ============ */ function _encodeTestMessage() internal view returns (bytes memory) { From 750d09bb0bdcb660ea79568c26d3d6d7e788c654 Mon Sep 17 00:00:00 2001 From: -f Date: Thu, 26 Sep 2024 12:48:40 +0530 Subject: [PATCH 39/47] spelling --- solidity/test/isms/ExternalBridgeTest.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/solidity/test/isms/ExternalBridgeTest.sol b/solidity/test/isms/ExternalBridgeTest.sol index 1fa4090650..7b200cf1fc 100644 --- a/solidity/test/isms/ExternalBridgeTest.sol +++ b/solidity/test/isms/ExternalBridgeTest.sol @@ -279,6 +279,6 @@ abstract contract ExternalBridgeTest is Test { address _sender ) internal virtual returns (bytes memory) {} - // meant to be mock an arbitrary succesful call made by the external bridge + // meant to be mock an arbitrary successful call made by the external bridge function verifyMessageId(bytes32 /*messageId*/) public payable {} } From 8df91052872dbfe77844ee82ee19dd404f215d88 Mon Sep 17 00:00:00 2001 From: -f Date: Thu, 26 Sep 2024 13:29:55 +0530 Subject: [PATCH 40/47] rm changeset --- .changeset/green-suits-dance.md | 5 ----- 1 file changed, 5 deletions(-) delete mode 100644 .changeset/green-suits-dance.md diff --git a/.changeset/green-suits-dance.md b/.changeset/green-suits-dance.md deleted file mode 100644 index a09a7745e7..0000000000 --- a/.changeset/green-suits-dance.md +++ /dev/null @@ -1,5 +0,0 @@ ---- -'@hyperlane-xyz/core': patch ---- - -Adding msg.value instead of updating in AuthorizedHookIsm From 9093369186993468571b5d411e712ddd9a30c98c Mon Sep 17 00:00:00 2001 From: -f Date: Fri, 4 Oct 2024 11:53:21 +0530 Subject: [PATCH 41/47] small --- solidity/contracts/hooks/ArbL2ToL1Hook.sol | 6 ++---- solidity/contracts/hooks/OPL2ToL1Hook.sol | 2 +- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/solidity/contracts/hooks/ArbL2ToL1Hook.sol b/solidity/contracts/hooks/ArbL2ToL1Hook.sol index 8d1b863931..76c67c0ec6 100644 --- a/solidity/contracts/hooks/ArbL2ToL1Hook.sol +++ b/solidity/contracts/hooks/ArbL2ToL1Hook.sol @@ -15,17 +15,15 @@ pragma solidity >=0.8.0; // ============ Internal Imports ============ import {Message} from "../libs/Message.sol"; +import {IPostDispatchHook} from "../interfaces/hooks/IPostDispatchHook.sol"; import {AbstractPostDispatchHook} from "./libs/AbstractMessageIdAuthHook.sol"; import {AbstractMessageIdAuthHook} from "./libs/AbstractMessageIdAuthHook.sol"; import {AbstractMessageIdAuthorizedIsm} from "../isms/hook/AbstractMessageIdAuthorizedIsm.sol"; import {StandardHookMetadata} from "./libs/StandardHookMetadata.sol"; import {Message} from "../libs/Message.sol"; import {TypeCasts} from "../libs/TypeCasts.sol"; -import {IPostDispatchHook} from "../interfaces/hooks/IPostDispatchHook.sol"; -import {MailboxClient} from "../client/MailboxClient.sol"; // ============ External Imports ============ -import {Address} from "@openzeppelin/contracts/utils/Address.sol"; import {ArbSys} from "@arbitrum/nitro-contracts/src/precompiles/ArbSys.sol"; /** @@ -43,7 +41,7 @@ contract ArbL2ToL1Hook is AbstractMessageIdAuthHook { // precompile contract on L2 for sending messages to L1 ArbSys public immutable arbSys; // child hook to call first - AbstractPostDispatchHook public immutable childHook; + IPostDispatchHook public immutable childHook; // ============ Constructor ============ diff --git a/solidity/contracts/hooks/OPL2ToL1Hook.sol b/solidity/contracts/hooks/OPL2ToL1Hook.sol index ab779b73c0..e82f4f4abf 100644 --- a/solidity/contracts/hooks/OPL2ToL1Hook.sol +++ b/solidity/contracts/hooks/OPL2ToL1Hook.sol @@ -40,7 +40,7 @@ contract OPL2ToL1Hook is AbstractMessageIdAuthHook { // precompile contract on L2 for sending messages to L1 ICrossDomainMessenger public immutable l2Messenger; // child hook to call first - AbstractPostDispatchHook public immutable childHook; + IPostDispatchHook public immutable childHook; // Minimum gas limit that the message can be executed with - OP specific uint32 public constant MIN_GAS_LIMIT = 300_000; From 56c6b1d225d67e5b4e0f5011a711839e17177695 Mon Sep 17 00:00:00 2001 From: -f Date: Wed, 23 Oct 2024 22:55:01 +0530 Subject: [PATCH 42/47] fix LZ double refund --- .../hooks/layer-zero/LayerZeroV2Hook.sol | 4 +- .../hooks/layerzero/LayerZeroV2Hook.t.sol | 38 +++++++++---------- 2 files changed, 21 insertions(+), 21 deletions(-) diff --git a/solidity/contracts/hooks/layer-zero/LayerZeroV2Hook.sol b/solidity/contracts/hooks/layer-zero/LayerZeroV2Hook.sol index 4d7d73d5cf..3a0955e592 100644 --- a/solidity/contracts/hooks/layer-zero/LayerZeroV2Hook.sol +++ b/solidity/contracts/hooks/layer-zero/LayerZeroV2Hook.sol @@ -78,7 +78,9 @@ contract LayerZeroV2Hook is AbstractMessageIdAuthHook { options, false // payInLzToken ); - lZEndpoint.send{value: msg.value}(msgParams, refundAddress); + + uint256 quote = _quoteDispatch(metadata, message); + lZEndpoint.send{value: quote}(msgParams, refundAddress); } /// @dev payInZRO is hardcoded to false because zro tokens should not be directly accepted diff --git a/solidity/test/hooks/layerzero/LayerZeroV2Hook.t.sol b/solidity/test/hooks/layerzero/LayerZeroV2Hook.t.sol index f1a6d08189..8b5d6004c1 100644 --- a/solidity/test/hooks/layerzero/LayerZeroV2Hook.t.sol +++ b/solidity/test/hooks/layerzero/LayerZeroV2Hook.t.sol @@ -147,15 +147,7 @@ contract LayerZeroV2HookTest is Test { vm.assume(balance < nativeFee - 1); vm.deal(address(this), balance); - vm.expectRevert( - abi.encodeWithSelector( - Errors.InsufficientFee.selector, - 100, - balance, - 0, - 0 - ) - ); + vm.expectRevert(); // OutOfFunds mailbox.dispatch{value: balance}( HYPERLANE_DEST_DOMAIN, address(crossChainCounterApp).addressToBytes32(), @@ -165,19 +157,25 @@ contract LayerZeroV2HookTest is Test { ); } - // TODO: fix double refund for LayerZeroV2Hook - // function testLzV2Hook_PostDispatch_refundExtraFee(uint256 balance) public { - // (uint256 nativeFee, bytes memory metadata) = testLzV2Hook_QuoteDispatch_returnsFeeAmount(); - // vm.assume(balance > nativeFee); + function testLzV2Hook_PostDispatch_refundExtraFee(uint256 balance) public { + ( + uint256 nativeFee, + bytes memory metadata + ) = testLzV2Hook_QuoteDispatch_returnsFeeAmount(); + vm.assume(balance > nativeFee); - // uint256 extraValue = balance - nativeFee; - // vm.deal(address(this), balance); + uint256 extraValue = balance - nativeFee; + vm.deal(address(this), balance); - // mailbox.dispatch{value: balance}( - // HYPERLANE_DEST_DOMAIN, address(crossChainCounterApp).addressToBytes32(), "Hello World!", metadata, hook - // ); - // assertEq(address(alice).balance, extraValue); - // } + mailbox.dispatch{value: balance}( + HYPERLANE_DEST_DOMAIN, + address(crossChainCounterApp).addressToBytes32(), + "Hello World!", + metadata, + hook + ); + assertEq(address(alice).balance, extraValue); + } function testLzV2Hook_PostDispatch_executesCrossChain() public { ( From e16958ea5e754c1541eb508e1beb53fdc49224b3 Mon Sep 17 00:00:00 2001 From: -f Date: Tue, 29 Oct 2024 15:57:03 +0530 Subject: [PATCH 43/47] docs(changeset): Checking for sufficient fees in `AbstractMessageIdAuthHook` and refund surplus --- .changeset/little-gifts-hammer.md | 5 +++++ solidity/contracts/hooks/libs/AbstractMessageIdAuthHook.sol | 5 ++++- 2 files changed, 9 insertions(+), 1 deletion(-) create mode 100644 .changeset/little-gifts-hammer.md diff --git a/.changeset/little-gifts-hammer.md b/.changeset/little-gifts-hammer.md new file mode 100644 index 0000000000..087e0145a5 --- /dev/null +++ b/.changeset/little-gifts-hammer.md @@ -0,0 +1,5 @@ +--- +'@hyperlane-xyz/core': minor +--- + +Checking for sufficient fees in `AbstractMessageIdAuthHook` and refund surplus diff --git a/solidity/contracts/hooks/libs/AbstractMessageIdAuthHook.sol b/solidity/contracts/hooks/libs/AbstractMessageIdAuthHook.sol index 49c76e708f..05c2c1d085 100644 --- a/solidity/contracts/hooks/libs/AbstractMessageIdAuthHook.sol +++ b/solidity/contracts/hooks/libs/AbstractMessageIdAuthHook.sol @@ -37,6 +37,7 @@ abstract contract AbstractMessageIdAuthHook is using Address for address payable; using StandardHookMetadata for bytes; using Message for bytes; + using TypeCasts for bytes32; // ============ Constants ============ @@ -91,7 +92,9 @@ abstract contract AbstractMessageIdAuthHook is uint256 _overpayment = msg.value - _quoteDispatch(metadata, message); if (_overpayment > 0) { - address _refundAddress = metadata.refundAddress(msg.sender); + address _refundAddress = metadata.refundAddress( + message.sender().bytes32ToAddress() + ); require( _refundAddress != address(0), "AbstractPostDispatchHook: no refund address" From 64e7628e45dd4c2ac02e66096982aad21c5492bc Mon Sep 17 00:00:00 2001 From: -f Date: Tue, 29 Oct 2024 15:58:45 +0530 Subject: [PATCH 44/47] rm --- .changeset/itchy-singers-hang.md | 5 ----- 1 file changed, 5 deletions(-) delete mode 100644 .changeset/itchy-singers-hang.md diff --git a/.changeset/itchy-singers-hang.md b/.changeset/itchy-singers-hang.md deleted file mode 100644 index 97096ff1a9..0000000000 --- a/.changeset/itchy-singers-hang.md +++ /dev/null @@ -1,5 +0,0 @@ ---- -'@hyperlane-xyz/core': patch ---- - -Patched OPL2ToL1Ism to check for correct messageId for external call in verify From 1ed932db51f09a1300487bc7ed0c4b44604f83c0 Mon Sep 17 00:00:00 2001 From: -f Date: Tue, 29 Oct 2024 16:08:09 +0530 Subject: [PATCH 45/47] docs(changeset): Checking for sufficient fees in `AbstractMessageIdAuthHook` and refund surplus --- .changeset/{little-gifts-hammer.md => cuddly-baboons-drive.md} | 1 + typescript/sdk/src/hook/EvmHookModule.ts | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) rename .changeset/{little-gifts-hammer.md => cuddly-baboons-drive.md} (80%) diff --git a/.changeset/little-gifts-hammer.md b/.changeset/cuddly-baboons-drive.md similarity index 80% rename from .changeset/little-gifts-hammer.md rename to .changeset/cuddly-baboons-drive.md index 087e0145a5..b5a02fb3d1 100644 --- a/.changeset/little-gifts-hammer.md +++ b/.changeset/cuddly-baboons-drive.md @@ -1,4 +1,5 @@ --- +'@hyperlane-xyz/sdk': minor '@hyperlane-xyz/core': minor --- diff --git a/typescript/sdk/src/hook/EvmHookModule.ts b/typescript/sdk/src/hook/EvmHookModule.ts index a1b6d14120..f4b3ca88d9 100644 --- a/typescript/sdk/src/hook/EvmHookModule.ts +++ b/typescript/sdk/src/hook/EvmHookModule.ts @@ -871,7 +871,7 @@ export class EvmHookModule extends HyperlaneModule< this.multiProvider.getDomainId(config.destinationChain), addressToBytes32(arbL2ToL1IsmAddress), config.arbSys, - BigNumber.from(200_000), // 2x estimate of executeTransaction call overhead + '200000', // 2x estimate of executeTransaction call overhead ], ); // set authorized hook on arbL2ToL1 ism From 255f59873b54290727c6b5dbbbfa73bd4d57ef2b Mon Sep 17 00:00:00 2001 From: -f Date: Wed, 30 Oct 2024 15:26:06 +0530 Subject: [PATCH 46/47] arb l2->l1 sdk fix --- typescript/sdk/src/hook/EvmHookModule.ts | 4 +++- typescript/sdk/src/hook/EvmHookReader.ts | 3 +++ typescript/sdk/src/hook/schemas.ts | 1 + .../src/ism/metadata/arbL2ToL1.hardhat-test.ts | 15 +++++++++++++++ 4 files changed, 22 insertions(+), 1 deletion(-) diff --git a/typescript/sdk/src/hook/EvmHookModule.ts b/typescript/sdk/src/hook/EvmHookModule.ts index f4b3ca88d9..b09099464b 100644 --- a/typescript/sdk/src/hook/EvmHookModule.ts +++ b/typescript/sdk/src/hook/EvmHookModule.ts @@ -862,6 +862,8 @@ export class EvmHookModule extends HyperlaneModule< this.multiProvider.getSignerOrProvider(config.destinationChain), ); + const childHook = await this.deploy({ config: config.childHook }); + // deploy arbL1ToL1 hook const hook = await this.deployer.deployContract( chain, @@ -871,7 +873,7 @@ export class EvmHookModule extends HyperlaneModule< this.multiProvider.getDomainId(config.destinationChain), addressToBytes32(arbL2ToL1IsmAddress), config.arbSys, - '200000', // 2x estimate of executeTransaction call overhead + childHook.address, ], ); // set authorized hook on arbL2ToL1 ism diff --git a/typescript/sdk/src/hook/EvmHookReader.ts b/typescript/sdk/src/hook/EvmHookReader.ts index 1ed1f42ab7..f84eecc8aa 100644 --- a/typescript/sdk/src/hook/EvmHookReader.ts +++ b/typescript/sdk/src/hook/EvmHookReader.ts @@ -365,11 +365,14 @@ export class EvmHookReader extends HyperlaneReader implements HookReader { const destinationChainName = this.multiProvider.getChainName(destinationDomain); + const childHookAddress = await hook.childHook(); + const childHookConfig = await this.deriveHookConfig(childHookAddress); const config: WithAddress = { address, type: HookType.ARB_L2_TO_L1, destinationChain: destinationChainName, arbSys, + childHook: childHookConfig, }; this._cache.set(address, config); diff --git a/typescript/sdk/src/hook/schemas.ts b/typescript/sdk/src/hook/schemas.ts index 1111098e8c..16bd01b27f 100644 --- a/typescript/sdk/src/hook/schemas.ts +++ b/typescript/sdk/src/hook/schemas.ts @@ -46,6 +46,7 @@ export const ArbL2ToL1HookSchema = z.object({ 'address of the bridge contract on L1, optional only needed for non @arbitrum/sdk chains', ), destinationChain: z.string(), + childHook: z.lazy((): z.ZodSchema => HookConfigSchema), }); export const IgpSchema = OwnableSchema.extend({ diff --git a/typescript/sdk/src/ism/metadata/arbL2ToL1.hardhat-test.ts b/typescript/sdk/src/ism/metadata/arbL2ToL1.hardhat-test.ts index 5195710674..f4a0335cd0 100644 --- a/typescript/sdk/src/ism/metadata/arbL2ToL1.hardhat-test.ts +++ b/typescript/sdk/src/ism/metadata/arbL2ToL1.hardhat-test.ts @@ -97,6 +97,21 @@ describe('ArbL2ToL1MetadataBuilder', () => { type: HookType.ARB_L2_TO_L1, arbSys: mockArbSys.address, destinationChain: destination, + childHook: { + type: HookType.INTERCHAIN_GAS_PAYMASTER, + beneficiary: relayer.address, + owner: relayer.address, + oracleKey: relayer.address, + overhead: { + [destination]: 200000, + }, + oracleConfig: { + [destination]: { + gasPrice: '20', + tokenExchangeRate: '10000000000', + }, + }, + }, }, }; From 5edc95a54487d6589f0d125d80d2f05c30f16bea Mon Sep 17 00:00:00 2001 From: -f Date: Thu, 31 Oct 2024 06:45:48 +0530 Subject: [PATCH 47/47] test1->test4 --- .../sdk/src/ism/metadata/arbL2ToL1.hardhat-test.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/typescript/sdk/src/ism/metadata/arbL2ToL1.hardhat-test.ts b/typescript/sdk/src/ism/metadata/arbL2ToL1.hardhat-test.ts index f4a0335cd0..f2b66d9cb9 100644 --- a/typescript/sdk/src/ism/metadata/arbL2ToL1.hardhat-test.ts +++ b/typescript/sdk/src/ism/metadata/arbL2ToL1.hardhat-test.ts @@ -41,7 +41,7 @@ import { ArbL2ToL1MetadataBuilder } from './arbL2ToL1.js'; import { MetadataContext } from './builder.js'; describe('ArbL2ToL1MetadataBuilder', () => { - const origin: ChainName = 'test1'; + const origin: ChainName = 'test4'; const destination: ChainName = 'test2'; let core: HyperlaneCore; let ismFactory: HyperlaneIsmFactory; @@ -93,7 +93,7 @@ describe('ArbL2ToL1MetadataBuilder', () => { [], ); hookConfig = { - test1: { + test4: { type: HookType.ARB_L2_TO_L1, arbSys: mockArbSys.address, destinationChain: destination, @@ -115,7 +115,7 @@ describe('ArbL2ToL1MetadataBuilder', () => { }, }; - factoryContracts = contractsMap.test1; + factoryContracts = contractsMap.test4; proxyFactoryAddresses = Object.keys(factoryContracts).reduce((acc, key) => { acc[key] = contractsMap[origin][key as keyof ProxyFactoryFactories].address; @@ -126,11 +126,11 @@ describe('ArbL2ToL1MetadataBuilder', () => { new MockArbBridge__factory(), [], ); - hookConfig.test1.bridge = arbBridge.address; + hookConfig.test4.bridge = arbBridge.address; const hookModule = await EvmHookModule.create({ chain: origin, - config: hookConfig.test1, + config: hookConfig.test4, proxyFactoryFactories: proxyFactoryAddresses, coreAddresses: core.getAddresses(origin), multiProvider,