From 98d0e06e28698fbdff74a4f0f98660e750f900fd Mon Sep 17 00:00:00 2001 From: Maurelian Date: Sat, 28 May 2022 22:08:46 -0400 Subject: [PATCH 1/7] contracts: Fix OZ-M-01 refund arguments --- .../contracts/test/L2StandardBridge.t.sol | 19 +++++++++++++++++-- .../contracts/universal/StandardBridge.sol | 4 +++- 2 files changed, 20 insertions(+), 3 deletions(-) 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/StandardBridge.sol b/packages/contracts-bedrock/contracts/universal/StandardBridge.sol index d861f71953fdc..0b3849267dd39 100644 --- a/packages/contracts-bedrock/contracts/universal/StandardBridge.sol +++ b/packages/contracts-bedrock/contracts/universal/StandardBridge.sol @@ -236,11 +236,13 @@ abstract contract StandardBridge { } 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 From 04803ac804771bb3424550f38d975753c28aaa01 Mon Sep 17 00:00:00 2001 From: Maurelian Date: Tue, 31 May 2022 16:19:45 -0400 Subject: [PATCH 2/7] contracts: Fix OZ-M-02 by removing donateEth() --- .../contracts/test/L1StandardBridge.t.sol | 9 --------- .../contracts/universal/StandardBridge.sol | 5 ----- 2 files changed, 14 deletions(-) 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/universal/StandardBridge.sol b/packages/contracts-bedrock/contracts/universal/StandardBridge.sol index 0b3849267dd39..a1ad29f960fd1 100644 --- a/packages/contracts-bedrock/contracts/universal/StandardBridge.sol +++ b/packages/contracts-bedrock/contracts/universal/StandardBridge.sol @@ -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. From 762092f29ddab1df2ff38e1746016d98d004c969 Mon Sep 17 00:00:00 2001 From: Maurelian Date: Wed, 1 Jun 2022 11:34:05 -0400 Subject: [PATCH 3/7] contracts: Fix OZ-M-04 Disallow reentrant withdrawals --- packages/contracts-bedrock/.gas-snapshot | 24 +++++++++---------- .../contracts/L1/OptimismPortal.sol | 2 +- 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/packages/contracts-bedrock/.gas-snapshot b/packages/contracts-bedrock/.gas-snapshot index 7fc1b5101946d..36c88292689a2 100644 --- a/packages/contracts-bedrock/.gas-snapshot +++ b/packages/contracts-bedrock/.gas-snapshot @@ -33,12 +33,11 @@ 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_depositERC20() (gas: 452837) +L1StandardBridge_Test:test_depositERC20To() (gas: 454614) L1StandardBridge_Test:test_depositETH() (gas: 247054) L1StandardBridge_Test:test_depositETHTo() (gas: 204938) -L1StandardBridge_Test:test_donateETH() (gas: 17545) -L1StandardBridge_Test:test_finalizeERC20Withdrawal() (gas: 438824) +L1StandardBridge_Test:test_finalizeERC20Withdrawal() (gas: 438752) L1StandardBridge_Test:test_finalizeETHWithdrawal() (gas: 48005) L1StandardBridge_Test:test_initialize() (gas: 14885) L1StandardBridge_Test:test_onlyEOADepositERC20() (gas: 12085) @@ -71,14 +70,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_ERC20BridgeFailed_whenLocalTokenIsBridge() (gas: 133074) +L2StandardBridge_Test:test_cannotWithdrawEthWithoutSendingIt() (gas: 21611) +L2StandardBridge_Test:test_finalizeDeposit() (gas: 93100) 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_initialize() (gas: 14856) +L2StandardBridge_Test:test_receive() (gas: 136392) +L2StandardBridge_Test:test_withdraw() (gas: 352742) +L2StandardBridge_Test:test_withdrawTo() (gas: 353463) +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 +164,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: 147278) 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" From ba109a7eeb9fa4e57bcea582948de5b1465eca01 Mon Sep 17 00:00:00 2001 From: Maurelian Date: Wed, 1 Jun 2022 13:27:05 -0400 Subject: [PATCH 4/7] contracts: Fix OZ-M-05 rename _data to _extraData This commit also expands on the natspec comments to clarify how the additional data may be used. fixup! contracts: Fix OZ-M-05 rename _data to _extraData --- .../contracts/L1/L1StandardBridge.sol | 128 ++++++++++-------- .../contracts/L2/L2StandardBridge.sol | 84 ++++++------ .../contracts/universal/StandardBridge.sol | 60 ++++---- 3 files changed, 143 insertions(+), 129 deletions(-) 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/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/universal/StandardBridge.sol b/packages/contracts-bedrock/contracts/universal/StandardBridge.sol index a1ad29f960fd1..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 ); /************* @@ -139,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); } /** @@ -150,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); } /** @@ -163,7 +163,7 @@ abstract contract StandardBridge { address _remoteToken, uint256 _amount, uint32 _minGasLimit, - bytes calldata _data + bytes calldata _extraData ) public virtual onlyEOA { _initiateBridgeERC20( _localToken, @@ -172,7 +172,7 @@ abstract contract StandardBridge { msg.sender, _amount, _minGasLimit, - _data + _extraData ); } @@ -185,7 +185,7 @@ abstract contract StandardBridge { address _to, uint256 _amount, uint32 _minGasLimit, - bytes calldata _data + bytes calldata _extraData ) public virtual { _initiateBridgeERC20( _localToken, @@ -194,7 +194,7 @@ abstract contract StandardBridge { _to, _amount, _minGasLimit, - _data + _extraData ); } @@ -205,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."); } @@ -224,10 +224,10 @@ 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. @@ -240,9 +240,9 @@ abstract contract StandardBridge { _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); } } @@ -293,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 ); } @@ -314,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. @@ -340,7 +346,7 @@ abstract contract StandardBridge { _to, _amount, _minGasLimit, - _data + _extraData ); } @@ -354,7 +360,7 @@ abstract contract StandardBridge { address _to, uint256 _amount, uint32 _minGasLimit, - bytes calldata _data + bytes calldata _extraData ) internal { messenger.sendMessage( address(otherBridge), @@ -368,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); } /** From 972c023dd509d57f290ebd73877abbdd44dbb648 Mon Sep 17 00:00:00 2001 From: Maurelian Date: Sat, 28 May 2022 20:14:03 -0400 Subject: [PATCH 5/7] contracts: Fix OZ-M-06 - Unpause functionality --- packages/contracts-bedrock/.gas-snapshot | 52 ++++++++++--------- .../test/L1CrossDomainMessenger.t.sol | 15 ++++++ .../universal/CrossDomainMessenger.sol | 7 +++ 3 files changed, 49 insertions(+), 25 deletions(-) diff --git a/packages/contracts-bedrock/.gas-snapshot b/packages/contracts-bedrock/.gas-snapshot index 36c88292689a2..4fbd36d97d290 100644 --- a/packages/contracts-bedrock/.gas-snapshot +++ b/packages/contracts-bedrock/.gas-snapshot @@ -23,20 +23,22 @@ 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: 452837) -L1StandardBridge_Test:test_depositERC20To() (gas: 454614) -L1StandardBridge_Test:test_depositETH() (gas: 247054) -L1StandardBridge_Test:test_depositETHTo() (gas: 204938) +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) @@ -44,17 +46,17 @@ 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) @@ -70,14 +72,14 @@ L2OutputOracleTest:test_deleteL2Output() (gas: 66081) L2OutputOracleTest:test_getL2Output() (gas: 76274) L2OutputOracleTest:test_latestBlockNumber() (gas: 70075) L2OutputOracleTest:test_nextBlockNumber() (gas: 9279) -L2StandardBridge_Test:test_ERC20BridgeFailed_whenLocalTokenIsBridge() (gas: 133074) +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: 140106) +L2StandardBridge_Test:test_finalizeDeposit_failsToCompleteOutboundTransfer() (gas: 140129) L2StandardBridge_Test:test_initialize() (gas: 14856) -L2StandardBridge_Test:test_receive() (gas: 136392) -L2StandardBridge_Test:test_withdraw() (gas: 352742) -L2StandardBridge_Test:test_withdrawTo() (gas: 353463) +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) @@ -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: 147278) +SequencerFeeVault_Test:test_withdraw() (gas: 147301) 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/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). From 650ca6d46178bd2737879b72b0f43b8e5c1aa0c4 Mon Sep 17 00:00:00 2001 From: Maurelian Date: Mon, 13 Jun 2022 14:15:57 -0400 Subject: [PATCH 6/7] chore: Add changeset and regenerate bindings --- .changeset/three-pants-punch.md | 9 +++++++++ 1 file changed, 9 insertions(+) create mode 100644 .changeset/three-pants-punch.md diff --git a/.changeset/three-pants-punch.md b/.changeset/three-pants-punch.md new file mode 100644 index 0000000000000..2a9f0d1412ddb --- /dev/null +++ b/.changeset/three-pants-punch.md @@ -0,0 +1,9 @@ +--- +'@eth-optimism/contracts-bedrock': minor +--- + +Fixes to medium severity OZ findings + +- Disallow reentrant withdrawals +- remove donateEth +- Correct ordering of \_from and \_to arguments on refunds of failed deposits From 365924350e75b1b913b8845d12395fc3b45c6191 Mon Sep 17 00:00:00 2001 From: Maurelian Date: Thu, 16 Jun 2022 00:06:36 -0400 Subject: [PATCH 7/7] chore: downgrade to patch --- .changeset/three-pants-punch.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/three-pants-punch.md b/.changeset/three-pants-punch.md index 2a9f0d1412ddb..46842a32cb696 100644 --- a/.changeset/three-pants-punch.md +++ b/.changeset/three-pants-punch.md @@ -1,5 +1,5 @@ --- -'@eth-optimism/contracts-bedrock': minor +'@eth-optimism/contracts-bedrock': patch --- Fixes to medium severity OZ findings