Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

L2Sender reinit fix #8797

Merged
merged 7 commits into from
Jan 5, 2024
Merged

L2Sender reinit fix #8797

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
9757f8c
contracts-bedrock: only set xDomainMsgSender on a fresh deployment
maurelian Dec 22, 2023
ca483fb
contracts-bedrock: Prevent double relaying a message with reentrancy
maurelian Dec 22, 2023
1f114ed
contracts-bedrock: bump semvers
maurelian Jan 2, 2024
5217ce8
bindings and semver-lock
maurelian Jan 2, 2024
691302d
contracts-bedrock: Update slither report
maurelian Jan 2, 2024
7969713
contracts-bedrock: Update test to demonstrate non-replayability durin…
maurelian Jan 4, 2024
f80e4b9
contracts-bedrock: Do not reinit Portals L2 Sender if it is nonzero
maurelian Jan 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion op-bindings/bindings/l1crossdomainmessenger.go
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion op-bindings/bindings/l1crossdomainmessenger_more.go
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion op-bindings/bindings/l2crossdomainmessenger.go
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion op-bindings/bindings/l2crossdomainmessenger_more.go
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion op-bindings/bindings/optimismportal.go
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion op-bindings/bindings/optimismportal_more.go
View file
Open in desktop

Large diffs are not rendered by default.

12 changes: 6 additions & 6 deletions packages/contracts-bedrock/semver-lock.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,8 @@
"sourceCodeHash": "0x0c2f8fdc6130397f84d5867000110ef5ee864946cbc159f9b78665c29333b4f7"
},
"src/L1/L1CrossDomainMessenger.sol": {
"initCodeHash": "0x2e30441e86771fb65dac0dd021fa5bab8b6b4b224386594b76cc2bfc5146832f",
"sourceCodeHash": "0x508ac7c5628d226f5e3ec6593f6c28aab969f70223c678eb0e870155d24b1da4"
"initCodeHash": "0x3c6e220b920562afbd3d57948d062b7acc16c74463755095c634060e243e29b6",
"sourceCodeHash": "0x593fd60e1a60d955a5526777bea40c3e48a5c07ac7c7da66ce46cd346a152cd6"
},
"src/L1/L1ERC721Bridge.sol": {
"initCodeHash": "0xca27bfb4742b3b44e9adb6018b0917ae3392e67e5f8242ab24828dd37245af16",
Expand All @@ -28,8 +28,8 @@
"sourceCodeHash": "0xb99ee58a672ed59f6bf529a618d4949f198bfda7c65664c9b2a2657070756c69"
},
"src/L1/OptimismPortal.sol": {
"initCodeHash": "0x627cdc470d54cd6eac977d9ec4ca463c96e8bd715582c4d1394ff2cd824c804f",
"sourceCodeHash": "0x2e738b965d2fab07f0af660a017071b1f23e0fe0d1ee543ed99da2687b7fd6b0"
"initCodeHash": "0xaa4733b87fab3a3a0700128cab3e272c02064f38fb0080251566cacefe5d7908",
"sourceCodeHash": "0xb4bb23bf2942de36bb001e3eb888a183840daa30024023cd0d6d0e150572f95a"
},
"src/L1/ProtocolVersions.sol": {
"initCodeHash": "0x72cd467e8bcf019c02675d72ab762e088bcc9cc0f1a4e9f587fa4589f7fdd1b8",
Expand Down Expand Up @@ -60,8 +60,8 @@
"sourceCodeHash": "0x3a94f273937d8908fb37dd2c495a6a0b9c3941fe68ccea51723f84eb343ba225"
},
"src/L2/L2CrossDomainMessenger.sol": {
"initCodeHash": "0x83c8e1eb0dca6061998a14f561f02952350b8bbf56542b7229e611de0338a6b1",
"sourceCodeHash": "0x167f90f8b75e6ae2d5326d5245384d1ec9dd28222e78388b9e9b11aea8da7348"
"initCodeHash": "0x77d6705a81f41eaea30b165aa5f20948000c2b013c5fcc45ba7985d9c947b308",
"sourceCodeHash": "0x233e2358a738323b513f06e49945fc47a67b9bd4a2f01c5cf30e88611c8d1be9"
},
"src/L2/L2ERC721Bridge.sol": {
"initCodeHash": "0x3b7357a0972db8cde39f6583d87291fc64ef424737498cf6927f8fcb3d7e368d",
Expand Down
102 changes: 51 additions & 51 deletions packages/contracts-bedrock/slither-report.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1216,70 +1216,70 @@
"impact": "Medium",
"confidence": "Medium",
"check": "tx-origin",
"description": "CrossDomainMessenger.relayMessage(uint256,address,address,uint256,uint256,bytes) (src/universal/CrossDomainMessenger.sol#211-306) uses tx.origin for authorization: tx.origin == Constants.ESTIMATION_ADDRESS (src/universal/CrossDomainMessenger.sol#279)\n",
"description": "CrossDomainMessenger.relayMessage(uint256,address,address,uint256,uint256,bytes) (src/universal/CrossDomainMessenger.sol#211-312) uses tx.origin for authorization: tx.origin == Constants.ESTIMATION_ADDRESS (src/universal/CrossDomainMessenger.sol#279)\n",
"type": "function",
"name": "relayMessage",
"start": 10427,
"length": 4822,
"length": 5186,
"filename_relative": "src/universal/CrossDomainMessenger.sol"
},
{
"id": "138bfe0b87067edda58ea1bb24d88b46c26dd0da82a6818486cf28b4cb185d01",
"impact": "Medium",
"confidence": "Medium",
"check": "tx-origin",
"description": "CrossDomainMessenger.relayMessage(uint256,address,address,uint256,uint256,bytes) (src/universal/CrossDomainMessenger.sol#211-306) uses tx.origin for authorization: tx.origin == Constants.ESTIMATION_ADDRESS (src/universal/CrossDomainMessenger.sol#279)\n",
"description": "CrossDomainMessenger.relayMessage(uint256,address,address,uint256,uint256,bytes) (src/universal/CrossDomainMessenger.sol#211-312) uses tx.origin for authorization: tx.origin == Constants.ESTIMATION_ADDRESS (src/universal/CrossDomainMessenger.sol#279)\n",
"type": "node",
"name": "tx.origin == Constants.ESTIMATION_ADDRESS",
"start": 14024,
"length": 41,
"filename_relative": "src/universal/CrossDomainMessenger.sol"
},
{
"id": "37ae06d1257a210b94d7ebd3a411a5e657e4fe6e09bdcb5405f76bcf5b2e6496",
"id": "297b0097a00d63b07fb57d6fc3cdb2b458872995f0927a8c7622feaf21ef9d42",
"impact": "Medium",
"confidence": "Medium",
"check": "tx-origin",
"description": "CrossDomainMessenger.relayMessage(uint256,address,address,uint256,uint256,bytes) (src/universal/CrossDomainMessenger.sol#211-306) uses tx.origin for authorization: tx.origin == Constants.ESTIMATION_ADDRESS (src/universal/CrossDomainMessenger.sol#302)\n",
"description": "CrossDomainMessenger.relayMessage(uint256,address,address,uint256,uint256,bytes) (src/universal/CrossDomainMessenger.sol#211-312) uses tx.origin for authorization: tx.origin == Constants.ESTIMATION_ADDRESS (src/universal/CrossDomainMessenger.sol#308)\n",
"type": "function",
"name": "relayMessage",
"start": 10427,
"length": 4822,
"length": 5186,
"filename_relative": "src/universal/CrossDomainMessenger.sol"
},
{
"id": "37ae06d1257a210b94d7ebd3a411a5e657e4fe6e09bdcb5405f76bcf5b2e6496",
"id": "297b0097a00d63b07fb57d6fc3cdb2b458872995f0927a8c7622feaf21ef9d42",
"impact": "Medium",
"confidence": "Medium",
"check": "tx-origin",
"description": "CrossDomainMessenger.relayMessage(uint256,address,address,uint256,uint256,bytes) (src/universal/CrossDomainMessenger.sol#211-306) uses tx.origin for authorization: tx.origin == Constants.ESTIMATION_ADDRESS (src/universal/CrossDomainMessenger.sol#302)\n",
"description": "CrossDomainMessenger.relayMessage(uint256,address,address,uint256,uint256,bytes) (src/universal/CrossDomainMessenger.sol#211-312) uses tx.origin for authorization: tx.origin == Constants.ESTIMATION_ADDRESS (src/universal/CrossDomainMessenger.sol#308)\n",
"type": "node",
"name": "tx.origin == Constants.ESTIMATION_ADDRESS",
"start": 15102,
"start": 15466,
"length": 41,
"filename_relative": "src/universal/CrossDomainMessenger.sol"
},
{
"id": "6b52fad833e1120044d0d5364441efdf7e7d9f3988885eaeb83816ee433b43ea",
"id": "9499bea1e26f68282279d9bf87f2ba9c95d500ba55be2f97dd49060d65abc6e7",
"impact": "Medium",
"confidence": "Medium",
"check": "tx-origin",
"description": "OptimismPortal.finalizeWithdrawalTransaction(Types.WithdrawalTransaction) (src/L1/OptimismPortal.sol#264-347) uses tx.origin for authorization: success == false && tx.origin == Constants.ESTIMATION_ADDRESS (src/L1/OptimismPortal.sol#344)\n",
"description": "OptimismPortal.finalizeWithdrawalTransaction(Types.WithdrawalTransaction) (src/L1/OptimismPortal.sol#266-349) uses tx.origin for authorization: success == false && tx.origin == Constants.ESTIMATION_ADDRESS (src/L1/OptimismPortal.sol#346)\n",
"type": "function",
"name": "finalizeWithdrawalTransaction",
"start": 12831,
"start": 12883,
"length": 4841,
"filename_relative": "src/L1/OptimismPortal.sol"
},
{
"id": "6b52fad833e1120044d0d5364441efdf7e7d9f3988885eaeb83816ee433b43ea",
"id": "9499bea1e26f68282279d9bf87f2ba9c95d500ba55be2f97dd49060d65abc6e7",
"impact": "Medium",
"confidence": "Medium",
"check": "tx-origin",
"description": "OptimismPortal.finalizeWithdrawalTransaction(Types.WithdrawalTransaction) (src/L1/OptimismPortal.sol#264-347) uses tx.origin for authorization: success == false && tx.origin == Constants.ESTIMATION_ADDRESS (src/L1/OptimismPortal.sol#344)\n",
"description": "OptimismPortal.finalizeWithdrawalTransaction(Types.WithdrawalTransaction) (src/L1/OptimismPortal.sol#266-349) uses tx.origin for authorization: success == false && tx.origin == Constants.ESTIMATION_ADDRESS (src/L1/OptimismPortal.sol#346)\n",
"type": "node",
"name": "success == false && tx.origin == Constants.ESTIMATION_ADDRESS",
"start": 17535,
"start": 17587,
"length": 61,
"filename_relative": "src/L1/OptimismPortal.sol"
},
Expand Down Expand Up @@ -1320,75 +1320,75 @@
"filename_relative": "src/dispute/OutputBisectionGame.sol"
},
{
"id": "9ad67b006fc175893dd26d35a020a52dd8524709d87cca61212ee0e147eb992b",
"id": "251668d2d92d4034a249cc32c68668cf824fe41d8b21c9a3ead976e6c03bdaf4",
"impact": "Medium",
"confidence": "High",
"check": "write-after-write",
"description": "CrossDomainMessenger.xDomainMsgSender (src/universal/CrossDomainMessenger.sol#129) is written in both\n\txDomainMsgSender = _sender (src/universal/CrossDomainMessenger.sol#286)\n\txDomainMsgSender = Constants.DEFAULT_L2_SENDER (src/universal/CrossDomainMessenger.sol#288)\n",
"description": "OptimismPortal.l2Sender (src/L1/OptimismPortal.sol#53) is written in both\n\tl2Sender = _tx.sender (src/L1/OptimismPortal.sol#325)\n\tl2Sender = Constants.DEFAULT_L2_SENDER (src/L1/OptimismPortal.sol#337)\n",
"type": "variable",
"name": "xDomainMsgSender",
"start": 5784,
"length": 33,
"filename_relative": "src/universal/CrossDomainMessenger.sol"
"name": "l2Sender",
"start": 2603,
"length": 23,
"filename_relative": "src/L1/OptimismPortal.sol"
},
{
"id": "9ad67b006fc175893dd26d35a020a52dd8524709d87cca61212ee0e147eb992b",
"id": "251668d2d92d4034a249cc32c68668cf824fe41d8b21c9a3ead976e6c03bdaf4",
"impact": "Medium",
"confidence": "High",
"check": "write-after-write",
"description": "CrossDomainMessenger.xDomainMsgSender (src/universal/CrossDomainMessenger.sol#129) is written in both\n\txDomainMsgSender = _sender (src/universal/CrossDomainMessenger.sol#286)\n\txDomainMsgSender = Constants.DEFAULT_L2_SENDER (src/universal/CrossDomainMessenger.sol#288)\n",
"description": "OptimismPortal.l2Sender (src/L1/OptimismPortal.sol#53) is written in both\n\tl2Sender = _tx.sender (src/L1/OptimismPortal.sol#325)\n\tl2Sender = Constants.DEFAULT_L2_SENDER (src/L1/OptimismPortal.sol#337)\n",
"type": "node",
"name": "xDomainMsgSender = _sender",
"start": 14196,
"length": 26,
"filename_relative": "src/universal/CrossDomainMessenger.sol"
"name": "l2Sender = _tx.sender",
"start": 16288,
"length": 21,
"filename_relative": "src/L1/OptimismPortal.sol"
},
{
"id": "9ad67b006fc175893dd26d35a020a52dd8524709d87cca61212ee0e147eb992b",
"id": "251668d2d92d4034a249cc32c68668cf824fe41d8b21c9a3ead976e6c03bdaf4",
"impact": "Medium",
"confidence": "High",
"check": "write-after-write",
"description": "CrossDomainMessenger.xDomainMsgSender (src/universal/CrossDomainMessenger.sol#129) is written in both\n\txDomainMsgSender = _sender (src/universal/CrossDomainMessenger.sol#286)\n\txDomainMsgSender = Constants.DEFAULT_L2_SENDER (src/universal/CrossDomainMessenger.sol#288)\n",
"description": "OptimismPortal.l2Sender (src/L1/OptimismPortal.sol#53) is written in both\n\tl2Sender = _tx.sender (src/L1/OptimismPortal.sol#325)\n\tl2Sender = Constants.DEFAULT_L2_SENDER (src/L1/OptimismPortal.sol#337)\n",
"type": "node",
"name": "xDomainMsgSender = Constants.DEFAULT_L2_SENDER",
"start": 14329,
"length": 46,
"filename_relative": "src/universal/CrossDomainMessenger.sol"
"name": "l2Sender = Constants.DEFAULT_L2_SENDER",
"start": 17082,
"length": 38,
"filename_relative": "src/L1/OptimismPortal.sol"
},
{
"id": "aba76c5478a36b00ed8b66577c0cd1d3186109cf82c1767ce230c6fe72315c04",
"id": "9ad67b006fc175893dd26d35a020a52dd8524709d87cca61212ee0e147eb992b",
"impact": "Medium",
"confidence": "High",
"check": "write-after-write",
"description": "OptimismPortal.l2Sender (src/L1/OptimismPortal.sol#53) is written in both\n\tl2Sender = _tx.sender (src/L1/OptimismPortal.sol#323)\n\tl2Sender = Constants.DEFAULT_L2_SENDER (src/L1/OptimismPortal.sol#335)\n",
"description": "CrossDomainMessenger.xDomainMsgSender (src/universal/CrossDomainMessenger.sol#129) is written in both\n\txDomainMsgSender = _sender (src/universal/CrossDomainMessenger.sol#286)\n\txDomainMsgSender = Constants.DEFAULT_L2_SENDER (src/universal/CrossDomainMessenger.sol#288)\n",
"type": "variable",
"name": "l2Sender",
"start": 2603,
"length": 23,
"filename_relative": "src/L1/OptimismPortal.sol"
"name": "xDomainMsgSender",
"start": 5784,
"length": 33,
"filename_relative": "src/universal/CrossDomainMessenger.sol"
},
{
"id": "aba76c5478a36b00ed8b66577c0cd1d3186109cf82c1767ce230c6fe72315c04",
"id": "9ad67b006fc175893dd26d35a020a52dd8524709d87cca61212ee0e147eb992b",
"impact": "Medium",
"confidence": "High",
"check": "write-after-write",
"description": "OptimismPortal.l2Sender (src/L1/OptimismPortal.sol#53) is written in both\n\tl2Sender = _tx.sender (src/L1/OptimismPortal.sol#323)\n\tl2Sender = Constants.DEFAULT_L2_SENDER (src/L1/OptimismPortal.sol#335)\n",
"description": "CrossDomainMessenger.xDomainMsgSender (src/universal/CrossDomainMessenger.sol#129) is written in both\n\txDomainMsgSender = _sender (src/universal/CrossDomainMessenger.sol#286)\n\txDomainMsgSender = Constants.DEFAULT_L2_SENDER (src/universal/CrossDomainMessenger.sol#288)\n",
"type": "node",
"name": "l2Sender = _tx.sender",
"start": 16236,
"length": 21,
"filename_relative": "src/L1/OptimismPortal.sol"
"name": "xDomainMsgSender = _sender",
"start": 14196,
"length": 26,
"filename_relative": "src/universal/CrossDomainMessenger.sol"
},
{
"id": "aba76c5478a36b00ed8b66577c0cd1d3186109cf82c1767ce230c6fe72315c04",
"id": "9ad67b006fc175893dd26d35a020a52dd8524709d87cca61212ee0e147eb992b",
"impact": "Medium",
"confidence": "High",
"check": "write-after-write",
"description": "OptimismPortal.l2Sender (src/L1/OptimismPortal.sol#53) is written in both\n\tl2Sender = _tx.sender (src/L1/OptimismPortal.sol#323)\n\tl2Sender = Constants.DEFAULT_L2_SENDER (src/L1/OptimismPortal.sol#335)\n",
"description": "CrossDomainMessenger.xDomainMsgSender (src/universal/CrossDomainMessenger.sol#129) is written in both\n\txDomainMsgSender = _sender (src/universal/CrossDomainMessenger.sol#286)\n\txDomainMsgSender = Constants.DEFAULT_L2_SENDER (src/universal/CrossDomainMessenger.sol#288)\n",
"type": "node",
"name": "l2Sender = Constants.DEFAULT_L2_SENDER",
"start": 17030,
"length": 38,
"filename_relative": "src/L1/OptimismPortal.sol"
"name": "xDomainMsgSender = Constants.DEFAULT_L2_SENDER",
"start": 14329,
"length": 46,
"filename_relative": "src/universal/CrossDomainMessenger.sol"
}
]
4 changes: 2 additions & 2 deletions packages/contracts-bedrock/src/L1/L1CrossDomainMessenger.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,8 @@ contract L1CrossDomainMessenger is CrossDomainMessenger, ISemver {
SuperchainConfig public superchainConfig;

/// @notice Semantic version.
/// @custom:semver 2.1.1
string public constant version = "2.1.1";
/// @custom:semver 2.2.0
string public constant version = "2.2.0";

/// @notice Constructs the L1CrossDomainMessenger contract.
/// @param _portal Address of the OptimismPortal contract on this network.
Expand Down
8 changes: 5 additions & 3 deletions packages/contracts-bedrock/src/L1/OptimismPortal.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,8 +93,8 @@ contract OptimismPortal is Initializable, ResourceMetering, ISemver {
}

/// @notice Semantic version.
/// @custom:semver 2.3.0
string public constant version = "2.3.0";
/// @custom:semver 2.4.0
string public constant version = "2.4.0";

/// @notice Constructs the OptimismPortal contract.
/// @param _l2Oracle Address of the L2OutputOracle contract.
Expand All @@ -108,8 +108,10 @@ contract OptimismPortal is Initializable, ResourceMetering, ISemver {
/// @notice Initializer.
/// @param _superchainConfig Address of the SuperchainConfig contract.
function initialize(SuperchainConfig _superchainConfig) public initializer {
l2Sender = Constants.DEFAULT_L2_SENDER;
superchainConfig = _superchainConfig;
if (l2Sender == address(0)) {
l2Sender = Constants.DEFAULT_L2_SENDER;
}
__ResourceMetering_init();
}

Expand Down
4 changes: 2 additions & 2 deletions packages/contracts-bedrock/src/L2/L2CrossDomainMessenger.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,8 @@ import { Constants } from "src/libraries/Constants.sol";
/// L2 on the L2 side. Users are generally encouraged to use this contract instead of lower
/// level message passing contracts.
contract L2CrossDomainMessenger is CrossDomainMessenger, ISemver {
/// @custom:semver 1.8.0
string public constant version = "1.8.0";
/// @custom:semver 1.9.0
string public constant version = "1.9.0";

/// @notice Constructs the L2CrossDomainMessenger contract.
/// @param _l1CrossDomainMessenger Address of the L1CrossDomainMessenger contract.
Expand Down
14 changes: 13 additions & 1 deletion packages/contracts-bedrock/src/universal/CrossDomainMessenger.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -288,6 +288,12 @@ abstract contract CrossDomainMessenger is
xDomainMsgSender = Constants.DEFAULT_L2_SENDER;

if (success) {
// This check is identical to one above, but it ensures that the same message cannot be relayed
// twice, and adds a layer of protection against rentrancy.
require(
maurelian marked this conversation as resolved.
Show resolved Hide resolved
successfulMessages[versionedHash] == false,
"CrossDomainMessenger: message has already been relayed via reentrancy"
);
successfulMessages[versionedHash] = true;
emit RelayedMessage(versionedHash);
} else {
Expand Down Expand Up @@ -354,7 +360,13 @@ abstract contract CrossDomainMessenger is
/// @notice Initializer.
// solhint-disable-next-line func-name-mixedcase
function __CrossDomainMessenger_init() internal onlyInitializing {
xDomainMsgSender = Constants.DEFAULT_L2_SENDER;
// We only want to set the xDomainMsgSender to the default value if it hasn't been initialized yet,
// meaning that this is a fresh contract deployment.
// This prevents resetting the xDomainMsgSender to the default value during an upgrade, which would enable
// a reentrant withdrawal to sandwhich the upgrade replay a withdrawal twice.
if (xDomainMsgSender == address(0)) {
xDomainMsgSender = Constants.DEFAULT_L2_SENDER;
}
}

/// @notice Sends a low-level message to the other messenger. Needs to be implemented by child
Expand Down
Loading
Loading