Skip to content

N
nebula
Commit b3ec2877 authored by mergify[bot]'s avatar mergify[bot] Committed by GitHub
Browse files
Options

Merge pull request #4919 from ethereum-optimism/clabby/ctb/xdm-per-message-reentrancy 

fix(ctb): Per-message reentrancy guard for `relayMessage`
parents 04cd7b2c 6b9373f3
Expand all Show whitespace changes
Inline Side-by-side
Showing with 477 additions and 138 deletions
.changeset/silly-mangos-shop.md 0 → 100644
View file @ b3ec2877
---
'@eth-optimism/contracts-bedrock': minor
---
Change the `relayMessage` reentrancy guard in the XDMs to be per-message.
op-chain-ops/genesis/config.go
View file @ b3ec2877
......@@ -415,8 +415,6 @@ func NewL2StorageConfig(config *DeployConfig, block *types.Block) (state.Storage
storage["L2CrossDomainMessenger"] = state.StorageValues{
"_initialized": 1,
"_owner": config.ProxyAdminOwner,
// re-entrency lock
"_status": 1,
"_initializing": false,
"_paused": false,
"xDomainMsgSender": "0x000000000000000000000000000000000000dEaD",
......
packages/contracts-bedrock/.gas-snapshot
View file @ b3ec2877
......@@ -21,21 +21,21 @@ 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: 63695)
CrossDomainOwnable2_Test:test_onlyOwner_notOwner_reverts() (gas: 16588)
CrossDomainOwnable2_Test:test_onlyOwner_succeeds() (gas: 77766)
CrossDomainOwnable3_Test:test_constructor_succeeds() (gas: 10510)
CrossDomainOwnable3_Test:test_crossDomainOnlyOwner_notMessenger_reverts() (gas: 28311)
CrossDomainOwnable3_Test:test_crossDomainOnlyOwner_notOwner2_reverts() (gas: 78170)
CrossDomainOwnable3_Test:test_crossDomainOnlyOwner_notOwner_reverts() (gas: 32000)
CrossDomainOwnable3_Test:test_crossDomainTransferOwnership_succeeds() (gas: 95748)
CrossDomainOwnable3_Test:test_localOnlyOwner_notOwner_reverts() (gas: 13237)
CrossDomainOwnable3_Test:test_localOnlyOwner_succeeds() (gas: 35197)
CrossDomainOwnable3_Test:test_localTransferOwnership_succeeds() (gas: 52084)
CrossDomainOwnable3_Test:test_transferOwnershipNoLocal_succeeds() (gas: 48565)
CrossDomainOwnable3_Test:test_transferOwnership_noLocal_zeroAddress_reverts() (gas: 12039)
CrossDomainOwnable2_Test:test_onlyOwner_succeeds() (gas: 78212)
CrossDomainOwnable3_Test:test_constructor_succeeds() (gas: 10576)
CrossDomainOwnable3_Test:test_crossDomainOnlyOwner_notMessenger_reverts() (gas: 28289)
CrossDomainOwnable3_Test:test_crossDomainOnlyOwner_notOwner2_reverts() (gas: 78594)
CrossDomainOwnable3_Test:test_crossDomainOnlyOwner_notOwner_reverts() (gas: 31978)
CrossDomainOwnable3_Test:test_crossDomainTransferOwnership_succeeds() (gas: 96172)
CrossDomainOwnable3_Test:test_localOnlyOwner_notOwner_reverts() (gas: 13215)
CrossDomainOwnable3_Test:test_localOnlyOwner_succeeds() (gas: 35220)
CrossDomainOwnable3_Test:test_localTransferOwnership_succeeds() (gas: 52128)
CrossDomainOwnable3_Test:test_transferOwnershipNoLocal_succeeds() (gas: 48632)
CrossDomainOwnable3_Test:test_transferOwnership_noLocalZeroAddress_reverts() (gas: 12037)
CrossDomainOwnable3_Test:test_transferOwnership_notOwner_reverts() (gas: 13414)
CrossDomainOwnable3_Test:test_transferOwnership_zeroAddress_reverts() (gas: 12058)
CrossDomainOwnable3_Test:test_transferOwnership_zeroAddress_reverts() (gas: 12036)
DeployerWhitelist_Test:test_owner_succeeds() (gas: 7538)
DeployerWhitelist_Test:test_storageSlots_succeeds() (gas: 33395)
FeeVault_Test:test_constructor_succeeds() (gas: 10647)
......@@ -70,24 +70,25 @@ L1BlockNumberTest:test_getL1BlockNumber_succeeds() (gas: 10603)
L1BlockNumberTest:test_receive_succeeds() (gas: 25340)
L1CrossDomainMessenger_Test:test_messageVersion_succeeds() (gas: 24781)
L1CrossDomainMessenger_Test:test_pause_callerIsNotOwner_reverts() (gas: 24517)
L1CrossDomainMessenger_Test:test_pause_succeeds() (gas: 52942)
L1CrossDomainMessenger_Test:test_relayMessage_legacyOldReplay_reverts() (gas: 56574)
L1CrossDomainMessenger_Test:test_relayMessage_legacyRetryAfterFailureThenSuccess_reverts() (gas: 212474)
L1CrossDomainMessenger_Test:test_relayMessage_legacyRetryAfterFailure_succeeds() (gas: 203204)
L1CrossDomainMessenger_Test:test_relayMessage_legacyRetryAfterSuccess_reverts() (gas: 126420)
L1CrossDomainMessenger_Test:test_relayMessage_legacy_succeeds() (gas: 76600)
L1CrossDomainMessenger_Test:test_relayMessage_paused_reverts() (gas: 60476)
L1CrossDomainMessenger_Test:test_relayMessage_reentrancy_reverts() (gas: 190799)
L1CrossDomainMessenger_Test:test_relayMessage_retryAfterFailure_succeeds() (gas: 197144)
L1CrossDomainMessenger_Test:test_relayMessage_succeeds() (gas: 73558)
L1CrossDomainMessenger_Test:test_relayMessage_toSystemContract_reverts() (gas: 65827)
L1CrossDomainMessenger_Test:test_relayMessage_v2_reverts() (gas: 19500)
L1CrossDomainMessenger_Test:test_replayMessage_withValue_reverts() (gas: 38220)
L1CrossDomainMessenger_Test:test_pause_succeeds() (gas: 52964)
L1CrossDomainMessenger_Test:test_relayMessage_legacyOldReplay_reverts() (gas: 51545)
L1CrossDomainMessenger_Test:test_relayMessage_legacyRetryAfterFailureThenSuccess_reverts() (gas: 230701)
L1CrossDomainMessenger_Test:test_relayMessage_legacyRetryAfterFailure_succeeds() (gas: 204067)
L1CrossDomainMessenger_Test:test_relayMessage_legacyRetryAfterSuccess_reverts() (gas: 144199)
L1CrossDomainMessenger_Test:test_relayMessage_legacy_succeeds() (gas: 77026)
L1CrossDomainMessenger_Test:test_relayMessage_paused_reverts() (gas: 55447)
L1CrossDomainMessenger_Test:test_relayMessage_reentrancyDiffMessage_succeeds() (gas: 723099)
L1CrossDomainMessenger_Test:test_relayMessage_reentrancySameMessage_reverts() (gas: 660069)
L1CrossDomainMessenger_Test:test_relayMessage_retryAfterFailure_succeeds() (gas: 197996)
L1CrossDomainMessenger_Test:test_relayMessage_succeeds() (gas: 73984)
L1CrossDomainMessenger_Test:test_relayMessage_toSystemContract_reverts() (gas: 100511)
L1CrossDomainMessenger_Test:test_relayMessage_v2_reverts() (gas: 14471)
L1CrossDomainMessenger_Test:test_replayMessage_withValue_reverts() (gas: 55573)
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_callerIsNotOwner_reverts() (gas: 24538)
L1CrossDomainMessenger_Test:test_unpause_succeeds() (gas: 45185)
L1CrossDomainMessenger_Test:test_xDomainMessageSender_reset_succeeds() (gas: 84020)
L1CrossDomainMessenger_Test:test_xDomainMessageSender_reset_succeeds() (gas: 84446)
L1CrossDomainMessenger_Test:test_xDomainSender_notSet_reverts() (gas: 24274)
L1ERC721Bridge_Test:test_bridgeERC721To_localTokenZeroAddress_reverts() (gas: 52730)
L1ERC721Bridge_Test:test_bridgeERC721To_remoteTokenZeroAddress_reverts() (gas: 27332)
......@@ -123,18 +124,19 @@ L1StandardBridge_FinalizeETHWithdrawal_Test:test_finalizeETHWithdrawal_succeeds(
L1StandardBridge_Getter_Test:test_getters_succeeds() (gas: 32151)
L1StandardBridge_Initialize_Test:test_initialize_succeeds() (gas: 22005)
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_messageVersion_succeeds() (gas: 8412)
L2CrossDomainMessenger_Test:test_pause_notOwner_reverts() (gas: 10860)
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: 36500)
L2CrossDomainMessenger_Test:test_relayMessage_reentrancyDiffMessage_succeeds() (gas: 683845)
L2CrossDomainMessenger_Test:test_relayMessage_reentrancySameMessage_reverts() (gas: 629342)
L2CrossDomainMessenger_Test:test_relayMessage_retry_succeeds() (gas: 169219)
L2CrossDomainMessenger_Test:test_relayMessage_succeeds() (gas: 56856)
L2CrossDomainMessenger_Test:test_relayMessage_toSystemContract_reverts() (gas: 53532)
L2CrossDomainMessenger_Test:test_relayMessage_v2_reverts() (gas: 13839)
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: 56367)
L2CrossDomainMessenger_Test:test_xDomainSender_senderNotSet_reverts() (gas: 10524)
L2ERC721Bridge_Test:test_bridgeERC721To_localTokenZeroAddress_reverts() (gas: 26454)
L2ERC721Bridge_Test:test_bridgeERC721To_remoteTokenZeroAddress_reverts() (gas: 21770)
......
packages/contracts-bedrock/.storage-layout
View file @ b3ec2877
......@@ -7,7 +7,7 @@
=======================
| Name | Type | Slot | Offset | Bytes | Contract |
|--------------------|--------------------------|------|--------|-------|----------------------------------------------------------------|
|------------------------|--------------------------|------|--------|-------|----------------------------------------------------------------|
| spacer_0_0_20 | address | 0 | 0 | 20 | contracts/L1/L1CrossDomainMessenger.sol:L1CrossDomainMessenger |
| _initialized | uint8 | 0 | 20 | 1 | contracts/L1/L1CrossDomainMessenger.sol:L1CrossDomainMessenger |
| _initializing | bool | 0 | 21 | 1 | contracts/L1/L1CrossDomainMessenger.sol:L1CrossDomainMessenger |
......@@ -16,15 +16,16 @@
| __gap | uint256[49] | 52 | 0 | 1568 | contracts/L1/L1CrossDomainMessenger.sol:L1CrossDomainMessenger |
| _paused | bool | 101 | 0 | 1 | contracts/L1/L1CrossDomainMessenger.sol:L1CrossDomainMessenger |
| __gap | uint256[49] | 102 | 0 | 1568 | contracts/L1/L1CrossDomainMessenger.sol:L1CrossDomainMessenger |
| _status | uint256 | 151 | 0 | 32 | contracts/L1/L1CrossDomainMessenger.sol:L1CrossDomainMessenger |
| __gap | uint256[49] | 152 | 0 | 1568 | contracts/L1/L1CrossDomainMessenger.sol:L1CrossDomainMessenger |
| spacer_151_0_32 | uint256 | 151 | 0 | 32 | contracts/L1/L1CrossDomainMessenger.sol:L1CrossDomainMessenger |
| __gap_reentrancy_guard | uint256[49] | 152 | 0 | 1568 | contracts/L1/L1CrossDomainMessenger.sol:L1CrossDomainMessenger |
| spacer_201_0_32 | mapping(bytes32 => bool) | 201 | 0 | 32 | contracts/L1/L1CrossDomainMessenger.sol:L1CrossDomainMessenger |
| spacer_202_0_32 | mapping(bytes32 => bool) | 202 | 0 | 32 | contracts/L1/L1CrossDomainMessenger.sol:L1CrossDomainMessenger |
| successfulMessages | mapping(bytes32 => bool) | 203 | 0 | 32 | contracts/L1/L1CrossDomainMessenger.sol:L1CrossDomainMessenger |
| 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 |
| __gap | uint256[42] | 207 | 0 | 1344 | 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 |
=======================
➡ contracts/L1/L1StandardBridge.sol:L1StandardBridge
......@@ -115,7 +116,7 @@
=======================
| Name | Type | Slot | Offset | Bytes | Contract |
|--------------------|--------------------------|------|--------|-------|----------------------------------------------------------------|
|------------------------|--------------------------|------|--------|-------|----------------------------------------------------------------|
| spacer_0_0_20 | address | 0 | 0 | 20 | contracts/L2/L2CrossDomainMessenger.sol:L2CrossDomainMessenger |
| _initialized | uint8 | 0 | 20 | 1 | contracts/L2/L2CrossDomainMessenger.sol:L2CrossDomainMessenger |
| _initializing | bool | 0 | 21 | 1 | contracts/L2/L2CrossDomainMessenger.sol:L2CrossDomainMessenger |
......@@ -124,15 +125,16 @@
| __gap | uint256[49] | 52 | 0 | 1568 | contracts/L2/L2CrossDomainMessenger.sol:L2CrossDomainMessenger |
| _paused | bool | 101 | 0 | 1 | contracts/L2/L2CrossDomainMessenger.sol:L2CrossDomainMessenger |
| __gap | uint256[49] | 102 | 0 | 1568 | contracts/L2/L2CrossDomainMessenger.sol:L2CrossDomainMessenger |
| _status | uint256 | 151 | 0 | 32 | contracts/L2/L2CrossDomainMessenger.sol:L2CrossDomainMessenger |
| __gap | uint256[49] | 152 | 0 | 1568 | contracts/L2/L2CrossDomainMessenger.sol:L2CrossDomainMessenger |
| spacer_151_0_32 | uint256 | 151 | 0 | 32 | contracts/L2/L2CrossDomainMessenger.sol:L2CrossDomainMessenger |
| __gap_reentrancy_guard | uint256[49] | 152 | 0 | 1568 | contracts/L2/L2CrossDomainMessenger.sol:L2CrossDomainMessenger |
| spacer_201_0_32 | mapping(bytes32 => bool) | 201 | 0 | 32 | contracts/L2/L2CrossDomainMessenger.sol:L2CrossDomainMessenger |
| spacer_202_0_32 | mapping(bytes32 => bool) | 202 | 0 | 32 | contracts/L2/L2CrossDomainMessenger.sol:L2CrossDomainMessenger |
| successfulMessages | mapping(bytes32 => bool) | 203 | 0 | 32 | contracts/L2/L2CrossDomainMessenger.sol:L2CrossDomainMessenger |
| 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 |
| __gap | uint256[42] | 207 | 0 | 1344 | 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 |
=======================
➡ contracts/L2/L2StandardBridge.sol:L2StandardBridge
......
packages/contracts-bedrock/contracts/L1/L1CrossDomainMessenger.sol
View file @ b3ec2877
......@@ -20,12 +20,12 @@ contract L1CrossDomainMessenger is CrossDomainMessenger, Semver {
OptimismPortal public immutable PORTAL;
/**
* @custom:semver 1.0.0
* @custom:semver 1.1.0
*
* @param _portal Address of the OptimismPortal contract on this network.
*/
constructor(OptimismPortal _portal)
Semver(1, 0, 0)
Semver(1, 1, 0)
CrossDomainMessenger(Predeploys.L2_CROSS_DOMAIN_MESSENGER)
{
PORTAL = _portal;
......
packages/contracts-bedrock/contracts/L2/L2CrossDomainMessenger.sol
View file @ b3ec2877
......@@ -17,12 +17,12 @@ import { L2ToL1MessagePasser } from "./L2ToL1MessagePasser.sol";
*/
contract L2CrossDomainMessenger is CrossDomainMessenger, Semver {
/**
* @custom:semver 1.0.0
* @custom:semver 1.1.0
*
* @param _l1CrossDomainMessenger Address of the L1CrossDomainMessenger contract.
*/
constructor(address _l1CrossDomainMessenger)
Semver(1, 0, 0)
Semver(1, 1, 0)
CrossDomainMessenger(_l1CrossDomainMessenger)
{
initialize();
......
packages/contracts-bedrock/contracts/test/CommonTest.t.sol
View file @ b3ec2877
......@@ -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/CrossDomainOwnable3.t.sol
View file @ b3ec2877
......@@ -149,7 +149,7 @@ contract CrossDomainOwnable3_Test is Messenger_Initializer {
setter.transferOwnership({ _owner: address(0), _isLocal: true });
}
function test_transferOwnership_noLocal_zeroAddress_reverts() public {
function test_transferOwnership_noLocalZeroAddress_reverts() public {
vm.prank(setter.owner());
vm.expectRevert("Ownable: new owner is the zero address");
setter.transferOwnership(address(0));
......
packages/contracts-bedrock/contracts/test/L1CrossDomainMessenger.t.sol
View file @ b3ec2877
......@@ -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 */
......@@ -340,52 +340,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(
// 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({ _nonce: 0, _version: 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({ _nonce: 0, _version: 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({ _nonce: 0, _version: 1 }),
sender,
target,
0,
0,
messageA
);
bytes32 hashB = Hashing.hashCrossDomainMessage(
Encoding.encodeVersionedNonce({ _nonce: 0, _version: 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({ _nonce: 0, _version: 1 }),
sender,
target,
0,
0,
messageA
);
vm.expectCall(target, messageB);
L1Messenger.relayMessage(
Encoding.encodeVersionedNonce({ _nonce: 0, _version: 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({ _nonce: 0, _version: 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({ _nonce: 0, _version: 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 @ b3ec2877
// 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";
......@@ -255,51 +255,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 @ b3ec2877
......@@ -7,9 +7,6 @@ import {
import {
PausableUpgradeable
} from "@openzeppelin/contracts-upgradeable/security/PausableUpgradeable.sol";
import {
ReentrancyGuardUpgradeable
} from "@openzeppelin/contracts-upgradeable/security/ReentrancyGuardUpgradeable.sol";
import { SafeCall } from "../libraries/SafeCall.sol";
import { Hashing } from "../libraries/Hashing.sol";
import { Encoding } from "../libraries/Encoding.sol";
......@@ -43,8 +40,7 @@ contract CrossDomainMessengerLegacySpacer {
abstract contract CrossDomainMessenger is
CrossDomainMessengerLegacySpacer,
OwnableUpgradeable,
PausableUpgradeable,
ReentrancyGuardUpgradeable
PausableUpgradeable
{
/**
* @notice Current message version identifier.
......@@ -86,6 +82,18 @@ abstract contract CrossDomainMessenger is
*/
address public immutable OTHER_MESSENGER;
/**
* @custom:spacer ReentrancyGuardUpgradeable's `_status` field.
* @notice Spacer for backwards compatibility
*/
uint256 private spacer_151_0_32;
/**
* @custom:spacer ReentrancyGuardUpgradeable
* @notice Spacer for backwards compatibility
*/
uint256[49] private __gap_reentrancy_guard;
/**
* @custom:legacy
* @custom:spacer blockedMessages
......@@ -129,12 +137,17 @@ abstract contract CrossDomainMessenger is
*/
mapping(bytes32 => bool) public failedMessages;
/**
* @notice A mapping of hashes to reentrancy locks.
*/
mapping(bytes32 => bool) internal reentrancyLocks;
/**
* @notice Reserve extra slots in the storage layout for future upgrades.
* A gap size of 41 was chosen here, so that the first slot used in a child contract
* would be a multiple of 50.
*/
uint256[42] private __gap;
uint256[41] private __gap;
/**
* @notice Emitted whenever a message is sent to the other chain.
......@@ -260,7 +273,7 @@ abstract contract CrossDomainMessenger is
uint256 _value,
uint256 _minGasLimit,
bytes calldata _message
) external payable nonReentrant whenNotPaused {
) external payable whenNotPaused {
(, uint16 version) = Encoding.decodeVersionedNonce(_nonce);
require(
version < 2,
......@@ -288,6 +301,13 @@ abstract contract CrossDomainMessenger is
_message
);
// Check if the reentrancy lock for the `versionedHash` is already set.
if (reentrancyLocks[versionedHash]) {
revert("ReentrancyGuard: reentrant call");
}
// Trigger the reentrancy lock for `versionedHash`
reentrancyLocks[versionedHash] = true;
if (_isOtherMessenger()) {
// These properties should always hold when the message is first submitted (as
// opposed to being replayed).
......@@ -340,6 +360,9 @@ abstract contract CrossDomainMessenger is
revert("CrossDomainMessenger: failed to relay message");
}
}
// Clear the reentrancy lock for `versionedHash`
reentrancyLocks[versionedHash] = false;
}
/**
......@@ -403,7 +426,6 @@ abstract contract CrossDomainMessenger is
__Context_init_unchained();
__Ownable_init_unchained();
__Pausable_init_unchained();
__ReentrancyGuard_init_unchained();
}
/**
......
packages/contracts-bedrock/layout-lock.json
View file @ b3ec2877
......@@ -5,6 +5,11 @@
"offset": 0,
"length": 20
},
"spacer_151_0_32": {
"slot": 151,
"offset": 0,
"length": 32
},
"spacer_201_0_32": {
"slot": 201,
"offset": 0,
......@@ -22,6 +27,11 @@
"offset": 0,
"length": 20
},
"spacer_151_0_32": {
"slot": 151,
"offset": 0,
"length": 32
},
"spacer_201_0_32": {
"slot": 201,
"offset": 0,
......
packages/contracts-bedrock/tasks/check-l2.ts
View file @ b3ec2877
......@@ -242,7 +242,11 @@ const check = {
signer
)
await assertSemver(L2CrossDomainMessenger, 'L2CrossDomainMessenger')
await assertSemver(
L2CrossDomainMessenger,
'L2CrossDomainMessenger',
'1.1.0'
)
const xDomainMessageSenderSlot = await signer.provider.getStorageAt(
predeploys.L2CrossDomainMessenger,
......
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