OZ Audit fix medium severity issues (on Bedrock)

contracts-bedrock/.gas-snapshot

@@ -7,39 +7,40 @@ L1BlockTest:test_timestamp() (gas: 7661)
 L1BlockNumberTest:test_fallback() (gas: 10710)
 L1BlockNumberTest:test_getL1BlockNumber() (gas: 10589)
 L1BlockNumberTest:test_receive() (gas: 17440)
-L1CrossDomainMessenger_Test:testCannot_L1MessengerPause() (gas: 10909)
-L1CrossDomainMessenger_Test:test_L1MessengerMessageVersion() (gas: 8366)
-L1CrossDomainMessenger_Test:test_L1MessengerPause() (gas: 31882)
-L1CrossDomainMessenger_Test:test_L1MessengerRelayMessageSucceeds() (gas: 61193)
-L1CrossDomainMessenger_Test:test_L1MessengerRelayMessageToSystemContract() (gas: 44815)
-L1CrossDomainMessenger_Test:test_L1MessengerRelayShouldRevertIfPaused() (gas: 41631)
-L1CrossDomainMessenger_Test:test_L1MessengerSendMessage() (gas: 78105)
-L1CrossDomainMessenger_Test:test_L1MessengerTwiceSendMessage() (gas: 66345)
-L1CrossDomainMessenger_Test:test_L1MessengerXDomainSenderReverts() (gas: 10588)
-L1CrossDomainMessenger_Test:test_L1MessengerxDomainMessageSenderResets() (gas: 58489)
-L1StandardBridge_Test:test_depositERC20() (gas: 371479)
-L1StandardBridge_Test:test_depositERC20To() (gas: 373256)
-L1StandardBridge_Test:test_depositETH() (gas: 105126)
-L1StandardBridge_Test:test_depositETHTo() (gas: 111945)
-L1StandardBridge_Test:test_donateETH() (gas: 17523)
-L1StandardBridge_Test:test_finalizeERC20Withdrawal() (gas: 438817)
-L1StandardBridge_Test:test_finalizeETHWithdrawal() (gas: 47983)
-L1StandardBridge_Test:test_initialize() (gas: 14863)
+L1CrossDomainMessenger_Test:testCannot_L1MessengerPause() (gas: 10844)
+L1CrossDomainMessenger_Test:testCannot_L1MessengerUnpause() (gas: 10858)
+L1CrossDomainMessenger_Test:test_L1MessengerMessageVersion() (gas: 8388)
+L1CrossDomainMessenger_Test:test_L1MessengerPause() (gas: 31815)
+L1CrossDomainMessenger_Test:test_L1MessengerRelayMessageSucceeds() (gas: 61215)
+L1CrossDomainMessenger_Test:test_L1MessengerRelayMessageToSystemContract() (gas: 44837)
+L1CrossDomainMessenger_Test:test_L1MessengerRelayShouldRevertIfPaused() (gas: 41587)
+L1CrossDomainMessenger_Test:test_L1MessengerSendMessage() (gas: 78150)
+L1CrossDomainMessenger_Test:test_L1MessengerTwiceSendMessage() (gas: 66413)
+L1CrossDomainMessenger_Test:test_L1MessengerUnpause() (gas: 23804)
+L1CrossDomainMessenger_Test:test_L1MessengerXDomainSenderReverts() (gas: 10599)
+L1CrossDomainMessenger_Test:test_L1MessengerxDomainMessageSenderResets() (gas: 58533)
+L1StandardBridge_Test:test_depositERC20() (gas: 371461)
+L1StandardBridge_Test:test_depositERC20To() (gas: 373238)
+L1StandardBridge_Test:test_depositETH() (gas: 105084)
+L1StandardBridge_Test:test_depositETHTo() (gas: 111968)
+L1StandardBridge_Test:test_finalizeERC20Withdrawal() (gas: 438745)
+L1StandardBridge_Test:test_finalizeETHWithdrawal() (gas: 47960)
+L1StandardBridge_Test:test_initialize() (gas: 14885)
 L1StandardBridge_Test:test_onlyEOADepositERC20() (gas: 12085)
 L1StandardBridge_Test:test_onlyEOADepositETH() (gas: 30637)
 L1StandardBridge_Test:test_onlyL2BridgeFinalizeERC20Withdrawal() (gas: 23565)
-L1StandardBridge_Test:test_onlyPortalFinalizeERC20Withdrawal() (gas: 22897)
-L1StandardBridge_Test:test_receive() (gas: 99476)
-L2CrossDomainMessenger_Test:testCannot_L2MessengerPause() (gas: 10843)
-L2CrossDomainMessenger_Test:test_L2MessengerMessageVersion() (gas: 8410)
-L2CrossDomainMessenger_Test:test_L2MessengerPause() (gas: 31837)
-L2CrossDomainMessenger_Test:test_L2MessengerRelayMessageSucceeds() (gas: 57493)
+L1StandardBridge_Test:test_onlyPortalFinalizeERC20Withdrawal() (gas: 22919)
+L1StandardBridge_Test:test_receive() (gas: 99499)
+L2CrossDomainMessenger_Test:testCannot_L2MessengerPause() (gas: 10865)
+L2CrossDomainMessenger_Test:test_L2MessengerMessageVersion() (gas: 8432)
+L2CrossDomainMessenger_Test:test_L2MessengerPause() (gas: 31815)
+L2CrossDomainMessenger_Test:test_L2MessengerRelayMessageSucceeds() (gas: 57538)
 L2CrossDomainMessenger_Test:test_L2MessengerRelayMessageToSystemContract() (gas: 24567)
-L2CrossDomainMessenger_Test:test_L2MessengerRelayShouldRevertIfPaused() (gas: 41599)
+L2CrossDomainMessenger_Test:test_L2MessengerRelayShouldRevertIfPaused() (gas: 41578)
 L2CrossDomainMessenger_Test:test_L2MessengerSendMessage() (gas: 119681)
 L2CrossDomainMessenger_Test:test_L2MessengerTwiceSendMessage() (gas: 133118)
-L2CrossDomainMessenger_Test:test_L2MessengerXDomainSenderReverts() (gas: 10588)
-L2CrossDomainMessenger_Test:test_L2MessengerxDomainMessageSenderResets() (gas: 54859)
+L2CrossDomainMessenger_Test:test_L2MessengerXDomainSenderReverts() (gas: 10621)
+L2CrossDomainMessenger_Test:test_L2MessengerxDomainMessageSenderResets() (gas: 54925)
 L2OutputOracleTest:testCannot_appendCurrentTimestamp() (gas: 18627)
 L2OutputOracleTest:testCannot_appendEmptyOutput() (gas: 16734)
 L2OutputOracleTest:testCannot_appendFutureTimestamp() (gas: 18708)
@@ -55,11 +56,12 @@ L2OutputOracleTest:test_deleteL2Output() (gas: 64320)
 L2OutputOracleTest:test_getL2Output() (gas: 74601)
 L2OutputOracleTest:test_latestBlockTimestamp() (gas: 68377)
 L2OutputOracleTest:test_nextTimestamp() (gas: 9236)
-L2StandardBridge_Test:test_finalizeDeposit() (gas: 93169)
-L2StandardBridge_Test:test_initialize() (gas: 14812)
-L2StandardBridge_Test:test_receive() (gas: 136437)
-L2StandardBridge_Test:test_withdraw() (gas: 352632)
-L2StandardBridge_Test:test_withdrawTo() (gas: 353501)
+L2StandardBridge_Test:test_ERC20BridgeFailed_whenLocalTokenIsBridge() (gas: 133074)
+L2StandardBridge_Test:test_finalizeDeposit() (gas: 93165)
+L2StandardBridge_Test:test_initialize() (gas: 14834)
+L2StandardBridge_Test:test_receive() (gas: 136392)
+L2StandardBridge_Test:test_withdraw() (gas: 352629)
+L2StandardBridge_Test:test_withdrawTo() (gas: 353463)
 L2ToL1MessagePasserTest:test_burn() (gas: 112001)
 L2ToL1MessagePasserTest:test_initiateWithdrawal_fromContract() (gas: 67935)
 L2ToL1MessagePasserTest:test_initiateWithdrawal_fromEOA() (gas: 74851)

contracts-bedrock/contracts/L1/L1StandardBridge.sol

@@ -24,14 +24,14 @@ contract L1StandardBridge is StandardBridge {
         address indexed _from,
         address indexed _to,
         uint256 _amount,
-        bytes _data
+        bytes _extraData
     );
 
     event ETHWithdrawalFinalized(
         address indexed _from,
         address indexed _to,
         uint256 _amount,
-        bytes _data
+        bytes _extraData
     );
 
     event ERC20DepositInitiated(
@@ -40,7 +40,7 @@ contract L1StandardBridge is StandardBridge {
         address indexed _from,
         address _to,
         uint256 _amount,
-        bytes _data
+        bytes _extraData
     );
 
     event ERC20WithdrawalFinalized(
@@ -49,7 +49,7 @@ contract L1StandardBridge is StandardBridge {
         address indexed _from,
         address _to,
         uint256 _amount,
-        bytes _data
+        bytes _extraData
     );
 
     /********************
@@ -76,29 +76,29 @@ contract L1StandardBridge is StandardBridge {
     /**
      * @dev Deposit an amount of the ETH to the caller's balance on L2.
      * @param _minGasLimit limit required to complete the deposit on L2.
-     * @param _data Optional data to forward to L2. This data is provided
-     *        solely as a convenience for external contracts. Aside from enforcing a maximum
-     *        length, these contracts provide no guarantees about its content.
+     * @param _extraData Optional data to forward to L2. This data is provided solely as a
+     *        convenience for external contracts which may validate that the data is included in the
+     *        CrossDomainMessenger's sentMessages mapping.
      */
-    function depositETH(uint32 _minGasLimit, bytes calldata _data) external payable onlyEOA {
-        _initiateETHDeposit(msg.sender, msg.sender, _minGasLimit, _data);
+    function depositETH(uint32 _minGasLimit, bytes calldata _extraData) external payable onlyEOA {
+        _initiateETHDeposit(msg.sender, msg.sender, _minGasLimit, _extraData);
     }
 
     /**
      * @dev Deposit an amount of ETH to a recipient's balance on L2. Note that if ETH is sent to a
      *      contract on L2 and the call fails, then that ETH will be locked in the L2StandardBridge.
      * @param _to L2 address to credit the deposit to.
      * @param _minGasLimit Gas limit required to complete the deposit on L2.
-     * @param _data Optional data to forward to L2. This data is provided
-     *        solely as a convenience for external contracts. Aside from enforcing a maximum
-     *        length, these contracts provide no guarantees about its content.
+     * @param _extraData Optional data to forward to L2. This data is provided solely as a
+     *        convenience for external contracts which may validate that the data is included in the
+     *        CrossDomainMessenger's sentMessages mapping.
      */
     function depositETHTo(
         address _to,
         uint32 _minGasLimit,
-        bytes calldata _data
+        bytes calldata _extraData
     ) external payable {
-        _initiateETHDeposit(msg.sender, _to, _minGasLimit, _data);
+        _initiateETHDeposit(msg.sender, _to, _minGasLimit, _extraData);
     }
 
     /**
@@ -107,16 +107,17 @@ contract L1StandardBridge is StandardBridge {
      * @param _l2Token Address of the L2 token we are depositing to.
      * @param _amount Amount of the ERC20 to deposit.
      * @param _minGasLimit limit required to complete the deposit on L2.
-     * @param _data Optional data to forward to L2. This data is provided
-     *        solely as a convenience for external contracts. Aside from enforcing a maximum
-     *        length, these contracts provide no guarantees about its content.
+     * @param _extraData Optional data to forward to L2.  This data is not forwarded to the
+     *        token contract and is provided solely as a convenience for external contracts
+     *        which may validate that the data is included in the CrossDomainMessenger's
+     *        sentMessages mapping.
      */
     function depositERC20(
         address _l1Token,
         address _l2Token,
         uint256 _amount,
         uint32 _minGasLimit,
-        bytes calldata _data
+        bytes calldata _extraData
     ) external virtual onlyEOA {
         _initiateERC20Deposit(
             _l1Token,
@@ -125,7 +126,7 @@ contract L1StandardBridge is StandardBridge {
             msg.sender,
             _amount,
             _minGasLimit,
-            _data
+            _extraData
         );
     }
 
@@ -136,19 +137,28 @@ contract L1StandardBridge is StandardBridge {
      * @param _to L2 address to credit the deposit to.
      * @param _amount Amount of the ERC20 to deposit.
      * @param _minGasLimit Gas limit required to complete the deposit on L2.
-     * @param _data Optional data to forward to L2. This data is provided
-     *        solely as a convenience for external contracts. Aside from enforcing a maximum
-     *        length, these contracts provide no guarantees about its content.
+     * @param _extraData Optional data to forward to L2.  This data is not forwarded to the
+     *        token contract and is provided solely as a convenience for external contracts
+     *        which may validate that the data is included in the CrossDomainMessenger's
+     *        sentMessages mapping.
      */
     function depositERC20To(
         address _l1Token,
         address _l2Token,
         address _to,
         uint256 _amount,
         uint32 _minGasLimit,
-        bytes calldata _data
+        bytes calldata _extraData
     ) external virtual {
-        _initiateERC20Deposit(_l1Token, _l2Token, msg.sender, _to, _amount, _minGasLimit, _data);
+        _initiateERC20Deposit(
+            _l1Token,
+            _l2Token,
+            msg.sender,
+            _to,
+            _amount,
+            _minGasLimit,
+            _extraData
+        );
     }
 
     function finalizeETHWithdrawal(
@@ -171,20 +181,21 @@ contract L1StandardBridge is StandardBridge {
      * @param _from L2 address initiating the transfer.
      * @param _to L1 address to credit the withdrawal to.
      * @param _amount Amount of the ERC20 to deposit.
-     * @param _data Data provided by the sender on L2. This data is provided
-     *   solely as a convenience for external contracts. Aside from enforcing a maximum
-     *   length, these contracts provide no guarantees about its content.
+     * @param _extraData Data provided by the sender on L2. This data is not forwarded to the
+     *   token contract. It is provided solely as a convenience for external contracts which
+     *   may validate that the data is included in the CrossDomainMessenger's sentMessages
+     *   mapping.
      */
     function finalizeERC20Withdrawal(
         address _l1Token,
         address _l2Token,
         address _from,
         address _to,
         uint256 _amount,
-        bytes calldata _data
+        bytes calldata _extraData
     ) external onlyOtherBridge {
-        emit ERC20WithdrawalFinalized(_l1Token, _l2Token, _from, _to, _amount, _data);
-        finalizeBridgeERC20(_l1Token, _l2Token, _from, _to, _amount, _data);
+        emit ERC20WithdrawalFinalized(_l1Token, _l2Token, _from, _to, _amount, _extraData);
+        finalizeBridgeERC20(_l1Token, _l2Token, _from, _to, _amount, _extraData);
     }
 
     /**********************
@@ -195,10 +206,10 @@ contract L1StandardBridge is StandardBridge {
         address _from,
         address _to,
         uint32 _minGasLimit,
-        bytes memory _data
+        bytes memory _extraData
     ) internal {
-        emit ETHDepositInitiated(_from, _to, msg.value, _data);
-        _initiateBridgeETH(_from, _to, msg.value, _minGasLimit, _data);
+        emit ETHDepositInitiated(_from, _to, msg.value, _extraData);
+        _initiateBridgeETH(_from, _to, msg.value, _minGasLimit, _extraData);
     }
 
     function _initiateERC20Deposit(
@@ -208,9 +219,9 @@ contract L1StandardBridge is StandardBridge {
         address _to,
         uint256 _amount,
         uint32 _minGasLimit,
-        bytes calldata _data
+        bytes calldata _extraData
     ) internal {
-        emit ERC20DepositInitiated(_l1Token, _l2Token, _from, _to, _amount, _data);
-        _initiateBridgeERC20(_l1Token, _l2Token, _from, _to, _amount, _minGasLimit, _data);
+        emit ERC20DepositInitiated(_l1Token, _l2Token, _from, _to, _amount, _extraData);
+        _initiateBridgeERC20(_l1Token, _l2Token, _from, _to, _amount, _minGasLimit, _extraData);
     }
 }

contracts-bedrock/contracts/L1/OptimismPortal.sol

@@ -182,7 +182,11 @@ contract OptimismPortal {
         WithdrawalVerifier.OutputRootProof calldata _outputRootProof,
         bytes calldata _withdrawalProof
     ) external payable {
-        // Prevent reentrency
+        // Prevent immediate reentrancy to other functions on this contract.
+        // Note that reentrancy to _this_ function is also prevented by the l2Sender check below,
+        // but this check is also an efficient mechanism of preventing the portal from calling
+        // its own receive() or depositTransaction() functions, without the need for a nonReentrant
+        // modifier.
         require(_target != address(this), "Cannot send message to self.");
 
         // Get the output root.
@@ -237,6 +241,12 @@ contract OptimismPortal {
             "Insufficient gas to finalize withdrawal."
         );
 
+        // Disallow withdrawals during reentrancy to this function.
+        require(
+            l2Sender == DEFAULT_L2_SENDER,
+            "Cannot withdraw a transaction within another withdrawal transaction."
+        );
+
         // Set the l2Sender so that other contracts can know which account
         // on L2 is making the withdrawal
         l2Sender = _sender;