diff --git a/.changeset/three-pants-punch.md b/.changeset/three-pants-punch.md new file mode 100644 index 0000000000000..46842a32cb696 --- /dev/null +++ b/.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 diff --git a/packages/contracts-bedrock/.gas-snapshot b/packages/contracts-bedrock/.gas-snapshot index 7fc1b5101946d..4fbd36d97d290 100644 --- a/packages/contracts-bedrock/.gas-snapshot +++ b/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) diff --git a/packages/contracts-bedrock/contracts/L1/L1StandardBridge.sol b/packages/contracts-bedrock/contracts/L1/L1StandardBridge.sol index 9e8218712df87..00ac13afab5b6 100644 --- a/packages/contracts-bedrock/contracts/L1/L1StandardBridge.sol +++ b/packages/contracts-bedrock/contracts/L1/L1StandardBridge.sol @@ -16,44 +16,44 @@ 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, @@ -61,19 +61,19 @@ contract L1StandardBridge is StandardBridge { 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, @@ -81,7 +81,7 @@ contract L1StandardBridge is StandardBridge { 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,40 +187,48 @@ 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, @@ -228,10 +236,10 @@ contract L1StandardBridge is StandardBridge { 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); } } diff --git a/packages/contracts-bedrock/contracts/L1/OptimismPortal.sol b/packages/contracts-bedrock/contracts/L1/OptimismPortal.sol index 4c4f7cda13057..5b716843e52bf 100644 --- a/packages/contracts-bedrock/contracts/L1/OptimismPortal.sol +++ b/packages/contracts-bedrock/contracts/L1/OptimismPortal.sol @@ -165,7 +165,7 @@ contract OptimismPortal is ResourceMetering { WithdrawalVerifier.OutputRootProof calldata _outputRootProof, bytes calldata _withdrawalProof ) external payable { - // Prevent reentrancy. + // Prevent nested withdrawals within withdrawals. require( l2Sender == DEFAULT_L2_SENDER, "OptimismPortal: can only trigger one withdrawal per transaction" diff --git a/packages/contracts-bedrock/contracts/L2/L2StandardBridge.sol b/packages/contracts-bedrock/contracts/L2/L2StandardBridge.sol index 61f6e8c947ed0..d7f1eec7fa409 100644 --- a/packages/contracts-bedrock/contracts/L2/L2StandardBridge.sol +++ b/packages/contracts-bedrock/contracts/L2/L2StandardBridge.sol @@ -22,12 +22,12 @@ contract L2StandardBridge is StandardBridge { * @custom:legacy * @notice Emitted whenever a withdrawal from L2 to L1 is initiated. * - * @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 WithdrawalInitiated( address indexed _l1Token, @@ -35,19 +35,19 @@ contract L2StandardBridge is StandardBridge { address indexed _from, address _to, uint256 _amount, - bytes _data + bytes _extraData ); /** * @custom:legacy * @notice Emitted whenever an ERC20 deposit is finalized. * - * @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 DepositFinalized( address indexed _l1Token, @@ -55,19 +55,19 @@ contract L2StandardBridge is StandardBridge { address indexed _from, address _to, uint256 _amount, - bytes _data + bytes _extraData ); /** * @custom:legacy * @notice Emitted whenever a deposit fails. * - * @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 DepositFailed( address indexed _l1Token, @@ -75,7 +75,7 @@ contract L2StandardBridge is StandardBridge { address indexed _from, address _to, uint256 _amount, - bytes _data + bytes _extraData ); /** @@ -94,15 +94,15 @@ contract L2StandardBridge is StandardBridge { * @param _l2Token Address of the L2 token to withdraw. * @param _amount Amount of the L2 token to withdraw. * @param _minGasLimit Minimum gas limit to use for the transaction. - * @param _data Extra data attached to the withdrawal. + * @param _extraData Extra data attached to the withdrawal. */ function withdraw( address _l2Token, uint256 _amount, uint32 _minGasLimit, - bytes calldata _data + bytes calldata _extraData ) external payable virtual onlyEOA { - _initiateWithdrawal(_l2Token, msg.sender, msg.sender, _amount, _minGasLimit, _data); + _initiateWithdrawal(_l2Token, msg.sender, msg.sender, _amount, _minGasLimit, _extraData); } /** @@ -117,28 +117,28 @@ contract L2StandardBridge is StandardBridge { * @param _to Recipient account on L1. * @param _amount Amount of the L2 token to withdraw. * @param _minGasLimit Minimum gas limit to use for the transaction. - * @param _data Extra data attached to the withdrawal. + * @param _extraData Extra data attached to the withdrawal. */ function withdrawTo( address _l2Token, address _to, uint256 _amount, uint32 _minGasLimit, - bytes calldata _data + bytes calldata _extraData ) external payable virtual { - _initiateWithdrawal(_l2Token, msg.sender, _to, _amount, _minGasLimit, _data); + _initiateWithdrawal(_l2Token, msg.sender, _to, _amount, _minGasLimit, _extraData); } /** * @custom:legacy * @notice Finalizes a deposit from L1 to L2. * - * @param _l1Token Address of the L1 token to deposit. - * @param _l2Token Address of the corresponding L2 token. - * @param _from Address of the depositor. - * @param _to Address of the recipient. - * @param _amount Amount of the tokens being deposited. - * @param _data Extra data attached to the deposit. + * @param _l1Token Address of the L1 token to deposit. + * @param _l2Token Address of the corresponding L2 token. + * @param _from Address of the depositor. + * @param _to Address of the recipient. + * @param _amount Amount of the tokens being deposited. + * @param _extraData Extra data attached to the deposit. */ function finalizeDeposit( address _l1Token, @@ -146,14 +146,14 @@ contract L2StandardBridge is StandardBridge { address _from, address _to, uint256 _amount, - bytes calldata _data + bytes calldata _extraData ) external payable virtual { if (_l1Token == address(0) && _l2Token == Lib_PredeployAddresses.OVM_ETH) { - finalizeBridgeETH(_from, _to, _amount, _data); + finalizeBridgeETH(_from, _to, _amount, _extraData); } else { - finalizeBridgeERC20(_l2Token, _l1Token, _from, _to, _amount, _data); + finalizeBridgeERC20(_l2Token, _l1Token, _from, _to, _amount, _extraData); } - emit DepositFinalized(_l1Token, _l2Token, _from, _to, _amount, _data); + emit DepositFinalized(_l1Token, _l2Token, _from, _to, _amount, _extraData); } /** @@ -165,7 +165,7 @@ contract L2StandardBridge is StandardBridge { * @param _to Recipient account on L1. * @param _amount Amount of the L2 token to withdraw. * @param _minGasLimit Minimum gas limit to use for the transaction. - * @param _data Extra data attached to the withdrawal. + * @param _extraData Extra data attached to the withdrawal. */ function _initiateWithdrawal( address _l2Token, @@ -173,15 +173,15 @@ contract L2StandardBridge is StandardBridge { address _to, uint256 _amount, uint32 _minGasLimit, - bytes calldata _data + bytes calldata _extraData ) internal { address l1Token = OptimismMintableERC20(_l2Token).l1Token(); if (_l2Token == Lib_PredeployAddresses.OVM_ETH) { require(msg.value == _amount, "ETH withdrawals must include sufficient ETH value."); - _initiateBridgeETH(_from, _to, _amount, _minGasLimit, _data); + _initiateBridgeETH(_from, _to, _amount, _minGasLimit, _extraData); } else { - _initiateBridgeERC20(_l2Token, l1Token, _from, _to, _amount, _minGasLimit, _data); + _initiateBridgeERC20(_l2Token, l1Token, _from, _to, _amount, _minGasLimit, _extraData); } - emit WithdrawalInitiated(l1Token, _l2Token, _from, _to, _amount, _data); + emit WithdrawalInitiated(l1Token, _l2Token, _from, _to, _amount, _extraData); } } diff --git a/packages/contracts-bedrock/contracts/test/L1CrossDomainMessenger.t.sol b/packages/contracts-bedrock/contracts/test/L1CrossDomainMessenger.t.sol index 8c6c93cada526..716d73dda4633 100644 --- a/packages/contracts-bedrock/contracts/test/L1CrossDomainMessenger.t.sol +++ b/packages/contracts-bedrock/contracts/test/L1CrossDomainMessenger.t.sol @@ -42,6 +42,21 @@ contract L1CrossDomainMessenger_Test is Messenger_Initializer { L1Messenger.pause(); } + // unpause: should unpause the contract when called by the current owner + function test_L1MessengerUnpause() external { + L1Messenger.pause(); + assert(L1Messenger.paused()); + L1Messenger.unpause(); + assert(!L1Messenger.paused()); + } + + // unpause: should not unpause the contract when called by account other than the owner + function testCannot_L1MessengerUnpause() external { + vm.expectRevert("Ownable: caller is not the owner"); + vm.prank(address(0xABBA)); + L1Messenger.unpause(); + } + // the version is encoded in the nonce function test_L1MessengerMessageVersion() external { assertEq( diff --git a/packages/contracts-bedrock/contracts/test/L1StandardBridge.t.sol b/packages/contracts-bedrock/contracts/test/L1StandardBridge.t.sol index 01e28f42b606c..97120f7e89b2c 100644 --- a/packages/contracts-bedrock/contracts/test/L1StandardBridge.t.sol +++ b/packages/contracts-bedrock/contracts/test/L1StandardBridge.t.sol @@ -390,13 +390,4 @@ contract L1StandardBridge_Test is Bridge_Initializer { hex"" ); } - - // donateETH - // - can send ETH to the contract - function test_donateETH() external { - assertEq(address(L1Bridge).balance, 0); - vm.prank(alice); - L1Bridge.donateETH{ value: 1000 }(); - assertEq(address(L1Bridge).balance, 1000); - } } diff --git a/packages/contracts-bedrock/contracts/test/L2StandardBridge.t.sol b/packages/contracts-bedrock/contracts/test/L2StandardBridge.t.sol index 9c19b333eecea..01c5f7e3cb6e7 100644 --- a/packages/contracts-bedrock/contracts/test/L2StandardBridge.t.sol +++ b/packages/contracts-bedrock/contracts/test/L2StandardBridge.t.sol @@ -146,13 +146,12 @@ contract L2StandardBridge_Test is Bridge_Initializer { // - invalid deposit calls Withdrawer.initiateWithdrawal function test_finalizeDeposit_failsToCompleteOutboundTransfer() external { // TODO: events and calls - address invalidL2Token = address(0x1234); - vm.mockCall( address(L2Bridge.messenger()), abi.encodeWithSelector(CrossDomainMessenger.xDomainMessageSender.selector), abi.encode(address(L2Bridge.otherBridge())) ); + address invalidL2Token = address(0x1234); vm.prank(address(L2Messenger)); vm.expectEmit(true, true, true, true); emit ERC20BridgeInitiated( @@ -181,5 +180,21 @@ contract L2StandardBridge_Test is Bridge_Initializer { hex"" ); } + + // finalizeBridgeERC20 + // - fails when the local token's address equals bridge address + function test_ERC20BridgeFailed_whenLocalTokenIsBridge() external { + vm.mockCall( + address(L2Bridge.messenger()), + abi.encodeWithSelector(CrossDomainMessenger.xDomainMessageSender.selector), + abi.encode(address(L2Bridge.otherBridge())) + ); + // fails when the local token's address equals bridge address + vm.expectEmit(true, true, true, true); + emit ERC20BridgeFailed(address(L2Bridge), address(L1Token), alice, bob, 100, hex""); + + vm.prank(address(L2Messenger)); + L2Bridge.finalizeDeposit(address(L1Token), address(L2Bridge), alice, bob, 100, hex""); + } } diff --git a/packages/contracts-bedrock/contracts/universal/CrossDomainMessenger.sol b/packages/contracts-bedrock/contracts/universal/CrossDomainMessenger.sol index e0ecdba7ba2da..007c1a61f26cb 100644 --- a/packages/contracts-bedrock/contracts/universal/CrossDomainMessenger.sol +++ b/packages/contracts-bedrock/contracts/universal/CrossDomainMessenger.sol @@ -101,6 +101,13 @@ abstract contract CrossDomainMessenger is _pause(); } + /** + * Unpause relaying. + */ + function unpause() external onlyOwner { + _unpause(); + } + /** * Retrieves the address of the x-domain message sender. Will throw an error * if the sender is not currently set (equal to the default sender). diff --git a/packages/contracts-bedrock/contracts/universal/StandardBridge.sol b/packages/contracts-bedrock/contracts/universal/StandardBridge.sol index d861f71953fdc..e03eb719bd34a 100644 --- a/packages/contracts-bedrock/contracts/universal/StandardBridge.sol +++ b/packages/contracts-bedrock/contracts/universal/StandardBridge.sol @@ -31,14 +31,14 @@ abstract contract StandardBridge { address indexed _from, address indexed _to, uint256 _amount, - bytes _data + bytes _extraData ); event ETHBridgeFinalized( address indexed _from, address indexed _to, uint256 _amount, - bytes _data + bytes _extraData ); event ERC20BridgeInitiated( @@ -47,7 +47,7 @@ abstract contract StandardBridge { address indexed _from, address _to, uint256 _amount, - bytes _data + bytes _extraData ); event ERC20BridgeFinalized( @@ -56,7 +56,7 @@ abstract contract StandardBridge { address indexed _from, address _to, uint256 _amount, - bytes _data + bytes _extraData ); event ERC20BridgeFailed( @@ -65,7 +65,7 @@ abstract contract StandardBridge { address indexed _from, address _to, uint256 _amount, - bytes _data + bytes _extraData ); /************* @@ -128,11 +128,6 @@ abstract contract StandardBridge { * Public Functions * ********************/ - /** - * @notice Send ETH to this contract. This is used during upgrades - */ - function donateETH() external payable {} - /** * @notice EOAs can simply send ETH to this contract to have it be deposited * to L2 through the standard bridge. @@ -144,8 +139,8 @@ abstract contract StandardBridge { /** * @notice Send ETH to the message sender on the remote domain */ - function bridgeETH(uint32 _minGasLimit, bytes calldata _data) public payable onlyEOA { - _initiateBridgeETH(msg.sender, msg.sender, msg.value, _minGasLimit, _data); + function bridgeETH(uint32 _minGasLimit, bytes calldata _extraData) public payable onlyEOA { + _initiateBridgeETH(msg.sender, msg.sender, msg.value, _minGasLimit, _extraData); } /** @@ -155,9 +150,9 @@ abstract contract StandardBridge { function bridgeETHTo( address _to, uint32 _minGasLimit, - bytes calldata _data + bytes calldata _extraData ) public payable { - _initiateBridgeETH(msg.sender, _to, msg.value, _minGasLimit, _data); + _initiateBridgeETH(msg.sender, _to, msg.value, _minGasLimit, _extraData); } /** @@ -168,7 +163,7 @@ abstract contract StandardBridge { address _remoteToken, uint256 _amount, uint32 _minGasLimit, - bytes calldata _data + bytes calldata _extraData ) public virtual onlyEOA { _initiateBridgeERC20( _localToken, @@ -177,7 +172,7 @@ abstract contract StandardBridge { msg.sender, _amount, _minGasLimit, - _data + _extraData ); } @@ -190,7 +185,7 @@ abstract contract StandardBridge { address _to, uint256 _amount, uint32 _minGasLimit, - bytes calldata _data + bytes calldata _extraData ) public virtual { _initiateBridgeERC20( _localToken, @@ -199,7 +194,7 @@ abstract contract StandardBridge { _to, _amount, _minGasLimit, - _data + _extraData ); } @@ -210,12 +205,12 @@ abstract contract StandardBridge { address _from, address _to, uint256 _amount, - bytes calldata _data + bytes calldata _extraData ) public payable onlyOtherBridge { require(msg.value == _amount, "Amount sent does not match amount required."); require(_to != address(this), "Cannot send to self."); - emit ETHBridgeFinalized(_from, _to, _amount, _data); + emit ETHBridgeFinalized(_from, _to, _amount, _extraData); (bool success, ) = _to.call{ value: _amount }(new bytes(0)); require(success, "ETH transfer failed."); } @@ -229,23 +224,25 @@ abstract contract StandardBridge { address _from, address _to, uint256 _amount, - bytes calldata _data + bytes calldata _extraData ) public onlyOtherBridge { try this.completeOutboundTransfer(_localToken, _remoteToken, _to, _amount) { - emit ERC20BridgeFinalized(_localToken, _remoteToken, _from, _to, _amount, _data); + emit ERC20BridgeFinalized(_localToken, _remoteToken, _from, _to, _amount, _extraData); } catch { // Something went wrong during the bridging process, return to sender. // Can happen if a bridge UI specifies the wrong L2 token. + // We reverse both the local and remote token addresses, as well as the to and from + // addresses. This will preserve the accuracy of accounting based on emitted events. _initiateBridgeERC20Unchecked( _localToken, _remoteToken, - _from, _to, + _from, _amount, 0, // _minGasLimit, 0 is fine here - _data + _extraData ); - emit ERC20BridgeFailed(_localToken, _remoteToken, _from, _to, _amount, _data); + emit ERC20BridgeFailed(_localToken, _remoteToken, _from, _to, _amount, _extraData); } } @@ -296,13 +293,19 @@ abstract contract StandardBridge { address _to, uint256 _amount, uint32 _minGasLimit, - bytes memory _data + bytes memory _extraData ) internal { - emit ETHBridgeInitiated(_from, _to, _amount, _data); + emit ETHBridgeInitiated(_from, _to, _amount, _extraData); messenger.sendMessage{ value: _amount }( address(otherBridge), - abi.encodeWithSelector(this.finalizeBridgeETH.selector, _from, _to, _amount, _data), + abi.encodeWithSelector( + this.finalizeBridgeETH.selector, + _from, + _to, + _amount, + _extraData + ), _minGasLimit ); } @@ -317,7 +320,7 @@ abstract contract StandardBridge { address _to, uint256 _amount, uint32 _minGasLimit, - bytes calldata _data + bytes calldata _extraData ) internal { // Make sure external function calls can't be used to trigger calls to // completeOutboundTransfer. We only make external (write) calls to _localToken. @@ -343,7 +346,7 @@ abstract contract StandardBridge { _to, _amount, _minGasLimit, - _data + _extraData ); } @@ -357,7 +360,7 @@ abstract contract StandardBridge { address _to, uint256 _amount, uint32 _minGasLimit, - bytes calldata _data + bytes calldata _extraData ) internal { messenger.sendMessage( address(otherBridge), @@ -371,12 +374,12 @@ abstract contract StandardBridge { _from, _to, _amount, - _data + _extraData ), _minGasLimit ); - emit ERC20BridgeInitiated(_localToken, _remoteToken, _from, _to, _amount, _data); + emit ERC20BridgeInitiated(_localToken, _remoteToken, _from, _to, _amount, _extraData); } /**