Skip to content

N
nebula
Commit f644f853 authored by clabby's avatar clabby
Browse files
Options

Change `relayMessage`'s reentrancy guard to use a per-message reentrancy guard

parent 240e1fa2
Show whitespace changes
Inline Side-by-side
Showing with 396 additions and 61 deletions
packages/contracts-bedrock/.gas-snapshot
View file @ f644f853
......@@ -21,7 +21,7 @@ CrossDomainOwnableThroughPortal_Test:test_depositTransaction_crossDomainOwner_su
CrossDomainOwnable_Test:test_onlyOwner_notOwner_reverts() (gas: 10530)
CrossDomainOwnable_Test:test_onlyOwner_succeeds() (gas: 34861)
CrossDomainOwnable2_Test:test_onlyOwner_notMessenger_reverts() (gas: 8416)
CrossDomainOwnable2_Test:test_onlyOwner_notOwner2_reverts() (gas: 61738)
CrossDomainOwnable2_Test:test_onlyOwner_notOwner2_reverts() (gas: 64332)
CrossDomainOwnable2_Test:test_onlyOwner_notOwner_reverts() (gas: 16588)
CrossDomainOwnable2_Test:test_onlyOwner_succeeds() (gas: 77766)
CrossDomainOwnable3_Test:test_constructor_succeeds() (gas: 10510)
......@@ -87,7 +87,7 @@ L1CrossDomainMessenger_Test:test_sendMessage_succeeds() (gas: 299710)
L1CrossDomainMessenger_Test:test_sendMessage_twice_succeeds() (gas: 1490566)
L1CrossDomainMessenger_Test:test_unpause_callerIsNotOwner_reverts() (gas: 24516)
L1CrossDomainMessenger_Test:test_unpause_succeeds() (gas: 45185)
L1CrossDomainMessenger_Test:test_xDomainMessageSender_reset_succeeds() (gas: 84020)
L1CrossDomainMessenger_Test:test_xDomainMessageSender_reset_succeeds() (gas: 85083)
L1CrossDomainMessenger_Test:test_xDomainSender_notSet_reverts() (gas: 24274)
L1ERC721Bridge_Test:test_bridgeERC721To_localTokenZeroAddress_reverts() (gas: 52730)
L1ERC721Bridge_Test:test_bridgeERC721To_remoteTokenZeroAddress_reverts() (gas: 27332)
......@@ -126,15 +126,16 @@ L1StandardBridge_Receive_Test:test_receive_succeeds() (gas: 520198)
L2CrossDomainMessenger_Test:test_messageVersion_succeeds() (gas: 8389)
L2CrossDomainMessenger_Test:test_pause_notOwner_reverts() (gas: 10837)
L2CrossDomainMessenger_Test:test_pause_succeeds() (gas: 31846)
L2CrossDomainMessenger_Test:test_relayMessage_paused_reverts() (gas: 41551)
L2CrossDomainMessenger_Test:test_relayMessage_reentrancy_reverts() (gas: 167727)
L2CrossDomainMessenger_Test:test_relayMessage_retry_succeeds() (gas: 168327)
L2CrossDomainMessenger_Test:test_relayMessage_succeeds() (gas: 53190)
L2CrossDomainMessenger_Test:test_relayMessage_toSystemContract_reverts() (gas: 36201)
L2CrossDomainMessenger_Test:test_relayMessage_v2_reverts() (gas: 18868)
L2CrossDomainMessenger_Test:test_relayMessage_paused_reverts() (gas: 59603)
L2CrossDomainMessenger_Test:test_relayMessage_reentrancyDiffMessage_succeeds() (gas: 686507)
L2CrossDomainMessenger_Test:test_relayMessage_reentrancySameMessage_reverts() (gas: 630624)
L2CrossDomainMessenger_Test:test_relayMessage_retry_succeeds() (gas: 170647)
L2CrossDomainMessenger_Test:test_relayMessage_succeeds() (gas: 57428)
L2CrossDomainMessenger_Test:test_relayMessage_toSystemContract_reverts() (gas: 54435)
L2CrossDomainMessenger_Test:test_relayMessage_v2_reverts() (gas: 11925)
L2CrossDomainMessenger_Test:test_sendMessage_succeeds() (gas: 122621)
L2CrossDomainMessenger_Test:test_sendMessage_twice_succeeds() (gas: 134715)
L2CrossDomainMessenger_Test:test_xDomainMessageSender_reset_succeeds() (gas: 52578)
L2CrossDomainMessenger_Test:test_sendMessage_twice_succeeds() (gas: 134738)
L2CrossDomainMessenger_Test:test_xDomainMessageSender_reset_succeeds() (gas: 56860)
L2CrossDomainMessenger_Test:test_xDomainSender_senderNotSet_reverts() (gas: 10524)
L2ERC721Bridge_Test:test_bridgeERC721To_localTokenZeroAddress_reverts() (gas: 26454)
L2ERC721Bridge_Test:test_bridgeERC721To_remoteTokenZeroAddress_reverts() (gas: 21770)
......@@ -237,6 +238,7 @@ MintManager_mint_Test:test_mint_moreThanCap_reverts() (gas: 142478)
MintManager_upgrade_Test:test_upgrade_fromNotOwner_reverts() (gas: 10929)
MintManager_upgrade_Test:test_upgrade_fromOwner_succeeds() (gas: 23411)
MintManager_upgrade_Test:test_upgrade_toZeroAddress_reverts() (gas: 10958)
OptimismPortal_Sherlock87_Test:test_finalizeWithdrawalTransaction_gasTooLowBrick_succeeds() (gas: 1073643)
OptimismMintableERC20_Test:test_bridge_succeeds() (gas: 7643)
OptimismMintableERC20_Test:test_burn_notBridge_reverts() (gas: 11142)
OptimismMintableERC20_Test:test_burn_succeeds() (gas: 50960)
......
packages/contracts-bedrock/contracts/test/CommonTest.t.sol
View file @ f644f853
......@@ -684,3 +684,64 @@ contract CallerCaller {
}
}
}
// Used for testing the `CrossDomainMessenger`'s per-message reentrancy guard.
contract ConfigurableCaller {
bool doRevert = true;
address target;
bytes payload;
event WhatHappened(bool success, bytes returndata);
/**
* @notice Call the configured target with the configured payload OR revert.
*/
function call() external {
if (doRevert) {
revert("ConfigurableCaller: revert");
} else {
(bool success, bytes memory returndata) = address(target).call(payload);
emit WhatHappened(success, returndata);
assembly {
switch success
case 0 {
revert(add(returndata, 0x20), mload(returndata))
}
default {
return(add(returndata, 0x20), mload(returndata))
}
}
}
}
/**
* @notice Set whether or not to have `call` revert.
*/
function setDoRevert(bool _doRevert) external {
doRevert = _doRevert;
}
/**
* @notice Set the target for the call made in `call`.
*/
function setTarget(address _target) external {
target = _target;
}
/**
* @notice Set the payload for the call made in `call`.
*/
function setPayload(bytes calldata _payload) external {
payload = _payload;
}
/**
* @notice Fallback function that reverts if `doRevert` is true.
* Otherwise, it does nothing.
*/
fallback() external {
if (doRevert) {
revert("ConfigurableCaller: revert");
}
}
}
packages/contracts-bedrock/contracts/test/L1CrossDomainMessenger.t.sol
View file @ f644f853
......@@ -2,7 +2,7 @@
pragma solidity 0.8.15;
/* Testing utilities */
import { Messenger_Initializer, Reverter, CallerCaller } from "./CommonTest.t.sol";
import { Messenger_Initializer, Reverter, ConfigurableCaller } from "./CommonTest.t.sol";
import { L2OutputOracle_Initializer } from "./L2OutputOracle.t.sol";
/* Libraries */
......@@ -148,9 +148,7 @@ contract L1CrossDomainMessenger_Test is Messenger_Initializer {
vm.store(address(op), bytes32(senderSlotIndex), bytes32(abi.encode(sender)));
// Expect a revert.
vm.expectRevert(
"CrossDomainMessenger: only version 0 or 1 messages are supported at this time"
);
vm.expectRevert("Hashing: unknown cross domain message version");
// Try to relay a v2 message.
vm.prank(address(op));
......@@ -340,52 +338,171 @@ contract L1CrossDomainMessenger_Test is Messenger_Initializer {
assertEq(L1Messenger.failedMessages(hash), true);
}
// relayMessage: should revert if recipient is trying to reenter
function test_relayMessage_reentrancy_reverts() external {
address target = address(0xabcd);
// relayMessage: Should revert if the recipient is trying to reenter with the
// same message.
function test_relayMessage_reentrancySameMessage_reverts() external {
ConfigurableCaller caller = new ConfigurableCaller();
address target = address(caller);
address sender = Predeploys.L2_CROSS_DOMAIN_MESSENGER;
bytes memory message = abi.encodeWithSelector(
L1Messenger.relayMessage.selector,
bytes memory callMessage = abi.encodeWithSelector(caller.call.selector);
bytes32 hash = Hashing.hashCrossDomainMessage(
Encoding.encodeVersionedNonce({ _nonce: 0, _version: 1 }),
sender,
target,
0,
0,
hex"1111"
callMessage
);
bytes32 hash = Hashing.hashCrossDomainMessage(
Encoding.encodeVersionedNonce({ _nonce: 0, _version: 1 }),
// Set the portal's `l2Sender` to the `sender`.
vm.store(address(op), bytes32(senderSlotIndex), bytes32(uint256(uint160(sender))));
// Act as the portal and call the `relayMessage` function with the `innerMessage`.
vm.prank(address(op));
vm.expectCall(target, callMessage);
L1Messenger.relayMessage(
Encoding.encodeVersionedNonce(0, 1),
sender,
target,
0,
0,
message
callMessage
);
vm.store(address(op), bytes32(senderSlotIndex), bytes32(abi.encode(sender)));
vm.etch(target, address(new CallerCaller()).code);
// Assert that the message failed to be relayed
assertFalse(L1Messenger.successfulMessages(hash));
assertTrue(L1Messenger.failedMessages(hash));
vm.expectEmit(true, true, true, true, target);
// Set the configurable caller's target to `L1Messenger` and set the payload to `relayMessage(...)`.
caller.setDoRevert(false);
caller.setTarget(address(L1Messenger));
caller.setPayload(
abi.encodeWithSelector(
L1Messenger.relayMessage.selector,
Encoding.encodeVersionedNonce(0, 1),
sender,
target,
0,
0,
callMessage
)
);
// Attempt to replay the failed message, which will *not* immediately revert this time around,
// but attempt to reenter `relayMessage` with the same message hash. The reentrancy attempt should
// revert.
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(
Encoding.encodeVersionedNonce({ _nonce: 0, _version: 1 }), // nonce
sender,
target,
0, // value
0,
message
0,
callMessage
);
assertEq(L1Messenger.successfulMessages(hash), false);
assertEq(L1Messenger.failedMessages(hash), true);
// Assert that the message still failed to be relayed.
assertFalse(L1Messenger.successfulMessages(hash));
assertTrue(L1Messenger.failedMessages(hash));
}
// relayMessage: should not revert if the recipient reenters `relayMessage` with a different
// message hash.
function test_relayMessage_reentrancyDiffMessage_succeeds() external {
ConfigurableCaller caller = new ConfigurableCaller();
address target = address(caller);
address sender = Predeploys.L2_CROSS_DOMAIN_MESSENGER;
bytes memory messageA = abi.encodeWithSelector(caller.call.selector);
bytes memory messageB = hex"";
bytes32 hashA = Hashing.hashCrossDomainMessage(
Encoding.encodeVersionedNonce(0, 1),
sender,
target,
0,
0,
messageA
);
bytes32 hashB = Hashing.hashCrossDomainMessage(
Encoding.encodeVersionedNonce(0, 1),
sender,
target,
0,
0,
messageB
);
// Set the portal's `l2Sender` to the `sender`.
vm.store(address(op), bytes32(senderSlotIndex), bytes32(uint256(uint160(sender))));
// Act as the portal and call the `relayMessage` function with both `messageA` and `messageB`.
vm.startPrank(address(op));
vm.expectCall(target, messageA);
L1Messenger.relayMessage(
Encoding.encodeVersionedNonce(0, 1),
sender,
target,
0,
0,
messageA
);
vm.expectCall(target, messageB);
L1Messenger.relayMessage(
Encoding.encodeVersionedNonce(0, 1),
sender,
target,
0,
0,
messageB
);
// Stop acting as the portal
vm.stopPrank();
// Assert that both messages failed to be relayed
assertFalse(L1Messenger.successfulMessages(hashA));
assertFalse(L1Messenger.successfulMessages(hashB));
assertTrue(L1Messenger.failedMessages(hashA));
assertTrue(L1Messenger.failedMessages(hashB));
// Set the configurable caller's target to `L1Messenger` and set the payload to `relayMessage(...)`.
caller.setDoRevert(false);
caller.setTarget(address(L1Messenger));
caller.setPayload(
abi.encodeWithSelector(
L1Messenger.relayMessage.selector,
Encoding.encodeVersionedNonce(0, 1),
sender,
target,
0,
0,
messageB
)
);
// Attempt to replay the failed message, which will *not* immediately revert this time around,
// but attempt to reenter `relayMessage` with messageB. The reentrancy attempt should succeed
// because the message hashes are different.
vm.expectEmit(true, true, true, true, target);
emit WhatHappened(true, hex"");
L1Messenger.relayMessage(
Encoding.encodeVersionedNonce(0, 1),
sender,
target,
0,
0,
messageA
);
// Assert that both messages are now in the `successfulMessages` mapping.
assertTrue(L1Messenger.successfulMessages(hashA));
assertTrue(L1Messenger.successfulMessages(hashB));
}
function test_relayMessage_legacy_succeeds() external {
......
packages/contracts-bedrock/contracts/test/L2CrossDomainMessenger.t.sol
View file @ f644f853
// SPDX-License-Identifier: MIT
pragma solidity 0.8.15;
import { Messenger_Initializer, Reverter, CallerCaller } from "./CommonTest.t.sol";
import { Messenger_Initializer, Reverter, ConfigurableCaller } from "./CommonTest.t.sol";
import { AddressAliasHelper } from "../vendor/AddressAliasHelper.sol";
import { L2ToL1MessagePasser } from "../L2/L2ToL1MessagePasser.sol";
......@@ -99,9 +99,7 @@ contract L2CrossDomainMessenger_Test is Messenger_Initializer {
address caller = AddressAliasHelper.applyL1ToL2Alias(address(L1Messenger));
// Expect a revert.
vm.expectRevert(
"CrossDomainMessenger: only version 0 or 1 messages are supported at this time"
);
vm.expectRevert("Hashing: unknown cross domain message version");
// Try to relay a v2 message.
vm.prank(caller);
......@@ -255,51 +253,167 @@ contract L2CrossDomainMessenger_Test is Messenger_Initializer {
assertEq(L2Messenger.failedMessages(hash), true);
}
// relayMessage: should revert if recipient is trying to reenter
function test_relayMessage_reentrancy_reverts() external {
address target = address(0xabcd);
// relayMessage: Should revert if the recipient is trying to reenter with the
// same message.
function test_relayMessage_reentrancySameMessage_reverts() external {
ConfigurableCaller caller = new ConfigurableCaller();
address target = address(caller);
address sender = address(L1Messenger);
address caller = AddressAliasHelper.applyL1ToL2Alias(address(L1Messenger));
bytes memory message = abi.encodeWithSelector(
L2Messenger.relayMessage.selector,
address l1XDMAlias = AddressAliasHelper.applyL1ToL2Alias(address(L1Messenger));
bytes memory callMessage = abi.encodeWithSelector(caller.call.selector);
bytes32 hash = Hashing.hashCrossDomainMessage(
Encoding.encodeVersionedNonce(0, 1),
sender,
target,
0,
0,
hex"1111"
callMessage
);
bytes32 hash = Hashing.hashCrossDomainMessage(
// Act as the L1XDM and call the `relayMessage` function with the `innerMessage`.
vm.prank(l1XDMAlias);
vm.expectCall(target, callMessage);
L2Messenger.relayMessage(
Encoding.encodeVersionedNonce(0, 1),
sender,
target,
0,
0,
message
callMessage
);
vm.etch(target, address(new CallerCaller()).code);
// Assert that the message failed to be relayed
assertFalse(L2Messenger.successfulMessages(hash));
assertTrue(L2Messenger.failedMessages(hash));
vm.expectEmit(true, true, true, true, target);
// Set the configurable caller's target to `L2Messenger` and set the payload to `relayMessage(...)`.
caller.setDoRevert(false);
caller.setTarget(address(L2Messenger));
caller.setPayload(
abi.encodeWithSelector(
L2Messenger.relayMessage.selector,
Encoding.encodeVersionedNonce(0, 1),
sender,
target,
0,
0,
callMessage
)
);
// Attempt to replay the failed message, which will *not* immediately revert this time around,
// but attempt to reenter `relayMessage` with the same message hash. The reentrancy attempt should
// revert.
vm.expectEmit(true, true, true, true, target);
emit WhatHappened(
false,
abi.encodeWithSignature("Error(string)", "ReentrancyGuard: reentrant call")
);
L2Messenger.relayMessage(
Encoding.encodeVersionedNonce(0, 1),
sender,
target,
0,
0,
callMessage
);
vm.prank(caller);
vm.expectCall(target, message);
// Assert that the message still failed to be relayed.
assertFalse(L2Messenger.successfulMessages(hash));
assertTrue(L2Messenger.failedMessages(hash));
}
// relayMessage: should not revert if the recipient reenters `relayMessage` with a different
// message hash.
function test_relayMessage_reentrancyDiffMessage_succeeds() external {
ConfigurableCaller caller = new ConfigurableCaller();
address target = address(caller);
address sender = address(L1Messenger);
address l1XDMAlias = AddressAliasHelper.applyL1ToL2Alias(address(L1Messenger));
bytes memory messageA = abi.encodeWithSelector(caller.call.selector);
bytes memory messageB = hex"";
bytes32 hashA = Hashing.hashCrossDomainMessage(
Encoding.encodeVersionedNonce(0, 1),
sender,
target,
0,
0,
messageA
);
bytes32 hashB = Hashing.hashCrossDomainMessage(
Encoding.encodeVersionedNonce(0, 1),
sender,
target,
0,
0,
messageB
);
// Act as the L1XDM and call the `relayMessage` function with both `messageA` and `messageB`.
vm.startPrank(l1XDMAlias);
vm.expectCall(target, messageA);
L2Messenger.relayMessage(
Encoding.encodeVersionedNonce(0, 1), // nonce
Encoding.encodeVersionedNonce(0, 1),
sender,
target,
0, // value
0,
message
0,
messageA
);
vm.expectCall(target, messageB);
L2Messenger.relayMessage(
Encoding.encodeVersionedNonce(0, 1),
sender,
target,
0,
0,
messageB
);
assertEq(L2Messenger.successfulMessages(hash), false);
assertEq(L2Messenger.failedMessages(hash), true);
// Stop acting as the L1XDM
vm.stopPrank();
// Assert that both messages failed to be relayed
assertFalse(L2Messenger.successfulMessages(hashA));
assertFalse(L2Messenger.successfulMessages(hashB));
assertTrue(L2Messenger.failedMessages(hashA));
assertTrue(L2Messenger.failedMessages(hashB));
// Set the configurable caller's target to `L2Messenger` and set the payload to `relayMessage(...)`.
caller.setDoRevert(false);
caller.setTarget(address(L2Messenger));
caller.setPayload(
abi.encodeWithSelector(
L2Messenger.relayMessage.selector,
Encoding.encodeVersionedNonce(0, 1),
sender,
target,
0,
0,
messageB
)
);
// Attempt to replay the failed message, which will *not* immediately revert this time around,
// but attempt to reenter `relayMessage` with messageB. The reentrancy attempt should succeed
// because the message hashes are different.
vm.expectEmit(true, true, true, true, target);
emit WhatHappened(true, hex"");
L2Messenger.relayMessage(
Encoding.encodeVersionedNonce(0, 1),
sender,
target,
0,
0,
messageA
);
// Assert that both messages are now in the `successfulMessages` mapping.
assertTrue(L2Messenger.successfulMessages(hashA));
assertTrue(L2Messenger.successfulMessages(hashB));
}
}
packages/contracts-bedrock/contracts/universal/CrossDomainMessenger.sol
View file @ f644f853
......@@ -176,6 +176,45 @@ abstract contract CrossDomainMessenger is
*/
event FailedRelayedMessage(bytes32 indexed msgHash);
/**
* @notice Modifier for a per-message reentrancy guard.
*/
modifier perMessageNonReentrant(bytes32 _msgHash) {
bytes32 _hashMsgHash;
assembly {
// Re-hash the `_msgHash` with the `0xcafebabe` salt to reduce the possibility
// of collisions.
mstore(0x00, _msgHash)
mstore(0x20, 0xcafebabe)
_hashMsgHash := keccak256(0x00, 0x40)
// Check if the reentrancy lock for the `_msgHash` is set. If so, revert.
if sload(_hashMsgHash) {
// SAFETY: We're reverting, so it's fine that we're clobbering the free memory
// pointer.
// Store selector for "Error(string)" in scratch space
mstore(0x00, 0x08c379a0)
// Store pointer to the string in scratch space
mstore(0x20, 0x20)
// Zero-out the free memory pointer (prevents corruption of the length word)
mstore(0x40, 0x00)
// 31 length + "ReentrancyGuard: reentrant call"
// Note that we do not need to zero-out the memory at 0x60, as it is the zero slot.
mstore(0x5f, 0x1f5265656e7472616e637947756172643a207265656e7472616e742063616c6c)
// Revert with 'Error("ReentrancyGuard: reentrant call")'
revert(0x1c, 0x64)
}
// Trigger the reentrancy lock for `_msgHash`.
sstore(_hashMsgHash, 0x01)
}
_;
assembly {
// Clear the reentrancy lock for `_msgHash`
sstore(_hashMsgHash, 0x00)
}
}
/**
* @param _otherMessenger Address of the messenger on the paired chain.
*/
......@@ -260,13 +299,15 @@ abstract contract CrossDomainMessenger is
uint256 _value,
uint256 _minGasLimit,
bytes calldata _message
) external payable nonReentrant whenNotPaused {
)
external
payable
perMessageNonReentrant(
Hashing.hashCrossDomainMessage(_nonce, _sender, _target, _value, _minGasLimit, _message)
)
whenNotPaused
{
(, uint16 version) = Encoding.decodeVersionedNonce(_nonce);
require(
version < 2,
"CrossDomainMessenger: only version 0 or 1 messages are supported at this time"
);
// If the message is version 0, then it's a migrated legacy withdrawal. We therefore need
// to check that the legacy version of the message has not already been relayed.
if (version == 0) {
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment