L2Sender reinit fix (#8864)

* contracts-bedrock: only set xDomainMsgSender on a fresh deployment

* contracts-bedrock: Prevent double relaying a message with reentrancy

* contracts-bedrock: bump semvers

* bindings and semver-lock

* contracts-bedrock: Update slither report

* contracts-bedrock: Update test to demonstrate non-replayability during upgrades

* contracts-bedrock: Do not reinit Portals L2 Sender if it is nonzero

--wip--

* contracts-bedrock: Use assert instead of require for unreachable errors

* Update L1CrossDomainMessenger.t.sol

* contracts-bedrock: update semver lock, bindings, slither

packages/contracts-bedrock/semver-lock.json

@@ -12,8 +12,8 @@
     "sourceCodeHash": "0x0c2f8fdc6130397f84d5867000110ef5ee864946cbc159f9b78665c29333b4f7"
   },
   "src/L1/L1CrossDomainMessenger.sol": {
-    "initCodeHash": "0x2e30441e86771fb65dac0dd021fa5bab8b6b4b224386594b76cc2bfc5146832f",
-    "sourceCodeHash": "0x508ac7c5628d226f5e3ec6593f6c28aab969f70223c678eb0e870155d24b1da4"
+    "initCodeHash": "0x681673e4801391bc30cfd037d6470bdda404b9da8b0813df2cbbd657056634a4",
+    "sourceCodeHash": "0x593fd60e1a60d955a5526777bea40c3e48a5c07ac7c7da66ce46cd346a152cd6"
   },
   "src/L1/L1ERC721Bridge.sol": {
     "initCodeHash": "0xca27bfb4742b3b44e9adb6018b0917ae3392e67e5f8242ab24828dd37245af16",
@@ -28,8 +28,8 @@
     "sourceCodeHash": "0xb99ee58a672ed59f6bf529a618d4949f198bfda7c65664c9b2a2657070756c69"
   },
   "src/L1/OptimismPortal.sol": {
-    "initCodeHash": "0x627cdc470d54cd6eac977d9ec4ca463c96e8bd715582c4d1394ff2cd824c804f",
-    "sourceCodeHash": "0x2e738b965d2fab07f0af660a017071b1f23e0fe0d1ee543ed99da2687b7fd6b0"
+    "initCodeHash": "0xaa4733b87fab3a3a0700128cab3e272c02064f38fb0080251566cacefe5d7908",
+    "sourceCodeHash": "0xb4bb23bf2942de36bb001e3eb888a183840daa30024023cd0d6d0e150572f95a"
   },
   "src/L1/ProtocolVersions.sol": {
     "initCodeHash": "0x72cd467e8bcf019c02675d72ab762e088bcc9cc0f1a4e9f587fa4589f7fdd1b8",
@@ -60,8 +60,8 @@
     "sourceCodeHash": "0x3a94f273937d8908fb37dd2c495a6a0b9c3941fe68ccea51723f84eb343ba225"
   },
   "src/L2/L2CrossDomainMessenger.sol": {
-    "initCodeHash": "0x83c8e1eb0dca6061998a14f561f02952350b8bbf56542b7229e611de0338a6b1",
-    "sourceCodeHash": "0x167f90f8b75e6ae2d5326d5245384d1ec9dd28222e78388b9e9b11aea8da7348"
+    "initCodeHash": "0xf00c8dd81c959a07352313cfabc7ecbb187699373e65e9021e0b5dcf9d7a53a8",
+    "sourceCodeHash": "0x233e2358a738323b513f06e49945fc47a67b9bd4a2f01c5cf30e88611c8d1be9"
   },
   "src/L2/L2ERC721Bridge.sol": {
     "initCodeHash": "0x3b7357a0972db8cde39f6583d87291fc64ef424737498cf6927f8fcb3d7e368d",

packages/contracts-bedrock/src/L1/L1CrossDomainMessenger.sol

@@ -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.

packages/contracts-bedrock/src/L1/OptimismPortal.sol

@@ -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.
@@ -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();
     }
 

packages/contracts-bedrock/src/L2/L2CrossDomainMessenger.sol

@@ -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.

packages/contracts-bedrock/src/universal/CrossDomainMessenger.sol

@@ -288,6 +288,9 @@ 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.
+            assert(successfulMessages[versionedHash] == false);
             successfulMessages[versionedHash] = true;
             emit RelayedMessage(versionedHash);
         } else {
@@ -354,7 +357,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

packages/contracts-bedrock/test/L1/L1CrossDomainMessenger.t.sol

@@ -596,8 +596,8 @@ contract L1CrossDomainMessenger_Test is Bridge_Initializer {
     }
 }
 
-/// @dev A test demonstarting a reentrancy vulnerability in the CrossDomainMessenger contract, which
-///      is possible by intercepting and sandwhiching a signed Safe Transaction to upgrade it.
+/// @dev A regression test against a reentrancy vulnerability in the CrossDomainMessenger contract, which
+///      was possible by intercepting and sandwhiching a signed Safe Transaction to upgrade it.
 contract L1CrossDomainMessenger_ReinitReentryTest is Bridge_Initializer {
     bool attacked;
 
@@ -631,6 +631,8 @@ contract L1CrossDomainMessenger_ReinitReentryTest is Bridge_Initializer {
             l1CrossDomainMessenger.initialize(SuperchainConfig(superchainConfig));
 
             // attempt to re-replay the withdrawal
+            vm.expectEmit(address(l1CrossDomainMessenger));
+            emit FailedRelayedMessage(hash);
             l1CrossDomainMessenger.relayMessage(
                 Encoding.encodeVersionedNonce({ _nonce: 0, _version: 1 }), // nonce
                 sender,
@@ -642,23 +644,19 @@ contract L1CrossDomainMessenger_ReinitReentryTest is Bridge_Initializer {
         }
     }
 
-    /// @dev A test showing that the relayMessage function cannot be reentered by calling the `initialize()` function
-    /// within the relayed message.
-    function test_relayMessage_replayStraddlingReinit_succeeds() external {
+    /// @dev Tests that the relayMessage function cannot be reentered by calling the `initialize()` function within the
+    ///      relayed message.
+    function test_relayMessage_replayStraddlingReinit_reverts() external {
         uint256 balanceBeforeThis = address(this).balance;
         uint256 balanceBeforeMessenger = address(l1CrossDomainMessenger).balance;
 
         // A requisite for the attack is that the message has already been attempted and written to the failedMessages
         // mapping, so that it can be replayed.
         vm.store(address(l1CrossDomainMessenger), keccak256(abi.encode(hash, 206)), bytes32(uint256(1)));
-        assert(l1CrossDomainMessenger.failedMessages(hash));
+        assertTrue(l1CrossDomainMessenger.failedMessages(hash));
 
-        // Expect to see the message relayed twice.
         vm.expectEmit(address(l1CrossDomainMessenger));
-        emit RelayedMessage(hash);
-        vm.expectEmit(address(l1CrossDomainMessenger));
-        emit RelayedMessage(hash);
-
+        emit FailedRelayedMessage(hash);
         l1CrossDomainMessenger.relayMessage(
             Encoding.encodeVersionedNonce({ _nonce: 0, _version: 1 }), // nonce
             sender,
@@ -668,12 +666,11 @@ contract L1CrossDomainMessenger_ReinitReentryTest is Bridge_Initializer {
             selector
         );
 
-        // the message hash is now in the successfulMessages mapping
-        assert(l1CrossDomainMessenger.successfulMessages(hash));
-
-        // The balance of this contract was increased by twice the value of the message, and the messenger's
-        // balance was decreased by the same amount.
-        assertEq(address(this).balance, balanceBeforeThis + 2 * messageValue);
-        assertEq(address(l1CrossDomainMessenger).balance, balanceBeforeMessenger - 2 * messageValue);
+        // The message hash is not in the successfulMessages mapping.
+        assertFalse(l1CrossDomainMessenger.successfulMessages(hash));
+        // The balance of this contract is unchanged.
+        assertEq(address(this).balance, balanceBeforeThis);
+        // The balance of the messenger contract is unchanged.
+        assertEq(address(l1CrossDomainMessenger).balance, balanceBeforeMessenger);
     }
 }