From 694c6b418153366df1ae2acada73fc473e6a6ee2 Mon Sep 17 00:00:00 2001 From: Michael Amadi Date: Fri, 15 Nov 2024 11:25:42 +0100 Subject: [PATCH 1/5] improve tests --- .../test/L2/L2CrossDomainMessenger.t.sol | 77 ++++++++++++++++++- 1 file changed, 73 insertions(+), 4 deletions(-) diff --git a/packages/contracts-bedrock/test/L2/L2CrossDomainMessenger.t.sol b/packages/contracts-bedrock/test/L2/L2CrossDomainMessenger.t.sol index 131851783c79a..ee861f1b2c8db 100644 --- a/packages/contracts-bedrock/test/L2/L2CrossDomainMessenger.t.sol +++ b/packages/contracts-bedrock/test/L2/L2CrossDomainMessenger.t.sol @@ -5,6 +5,7 @@ pragma solidity 0.8.15; import { CommonTest } from "test/setup/CommonTest.sol"; import { Reverter } from "test/mocks/Callers.sol"; import { EIP1967Helper } from "test/mocks/EIP1967Helper.sol"; +import { stdError } from "forge-std/StdError.sol"; // Libraries import { Hashing } from "src/libraries/Hashing.sol"; @@ -148,17 +149,85 @@ contract L2CrossDomainMessenger_Test is CommonTest { assertEq(l2CrossDomainMessenger.failedMessages(hash), false); } - /// @dev Tests that `relayMessage` reverts if attempting to relay - /// a message sent to an L1 system contract. + /// @dev Tests that relayMessage reverts if caller is optimismPortal and the value sent does not match the amount + /// and if a failedMessage is attempted to be replayed via the optimismPortal + /// signified. + function test_relayMessage_callerIsOptimismPortalChecks_reverts() external { + // set the target to be the OptimismPortal + address target = alice; + address sender = address(l1CrossDomainMessenger); + address caller = AddressAliasHelper.applyL1ToL2Alias(address(l1CrossDomainMessenger)); + bytes memory message = hex"1111"; + + // cannot send a message as optimism portal but amount does not match msg.value + vm.deal(caller, 10 ether); + vm.prank(caller); + vm.expectRevert(stdError.assertionError); + l2CrossDomainMessenger.relayMessage{ value: 10 ether }( + Encoding.encodeVersionedNonce({ _nonce: 0, _version: 1 }), sender, target, 9 ether, 0, message + ); + + // make a failed message + vm.etch(target, hex"fe"); + vm.prank(caller); + l2CrossDomainMessenger.relayMessage( + Encoding.encodeVersionedNonce({ _nonce: 0, _version: 1 }), sender, target, 0, 0, message + ); + + // cannot replay messages when optimism portal is msg.sender + vm.prank(caller); + vm.expectRevert(stdError.assertionError); + l2CrossDomainMessenger.relayMessage( + Encoding.encodeVersionedNonce({ _nonce: 0, _version: 1 }), sender, target, 0, 0, message + ); + } + + /// @dev Tests that relayMessage reverts if attempting to relay a message + /// sent to an L1 system contract. function test_relayMessage_toSystemContract_reverts() external { - address target = address(l2ToL1MessagePasser); address sender = address(l1CrossDomainMessenger); address caller = AddressAliasHelper.applyL1ToL2Alias(address(l1CrossDomainMessenger)); bytes memory message = hex"1111"; + vm.store(address(optimismPortal), bytes32(0), bytes32(abi.encode(sender))); + vm.prank(caller); + vm.expectRevert("CrossDomainMessenger: cannot send message to blocked system address"); + l2CrossDomainMessenger.relayMessage( + Encoding.encodeVersionedNonce({ _nonce: 0, _version: 1 }), + sender, + address(l2CrossDomainMessenger), + 0, + 0, + message + ); + + vm.prank(caller); + vm.expectRevert("CrossDomainMessenger: cannot send message to blocked system address"); + l2CrossDomainMessenger.relayMessage( + Encoding.encodeVersionedNonce({ _nonce: 0, _version: 1 }), + sender, + address(l2ToL1MessagePasser), + 0, + 0, + message + ); + } + + /// @dev Tests that the relayMessage function reverts if the message called by non-optimismPortal but not a failed + /// message + function test_relayMessage_nonFailedMessageCalledByNonOtherMessenger_reverts() external { + address target = address(alice); + address sender = address(l1CrossDomainMessenger); + bytes memory message = hex"1111"; + + vm.store(address(optimismPortal), bytes32(0), bytes32(abi.encode(sender))); + + vm.prank(bob); vm.expectRevert("CrossDomainMessenger: message cannot be replayed"); - l1CrossDomainMessenger.relayMessage(Encoding.encodeVersionedNonce(0, 1), sender, target, 0, 0, message); + l2CrossDomainMessenger.relayMessage( + Encoding.encodeVersionedNonce({ _nonce: 0, _version: 1 }), sender, target, 0, 0, message + ); } /// @dev Tests that `relayMessage` correctly resets the `xDomainMessageSender` From 8d4c4b18f3b11cc885dadbef54f909a8ec50c3e6 Mon Sep 17 00:00:00 2001 From: Michael Amadi Date: Fri, 15 Nov 2024 12:03:53 +0100 Subject: [PATCH 2/5] fixes --- .../test/L2/L2CrossDomainMessenger.t.sol | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/packages/contracts-bedrock/test/L2/L2CrossDomainMessenger.t.sol b/packages/contracts-bedrock/test/L2/L2CrossDomainMessenger.t.sol index ee861f1b2c8db..c1dd6e290641e 100644 --- a/packages/contracts-bedrock/test/L2/L2CrossDomainMessenger.t.sol +++ b/packages/contracts-bedrock/test/L2/L2CrossDomainMessenger.t.sol @@ -150,9 +150,7 @@ contract L2CrossDomainMessenger_Test is CommonTest { } /// @dev Tests that relayMessage reverts if caller is optimismPortal and the value sent does not match the amount - /// and if a failedMessage is attempted to be replayed via the optimismPortal - /// signified. - function test_relayMessage_callerIsOptimismPortalChecks_reverts() external { + function test_relayMessage_callerIsOptimismPortalAndValueDoesNotMatchAmount_reverts() external { // set the target to be the OptimismPortal address target = alice; address sender = address(l1CrossDomainMessenger); @@ -166,6 +164,15 @@ contract L2CrossDomainMessenger_Test is CommonTest { l2CrossDomainMessenger.relayMessage{ value: 10 ether }( Encoding.encodeVersionedNonce({ _nonce: 0, _version: 1 }), sender, target, 9 ether, 0, message ); + } + + /// @dev Tests that relayMessage reverts if a failed message is attempted to be replayed via the optimismPortal + function test_relayMessage_callerIsOptimismPortalAndFailedMessageReplay_reverts() external { + // set the target to be the OptimismPortal + address target = alice; + address sender = address(l1CrossDomainMessenger); + address caller = AddressAliasHelper.applyL1ToL2Alias(address(l1CrossDomainMessenger)); + bytes memory message = hex"1111"; // make a failed message vm.etch(target, hex"fe"); From 4f52eb501023557a1ffa4ddbddea0909ba5428a5 Mon Sep 17 00:00:00 2001 From: Michael Amadi Date: Fri, 15 Nov 2024 19:04:22 +0100 Subject: [PATCH 3/5] fixes... --- .../test/L2/L2CrossDomainMessenger.t.sol | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/packages/contracts-bedrock/test/L2/L2CrossDomainMessenger.t.sol b/packages/contracts-bedrock/test/L2/L2CrossDomainMessenger.t.sol index c1dd6e290641e..9dcbd9c0b83fc 100644 --- a/packages/contracts-bedrock/test/L2/L2CrossDomainMessenger.t.sol +++ b/packages/contracts-bedrock/test/L2/L2CrossDomainMessenger.t.sol @@ -149,15 +149,15 @@ contract L2CrossDomainMessenger_Test is CommonTest { assertEq(l2CrossDomainMessenger.failedMessages(hash), false); } - /// @dev Tests that relayMessage reverts if caller is optimismPortal and the value sent does not match the amount - function test_relayMessage_callerIsOptimismPortalAndValueDoesNotMatchAmount_reverts() external { - // set the target to be the OptimismPortal + /// @dev Tests that relayMessage reverts if the value sent does not match the amount + function test_relayMessage_valueDoesNotMatchAmount_reverts() external { + // set the target to be the alice address target = alice; address sender = address(l1CrossDomainMessenger); address caller = AddressAliasHelper.applyL1ToL2Alias(address(l1CrossDomainMessenger)); bytes memory message = hex"1111"; - // cannot send a message as optimism portal but amount does not match msg.value + // cannot send a message where the amount inputted does not match the msg.value vm.deal(caller, 10 ether); vm.prank(caller); vm.expectRevert(stdError.assertionError); @@ -166,9 +166,10 @@ contract L2CrossDomainMessenger_Test is CommonTest { ); } - /// @dev Tests that relayMessage reverts if a failed message is attempted to be replayed via the optimismPortal - function test_relayMessage_callerIsOptimismPortalAndFailedMessageReplay_reverts() external { - // set the target to be the OptimismPortal + /// @dev Tests that relayMessage reverts if a failed message is attempted to be replayed and the caller is the other + /// messenger + function test_relayMessage_failedMessageReplay_reverts() external { + // set the target to be the alice address target = alice; address sender = address(l1CrossDomainMessenger); address caller = AddressAliasHelper.applyL1ToL2Alias(address(l1CrossDomainMessenger)); @@ -181,7 +182,7 @@ contract L2CrossDomainMessenger_Test is CommonTest { Encoding.encodeVersionedNonce({ _nonce: 0, _version: 1 }), sender, target, 0, 0, message ); - // cannot replay messages when optimism portal is msg.sender + // cannot replay messages when the caller is the other messenger vm.prank(caller); vm.expectRevert(stdError.assertionError); l2CrossDomainMessenger.relayMessage( From 1bad60f44447b6cdebe128fdcc9780c5d8d941c4 Mon Sep 17 00:00:00 2001 From: Michael Amadi Date: Mon, 18 Nov 2024 18:35:55 +0100 Subject: [PATCH 4/5] Apply suggestions from code review Co-authored-by: smartcontracts --- .../test/L2/L2CrossDomainMessenger.t.sol | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/contracts-bedrock/test/L2/L2CrossDomainMessenger.t.sol b/packages/contracts-bedrock/test/L2/L2CrossDomainMessenger.t.sol index 9dcbd9c0b83fc..469b1aa1bfd57 100644 --- a/packages/contracts-bedrock/test/L2/L2CrossDomainMessenger.t.sol +++ b/packages/contracts-bedrock/test/L2/L2CrossDomainMessenger.t.sol @@ -150,8 +150,8 @@ contract L2CrossDomainMessenger_Test is CommonTest { } /// @dev Tests that relayMessage reverts if the value sent does not match the amount - function test_relayMessage_valueDoesNotMatchAmount_reverts() external { - // set the target to be the alice + function test_relayMessage_fromOtherMessengerValueMismatch_reverts() external { + // set the target to be alice address target = alice; address sender = address(l1CrossDomainMessenger); address caller = AddressAliasHelper.applyL1ToL2Alias(address(l1CrossDomainMessenger)); @@ -168,7 +168,7 @@ contract L2CrossDomainMessenger_Test is CommonTest { /// @dev Tests that relayMessage reverts if a failed message is attempted to be replayed and the caller is the other /// messenger - function test_relayMessage_failedMessageReplay_reverts() external { + function test_relayMessage_fromOtherMessengerFailedMessageReplay_reverts() external { // set the target to be the alice address target = alice; address sender = address(l1CrossDomainMessenger); @@ -224,7 +224,7 @@ contract L2CrossDomainMessenger_Test is CommonTest { /// @dev Tests that the relayMessage function reverts if the message called by non-optimismPortal but not a failed /// message - function test_relayMessage_nonFailedMessageCalledByNonOtherMessenger_reverts() external { + function test_relayMessage_relayingNewMessageByExternalUser_reverts() external { address target = address(alice); address sender = address(l1CrossDomainMessenger); bytes memory message = hex"1111"; From 63110544efdbc0766b9d718e15737aa7ecc89eeb Mon Sep 17 00:00:00 2001 From: Michael Amadi Date: Mon, 18 Nov 2024 18:40:13 +0100 Subject: [PATCH 5/5] fixes --- .../test/L2/L2CrossDomainMessenger.t.sol | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/packages/contracts-bedrock/test/L2/L2CrossDomainMessenger.t.sol b/packages/contracts-bedrock/test/L2/L2CrossDomainMessenger.t.sol index 469b1aa1bfd57..f5ef2d6355954 100644 --- a/packages/contracts-bedrock/test/L2/L2CrossDomainMessenger.t.sol +++ b/packages/contracts-bedrock/test/L2/L2CrossDomainMessenger.t.sol @@ -169,7 +169,7 @@ contract L2CrossDomainMessenger_Test is CommonTest { /// @dev Tests that relayMessage reverts if a failed message is attempted to be replayed and the caller is the other /// messenger function test_relayMessage_fromOtherMessengerFailedMessageReplay_reverts() external { - // set the target to be the alice + // set the target to be alice address target = alice; address sender = address(l1CrossDomainMessenger); address caller = AddressAliasHelper.applyL1ToL2Alias(address(l1CrossDomainMessenger)); @@ -191,8 +191,8 @@ contract L2CrossDomainMessenger_Test is CommonTest { } /// @dev Tests that relayMessage reverts if attempting to relay a message - /// sent to an L1 system contract. - function test_relayMessage_toSystemContract_reverts() external { + /// sent to self + function test_relayMessage_toSelf_reverts() external { address sender = address(l1CrossDomainMessenger); address caller = AddressAliasHelper.applyL1ToL2Alias(address(l1CrossDomainMessenger)); bytes memory message = hex"1111"; @@ -209,6 +209,16 @@ contract L2CrossDomainMessenger_Test is CommonTest { 0, message ); + } + + /// @dev Tests that relayMessage reverts if attempting to relay a message + /// sent to the l2ToL1MessagePasser address + function test_relayMessage_toL2ToL1MessagePasser_reverts() external { + address sender = address(l1CrossDomainMessenger); + address caller = AddressAliasHelper.applyL1ToL2Alias(address(l1CrossDomainMessenger)); + bytes memory message = hex"1111"; + + vm.store(address(optimismPortal), bytes32(0), bytes32(abi.encode(sender))); vm.prank(caller); vm.expectRevert("CrossDomainMessenger: cannot send message to blocked system address");