Skip to content

Commit

Permalink
feat(ctb): Immediately fail any message upon reentry
Browse files Browse the repository at this point in the history
  • Loading branch information
maurelian authored and refcell committed Apr 18, 2023
1 parent a01d02c commit 74586d6
Show file tree
Hide file tree
Showing 11 changed files with 189 additions and 386 deletions.
2 changes: 1 addition & 1 deletion op-bindings/bindings/l1crossdomainmessenger.go

Large diffs are not rendered by default.

4 changes: 2 additions & 2 deletions op-bindings/bindings/l1crossdomainmessenger_more.go

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion op-bindings/bindings/l2crossdomainmessenger.go

Large diffs are not rendered by default.

4 changes: 2 additions & 2 deletions op-bindings/bindings/l2crossdomainmessenger_more.go

Large diffs are not rendered by default.

38 changes: 17 additions & 21 deletions packages/contracts-bedrock/.gas-snapshot
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,14 @@ Bytes_toNibbles_Test:test_toNibbles_expectedResult5Bytes_works() (gas: 6132)
Bytes_toNibbles_Test:test_toNibbles_zeroLengthInput_works() (gas: 944)
CrossDomainMessenger_BaseGas_Test:test_baseGas_succeeds() (gas: 20097)
CrossDomainOwnable2_Test:test_onlyOwner_notMessenger_reverts() (gas: 8416)
CrossDomainOwnable2_Test:test_onlyOwner_notOwner2_reverts() (gas: 61872)
CrossDomainOwnable2_Test:test_onlyOwner_notOwner2_reverts() (gas: 57254)
CrossDomainOwnable2_Test:test_onlyOwner_notOwner_reverts() (gas: 16566)
CrossDomainOwnable2_Test:test_onlyOwner_succeeds() (gas: 75933)
CrossDomainOwnable2_Test:test_onlyOwner_succeeds() (gas: 73282)
CrossDomainOwnable3_Test:test_constructor_succeeds() (gas: 10554)
CrossDomainOwnable3_Test:test_crossDomainOnlyOwner_notMessenger_reverts() (gas: 28334)
CrossDomainOwnable3_Test:test_crossDomainOnlyOwner_notOwner2_reverts() (gas: 76381)
CrossDomainOwnable3_Test:test_crossDomainOnlyOwner_notOwner2_reverts() (gas: 73730)
CrossDomainOwnable3_Test:test_crossDomainOnlyOwner_notOwner_reverts() (gas: 31978)
CrossDomainOwnable3_Test:test_crossDomainTransferOwnership_succeeds() (gas: 93938)
CrossDomainOwnable3_Test:test_crossDomainTransferOwnership_succeeds() (gas: 91287)
CrossDomainOwnable3_Test:test_localOnlyOwner_notOwner_reverts() (gas: 13193)
CrossDomainOwnable3_Test:test_localOnlyOwner_succeeds() (gas: 35220)
CrossDomainOwnable3_Test:test_localTransferOwnership_succeeds() (gas: 52128)
Expand Down Expand Up @@ -70,20 +70,18 @@ L1BlockTest:test_timestamp_succeeds() (gas: 7640)
L1BlockTest:test_updateValues_succeeds() (gas: 60482)
L1CrossDomainMessenger_Test:test_messageVersion_succeeds() (gas: 24715)
L1CrossDomainMessenger_Test:test_relayMessage_legacyOldReplay_reverts() (gas: 49394)
L1CrossDomainMessenger_Test:test_relayMessage_legacyRetryAfterFailureThenSuccess_reverts() (gas: 232952)
L1CrossDomainMessenger_Test:test_relayMessage_legacyRetryAfterFailure_succeeds() (gas: 206446)
L1CrossDomainMessenger_Test:test_relayMessage_legacyRetryAfterSuccess_reverts() (gas: 146819)
L1CrossDomainMessenger_Test:test_relayMessage_legacy_succeeds() (gas: 79729)
L1CrossDomainMessenger_Test:test_relayMessage_reentrancyDiffMessage_succeeds() (gas: 725335)
L1CrossDomainMessenger_Test:test_relayMessage_reentrancySameMessage_reverts() (gas: 662298)
L1CrossDomainMessenger_Test:test_relayMessage_retryAfterFailure_succeeds() (gas: 200353)
L1CrossDomainMessenger_Test:test_relayMessage_succeeds() (gas: 76665)
L1CrossDomainMessenger_Test:test_relayMessage_toSystemContract_reverts() (gas: 101282)
L1CrossDomainMessenger_Test:test_relayMessage_legacyRetryAfterFailureThenSuccess_reverts() (gas: 209286)
L1CrossDomainMessenger_Test:test_relayMessage_legacyRetryAfterFailure_succeeds() (gas: 203184)
L1CrossDomainMessenger_Test:test_relayMessage_legacyRetryAfterSuccess_reverts() (gas: 123784)
L1CrossDomainMessenger_Test:test_relayMessage_legacy_succeeds() (gas: 77098)
L1CrossDomainMessenger_Test:test_relayMessage_retryAfterFailure_succeeds() (gas: 197091)
L1CrossDomainMessenger_Test:test_relayMessage_succeeds() (gas: 74034)
L1CrossDomainMessenger_Test:test_relayMessage_toSystemContract_reverts() (gas: 56540)
L1CrossDomainMessenger_Test:test_relayMessage_v2_reverts() (gas: 12365)
L1CrossDomainMessenger_Test:test_replayMessage_withValue_reverts() (gas: 53445)
L1CrossDomainMessenger_Test:test_replayMessage_withValue_reverts() (gas: 31063)
L1CrossDomainMessenger_Test:test_sendMessage_succeeds() (gas: 304740)
L1CrossDomainMessenger_Test:test_sendMessage_twice_succeeds() (gas: 1496124)
L1CrossDomainMessenger_Test:test_xDomainMessageSender_reset_succeeds() (gas: 87194)
L1CrossDomainMessenger_Test:test_xDomainMessageSender_reset_succeeds() (gas: 84563)
L1CrossDomainMessenger_Test:test_xDomainSender_notSet_reverts() (gas: 24296)
L1ERC721Bridge_Test:test_bridgeERC721To_localTokenZeroAddress_reverts() (gas: 52707)
L1ERC721Bridge_Test:test_bridgeERC721To_remoteTokenZeroAddress_reverts() (gas: 27310)
Expand Down Expand Up @@ -120,15 +118,13 @@ L1StandardBridge_Getter_Test:test_getters_succeeds() (gas: 32173)
L1StandardBridge_Initialize_Test:test_initialize_succeeds() (gas: 22050)
L1StandardBridge_Receive_Test:test_receive_succeeds() (gas: 525438)
L2CrossDomainMessenger_Test:test_messageVersion_succeeds() (gas: 8411)
L2CrossDomainMessenger_Test:test_relayMessage_reentrancyDiffMessage_succeeds() (gas: 680395)
L2CrossDomainMessenger_Test:test_relayMessage_reentrancySameMessage_reverts() (gas: 626456)
L2CrossDomainMessenger_Test:test_relayMessage_retry_succeeds() (gas: 166461)
L2CrossDomainMessenger_Test:test_relayMessage_succeeds() (gas: 54980)
L2CrossDomainMessenger_Test:test_relayMessage_toSystemContract_reverts() (gas: 51448)
L2CrossDomainMessenger_Test:test_relayMessage_retry_succeeds() (gas: 163159)
L2CrossDomainMessenger_Test:test_relayMessage_succeeds() (gas: 48640)
L2CrossDomainMessenger_Test:test_relayMessage_toSystemContract_reverts() (gas: 29021)
L2CrossDomainMessenger_Test:test_relayMessage_v2_reverts() (gas: 11711)
L2CrossDomainMessenger_Test:test_sendMessage_succeeds() (gas: 122508)
L2CrossDomainMessenger_Test:test_sendMessage_twice_succeeds() (gas: 134826)
L2CrossDomainMessenger_Test:test_xDomainMessageSender_reset_succeeds() (gas: 54580)
L2CrossDomainMessenger_Test:test_xDomainMessageSender_reset_succeeds() (gas: 48139)
L2CrossDomainMessenger_Test:test_xDomainSender_senderNotSet_reverts() (gas: 10590)
L2ERC721Bridge_Test:test_bridgeERC721To_localTokenZeroAddress_reverts() (gas: 26431)
L2ERC721Bridge_Test:test_bridgeERC721To_remoteTokenZeroAddress_reverts() (gas: 21814)
Expand Down
6 changes: 2 additions & 4 deletions packages/contracts-bedrock/.storage-layout
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,7 @@
| xDomainMsgSender | address | 204 | 0 | 20 | contracts/L1/L1CrossDomainMessenger.sol:L1CrossDomainMessenger |
| msgNonce | uint240 | 205 | 0 | 30 | contracts/L1/L1CrossDomainMessenger.sol:L1CrossDomainMessenger |
| failedMessages | mapping(bytes32 => bool) | 206 | 0 | 32 | contracts/L1/L1CrossDomainMessenger.sol:L1CrossDomainMessenger |
| reentrancyLocks | mapping(bytes32 => bool) | 207 | 0 | 32 | contracts/L1/L1CrossDomainMessenger.sol:L1CrossDomainMessenger |
| __gap | uint256[41] | 208 | 0 | 1312 | contracts/L1/L1CrossDomainMessenger.sol:L1CrossDomainMessenger |
| __gap | uint256[42] | 207 | 0 | 1344 | contracts/L1/L1CrossDomainMessenger.sol:L1CrossDomainMessenger |

=======================
➡ contracts/L1/L1StandardBridge.sol:L1StandardBridge
Expand Down Expand Up @@ -135,8 +134,7 @@
| xDomainMsgSender | address | 204 | 0 | 20 | contracts/L2/L2CrossDomainMessenger.sol:L2CrossDomainMessenger |
| msgNonce | uint240 | 205 | 0 | 30 | contracts/L2/L2CrossDomainMessenger.sol:L2CrossDomainMessenger |
| failedMessages | mapping(bytes32 => bool) | 206 | 0 | 32 | contracts/L2/L2CrossDomainMessenger.sol:L2CrossDomainMessenger |
| reentrancyLocks | mapping(bytes32 => bool) | 207 | 0 | 32 | contracts/L2/L2CrossDomainMessenger.sol:L2CrossDomainMessenger |
| __gap | uint256[41] | 208 | 0 | 1312 | contracts/L2/L2CrossDomainMessenger.sol:L2CrossDomainMessenger |
| __gap | uint256[42] | 207 | 0 | 1344 | contracts/L2/L2CrossDomainMessenger.sol:L2CrossDomainMessenger |

=======================
➡ contracts/L2/L2StandardBridge.sol:L2StandardBridge
Expand Down
154 changes: 153 additions & 1 deletion packages/contracts-bedrock/contracts/test/CrossDomainMessenger.t.sol
Original file line number Diff line number Diff line change
@@ -1,7 +1,13 @@
// SPDX-License-Identifier: MIT
pragma solidity 0.8.15;

import { Messenger_Initializer, Reverter, CallerCaller } from "./CommonTest.t.sol";
import { Messenger_Initializer, Reverter, CallerCaller, CommonTest } from "./CommonTest.t.sol";
import { L1CrossDomainMessenger } from "../L1/L1CrossDomainMessenger.sol";

// Libraries
import { Predeploys } from "../libraries/Predeploys.sol";
import { Hashing } from "../libraries/Hashing.sol";
import { Encoding } from "../libraries/Encoding.sol";

// CrossDomainMessenger_Test is for testing functionality which is common to both the L1 and L2
// CrossDomainMessenger contracts. For simplicity, we use the L1 Messenger as the test contract.
Expand All @@ -17,3 +23,149 @@ contract CrossDomainMessenger_BaseGas_Test is Messenger_Initializer {
L1Messenger.baseGas(hex"ff", _minGasLimit);
}
}

/**
* @title ExternalRelay
* @notice A mock external contract called via the SafeCall inside
* the CrossDomainMessenger's `relayMessage` function.
*/
contract ExternalRelay is CommonTest {
address internal op;
address internal fuzzedSender;
L1CrossDomainMessenger internal L1Messenger;

event FailedRelayedMessage(bytes32 indexed msgHash);

constructor(L1CrossDomainMessenger _l1Messenger, address _op) {
L1Messenger = _l1Messenger;
op = _op;
}

/**
* @notice Internal helper function to relay a message and perform assertions.
*/
function _internalRelay(address _innerSender) internal {
address initialSender = L1Messenger.xDomainMessageSender();

bytes memory callMessage = getCallData();

bytes32 hash = Hashing.hashCrossDomainMessage({
_nonce: Encoding.encodeVersionedNonce({ _nonce: 0, _version: 1 }),
_sender: _innerSender,
_target: address(this),
_value: 0,
_gasLimit: 0,
_data: callMessage
});

vm.expectEmit(true, true, true, true);
emit FailedRelayedMessage(hash);

vm.prank(address(op));
L1Messenger.relayMessage({
_nonce: Encoding.encodeVersionedNonce({ _nonce: 0, _version: 1 }),
_sender: _innerSender,
_target: address(this),
_value: 0,
_minGasLimit: 0,
_message: callMessage
});

assertTrue(L1Messenger.failedMessages(hash));
assertFalse(L1Messenger.successfulMessages(hash));
assertEq(initialSender, L1Messenger.xDomainMessageSender());
}

/**
* @notice externalCallWithMinGas is called by the CrossDomainMessenger.
*/
function externalCallWithMinGas() external payable {
for (uint256 i = 0; i < 10; i++) {
address _innerSender;
unchecked {
_innerSender = address(uint160(uint256(uint160(fuzzedSender)) + i));
}
_internalRelay(_innerSender);
}
}

/**
* @notice Helper function to get the callData for an `externalCallWithMinGas
*/
function getCallData() public returns (bytes memory) {
return abi.encodeWithSelector(ExternalRelay.externalCallWithMinGas.selector);
}

/**
* @notice Helper function to set the fuzzed sender
*/
function setFuzzedSender(address _fuzzedSender) public {
fuzzedSender = _fuzzedSender;
}
}

/**
* @title CrossDomainMessenger_RelayMessage_Test
* @notice Fuzz tests re-entrancy into the CrossDomainMessenger relayMessage function.
*/
contract CrossDomainMessenger_RelayMessage_Test is Messenger_Initializer {
// Storage slot of the l2Sender
uint256 constant senderSlotIndex = 50;

ExternalRelay public er;

function setUp() public override {
super.setUp();
er = new ExternalRelay(L1Messenger, address(op));
}

/**
* @dev This test mocks an OptimismPortal call to the L1CrossDomainMessenger via
* the relayMessage function. The relayMessage function will then use SafeCall's
* callWithMinGas to call the target with call data packed in the callMessage.
* For this test, the callWithMinGas will call the mock ExternalRelay test contract
* defined above, executing the externalCallWithMinGas function which will try to
* re-enter the CrossDomainMessenger's relayMessage function, resulting in that message
* being recorded as failed.
*/
function testFuzz_relayMessageReenter_succeeds(address _sender, uint256 _gasLimit) external {
vm.assume(_sender != Predeploys.L2_CROSS_DOMAIN_MESSENGER);
address sender = Predeploys.L2_CROSS_DOMAIN_MESSENGER;

er.setFuzzedSender(_sender);
address target = address(er);
bytes memory callMessage = er.getCallData();

vm.expectCall(target, callMessage);

uint64 gasLimit = uint64(bound(_gasLimit, 0, 30_000_000));

bytes32 hash = Hashing.hashCrossDomainMessage({
_nonce: Encoding.encodeVersionedNonce({ _nonce: 0, _version: 1 }),
_sender: sender,
_target: target,
_value: 0,
_gasLimit: gasLimit,
_data: callMessage
});

// set the value of op.l2Sender() to be the L2 Cross Domain Messenger.
vm.store(address(op), bytes32(senderSlotIndex), bytes32(abi.encode(sender)));
vm.prank(address(op));
L1Messenger.relayMessage({
_nonce: Encoding.encodeVersionedNonce({ _nonce: 0, _version: 1 }),
_sender: sender,
_target: target,
_value: 0,
_minGasLimit: gasLimit,
_message: callMessage
});

assertTrue(L1Messenger.successfulMessages(hash));
assertEq(L1Messenger.failedMessages(hash), false);

// Ensures that the `xDomainMsgSender` is set back to `Predeploys.L2_CROSS_DOMAIN_MESSENGER`
vm.expectRevert("CrossDomainMessenger: xDomainMessageSender is not set");
L1Messenger.xDomainMessageSender();
}
}
Loading

0 comments on commit 74586d6

Please sign in to comment.