OZ Medium severity audit fixes (on develop)

.changeset/three-pants-punch.md

@@ -0,0 +1,9 @@
+---
+'@eth-optimism/contracts-bedrock': patch
+---
+
+Fixes to medium severity OZ findings
+
+- Disallow reentrant withdrawals
+- remove donateEth
+- Correct ordering of \_from and \_to arguments on refunds of failed deposits

packages/contracts-bedrock/.gas-snapshot

@@ -23,39 +23,40 @@ L1BlockTest:test_updateValues() (gas: 28215)
 L1BlockNumberTest:test_fallback() (gas: 18773)
 L1BlockNumberTest:test_getL1BlockNumber() (gas: 10589)
 L1BlockNumberTest:test_receive() (gas: 25436)
-L1CrossDomainMessenger_Test:testCannot_L1MessengerPause() (gas: 10909)
-L1CrossDomainMessenger_Test:test_L1MessengerMessageVersion() (gas: 8366)
-L1CrossDomainMessenger_Test:test_L1MessengerPause() (gas: 31882)
-L1CrossDomainMessenger_Test:test_L1MessengerRelayMessageSucceeds() (gas: 61239)
-L1CrossDomainMessenger_Test:test_L1MessengerRelayMessageToSystemContract() (gas: 44859)
-L1CrossDomainMessenger_Test:test_L1MessengerRelayShouldRevertIfPaused() (gas: 41631)
-L1CrossDomainMessenger_Test:test_L1MessengerSendMessage() (gas: 172148)
-L1CrossDomainMessenger_Test:test_L1MessengerTwiceSendMessage() (gas: 1254131)
-L1CrossDomainMessenger_Test:test_L1MessengerXDomainSenderReverts() (gas: 10566)
-L1CrossDomainMessenger_Test:test_L1MessengerxDomainMessageSenderResets() (gas: 58535)
-L1StandardBridge_Test:test_depositERC20() (gas: 452873)
-L1StandardBridge_Test:test_depositERC20To() (gas: 454650)
-L1StandardBridge_Test:test_depositETH() (gas: 247054)
-L1StandardBridge_Test:test_depositETHTo() (gas: 204938)
-L1StandardBridge_Test:test_donateETH() (gas: 17545)
-L1StandardBridge_Test:test_finalizeERC20Withdrawal() (gas: 438824)
+L1CrossDomainMessenger_Test:testCannot_L1MessengerPause() (gas: 10844)
+L1CrossDomainMessenger_Test:testCannot_L1MessengerUnpause() (gas: 10858)
+L1CrossDomainMessenger_Test:test_L1MessengerMessageVersion() (gas: 8388)
+L1CrossDomainMessenger_Test:test_L1MessengerPause() (gas: 31860)
+L1CrossDomainMessenger_Test:test_L1MessengerRelayMessageSucceeds() (gas: 61261)
+L1CrossDomainMessenger_Test:test_L1MessengerRelayMessageToSystemContract() (gas: 44881)
+L1CrossDomainMessenger_Test:test_L1MessengerRelayShouldRevertIfPaused() (gas: 41587)
+L1CrossDomainMessenger_Test:test_L1MessengerSendMessage() (gas: 172193)
+L1CrossDomainMessenger_Test:test_L1MessengerTwiceSendMessage() (gas: 1254199)
+L1CrossDomainMessenger_Test:test_L1MessengerUnpause() (gas: 23804)
+L1CrossDomainMessenger_Test:test_L1MessengerXDomainSenderReverts() (gas: 10599)
+L1CrossDomainMessenger_Test:test_L1MessengerxDomainMessageSenderResets() (gas: 58579)
+L1StandardBridge_Test:test_depositERC20() (gas: 452856)
+L1StandardBridge_Test:test_depositERC20To() (gas: 454632)
+L1StandardBridge_Test:test_depositETH() (gas: 247077)
+L1StandardBridge_Test:test_depositETHTo() (gas: 204961)
+L1StandardBridge_Test:test_finalizeERC20Withdrawal() (gas: 438752)
 L1StandardBridge_Test:test_finalizeETHWithdrawal() (gas: 48005)
 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: 22919)
-L1StandardBridge_Test:test_receive() (gas: 391794)
+L1StandardBridge_Test:test_receive() (gas: 391817)
 L2CrossDomainMessenger_Test:testCannot_L2MessengerPause() (gas: 10843)
 L2CrossDomainMessenger_Test:test_L2MessengerMessageVersion() (gas: 8410)
 L2CrossDomainMessenger_Test:test_L2MessengerPause() (gas: 31837)
 L2CrossDomainMessenger_Test:test_L2MessengerRelayMessageSucceeds() (gas: 57538)
 L2CrossDomainMessenger_Test:test_L2MessengerRelayMessageToSystemContract() (gas: 24567)
 L2CrossDomainMessenger_Test:test_L2MessengerRelayShouldRevertIfPaused() (gas: 41599)
-L2CrossDomainMessenger_Test:test_L2MessengerSendMessage() (gas: 119659)
-L2CrossDomainMessenger_Test:test_L2MessengerTwiceSendMessage() (gas: 133096)
-L2CrossDomainMessenger_Test:test_L2MessengerXDomainSenderReverts() (gas: 10588)
-L2CrossDomainMessenger_Test:test_L2MessengerxDomainMessageSenderResets() (gas: 54859)
+L2CrossDomainMessenger_Test:test_L2MessengerSendMessage() (gas: 119682)
+L2CrossDomainMessenger_Test:test_L2MessengerTwiceSendMessage() (gas: 133142)
+L2CrossDomainMessenger_Test:test_L2MessengerXDomainSenderReverts() (gas: 10599)
+L2CrossDomainMessenger_Test:test_L2MessengerxDomainMessageSenderResets() (gas: 54881)
 L2OutputOracleTest:testCannot_appendEmptyOutput() (gas: 18442)
 L2OutputOracleTest:testCannot_appendFutureTimetamp() (gas: 20072)
 L2OutputOracleTest:testCannot_appendOnWrongFork() (gas: 20710)
@@ -71,14 +72,15 @@ L2OutputOracleTest:test_deleteL2Output() (gas: 66081)
 L2OutputOracleTest:test_getL2Output() (gas: 76274)
 L2OutputOracleTest:test_latestBlockNumber() (gas: 70075)
 L2OutputOracleTest:test_nextBlockNumber() (gas: 9279)
-L2StandardBridge_Test:test_cannotWithdrawEthWithoutSendingIt() (gas: 21578)
-L2StandardBridge_Test:test_finalizeDeposit() (gas: 93165)
-L2StandardBridge_Test:test_finalizeDeposit_failsToCompleteOutboundTransfer() (gas: 140106)
-L2StandardBridge_Test:test_initialize() (gas: 14834)
-L2StandardBridge_Test:test_receive() (gas: 136459)
-L2StandardBridge_Test:test_withdraw() (gas: 352724)
-L2StandardBridge_Test:test_withdrawTo() (gas: 353480)
-L2StandardBridge_Test:test_withdraw_onlyEOA() (gas: 251952)
+L2StandardBridge_Test:test_ERC20BridgeFailed_whenLocalTokenIsBridge() (gas: 133097)
+L2StandardBridge_Test:test_cannotWithdrawEthWithoutSendingIt() (gas: 21611)
+L2StandardBridge_Test:test_finalizeDeposit() (gas: 93100)
+L2StandardBridge_Test:test_finalizeDeposit_failsToCompleteOutboundTransfer() (gas: 140129)
+L2StandardBridge_Test:test_initialize() (gas: 14856)
+L2StandardBridge_Test:test_receive() (gas: 136415)
+L2StandardBridge_Test:test_withdraw() (gas: 352760)
+L2StandardBridge_Test:test_withdrawTo() (gas: 353481)
+L2StandardBridge_Test:test_withdraw_onlyEOA() (gas: 251970)
 L2ToL1MessagePasserTest:test_burn() (gas: 112046)
 L2ToL1MessagePasserTest:test_initiateWithdrawal_fromContract() (gas: 67890)
 L2ToL1MessagePasserTest:test_initiateWithdrawal_fromEOA() (gas: 74851)
@@ -164,4 +166,4 @@ SequencerFeeVault_Test:test_constructor() (gas: 7611)
 SequencerFeeVault_Test:test_minWithdrawalAmount() (gas: 5429)
 SequencerFeeVault_Test:test_receive() (gas: 17280)
 SequencerFeeVault_Test:test_revertWithdraw() (gas: 9266)
-SequencerFeeVault_Test:test_withdraw() (gas: 147300)
+SequencerFeeVault_Test:test_withdraw() (gas: 147301)

packages/contracts-bedrock/contracts/L1/L1StandardBridge.sol

@@ -16,72 +16,72 @@ contract L1StandardBridge is StandardBridge {
      * @custom:legacy
      * @notice Emitted whenever a deposit of ETH from L1 into L2 is initiated.
      *
-     * @param _from   Address of the depositor.
-     * @param _to     Address of the recipient on L2.
-     * @param _amount Amount of ETH deposited.
-     * @param _data   Extra data attached to the deposit.
+     * @param _from      Address of the depositor.
+     * @param _to        Address of the recipient on L2.
+     * @param _amount    Amount of ETH deposited.
+     * @param _extraData Extra data attached to the deposit.
      */
     event ETHDepositInitiated(
         address indexed _from,
         address indexed _to,
         uint256 _amount,
-        bytes _data
+        bytes _extraData
     );
 
     /**
      * @custom:legacy
      * @notice Emitted whenever a withdrawal of ETH from L2 to L1 is finalized.
      *
-     * @param _from   Address of the withdrawer.
-     * @param _to     Address of the recipient on L1.
-     * @param _amount Amount of ETH withdrawn.
-     * @param _data   Extra data attached to the withdrawal.
+     * @param _from      Address of the withdrawer.
+     * @param _to        Address of the recipient on L1.
+     * @param _amount    Amount of ETH withdrawn.
+     * @param _extraData Extra data attached to the withdrawal.
      */
     event ETHWithdrawalFinalized(
         address indexed _from,
         address indexed _to,
         uint256 _amount,
-        bytes _data
+        bytes _extraData
     );
 
     /**
      * @custom:legacy
      * @notice Emitted whenever an ERC20 deposit is initiated.
      *
-     * @param _l1Token Address of the token on L1.
-     * @param _l2Token Address of the corresponding token on L2.
-     * @param _from    Address of the depositor.
-     * @param _to      Address of the recipient on L2.
-     * @param _amount  Amount of the ERC20 deposited.
-     * @param _data    Extra data attached to the deposit.
+     * @param _l1Token   Address of the token on L1.
+     * @param _l2Token   Address of the corresponding token on L2.
+     * @param _from      Address of the depositor.
+     * @param _to        Address of the recipient on L2.
+     * @param _amount    Amount of the ERC20 deposited.
+     * @param _extraData Extra data attached to the deposit.
      */
     event ERC20DepositInitiated(
         address indexed _l1Token,
         address indexed _l2Token,
         address indexed _from,
         address _to,
         uint256 _amount,
-        bytes _data
+        bytes _extraData
     );
 
     /**
      * @custom:legacy
      * @notice Emitted whenever an ERC20 withdrawal is finalized.
      *
-     * @param _l1Token Address of the token on L1.
-     * @param _l2Token Address of the corresponding token on L2.
-     * @param _from    Address of the withdrawer.
-     * @param _to      Address of the recipient on L1.
-     * @param _amount  Amount of the ERC20 withdrawn.
-     * @param _data    Extra data attached to the withdrawal.
+     * @param _l1Token   Address of the token on L1.
+     * @param _l2Token   Address of the corresponding token on L2.
+     * @param _from      Address of the withdrawer.
+     * @param _to        Address of the recipient on L1.
+     * @param _amount    Amount of the ERC20 withdrawn.
+     * @param _extraData Extra data attached to the withdrawal.
      */
     event ERC20WithdrawalFinalized(
         address indexed _l1Token,
         address indexed _l2Token,
         address indexed _from,
         address _to,
         uint256 _amount,
-        bytes _data
+        bytes _extraData
     );
 
     /**
@@ -108,12 +108,12 @@ contract L1StandardBridge is StandardBridge {
      * @notice Deposits some amount of ETH into the sender's account on L2.
      *
      * @param _minGasLimit Minimum gas limit for the deposit message on L2.
-     * @param _data        Optional data to forward to L2. Data supplied here will not be used to
+     * @param _extraData   Optional data to forward to L2. Data supplied here will not be used to
      *                     execute any code on L2 and is only emitted as extra data for the
      *                     convenience of off-chain tooling.
      */
-    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);
     }
 
     /**
@@ -126,16 +126,16 @@ contract L1StandardBridge is StandardBridge {
      *
      * @param _to          Address of the recipient on L2.
      * @param _minGasLimit Minimum gas limit for the deposit message on L2.
-     * @param _data        Optional data to forward to L2. Data supplied here will not be used to
+     * @param _extraData   Optional data to forward to L2. Data supplied here will not be used to
      *                     execute any code on L2 and is only emitted as extra data for the
      *                     convenience of off-chain tooling.
      */
     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);
     }
 
     /**
@@ -146,7 +146,7 @@ contract L1StandardBridge is StandardBridge {
      * @param _l2Token     Address of the corresponding token on L2.
      * @param _amount      Amount of the ERC20 to deposit.
      * @param _minGasLimit Minimum gas limit for the deposit message on L2.
-     * @param _data        Optional data to forward to L2. Data supplied here will not be used to
+     * @param _extraData   Optional data to forward to L2. Data supplied here will not be used to
      *                     execute any code on L2 and is only emitted as extra data for the
      *                     convenience of off-chain tooling.
      */
@@ -155,7 +155,7 @@ contract L1StandardBridge is StandardBridge {
         address _l2Token,
         uint256 _amount,
         uint32 _minGasLimit,
-        bytes calldata _data
+        bytes calldata _extraData
     ) external virtual onlyEOA {
         _initiateERC20Deposit(
             _l1Token,
@@ -164,7 +164,7 @@ contract L1StandardBridge is StandardBridge {
             msg.sender,
             _amount,
             _minGasLimit,
-            _data
+            _extraData
         );
     }
 
@@ -177,7 +177,7 @@ contract L1StandardBridge is StandardBridge {
      * @param _to          Address of the recipient on L2.
      * @param _amount      Amount of the ERC20 to deposit.
      * @param _minGasLimit Minimum gas limit for the deposit message on L2.
-     * @param _data        Optional data to forward to L2. Data supplied here will not be used to
+     * @param _extraData   Optional data to forward to L2. Data supplied here will not be used to
      *                     execute any code on L2 and is only emitted as extra data for the
      *                     convenience of off-chain tooling.
      */
@@ -187,51 +187,59 @@ contract L1StandardBridge is StandardBridge {
         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
+        );
     }
 
     /**
      * @custom:legacy
      * @notice Finalizes a withdrawal of ETH from L2.
      *
-     * @param _from   Address of the withdrawer on L2.
-     * @param _to     Address of the recipient on L1.
-     * @param _amount Amount of ETH to withdraw.
-     * @param _data   Optional data forwarded from L2.
+     * @param _from      Address of the withdrawer on L2.
+     * @param _to        Address of the recipient on L1.
+     * @param _amount    Amount of ETH to withdraw.
+     * @param _extraData Optional data forwarded from L2.
      */
     function finalizeETHWithdrawal(
         address _from,
         address _to,
         uint256 _amount,
-        bytes calldata _data
+        bytes calldata _extraData
     ) external payable onlyOtherBridge {
-        emit ETHWithdrawalFinalized(_from, _to, _amount, _data);
-        finalizeBridgeETH(_from, _to, _amount, _data);
+        emit ETHWithdrawalFinalized(_from, _to, _amount, _extraData);
+        finalizeBridgeETH(_from, _to, _amount, _extraData);
     }
 
     /**
      * @custom:legacy
      * @notice Finalizes a withdrawal of ERC20 tokens from L2.
      *
-     * @param _l1Token Address of the token on L1.
-     * @param _l2Token Address of the corresponding token on L2.
-     * @param _from    Address of the withdrawer on L2.
-     * @param _to      Address of the recipient on L1.
-     * @param _amount  Amount of ETH to withdraw.
-     * @param _data    Optional data forwarded from L2.
+     * @param _l1Token   Address of the token on L1.
+     * @param _l2Token   Address of the corresponding token on L2.
+     * @param _from      Address of the withdrawer on L2.
+     * @param _to        Address of the recipient on L1.
+     * @param _amount    Amount of ETH to withdraw.
+     * @param _extraData Optional data forwarded from L2.
      */
     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);
     }
 
     /**
@@ -240,16 +248,16 @@ contract L1StandardBridge is StandardBridge {
      * @param _from        Address of the sender on L1.
      * @param _to          Address of the recipient on L2.
      * @param _minGasLimit Minimum gas limit for the deposit message on L2.
-     * @param _data        Optional data to forward to L2.
+     * @param _extraData   Optional data to forward to L2.
      */
     function _initiateETHDeposit(
         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);
     }
 
     /**
@@ -261,7 +269,7 @@ contract L1StandardBridge is StandardBridge {
      * @param _to          Address of the recipient on L2.
      * @param _amount      Amount of the ERC20 to deposit.
      * @param _minGasLimit Minimum gas limit for the deposit message on L2.
-     * @param _data        Optional data to forward to L2.
+     * @param _extraData   Optional data to forward to L2.
      */
     function _initiateERC20Deposit(
         address _l1Token,
@@ -270,9 +278,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);
     }
 }