From 9ddffa5b7de52ccb7ebfef6b5a3c388fea939649 Mon Sep 17 00:00:00 2001 From: Maurelian Date: Thu, 2 Jun 2022 15:40:44 -0400 Subject: [PATCH 1/6] contracts: Fix OZ-N-03 Complex ERC165 implementation fixup! contracts: Fix OZ-N-03 Complex ERC165 implementation --- contracts-bedrock/.gas-snapshot | 6 +++--- .../contracts/test/OptimismMintableERC20.t.sol | 4 ++-- .../contracts/universal/OptimismMintableERC20.sol | 8 +++----- contracts-bedrock/contracts/universal/StandardBridge.sol | 2 +- .../contracts/universal/SupportedInterfaces.sol | 4 ++-- 5 files changed, 11 insertions(+), 13 deletions(-) diff --git a/contracts-bedrock/.gas-snapshot b/contracts-bedrock/.gas-snapshot index 2ca8b6a588242..3c750138b08bd 100644 --- a/contracts-bedrock/.gas-snapshot +++ b/contracts-bedrock/.gas-snapshot @@ -55,11 +55,11 @@ L2OutputOracleTest:test_deleteL2Output() (gas: 64320) L2OutputOracleTest:test_getL2Output() (gas: 74601) L2OutputOracleTest:test_latestBlockTimestamp() (gas: 68377) L2OutputOracleTest:test_nextTimestamp() (gas: 9236) -L2StandardBridge_Test:test_finalizeDeposit() (gas: 93169) +L2StandardBridge_Test:test_finalizeDeposit() (gas: 93143) L2StandardBridge_Test:test_initialize() (gas: 14812) L2StandardBridge_Test:test_receive() (gas: 136437) -L2StandardBridge_Test:test_withdraw() (gas: 352632) -L2StandardBridge_Test:test_withdrawTo() (gas: 353501) +L2StandardBridge_Test:test_withdraw() (gas: 352612) +L2StandardBridge_Test:test_withdrawTo() (gas: 353480) L2ToL1MessagePasserTest:test_burn() (gas: 112001) L2ToL1MessagePasserTest:test_initiateWithdrawal_fromContract() (gas: 67935) L2ToL1MessagePasserTest:test_initiateWithdrawal_fromEOA() (gas: 74851) diff --git a/contracts-bedrock/contracts/test/OptimismMintableERC20.t.sol b/contracts-bedrock/contracts/test/OptimismMintableERC20.t.sol index cc7f460d70839..3e70192ccab58 100644 --- a/contracts-bedrock/contracts/test/OptimismMintableERC20.t.sol +++ b/contracts-bedrock/contracts/test/OptimismMintableERC20.t.sol @@ -76,12 +76,12 @@ contract OptimismMintableTokenFactory_Test is Bridge_Initializer { emit log_bytes32(bytes32(iface1)); bytes4 iface2 = L2Token.l1Token.selector ^ L2Token.mint.selector ^ L2Token.burn.selector; - assertEq(iface2, type(L1TokenId).interfaceId); + assertEq(iface2, type(IL1Token).interfaceId); assert(L2Token.supportsInterface(iface2)); emit log_bytes32(bytes32(iface2)); bytes4 iface3 = L2Token.remoteToken.selector ^ L2Token.mint.selector ^ L2Token.burn.selector; - assertEq(iface3, type(RemoteTokenId).interfaceId); + assertEq(iface3, type(IRemoteToken).interfaceId); assert(L2Token.supportsInterface(iface3)); emit log_bytes32(bytes32(iface3)); diff --git a/contracts-bedrock/contracts/universal/OptimismMintableERC20.sol b/contracts-bedrock/contracts/universal/OptimismMintableERC20.sol index 8d7ff1afbb651..e9e1a46bcde3c 100644 --- a/contracts-bedrock/contracts/universal/OptimismMintableERC20.sol +++ b/contracts-bedrock/contracts/universal/OptimismMintableERC20.sol @@ -73,11 +73,9 @@ contract OptimismMintableERC20 is ERC20 { */ // slither-disable-next-line external-function function supportsInterface(bytes4 _interfaceId) public pure returns (bool) { - // Interfaces are ordered based on how often we expect each one to be queried for a small - // amount of gas savings. - bytes4 iface1 = type(L1TokenId).interfaceId; - bytes4 iface2 = type(RemoteTokenId).interfaceId; - bytes4 iface3 = type(IERC165).interfaceId; + bytes4 iface1 = type(IERC165).interfaceId; + bytes4 iface2 = type(IL1Token).interfaceId; + bytes4 iface3 = type(IRemoteToken).interfaceId; return _interfaceId == iface1 || _interfaceId == iface2 || _interfaceId == iface3; } diff --git a/contracts-bedrock/contracts/universal/StandardBridge.sol b/contracts-bedrock/contracts/universal/StandardBridge.sol index da8090d47fc1c..6191c1c7b6c12 100644 --- a/contracts-bedrock/contracts/universal/StandardBridge.sol +++ b/contracts-bedrock/contracts/universal/StandardBridge.sol @@ -384,7 +384,7 @@ abstract contract StandardBridge { * @return True if the token is an OptimismMintableERC20. */ function _isOptimismMintableERC20(address _token) internal view returns (bool) { - return ERC165Checker.supportsInterface(_token, type(L1TokenId).interfaceId); + return ERC165Checker.supportsInterface(_token, type(IL1Token).interfaceId); } /** diff --git a/contracts-bedrock/contracts/universal/SupportedInterfaces.sol b/contracts-bedrock/contracts/universal/SupportedInterfaces.sol index e35c5f54cdd8c..a616b92ccd7a1 100644 --- a/contracts-bedrock/contracts/universal/SupportedInterfaces.sol +++ b/contracts-bedrock/contracts/universal/SupportedInterfaces.sol @@ -4,7 +4,7 @@ pragma solidity ^0.8.9; // Import this here to make it available just by importing this file import { IERC165 } from "@openzeppelin/contracts/utils/introspection/IERC165.sol"; -interface RemoteTokenId { +interface IRemoteToken { function mint(address _to, uint256 _amount) external virtual; function burn(address _from, uint256 _amount) external virtual; @@ -12,7 +12,7 @@ interface RemoteTokenId { function remoteToken() external virtual; } -interface L1TokenId { +interface IL1Token { function mint(address _to, uint256 _amount) external virtual; function burn(address _from, uint256 _amount) external virtual; From bd0e3306001a3e7ef5283024499cdde4d9b0eecb Mon Sep 17 00:00:00 2001 From: Maurelian Date: Sat, 28 May 2022 22:08:46 -0400 Subject: [PATCH 2/6] contracts: Fix OZ-M-01 refund arguments --- .../contracts/test/CommonTest.t.sol | 9 +++++++++ .../contracts/test/L2StandardBridge.t.sol | 17 +++++++++++++++++ .../contracts/universal/StandardBridge.sol | 4 +++- 3 files changed, 29 insertions(+), 1 deletion(-) diff --git a/contracts-bedrock/contracts/test/CommonTest.t.sol b/contracts-bedrock/contracts/test/CommonTest.t.sol index 353ce619f430f..fccb3b60333a1 100644 --- a/contracts-bedrock/contracts/test/CommonTest.t.sol +++ b/contracts-bedrock/contracts/test/CommonTest.t.sol @@ -254,6 +254,15 @@ contract Bridge_Initializer is Messenger_Initializer { bytes _data ); + event ERC20BridgeFailed( + address indexed _localToken, + address indexed _remoteToken, + address indexed _from, + address _to, + uint256 _amount, + bytes _data + ); + function setUp() public virtual override { super.setUp(); diff --git a/contracts-bedrock/contracts/test/L2StandardBridge.t.sol b/contracts-bedrock/contracts/test/L2StandardBridge.t.sol index 2ce6a036090fb..1e73676d59d88 100644 --- a/contracts-bedrock/contracts/test/L2StandardBridge.t.sol +++ b/contracts-bedrock/contracts/test/L2StandardBridge.t.sol @@ -109,5 +109,22 @@ 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/contracts-bedrock/contracts/universal/StandardBridge.sol b/contracts-bedrock/contracts/universal/StandardBridge.sol index 6191c1c7b6c12..51a14b364edcc 100644 --- a/contracts-bedrock/contracts/universal/StandardBridge.sol +++ b/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( _remoteToken, _localToken, - _from, _to, + _from, _amount, 0, // _minGasLimit, 0 is fine here _data From 470ab7db97a8c6ca96e3df775b313ac002b347eb Mon Sep 17 00:00:00 2001 From: Maurelian Date: Tue, 31 May 2022 16:19:45 -0400 Subject: [PATCH 3/6] contracts: Fix OZ-M-02 by removing donateEth() --- contracts-bedrock/.gas-snapshot | 26 +++++++++---------- .../contracts/test/L1StandardBridge.t.sol | 9 ------- .../contracts/universal/StandardBridge.sol | 5 ---- 3 files changed, 13 insertions(+), 27 deletions(-) diff --git a/contracts-bedrock/.gas-snapshot b/contracts-bedrock/.gas-snapshot index 3c750138b08bd..55fa272d304b1 100644 --- a/contracts-bedrock/.gas-snapshot +++ b/contracts-bedrock/.gas-snapshot @@ -17,18 +17,17 @@ L1CrossDomainMessenger_Test:test_L1MessengerSendMessage() (gas: 78105) L1CrossDomainMessenger_Test:test_L1MessengerTwiceSendMessage() (gas: 66345) L1CrossDomainMessenger_Test:test_L1MessengerXDomainSenderReverts() (gas: 10588) L1CrossDomainMessenger_Test:test_L1MessengerxDomainMessageSenderResets() (gas: 58489) -L1StandardBridge_Test:test_depositERC20() (gas: 371479) -L1StandardBridge_Test:test_depositERC20To() (gas: 373256) -L1StandardBridge_Test:test_depositETH() (gas: 105126) +L1StandardBridge_Test:test_depositERC20() (gas: 371443) +L1StandardBridge_Test:test_depositERC20To() (gas: 373220) +L1StandardBridge_Test:test_depositETH() (gas: 105061) L1StandardBridge_Test:test_depositETHTo() (gas: 111945) -L1StandardBridge_Test:test_donateETH() (gas: 17523) -L1StandardBridge_Test:test_finalizeERC20Withdrawal() (gas: 438817) -L1StandardBridge_Test:test_finalizeETHWithdrawal() (gas: 47983) -L1StandardBridge_Test:test_initialize() (gas: 14863) +L1StandardBridge_Test:test_finalizeERC20Withdrawal() (gas: 438745) +L1StandardBridge_Test:test_finalizeETHWithdrawal() (gas: 47960) +L1StandardBridge_Test:test_initialize() (gas: 14885) L1StandardBridge_Test:test_onlyEOADepositERC20() (gas: 12085) L1StandardBridge_Test:test_onlyEOADepositETH() (gas: 30637) L1StandardBridge_Test:test_onlyL2BridgeFinalizeERC20Withdrawal() (gas: 23565) -L1StandardBridge_Test:test_onlyPortalFinalizeERC20Withdrawal() (gas: 22897) +L1StandardBridge_Test:test_onlyPortalFinalizeERC20Withdrawal() (gas: 22919) L1StandardBridge_Test:test_receive() (gas: 99476) L2CrossDomainMessenger_Test:testCannot_L2MessengerPause() (gas: 10843) L2CrossDomainMessenger_Test:test_L2MessengerMessageVersion() (gas: 8410) @@ -55,11 +54,12 @@ L2OutputOracleTest:test_deleteL2Output() (gas: 64320) L2OutputOracleTest:test_getL2Output() (gas: 74601) L2OutputOracleTest:test_latestBlockTimestamp() (gas: 68377) L2OutputOracleTest:test_nextTimestamp() (gas: 9236) -L2StandardBridge_Test:test_finalizeDeposit() (gas: 93143) -L2StandardBridge_Test:test_initialize() (gas: 14812) -L2StandardBridge_Test:test_receive() (gas: 136437) -L2StandardBridge_Test:test_withdraw() (gas: 352612) -L2StandardBridge_Test:test_withdrawTo() (gas: 353480) +L2StandardBridge_Test:test_ERC20BridgeFailed_whenLocalTokenIsBridge() (gas: 133074) +L2StandardBridge_Test:test_finalizeDeposit() (gas: 93165) +L2StandardBridge_Test:test_initialize() (gas: 14834) +L2StandardBridge_Test:test_receive() (gas: 136392) +L2StandardBridge_Test:test_withdraw() (gas: 352629) +L2StandardBridge_Test:test_withdrawTo() (gas: 353463) L2ToL1MessagePasserTest:test_burn() (gas: 112001) L2ToL1MessagePasserTest:test_initiateWithdrawal_fromContract() (gas: 67935) L2ToL1MessagePasserTest:test_initiateWithdrawal_fromEOA() (gas: 74851) diff --git a/contracts-bedrock/contracts/test/L1StandardBridge.t.sol b/contracts-bedrock/contracts/test/L1StandardBridge.t.sol index 6eeb1537c7727..706f2a9149130 100644 --- a/contracts-bedrock/contracts/test/L1StandardBridge.t.sol +++ b/contracts-bedrock/contracts/test/L1StandardBridge.t.sol @@ -394,13 +394,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/contracts-bedrock/contracts/universal/StandardBridge.sol b/contracts-bedrock/contracts/universal/StandardBridge.sol index 51a14b364edcc..c1483063b4111 100644 --- a/contracts-bedrock/contracts/universal/StandardBridge.sol +++ b/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 b73bb26ee58e32b641b2897284b927586d70b493 Mon Sep 17 00:00:00 2001 From: Maurelian Date: Wed, 1 Jun 2022 11:34:05 -0400 Subject: [PATCH 4/6] contracts: Fix OZ-M-04 Disallow reentrant withdrawals --- contracts-bedrock/contracts/L1/OptimismPortal.sol | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/contracts-bedrock/contracts/L1/OptimismPortal.sol b/contracts-bedrock/contracts/L1/OptimismPortal.sol index 03e7e50f5d40f..1970ba092ab60 100644 --- a/contracts-bedrock/contracts/L1/OptimismPortal.sol +++ b/contracts-bedrock/contracts/L1/OptimismPortal.sol @@ -182,7 +182,11 @@ contract OptimismPortal { WithdrawalVerifier.OutputRootProof calldata _outputRootProof, bytes calldata _withdrawalProof ) external payable { - // Prevent reentrency + // Prevent immediate reentrancy to other functions on this contract. + // Note that reentrancy to _this_ function is also prevented by the l2Sender check below, + // but this check is also an efficient mechanism of preventing the portal from calling + // its own receive() or depositTransaction() functions, without the need for a nonReentrant + // modifier. require(_target != address(this), "Cannot send message to self."); // Get the output root. @@ -237,6 +241,13 @@ contract OptimismPortal { "Insufficient gas to finalize withdrawal." ); + + // Disallow withdrawals during reentrancy to this function. + require( + l2Sender == DEFAULT_L2_SENDER, + "Cannot withdraw a transaction within another withdrawal transaction." + ); + // Set the l2Sender so that other contracts can know which account // on L2 is making the withdrawal l2Sender = _sender; From 1a64b9bc4db612030fe924cf2befeb6fc2e08bca Mon Sep 17 00:00:00 2001 From: Maurelian Date: Wed, 1 Jun 2022 13:27:05 -0400 Subject: [PATCH 5/6] 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 | 83 +++++++++++-------- .../contracts/L1/OptimismPortal.sol | 1 - .../contracts/L2/L2StandardBridge.sol | 45 ++++++---- .../contracts/universal/StandardBridge.sol | 60 ++++++++------ 4 files changed, 107 insertions(+), 82 deletions(-) diff --git a/contracts-bedrock/contracts/L1/L1StandardBridge.sol b/contracts-bedrock/contracts/L1/L1StandardBridge.sol index e0a5dd7382d5a..513c81900e057 100644 --- a/contracts-bedrock/contracts/L1/L1StandardBridge.sol +++ b/contracts-bedrock/contracts/L1/L1StandardBridge.sol @@ -24,14 +24,14 @@ contract L1StandardBridge is StandardBridge { address indexed _from, address indexed _to, uint256 _amount, - bytes _data + bytes _extraData ); event ETHWithdrawalFinalized( address indexed _from, address indexed _to, uint256 _amount, - bytes _data + bytes _extraData ); event ERC20DepositInitiated( @@ -40,7 +40,7 @@ contract L1StandardBridge is StandardBridge { address indexed _from, address _to, uint256 _amount, - bytes _data + bytes _extraData ); event ERC20WithdrawalFinalized( @@ -49,7 +49,7 @@ contract L1StandardBridge is StandardBridge { address indexed _from, address _to, uint256 _amount, - bytes _data + bytes _extraData ); /******************** @@ -76,12 +76,12 @@ contract L1StandardBridge is StandardBridge { /** * @dev Deposit an amount of the ETH to the caller's balance on L2. * @param _minGasLimit limit required to complete the deposit on L2. - * @param _data Optional data to forward to L2. This data is provided - * solely as a convenience for external contracts. Aside from enforcing a maximum - * length, these contracts provide no guarantees about its content. + * @param _extraData Optional data to forward to L2. This data is provided solely as a + * convenience for external contracts which may validate that the data is included in the + * CrossDomainMessenger's sentMessages mapping. */ - function depositETH(uint32 _minGasLimit, bytes calldata _data) external payable onlyEOA { - _initiateETHDeposit(msg.sender, msg.sender, _minGasLimit, _data); + function depositETH(uint32 _minGasLimit, bytes calldata _extraData) external payable onlyEOA { + _initiateETHDeposit(msg.sender, msg.sender, _minGasLimit, _extraData); } /** @@ -89,16 +89,16 @@ contract L1StandardBridge is StandardBridge { * contract on L2 and the call fails, then that ETH will be locked in the L2StandardBridge. * @param _to L2 address to credit the deposit to. * @param _minGasLimit Gas limit required to complete the deposit on L2. - * @param _data Optional data to forward to L2. This data is provided - * solely as a convenience for external contracts. Aside from enforcing a maximum - * length, these contracts provide no guarantees about its content. + * @param _extraData Optional data to forward to L2. This data is provided solely as a + * convenience for external contracts which may validate that the data is included in the + * CrossDomainMessenger's sentMessages mapping. */ function depositETHTo( address _to, uint32 _minGasLimit, - bytes calldata _data + bytes calldata _extraData ) external payable { - _initiateETHDeposit(msg.sender, _to, _minGasLimit, _data); + _initiateETHDeposit(msg.sender, _to, _minGasLimit, _extraData); } /** @@ -107,16 +107,17 @@ contract L1StandardBridge is StandardBridge { * @param _l2Token Address of the L2 token we are depositing to. * @param _amount Amount of the ERC20 to deposit. * @param _minGasLimit limit required to complete the deposit on L2. - * @param _data Optional data to forward to L2. This data is provided - * solely as a convenience for external contracts. Aside from enforcing a maximum - * length, these contracts provide no guarantees about its content. + * @param _extraData Optional data to forward to L2. This data is not forwarded to the + * token contract and is provided solely as a convenience for external contracts + * which may validate that the data is included in the CrossDomainMessenger's + * sentMessages mapping. */ function depositERC20( address _l1Token, address _l2Token, uint256 _amount, uint32 _minGasLimit, - bytes calldata _data + bytes calldata _extraData ) external virtual onlyEOA { _initiateERC20Deposit( _l1Token, @@ -125,7 +126,7 @@ contract L1StandardBridge is StandardBridge { msg.sender, _amount, _minGasLimit, - _data + _extraData ); } @@ -136,9 +137,10 @@ contract L1StandardBridge is StandardBridge { * @param _to L2 address to credit the deposit to. * @param _amount Amount of the ERC20 to deposit. * @param _minGasLimit Gas limit required to complete the deposit on L2. - * @param _data Optional data to forward to L2. This data is provided - * solely as a convenience for external contracts. Aside from enforcing a maximum - * length, these contracts provide no guarantees about its content. + * @param _extraData Optional data to forward to L2. This data is not forwarded to the + * token contract and is provided solely as a convenience for external contracts + * which may validate that the data is included in the CrossDomainMessenger's + * sentMessages mapping. */ function depositERC20To( address _l1Token, @@ -146,9 +148,17 @@ 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 + ); } function finalizeETHWithdrawal( @@ -171,9 +181,10 @@ contract L1StandardBridge is StandardBridge { * @param _from L2 address initiating the transfer. * @param _to L1 address to credit the withdrawal to. * @param _amount Amount of the ERC20 to deposit. - * @param _data Data provided by the sender on L2. This data is provided - * solely as a convenience for external contracts. Aside from enforcing a maximum - * length, these contracts provide no guarantees about its content. + * @param _extraData Data provided by the sender on L2. This data is not forwarded to the + * token contract. It is provided solely as a convenience for external contracts which + * may validate that the data is included in the CrossDomainMessenger's sentMessages + * mapping. */ function finalizeERC20Withdrawal( address _l1Token, @@ -181,10 +192,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); } /********************** @@ -195,10 +206,10 @@ contract L1StandardBridge is StandardBridge { address _from, address _to, uint32 _minGasLimit, - bytes memory _data + bytes memory _extraData ) internal { - emit ETHDepositInitiated(_from, _to, msg.value, _data); - _initiateBridgeETH(_from, _to, msg.value, _minGasLimit, _data); + emit ETHDepositInitiated(_from, _to, msg.value, _extraData); + _initiateBridgeETH(_from, _to, msg.value, _minGasLimit, _extraData); } function _initiateERC20Deposit( @@ -208,9 +219,9 @@ contract L1StandardBridge is StandardBridge { address _to, uint256 _amount, uint32 _minGasLimit, - bytes calldata _data + bytes calldata _extraData ) internal { - emit ERC20DepositInitiated(_l1Token, _l2Token, _from, _to, _amount, _data); - _initiateBridgeERC20(_l1Token, _l2Token, _from, _to, _amount, _minGasLimit, _data); + emit ERC20DepositInitiated(_l1Token, _l2Token, _from, _to, _amount, _extraData); + _initiateBridgeERC20(_l1Token, _l2Token, _from, _to, _amount, _minGasLimit, _extraData); } } diff --git a/contracts-bedrock/contracts/L1/OptimismPortal.sol b/contracts-bedrock/contracts/L1/OptimismPortal.sol index 1970ba092ab60..77af021e8ef51 100644 --- a/contracts-bedrock/contracts/L1/OptimismPortal.sol +++ b/contracts-bedrock/contracts/L1/OptimismPortal.sol @@ -241,7 +241,6 @@ contract OptimismPortal { "Insufficient gas to finalize withdrawal." ); - // Disallow withdrawals during reentrancy to this function. require( l2Sender == DEFAULT_L2_SENDER, diff --git a/contracts-bedrock/contracts/L2/L2StandardBridge.sol b/contracts-bedrock/contracts/L2/L2StandardBridge.sol index c4214d9a64af2..8df2e7893427d 100644 --- a/contracts-bedrock/contracts/L2/L2StandardBridge.sol +++ b/contracts-bedrock/contracts/L2/L2StandardBridge.sol @@ -27,7 +27,7 @@ contract L2StandardBridge is StandardBridge { address indexed _from, address _to, uint256 _amount, - bytes _data + bytes _extraData ); event DepositFinalized( @@ -36,7 +36,7 @@ contract L2StandardBridge is StandardBridge { address indexed _from, address _to, uint256 _amount, - bytes _data + bytes _extraData ); event DepositFailed( @@ -45,7 +45,7 @@ contract L2StandardBridge is StandardBridge { address indexed _from, address _to, uint256 _amount, - bytes _data + bytes _extraData ); /******************** @@ -65,15 +65,18 @@ contract L2StandardBridge is StandardBridge { * @param _l2Token The L2 token address to withdraw * @param _amount The amount of L2 token to withdraw * @param _minGasLimit The min gas limit in the withdrawing call - * @param _data Additional calldata to pass along + * @param _extraData Optional data to forward to L1. This data is not forwarded to the + * token contract. It is provided solely as a convenience for external contracts which + * may validate that the data is included in the CrossDomainMessenger's sentMessages + * mapping. */ function withdraw( address _l2Token, uint256 _amount, uint32 _minGasLimit, - bytes calldata _data + bytes calldata _extraData ) external payable virtual { - _initiateWithdrawal(_l2Token, msg.sender, msg.sender, _amount, _minGasLimit, _data); + _initiateWithdrawal(_l2Token, msg.sender, msg.sender, _amount, _minGasLimit, _extraData); } /** @@ -84,17 +87,20 @@ contract L2StandardBridge is StandardBridge { * @param _to The L1 account to withdraw to * @param _amount The amount of L2 token to withdraw * @param _minGasLimit The min gas limit in the withdrawing call - * @param _data Additional calldata to pass along + * @param _extraData Optional data to forward to L1. This data is not forwarded to the + * token contract. It is provided solely as a convenience for external contracts which + * may validate that the data is included in the CrossDomainMessenger's sentMessages + * mapping. */ function withdrawTo( address _l2Token, address _to, uint256 _amount, uint32 _minGasLimit, - bytes calldata _data + bytes calldata _extraData ) external payable virtual { // TODO: add onlyEOA check on ETH withdrawals to match L1Bridge.depositETHTo? - _initiateWithdrawal(_l2Token, msg.sender, _to, _amount, _minGasLimit, _data); + _initiateWithdrawal(_l2Token, msg.sender, _to, _amount, _minGasLimit, _extraData); } /** @@ -105,7 +111,10 @@ contract L2StandardBridge is StandardBridge { * @param _from The sender of the tokens * @param _to The recipient of the tokens * @param _amount The amount of tokens - * @param _data Additional calldata + * @param _extraData Data provided by the sender on L1. This data is not forwarded to the + * token contract. It is provided solely as a convenience for external contracts which + * may validate that the data is included in the CrossDomainMessenger's sentMessages + * mapping. */ function finalizeDeposit( address _l1Token, @@ -113,14 +122,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); } /********************** @@ -137,15 +146,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/contracts-bedrock/contracts/universal/StandardBridge.sol b/contracts-bedrock/contracts/universal/StandardBridge.sol index c1483063b4111..9c0ae2cb0d7da 100644 --- a/contracts-bedrock/contracts/universal/StandardBridge.sol +++ b/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), @@ -365,12 +371,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 c73da942bb0617ff407343d76c75870566b59698 Mon Sep 17 00:00:00 2001 From: Maurelian Date: Sat, 28 May 2022 20:14:03 -0400 Subject: [PATCH 6/6] contracts: Fix OZ-M-06 - Unpause functionality --- contracts-bedrock/.gas-snapshot | 46 ++++++++++--------- .../test/L1CrossDomainMessenger.t.sol | 15 ++++++ .../universal/CrossDomainMessenger.sol | 7 +++ 3 files changed, 46 insertions(+), 22 deletions(-) diff --git a/contracts-bedrock/.gas-snapshot b/contracts-bedrock/.gas-snapshot index 55fa272d304b1..fe89909dc3888 100644 --- a/contracts-bedrock/.gas-snapshot +++ b/contracts-bedrock/.gas-snapshot @@ -7,20 +7,22 @@ L1BlockTest:test_timestamp() (gas: 7661) L1BlockNumberTest:test_fallback() (gas: 10710) L1BlockNumberTest:test_getL1BlockNumber() (gas: 10589) L1BlockNumberTest:test_receive() (gas: 17440) -L1CrossDomainMessenger_Test:testCannot_L1MessengerPause() (gas: 10909) -L1CrossDomainMessenger_Test:test_L1MessengerMessageVersion() (gas: 8366) -L1CrossDomainMessenger_Test:test_L1MessengerPause() (gas: 31882) -L1CrossDomainMessenger_Test:test_L1MessengerRelayMessageSucceeds() (gas: 61193) -L1CrossDomainMessenger_Test:test_L1MessengerRelayMessageToSystemContract() (gas: 44815) -L1CrossDomainMessenger_Test:test_L1MessengerRelayShouldRevertIfPaused() (gas: 41631) -L1CrossDomainMessenger_Test:test_L1MessengerSendMessage() (gas: 78105) -L1CrossDomainMessenger_Test:test_L1MessengerTwiceSendMessage() (gas: 66345) -L1CrossDomainMessenger_Test:test_L1MessengerXDomainSenderReverts() (gas: 10588) -L1CrossDomainMessenger_Test:test_L1MessengerxDomainMessageSenderResets() (gas: 58489) -L1StandardBridge_Test:test_depositERC20() (gas: 371443) -L1StandardBridge_Test:test_depositERC20To() (gas: 373220) -L1StandardBridge_Test:test_depositETH() (gas: 105061) -L1StandardBridge_Test:test_depositETHTo() (gas: 111945) +L1CrossDomainMessenger_Test:testCannot_L1MessengerPause() (gas: 10844) +L1CrossDomainMessenger_Test:testCannot_L1MessengerUnpause() (gas: 10858) +L1CrossDomainMessenger_Test:test_L1MessengerMessageVersion() (gas: 8388) +L1CrossDomainMessenger_Test:test_L1MessengerPause() (gas: 31815) +L1CrossDomainMessenger_Test:test_L1MessengerRelayMessageSucceeds() (gas: 61215) +L1CrossDomainMessenger_Test:test_L1MessengerRelayMessageToSystemContract() (gas: 44837) +L1CrossDomainMessenger_Test:test_L1MessengerRelayShouldRevertIfPaused() (gas: 41587) +L1CrossDomainMessenger_Test:test_L1MessengerSendMessage() (gas: 78150) +L1CrossDomainMessenger_Test:test_L1MessengerTwiceSendMessage() (gas: 66413) +L1CrossDomainMessenger_Test:test_L1MessengerUnpause() (gas: 23804) +L1CrossDomainMessenger_Test:test_L1MessengerXDomainSenderReverts() (gas: 10599) +L1CrossDomainMessenger_Test:test_L1MessengerxDomainMessageSenderResets() (gas: 58533) +L1StandardBridge_Test:test_depositERC20() (gas: 371461) +L1StandardBridge_Test:test_depositERC20To() (gas: 373238) +L1StandardBridge_Test:test_depositETH() (gas: 105084) +L1StandardBridge_Test:test_depositETHTo() (gas: 111968) L1StandardBridge_Test:test_finalizeERC20Withdrawal() (gas: 438745) L1StandardBridge_Test:test_finalizeETHWithdrawal() (gas: 47960) L1StandardBridge_Test:test_initialize() (gas: 14885) @@ -28,17 +30,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: 99476) -L2CrossDomainMessenger_Test:testCannot_L2MessengerPause() (gas: 10843) -L2CrossDomainMessenger_Test:test_L2MessengerMessageVersion() (gas: 8410) -L2CrossDomainMessenger_Test:test_L2MessengerPause() (gas: 31837) -L2CrossDomainMessenger_Test:test_L2MessengerRelayMessageSucceeds() (gas: 57493) +L1StandardBridge_Test:test_receive() (gas: 99499) +L2CrossDomainMessenger_Test:testCannot_L2MessengerPause() (gas: 10865) +L2CrossDomainMessenger_Test:test_L2MessengerMessageVersion() (gas: 8432) +L2CrossDomainMessenger_Test:test_L2MessengerPause() (gas: 31815) +L2CrossDomainMessenger_Test:test_L2MessengerRelayMessageSucceeds() (gas: 57538) L2CrossDomainMessenger_Test:test_L2MessengerRelayMessageToSystemContract() (gas: 24567) -L2CrossDomainMessenger_Test:test_L2MessengerRelayShouldRevertIfPaused() (gas: 41599) +L2CrossDomainMessenger_Test:test_L2MessengerRelayShouldRevertIfPaused() (gas: 41578) L2CrossDomainMessenger_Test:test_L2MessengerSendMessage() (gas: 119681) L2CrossDomainMessenger_Test:test_L2MessengerTwiceSendMessage() (gas: 133118) -L2CrossDomainMessenger_Test:test_L2MessengerXDomainSenderReverts() (gas: 10588) -L2CrossDomainMessenger_Test:test_L2MessengerxDomainMessageSenderResets() (gas: 54859) +L2CrossDomainMessenger_Test:test_L2MessengerXDomainSenderReverts() (gas: 10621) +L2CrossDomainMessenger_Test:test_L2MessengerxDomainMessageSenderResets() (gas: 54925) L2OutputOracleTest:testCannot_appendCurrentTimestamp() (gas: 18627) L2OutputOracleTest:testCannot_appendEmptyOutput() (gas: 16734) L2OutputOracleTest:testCannot_appendFutureTimestamp() (gas: 18708) diff --git a/contracts-bedrock/contracts/test/L1CrossDomainMessenger.t.sol b/contracts-bedrock/contracts/test/L1CrossDomainMessenger.t.sol index 1fcb5bcb193c1..e20e87edea1d9 100644 --- a/contracts-bedrock/contracts/test/L1CrossDomainMessenger.t.sol +++ b/contracts-bedrock/contracts/test/L1CrossDomainMessenger.t.sol @@ -54,6 +54,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/contracts-bedrock/contracts/universal/CrossDomainMessenger.sol b/contracts-bedrock/contracts/universal/CrossDomainMessenger.sol index 7f8d2c0cd7fd8..88056f2df1398 100644 --- a/contracts-bedrock/contracts/universal/CrossDomainMessenger.sol +++ b/contracts-bedrock/contracts/universal/CrossDomainMessenger.sol @@ -100,6 +100,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).