From d22ab3be027c4a12759da6c6dee1ca376317a4e3 Mon Sep 17 00:00:00 2001 From: Kelvin Fichter Date: Thu, 14 Jul 2022 11:56:36 -0400 Subject: [PATCH] test(ctb): reentrancy on relay Adds tests for reentrancy on message relaying. --- packages/contracts-bedrock/.gas-snapshot | 32 ++++++++------- .../contracts/test/CommonTest.t.sol | 20 +++++++++ .../test/L1CrossDomainMessenger.t.sol | 41 ++++++++++++++++++- .../test/L2CrossDomainMessenger.t.sol | 40 +++++++++++++++++- 4 files changed, 116 insertions(+), 17 deletions(-) diff --git a/packages/contracts-bedrock/.gas-snapshot b/packages/contracts-bedrock/.gas-snapshot index 9c057f7c1c8fe..4038316a277e4 100644 --- a/packages/contracts-bedrock/.gas-snapshot +++ b/packages/contracts-bedrock/.gas-snapshot @@ -35,20 +35,21 @@ L1BlockTest:test_updateValues() (gas: 28216) L1BlockNumberTest:test_fallback() (gas: 18774) L1BlockNumberTest:test_getL1BlockNumber() (gas: 10657) L1BlockNumberTest:test_receive() (gas: 25437) -L1CrossDomainMessenger_Test:testCannot_L1MessengerPause() (gas: 24561) +L1CrossDomainMessenger_Test:testCannot_L1MessengerPause() (gas: 24539) L1CrossDomainMessenger_Test:testCannot_L1MessengerUnpause() (gas: 24508) -L1CrossDomainMessenger_Test:test_L1MessengerMessageVersion() (gas: 24725) -L1CrossDomainMessenger_Test:test_L1MessengerPause() (gas: 48017) -L1CrossDomainMessenger_Test:test_L1MessengerRelayMessageFirstStuckSecondSucceeds() (gas: 201762) -L1CrossDomainMessenger_Test:test_L1MessengerRelayMessageSucceeds() (gas: 77784) -L1CrossDomainMessenger_Test:test_L1MessengerRelayMessageToSystemContract() (gas: 67778) -L1CrossDomainMessenger_Test:test_L1MessengerRelayShouldRevertIfPaused() (gas: 60493) +L1CrossDomainMessenger_Test:test_L1MessengerMessageVersion() (gas: 24748) +L1CrossDomainMessenger_Test:test_L1MessengerPause() (gas: 48061) +L1CrossDomainMessenger_Test:test_L1MessengerRelayMessageFirstStuckSecondSucceeds() (gas: 201827) +L1CrossDomainMessenger_Test:test_L1MessengerRelayMessageRevertsOnReentrancy() (gas: 195116) +L1CrossDomainMessenger_Test:test_L1MessengerRelayMessageSucceeds() (gas: 77762) +L1CrossDomainMessenger_Test:test_L1MessengerRelayMessageToSystemContract() (gas: 67801) +L1CrossDomainMessenger_Test:test_L1MessengerRelayShouldRevertIfPaused() (gas: 60471) L1CrossDomainMessenger_Test:test_L1MessengerReplayMessageWithValue() (gas: 38127) -L1CrossDomainMessenger_Test:test_L1MessengerSendMessage() (gas: 297767) -L1CrossDomainMessenger_Test:test_L1MessengerTwiceSendMessage() (gas: 1490070) -L1CrossDomainMessenger_Test:test_L1MessengerUnpause() (gas: 40889) +L1CrossDomainMessenger_Test:test_L1MessengerSendMessage() (gas: 297745) +L1CrossDomainMessenger_Test:test_L1MessengerTwiceSendMessage() (gas: 1490048) +L1CrossDomainMessenger_Test:test_L1MessengerUnpause() (gas: 40908) L1CrossDomainMessenger_Test:test_L1MessengerXDomainSenderReverts() (gas: 24249) -L1CrossDomainMessenger_Test:test_L1MessengerxDomainMessageSenderResets() (gas: 86168) +L1CrossDomainMessenger_Test:test_L1MessengerxDomainMessageSenderResets() (gas: 86191) L1StandardBridge_Test:test_depositERC20() (gas: 578867) L1StandardBridge_Test:test_depositERC20To() (gas: 581048) L1StandardBridge_Test:test_depositETH() (gas: 372975) @@ -62,16 +63,17 @@ L1StandardBridge_Test:test_onlyL2BridgeFinalizeERC20Withdrawal() (gas: 36271) L1StandardBridge_Test:test_onlyPortalFinalizeERC20Withdrawal() (gas: 35600) L1StandardBridge_Test:test_receive() (gas: 519560) L2CrossDomainMessenger_Test:testCannot_L2MessengerPause() (gas: 10823) +L2CrossDomainMessenger_Test:test_L1MessengerRelayMessageRevertsOnReentrancy() (gas: 171968) L2CrossDomainMessenger_Test:test_L2MessengerMessageVersion() (gas: 8455) -L2CrossDomainMessenger_Test:test_L2MessengerPause() (gas: 31772) -L2CrossDomainMessenger_Test:test_L2MessengerRelayMessageFirstStuckSecondSucceeds() (gas: 173026) -L2CrossDomainMessenger_Test:test_L2MessengerRelayMessageSucceeds() (gas: 57288) +L2CrossDomainMessenger_Test:test_L2MessengerPause() (gas: 31750) +L2CrossDomainMessenger_Test:test_L2MessengerRelayMessageFirstStuckSecondSucceeds() (gas: 173004) +L2CrossDomainMessenger_Test:test_L2MessengerRelayMessageSucceeds() (gas: 57311) L2CrossDomainMessenger_Test:test_L2MessengerRelayMessageToSystemContract() (gas: 36115) L2CrossDomainMessenger_Test:test_L2MessengerRelayShouldRevertIfPaused() (gas: 41578) L2CrossDomainMessenger_Test:test_L2MessengerSendMessage() (gas: 120536) L2CrossDomainMessenger_Test:test_L2MessengerTwiceSendMessage() (gas: 133720) L2CrossDomainMessenger_Test:test_L2MessengerXDomainSenderReverts() (gas: 10554) -L2CrossDomainMessenger_Test:test_L2MessengerxDomainMessageSenderResets() (gas: 54754) +L2CrossDomainMessenger_Test:test_L2MessengerxDomainMessageSenderResets() (gas: 54732) L2OutputOracleTest:testCannot_ProposeWithUnmatchedBlockhash() (gas: 26786) L2OutputOracleTest:testCannot_deleteL2Output_ifNotOwner() (gas: 24844) L2OutputOracleTest:testCannot_deleteL2Output_withWrongRoot() (gas: 91114) diff --git a/packages/contracts-bedrock/contracts/test/CommonTest.t.sol b/packages/contracts-bedrock/contracts/test/CommonTest.t.sol index 0bab742510273..b05432bde76c6 100644 --- a/packages/contracts-bedrock/contracts/test/CommonTest.t.sol +++ b/packages/contracts-bedrock/contracts/test/CommonTest.t.sol @@ -168,6 +168,8 @@ contract Messenger_Initializer is L2OutputOracle_Initializer { event WithdrawalFinalized(bytes32 indexed, bool success); + event WhatHappened(bool success, bytes returndata); + function setUp() public virtual override { super.setUp(); @@ -419,3 +421,21 @@ contract Reverter { revert(); } } + +// Useful for testing reentrancy guards +contract CallerCaller { + event WhatHappened( + bool success, + bytes returndata + ); + + fallback() external { + (bool success, bytes memory returndata) = msg.sender.call(msg.data); + emit WhatHappened(success, returndata); + assembly { + switch success + case 0 { revert(add(returndata, 0x20), mload(returndata)) } + default { return(add(returndata, 0x20), mload(returndata)) } + } + } +} diff --git a/packages/contracts-bedrock/contracts/test/L1CrossDomainMessenger.t.sol b/packages/contracts-bedrock/contracts/test/L1CrossDomainMessenger.t.sol index db0e70c4be863..c74f1a0488ded 100644 --- a/packages/contracts-bedrock/contracts/test/L1CrossDomainMessenger.t.sol +++ b/packages/contracts-bedrock/contracts/test/L1CrossDomainMessenger.t.sol @@ -2,7 +2,7 @@ pragma solidity 0.8.10; /* Testing utilities */ -import { Messenger_Initializer, Reverter } from "./CommonTest.t.sol"; +import { Messenger_Initializer, Reverter, CallerCaller } from "./CommonTest.t.sol"; import { L2OutputOracle_Initializer } from "./L2OutputOracle.t.sol"; /* Libraries */ @@ -266,4 +266,43 @@ contract L1CrossDomainMessenger_Test is Messenger_Initializer { assertEq(L1Messenger.successfulMessages(hash), true); assertEq(L1Messenger.receivedMessages(hash), true); } + + // relayMessage: should revert if recipient is trying to reenter + function test_L1MessengerRelayMessageRevertsOnReentrancy() external { + address target = address(0xabcd); + address sender = PredeployAddresses.L2_CROSS_DOMAIN_MESSENGER; + bytes memory message = abi.encodeWithSelector( + L1Messenger.relayMessage.selector, + 0, + sender, + target, + 0, + 0, + hex"1111" + ); + + bytes32 hash = Hashing.hashCrossDomainMessage(0, sender, target, 0, 0, message); + + uint256 senderSlotIndex = 51; + vm.store(address(op), bytes32(senderSlotIndex), bytes32(abi.encode(sender))); + vm.etch(target, address(new CallerCaller()).code); + + vm.expectEmit(true, true, true, true, target); + + emit WhatHappened(false, abi.encodeWithSignature("Error(string)", "ReentrancyGuard: reentrant call")); + + vm.prank(address(op)); + vm.expectCall(target, message); + L1Messenger.relayMessage( + 0, // nonce + sender, + target, + 0, // value + 0, + message + ); + + assertEq(L1Messenger.successfulMessages(hash), false); + assertEq(L1Messenger.receivedMessages(hash), true); + } } diff --git a/packages/contracts-bedrock/contracts/test/L2CrossDomainMessenger.t.sol b/packages/contracts-bedrock/contracts/test/L2CrossDomainMessenger.t.sol index 84c086b9ad206..86b5070a88a54 100644 --- a/packages/contracts-bedrock/contracts/test/L2CrossDomainMessenger.t.sol +++ b/packages/contracts-bedrock/contracts/test/L2CrossDomainMessenger.t.sol @@ -1,7 +1,7 @@ //SPDX-License-Identifier: MIT pragma solidity 0.8.10; -import { Messenger_Initializer, Reverter } from "./CommonTest.t.sol"; +import { Messenger_Initializer, Reverter, CallerCaller } from "./CommonTest.t.sol"; import { AddressAliasHelper } from "../vendor/AddressAliasHelper.sol"; import { L2ToL1MessagePasser } from "../L2/L2ToL1MessagePasser.sol"; @@ -212,4 +212,42 @@ contract L2CrossDomainMessenger_Test is Messenger_Initializer { assertEq(L2Messenger.successfulMessages(hash), true); assertEq(L2Messenger.receivedMessages(hash), true); } + + // relayMessage: should revert if recipient is trying to reenter + function test_L1MessengerRelayMessageRevertsOnReentrancy() external { + address target = address(0xabcd); + address sender = address(L1Messenger); + address caller = AddressAliasHelper.applyL1ToL2Alias(address(L1Messenger)); + bytes memory message = abi.encodeWithSelector( + L2Messenger.relayMessage.selector, + 0, + sender, + target, + 0, + 0, + hex"1111" + ); + + bytes32 hash = Hashing.hashCrossDomainMessage(0, sender, target, 0, 0, message); + + vm.etch(target, address(new CallerCaller()).code); + + vm.expectEmit(true, true, true, true, target); + + emit WhatHappened(false, abi.encodeWithSignature("Error(string)", "ReentrancyGuard: reentrant call")); + + vm.prank(caller); + vm.expectCall(target, message); + L2Messenger.relayMessage( + 0, // nonce + sender, + target, + 0, // value + 0, + message + ); + + assertEq(L2Messenger.successfulMessages(hash), false); + assertEq(L2Messenger.receivedMessages(hash), true); + } }