diff --git a/contracts-bedrock/.gas-snapshot b/contracts-bedrock/.gas-snapshot index 9e060ccc87d71..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: 61129) -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: 58425) -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: 57429) +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: 54795) +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,25 +56,27 @@ 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: 352626) -L2StandardBridge_Test:test_withdrawTo() (gas: 353495) +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) OptimismMintableTokenFactory_Test:test_bridge() (gas: 9850) OptimismMintableTokenFactory_Test:test_burn() (gas: 52791) OptimismMintableTokenFactory_Test:test_burnRevertsFromNotBridge() (gas: 13211) +OptimismMintableTokenFactory_Test:test_erc165_supportsInterface() (gas: 10999) OptimismMintableTokenFactory_Test:test_l1Token() (gas: 9779) OptimismMintableTokenFactory_Test:test_l2Bridge() (gas: 9768) -OptimismMintableTokenFactory_Test:test_mint() (gas: 65732) -OptimismMintableTokenFactory_Test:test_mintRevertsFromNotBridge() (gas: 13213) +OptimismMintableTokenFactory_Test:test_mint() (gas: 65687) +OptimismMintableTokenFactory_Test:test_mintRevertsFromNotBridge() (gas: 13235) OptimismMintableTokenFactory_Test:test_remoteToken() (gas: 9762) OptimismMintableTokenFactory_Test:test_bridge() (gas: 9772) -OptimismMintableTokenFactory_Test:test_createStandardL2Token() (gas: 1106538) -OptimismMintableTokenFactory_Test:test_createStandardL2TokenSameTwice() (gas: 2193987) +OptimismMintableTokenFactory_Test:test_createStandardL2Token() (gas: 1100125) +OptimismMintableTokenFactory_Test:test_createStandardL2TokenSameTwice() (gas: 2181161) OptimismMintableTokenFactory_Test:test_createStandardL2TokenShouldRevertIfRemoteIsZero() (gas: 9374) OptimismMintableTokenFactory_Test:test_initializeShouldRevert() (gas: 12696) OptimismPortal_Test:test_OptimismPortalConstructor() (gas: 11302) diff --git a/contracts-bedrock/contracts/L1/L1StandardBridge.sol b/contracts-bedrock/contracts/L1/L1StandardBridge.sol index 7ac3b80b1e582..513c81900e057 100644 --- a/contracts-bedrock/contracts/L1/L1StandardBridge.sol +++ b/contracts-bedrock/contracts/L1/L1StandardBridge.sol @@ -11,6 +11,9 @@ import { StandardBridge } from "../universal/StandardBridge.sol"; * @dev The L1 ETH and ERC20 Bridge is a contract which stores deposited L1 funds and standard * tokens that are in use on L2. It synchronizes a corresponding L2 Bridge, informing it of deposits * and listening to it for newly finalized withdrawals. + * Note that this contract is not intended to support all variations of ERC20 tokens; this + * includes, but is not limited to tokens with transfer fees, rebasing tokens, and + * tokens with blocklists. */ contract L1StandardBridge is StandardBridge { /********** @@ -21,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( @@ -37,7 +40,7 @@ contract L1StandardBridge is StandardBridge { address indexed _from, address _to, uint256 _amount, - bytes _data + bytes _extraData ); event ERC20WithdrawalFinalized( @@ -46,7 +49,7 @@ contract L1StandardBridge is StandardBridge { address indexed _from, address _to, uint256 _amount, - bytes _data + bytes _extraData ); /******************** @@ -73,46 +76,48 @@ 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); } /** - * @dev Deposit an amount of ETH to a recipient's balance on L2. - * @param _to L2 address to credit the withdrawal to. + * @dev Deposit an amount of ETH to a recipient's balance on L2. Note that if ETH is sent to a + * 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); } /** * @dev deposit an amount of the ERC20 to the caller's balance on L2. - * @param _l1Token Address of the L1 ERC20 we are depositing - * @param _l2Token Address of the L1 respective L2 ERC20 - * @param _amount Amount of the ERC20 to deposit + * @param _l1Token Address of the L1 ERC20 we are depositing. + * @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, @@ -121,20 +126,21 @@ contract L1StandardBridge is StandardBridge { msg.sender, _amount, _minGasLimit, - _data + _extraData ); } /** * @dev deposit an amount of ERC20 to a recipient's balance on L2. - * @param _l1Token Address of the L1 ERC20 we are depositing - * @param _l2Token Address of the L1 respective L2 ERC20 - * @param _to L2 address to credit the withdrawal to. + * @param _l1Token Address of the L1 ERC20 we are depositing. + * @param _l2Token Address of the L2 token we are depositing to. + * @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, @@ -142,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( @@ -167,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, @@ -177,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); } /********************** @@ -191,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( @@ -204,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/L2OutputOracle.sol b/contracts-bedrock/contracts/L1/L2OutputOracle.sol index a02d5a255b54e..b7826daa1b5af 100644 --- a/contracts-bedrock/contracts/L1/L2OutputOracle.sol +++ b/contracts-bedrock/contracts/L1/L2OutputOracle.sol @@ -73,6 +73,7 @@ contract L2OutputOracle is Ownable { * @param _historicalTotalBlocks The number of blocks that preceding the * initialization of the L2 chain. * @param _startingBlockTimestamp The timestamp to start L2 block at. + * @param sequencer The address of the sequencer. */ constructor( uint256 _submissionInterval, diff --git a/contracts-bedrock/contracts/L1/OptimismPortal.sol b/contracts-bedrock/contracts/L1/OptimismPortal.sol index a0656cde315f4..77af021e8ef51 100644 --- a/contracts-bedrock/contracts/L1/OptimismPortal.sol +++ b/contracts-bedrock/contracts/L1/OptimismPortal.sol @@ -67,6 +67,16 @@ contract OptimismPortal { */ address internal constant DEFAULT_L2_SENDER = 0x000000000000000000000000000000000000dEaD; + /** + * @notice The L2 gas limit set when eth is deposited using the receive() function. + */ + uint64 internal constant RECEIVE_DEFAULT_GAS_LIMIT = 100_000; + + /** + * @notice Additional gas reserved for clean up after finalizing a transaction withdrawal. + */ + uint256 internal constant FINALIZE_GAS_BUFFER = 20_000; + /************* * Variables * *************/ @@ -83,7 +93,7 @@ contract OptimismPortal { /** * @notice Public variable which can be used to read the address of the L2 account which - * initated the withdrawal. Can also be used to determine whether or not execution is occuring + * initiated the withdrawal. Can also be used to determine whether or not execution is occuring * downstream of a call to finalizeWithdrawalTransaction(). */ address public l2Sender = DEFAULT_L2_SENDER; @@ -111,10 +121,11 @@ contract OptimismPortal { * @notice Accepts value so that users can send ETH directly to this contract and * have the funds be deposited to their address on L2. * @dev This is intended as a convenience function for EOAs. Contracts should call the - * depositTransaction() function directly. + * depositTransaction() function directly otherwise any deposited funds will be lost due to + * address aliasing. */ receive() external payable { - depositTransaction(msg.sender, msg.value, 100000, false, bytes("")); + depositTransaction(msg.sender, msg.value, RECEIVE_DEFAULT_GAS_LIMIT, false, bytes("")); } /** @@ -171,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. @@ -221,7 +236,16 @@ contract OptimismPortal { finalizedWithdrawals[withdrawalHash] = true; // Save enough gas so that the call cannot use up all of the gas - require(gasleft() >= _gasLimit + 20000, "Insufficient gas to finalize withdrawal."); + require( + gasleft() >= _gasLimit + FINALIZE_GAS_BUFFER, + "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 diff --git a/contracts-bedrock/contracts/L2/L2StandardBridge.sol b/contracts-bedrock/contracts/L2/L2StandardBridge.sol index 000234606ecc0..8df2e7893427d 100644 --- a/contracts-bedrock/contracts/L2/L2StandardBridge.sol +++ b/contracts-bedrock/contracts/L2/L2StandardBridge.sol @@ -11,6 +11,9 @@ import { OptimismMintableERC20 } from "../universal/OptimismMintableERC20.sol"; * @title L2StandardBridge * @dev This contract is an L2 predeploy that is responsible for facilitating * deposits of tokens from L1 to L2. + * Note that this contract is not intended to support all variations of ERC20 tokens; this + * includes, but is not limited to tokens with transfer fees, rebasing tokens, and + * tokens with blocklists. * TODO: ensure that this has 1:1 backwards compatibility */ contract L2StandardBridge is StandardBridge { @@ -24,7 +27,7 @@ contract L2StandardBridge is StandardBridge { address indexed _from, address _to, uint256 _amount, - bytes _data + bytes _extraData ); event DepositFinalized( @@ -33,7 +36,7 @@ contract L2StandardBridge is StandardBridge { address indexed _from, address _to, uint256 _amount, - bytes _data + bytes _extraData ); event DepositFailed( @@ -42,7 +45,7 @@ contract L2StandardBridge is StandardBridge { address indexed _from, address _to, uint256 _amount, - bytes _data + bytes _extraData ); /******************** @@ -62,33 +65,42 @@ 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); } /** - * @notice Withdraw tokens to an address on L1 + * @notice Withdraw tokens to an address on L1. Note that if ETH is sent to a + * contract on L1 and the call fails, then that ETH will be locked in the + * L1StandardBridge. * @param _l2Token The L2 token address to withdraw * @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 { - _initiateWithdrawal(_l2Token, msg.sender, _to, _amount, _minGasLimit, _data); + // TODO: add onlyEOA check on ETH withdrawals to match L1Bridge.depositETHTo? + _initiateWithdrawal(_l2Token, msg.sender, _to, _amount, _minGasLimit, _extraData); } /** @@ -99,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, @@ -107,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); } /********************** @@ -124,7 +139,6 @@ contract L2StandardBridge is StandardBridge { /** * @notice Handle withdrawals, taking into account the legacy form of ETH * when it was represented as an ERC20 at the OVM_ETH contract. - * TODO: require(msg.value == _value) for OVM_ETH case? */ function _initiateWithdrawal( address _l2Token, @@ -132,14 +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) { - _initiateBridgeETH(_from, _to, _amount, _minGasLimit, _data); + require(msg.value == _amount, "ETH withdrawals must include sufficient ETH value."); + _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, msg.sender, _to, _amount, _data); + emit WithdrawalInitiated(l1Token, _l2Token, _from, _to, _amount, _extraData); } } diff --git a/contracts-bedrock/contracts/L2/L2ToL1MessagePasser.sol b/contracts-bedrock/contracts/L2/L2ToL1MessagePasser.sol index 99eeb21e8c8d3..13765892f93a7 100644 --- a/contracts-bedrock/contracts/L2/L2ToL1MessagePasser.sol +++ b/contracts-bedrock/contracts/L2/L2ToL1MessagePasser.sol @@ -36,6 +36,15 @@ contract L2ToL1MessagePasser { */ event WithdrawerBalanceBurnt(uint256 indexed amount); + /************* + * Constants * + *************/ + + /** + * @notice The L1 gas limit set when eth is withdrawn using the receive() function. + */ + uint256 internal constant RECEIVE_DEFAULT_GAS_LIMIT = 100_000; + /************* * Variables * *************/ @@ -60,7 +69,7 @@ contract L2ToL1MessagePasser { * TODO: maybe this should be only EOA */ receive() external payable { - initiateWithdrawal(msg.sender, 100000, bytes("")); + initiateWithdrawal(msg.sender, RECEIVE_DEFAULT_GAS_LIMIT, bytes("")); } /** 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 3dd49cf81d36a..3e70192ccab58 100644 --- a/contracts-bedrock/contracts/test/OptimismMintableERC20.t.sol +++ b/contracts-bedrock/contracts/test/OptimismMintableERC20.t.sol @@ -3,6 +3,8 @@ pragma solidity 0.8.10; import { Bridge_Initializer } from "./CommonTest.t.sol"; import { LibRLP } from "./Lib_RLP.t.sol"; +import "../universal/SupportedInterfaces.sol"; +import { IERC165 } from "@openzeppelin/contracts/utils/introspection/IERC165.sol"; contract OptimismMintableTokenFactory_Test is Bridge_Initializer { event Mint(address indexed _account, uint256 _amount); @@ -64,4 +66,24 @@ contract OptimismMintableTokenFactory_Test is Bridge_Initializer { vm.prank(address(alice)); L2Token.burn(alice, 100); } + + function test_erc165_supportsInterface() external { + // The assertEq calls in this test are comparing the manual calculation of the iface, + // with what is returned by the solidity's type().interfaceId, just to be safe. + bytes4 iface1 = bytes4(keccak256("supportsInterface(bytes4)")); + assertEq(iface1, type(IERC165).interfaceId); + assert(L2Token.supportsInterface(iface1)); + emit log_bytes32(bytes32(iface1)); + + bytes4 iface2 = L2Token.l1Token.selector ^ L2Token.mint.selector ^ L2Token.burn.selector; + 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(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 e1dfe0a8d1b5f..88056f2df1398 100644 --- a/contracts-bedrock/contracts/universal/CrossDomainMessenger.sol +++ b/contracts-bedrock/contracts/universal/CrossDomainMessenger.sol @@ -56,7 +56,10 @@ abstract contract CrossDomainMessenger is uint32 public constant MIN_GAS_DYNAMIC_OVERHEAD = 1; - uint32 public constant MIN_GAS_CONSTANT_OVERHEAD = 100000; + uint32 public constant MIN_GAS_CONSTANT_OVERHEAD = 100_000; + + uint256 internal constant RELAY_GAS_REQUIRED = 45_000; + uint256 internal constant RELAY_GAS_BUFFER = RELAY_GAS_REQUIRED - 5000; /************* * Variables * @@ -97,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). @@ -211,12 +221,15 @@ abstract contract CrossDomainMessenger is require(successfulMessages[versionedHash] == false, "Message has already been relayed."); // TODO: Make sure this will always give us enough gas. - require(gasleft() >= _minGasLimit + 45000, "Insufficient gas to relay message."); + require( + gasleft() >= _minGasLimit + RELAY_GAS_REQUIRED, + "Insufficient gas to relay message." + ); xDomainMsgSender = _sender; (bool success, ) = ExcessivelySafeCall.excessivelySafeCall( _target, - gasleft() - 40000, + gasleft() - RELAY_GAS_BUFFER, _value, 0, _message diff --git a/contracts-bedrock/contracts/universal/OptimismMintableERC20.sol b/contracts-bedrock/contracts/universal/OptimismMintableERC20.sol index 90decd2439543..e9e1a46bcde3c 100644 --- a/contracts-bedrock/contracts/universal/OptimismMintableERC20.sol +++ b/contracts-bedrock/contracts/universal/OptimismMintableERC20.sol @@ -2,6 +2,7 @@ pragma solidity ^0.8.9; import { ERC20 } from "@openzeppelin/contracts/token/ERC20/ERC20.sol"; +import "./SupportedInterfaces.sol"; /** * @title OptimismMintableERC20 @@ -72,10 +73,10 @@ contract OptimismMintableERC20 is ERC20 { */ // slither-disable-next-line external-function function supportsInterface(bytes4 _interfaceId) public pure returns (bool) { - bytes4 iface1 = bytes4(keccak256("supportsInterface(bytes4)")); // ERC165 - bytes4 iface2 = this.l1Token.selector ^ this.mint.selector ^ this.burn.selector; - bytes4 iface3 = this.remoteToken.selector ^ this.mint.selector ^ this.burn.selector; - return _interfaceId == iface1 || _interfaceId == iface3 || _interfaceId == iface2; + 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/OptimismMintableTokenFactory.sol b/contracts-bedrock/contracts/universal/OptimismMintableTokenFactory.sol index fc482c4e5cca3..7658c0915f934 100644 --- a/contracts-bedrock/contracts/universal/OptimismMintableTokenFactory.sol +++ b/contracts-bedrock/contracts/universal/OptimismMintableTokenFactory.sol @@ -43,6 +43,7 @@ contract OptimismMintableTokenFactory { * @param _remoteToken Address of the corresponding L1 token. * @param _name ERC20 name. * @param _symbol ERC20 symbol. + * @return Address of the new token. */ function createStandardL2Token( address _remoteToken, diff --git a/contracts-bedrock/contracts/universal/StandardBridge.sol b/contracts-bedrock/contracts/universal/StandardBridge.sol index 8b901f6c37537..9c0ae2cb0d7da 100644 --- a/contracts-bedrock/contracts/universal/StandardBridge.sol +++ b/contracts-bedrock/contracts/universal/StandardBridge.sol @@ -3,6 +3,7 @@ pragma solidity ^0.8.9; /* Interface Imports */ import { IERC20 } from "@openzeppelin/contracts/token/ERC20/IERC20.sol"; +import "./SupportedInterfaces.sol"; /* Library Imports */ import { ERC165Checker } from "@openzeppelin/contracts/utils/introspection/ERC165Checker.sol"; @@ -30,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( @@ -46,7 +47,7 @@ abstract contract StandardBridge { address indexed _from, address _to, uint256 _amount, - bytes _data + bytes _extraData ); event ERC20BridgeFinalized( @@ -55,7 +56,7 @@ abstract contract StandardBridge { address indexed _from, address _to, uint256 _amount, - bytes _data + bytes _extraData ); event ERC20BridgeFailed( @@ -64,9 +65,18 @@ abstract contract StandardBridge { address indexed _from, address _to, uint256 _amount, - bytes _data + bytes _extraData ); + /************* + * Constants * + *************/ + + /** + * @notice The L2 gas limit set when eth is depoisited using the receive() function. + */ + uint32 internal constant RECEIVE_DEFAULT_GAS_LIMIT = 200_000; + /************* * Variables * *************/ @@ -118,35 +128,31 @@ 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. */ receive() external payable onlyEOA { - _initiateBridgeETH(msg.sender, msg.sender, msg.value, 200_000, bytes("")); + _initiateBridgeETH(msg.sender, msg.sender, msg.value, RECEIVE_DEFAULT_GAS_LIMIT, bytes("")); } /** * @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); } /** - * @notice Send ETH to a specified account on the remote domain + * @notice Send ETH to a specified account on the remote domain. Note that if ETH is sent to a + * contract and the call fails, then that ETH will be locked in the other bridge. */ 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); } /** @@ -157,7 +163,7 @@ abstract contract StandardBridge { address _remoteToken, uint256 _amount, uint32 _minGasLimit, - bytes calldata _data + bytes calldata _extraData ) public virtual onlyEOA { _initiateBridgeERC20( _localToken, @@ -166,7 +172,7 @@ abstract contract StandardBridge { msg.sender, _amount, _minGasLimit, - _data + _extraData ); } @@ -179,7 +185,7 @@ abstract contract StandardBridge { address _to, uint256 _amount, uint32 _minGasLimit, - bytes calldata _data + bytes calldata _extraData ) public virtual { _initiateBridgeERC20( _localToken, @@ -188,7 +194,7 @@ abstract contract StandardBridge { _to, _amount, _minGasLimit, - _data + _extraData ); } @@ -199,14 +205,14 @@ 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, "TransferHelper::safeTransferETH: ETH transfer failed"); + require(success, "ETH transfer failed."); } /** @@ -218,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); } } @@ -285,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 ); } @@ -306,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. @@ -318,7 +332,7 @@ abstract contract StandardBridge { "Wrong remote token for Optimism Mintable ERC20 local token" ); - OptimismMintableERC20(_localToken).burn(msg.sender, _amount); + OptimismMintableERC20(_localToken).burn(_from, _amount); } else { // TODO: Do we need to confirm that the transfer was successful? IERC20(_localToken).safeTransferFrom(_from, address(this), _amount); @@ -332,7 +346,7 @@ abstract contract StandardBridge { _to, _amount, _minGasLimit, - _data + _extraData ); } @@ -346,7 +360,7 @@ abstract contract StandardBridge { address _to, uint256 _amount, uint32 _minGasLimit, - bytes calldata _data + bytes calldata _extraData ) internal { messenger.sendMessage( address(otherBridge), @@ -357,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); } /** @@ -373,8 +387,7 @@ abstract contract StandardBridge { * @return True if the token is an OptimismMintableERC20. */ function _isOptimismMintableERC20(address _token) internal view returns (bool) { - // 0x1d1d8b63 is mint ^ burn ^ l1Token - return ERC165Checker.supportsInterface(_token, 0x1d1d8b63); + return ERC165Checker.supportsInterface(_token, type(IL1Token).interfaceId); } /** diff --git a/contracts-bedrock/contracts/universal/SupportedInterfaces.sol b/contracts-bedrock/contracts/universal/SupportedInterfaces.sol new file mode 100644 index 0000000000000..a616b92ccd7a1 --- /dev/null +++ b/contracts-bedrock/contracts/universal/SupportedInterfaces.sol @@ -0,0 +1,21 @@ +// SPDX-License-Identifier: MIT +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 IRemoteToken { + function mint(address _to, uint256 _amount) external virtual; + + function burn(address _from, uint256 _amount) external virtual; + + function remoteToken() external virtual; +} + +interface IL1Token { + function mint(address _to, uint256 _amount) external virtual; + + function burn(address _from, uint256 _amount) external virtual; + + function l1Token() external virtual; +}