diff --git a/contracts-bedrock/.gas-snapshot b/contracts-bedrock/.gas-snapshot index 2ca8b6a588242..fe89909dc3888 100644 --- a/contracts-bedrock/.gas-snapshot +++ b/contracts-bedrock/.gas-snapshot @@ -7,39 +7,40 @@ L1BlockTest:test_timestamp() (gas: 7661) L1BlockNumberTest:test_fallback() (gas: 10710) L1BlockNumberTest:test_getL1BlockNumber() (gas: 10589) L1BlockNumberTest:test_receive() (gas: 17440) -L1CrossDomainMessenger_Test:testCannot_L1MessengerPause() (gas: 10909) -L1CrossDomainMessenger_Test:test_L1MessengerMessageVersion() (gas: 8366) -L1CrossDomainMessenger_Test:test_L1MessengerPause() (gas: 31882) -L1CrossDomainMessenger_Test:test_L1MessengerRelayMessageSucceeds() (gas: 61193) -L1CrossDomainMessenger_Test:test_L1MessengerRelayMessageToSystemContract() (gas: 44815) -L1CrossDomainMessenger_Test:test_L1MessengerRelayShouldRevertIfPaused() (gas: 41631) -L1CrossDomainMessenger_Test:test_L1MessengerSendMessage() (gas: 78105) -L1CrossDomainMessenger_Test:test_L1MessengerTwiceSendMessage() (gas: 66345) -L1CrossDomainMessenger_Test:test_L1MessengerXDomainSenderReverts() (gas: 10588) -L1CrossDomainMessenger_Test:test_L1MessengerxDomainMessageSenderResets() (gas: 58489) -L1StandardBridge_Test:test_depositERC20() (gas: 371479) -L1StandardBridge_Test:test_depositERC20To() (gas: 373256) -L1StandardBridge_Test:test_depositETH() (gas: 105126) -L1StandardBridge_Test:test_depositETHTo() (gas: 111945) -L1StandardBridge_Test:test_donateETH() (gas: 17523) -L1StandardBridge_Test:test_finalizeERC20Withdrawal() (gas: 438817) -L1StandardBridge_Test:test_finalizeETHWithdrawal() (gas: 47983) -L1StandardBridge_Test:test_initialize() (gas: 14863) +L1CrossDomainMessenger_Test:testCannot_L1MessengerPause() (gas: 10844) +L1CrossDomainMessenger_Test:testCannot_L1MessengerUnpause() (gas: 10858) +L1CrossDomainMessenger_Test:test_L1MessengerMessageVersion() (gas: 8388) +L1CrossDomainMessenger_Test:test_L1MessengerPause() (gas: 31815) +L1CrossDomainMessenger_Test:test_L1MessengerRelayMessageSucceeds() (gas: 61215) +L1CrossDomainMessenger_Test:test_L1MessengerRelayMessageToSystemContract() (gas: 44837) +L1CrossDomainMessenger_Test:test_L1MessengerRelayShouldRevertIfPaused() (gas: 41587) +L1CrossDomainMessenger_Test:test_L1MessengerSendMessage() (gas: 78150) +L1CrossDomainMessenger_Test:test_L1MessengerTwiceSendMessage() (gas: 66413) +L1CrossDomainMessenger_Test:test_L1MessengerUnpause() (gas: 23804) +L1CrossDomainMessenger_Test:test_L1MessengerXDomainSenderReverts() (gas: 10599) +L1CrossDomainMessenger_Test:test_L1MessengerxDomainMessageSenderResets() (gas: 58533) +L1StandardBridge_Test:test_depositERC20() (gas: 371461) +L1StandardBridge_Test:test_depositERC20To() (gas: 373238) +L1StandardBridge_Test:test_depositETH() (gas: 105084) +L1StandardBridge_Test:test_depositETHTo() (gas: 111968) +L1StandardBridge_Test:test_finalizeERC20Withdrawal() (gas: 438745) +L1StandardBridge_Test:test_finalizeETHWithdrawal() (gas: 47960) +L1StandardBridge_Test:test_initialize() (gas: 14885) L1StandardBridge_Test:test_onlyEOADepositERC20() (gas: 12085) L1StandardBridge_Test:test_onlyEOADepositETH() (gas: 30637) L1StandardBridge_Test:test_onlyL2BridgeFinalizeERC20Withdrawal() (gas: 23565) -L1StandardBridge_Test:test_onlyPortalFinalizeERC20Withdrawal() (gas: 22897) -L1StandardBridge_Test:test_receive() (gas: 99476) -L2CrossDomainMessenger_Test:testCannot_L2MessengerPause() (gas: 10843) -L2CrossDomainMessenger_Test:test_L2MessengerMessageVersion() (gas: 8410) -L2CrossDomainMessenger_Test:test_L2MessengerPause() (gas: 31837) -L2CrossDomainMessenger_Test:test_L2MessengerRelayMessageSucceeds() (gas: 57493) +L1StandardBridge_Test:test_onlyPortalFinalizeERC20Withdrawal() (gas: 22919) +L1StandardBridge_Test:test_receive() (gas: 99499) +L2CrossDomainMessenger_Test:testCannot_L2MessengerPause() (gas: 10865) +L2CrossDomainMessenger_Test:test_L2MessengerMessageVersion() (gas: 8432) +L2CrossDomainMessenger_Test:test_L2MessengerPause() (gas: 31815) +L2CrossDomainMessenger_Test:test_L2MessengerRelayMessageSucceeds() (gas: 57538) L2CrossDomainMessenger_Test:test_L2MessengerRelayMessageToSystemContract() (gas: 24567) -L2CrossDomainMessenger_Test:test_L2MessengerRelayShouldRevertIfPaused() (gas: 41599) +L2CrossDomainMessenger_Test:test_L2MessengerRelayShouldRevertIfPaused() (gas: 41578) L2CrossDomainMessenger_Test:test_L2MessengerSendMessage() (gas: 119681) L2CrossDomainMessenger_Test:test_L2MessengerTwiceSendMessage() (gas: 133118) -L2CrossDomainMessenger_Test:test_L2MessengerXDomainSenderReverts() (gas: 10588) -L2CrossDomainMessenger_Test:test_L2MessengerxDomainMessageSenderResets() (gas: 54859) +L2CrossDomainMessenger_Test:test_L2MessengerXDomainSenderReverts() (gas: 10621) +L2CrossDomainMessenger_Test:test_L2MessengerxDomainMessageSenderResets() (gas: 54925) L2OutputOracleTest:testCannot_appendCurrentTimestamp() (gas: 18627) L2OutputOracleTest:testCannot_appendEmptyOutput() (gas: 16734) L2OutputOracleTest:testCannot_appendFutureTimestamp() (gas: 18708) @@ -55,11 +56,12 @@ L2OutputOracleTest:test_deleteL2Output() (gas: 64320) L2OutputOracleTest:test_getL2Output() (gas: 74601) L2OutputOracleTest:test_latestBlockTimestamp() (gas: 68377) L2OutputOracleTest:test_nextTimestamp() (gas: 9236) -L2StandardBridge_Test:test_finalizeDeposit() (gas: 93169) -L2StandardBridge_Test:test_initialize() (gas: 14812) -L2StandardBridge_Test:test_receive() (gas: 136437) -L2StandardBridge_Test:test_withdraw() (gas: 352632) -L2StandardBridge_Test:test_withdrawTo() (gas: 353501) +L2StandardBridge_Test:test_ERC20BridgeFailed_whenLocalTokenIsBridge() (gas: 133074) +L2StandardBridge_Test:test_finalizeDeposit() (gas: 93165) +L2StandardBridge_Test:test_initialize() (gas: 14834) +L2StandardBridge_Test:test_receive() (gas: 136392) +L2StandardBridge_Test:test_withdraw() (gas: 352629) +L2StandardBridge_Test:test_withdrawTo() (gas: 353463) L2ToL1MessagePasserTest:test_burn() (gas: 112001) L2ToL1MessagePasserTest:test_initiateWithdrawal_fromContract() (gas: 67935) L2ToL1MessagePasserTest:test_initiateWithdrawal_fromEOA() (gas: 74851) 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 03e7e50f5d40f..77af021e8ef51 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,12 @@ contract OptimismPortal { "Insufficient gas to finalize withdrawal." ); + // Disallow withdrawals during reentrancy to this function. + require( + l2Sender == DEFAULT_L2_SENDER, + "Cannot withdraw a transaction within another withdrawal transaction." + ); + // Set the l2Sender so that other contracts can know which account // on L2 is making the withdrawal l2Sender = _sender; 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/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/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/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/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/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/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). 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..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 ); /************* @@ -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( _remoteToken, _localToken, - _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), @@ -368,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); } /** @@ -384,7 +387,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;