bedrock: Address review feedback

packages/contracts-bedrock/.gas-snapshot

@@ -71,12 +71,14 @@ L2OutputOracleTest:test_deleteL2Output() (gas: 64338)
 L2OutputOracleTest:test_getL2Output() (gas: 74601)
 L2OutputOracleTest:test_latestBlockTimestamp() (gas: 68377)
 L2OutputOracleTest:test_nextTimestamp() (gas: 9236)
-L2StandardBridge_Test:test_finalizeDeposit() (gas: 93143)
+L2StandardBridge_Test:test_cannotWithdrawEthWithoutSendingIt() (gas: 21578)
+L2StandardBridge_Test:test_finalizeDeposit() (gas: 93165)
 L2StandardBridge_Test:test_finalizeDeposit_failsToCompleteOutboundTransfer() (gas: 140106)
-L2StandardBridge_Test:test_initialize() (gas: 14812)
-L2StandardBridge_Test:test_receive() (gas: 136415)
-L2StandardBridge_Test:test_withdraw() (gas: 352612)
-L2StandardBridge_Test:test_withdrawTo() (gas: 353463)
+L2StandardBridge_Test:test_initialize() (gas: 14834)
+L2StandardBridge_Test:test_receive() (gas: 136459)
+L2StandardBridge_Test:test_withdraw() (gas: 352724)
+L2StandardBridge_Test:test_withdrawTo() (gas: 353480)
+L2StandardBridge_Test:test_withdraw_onlyEOA() (gas: 251952)
 L2ToL1MessagePasserTest:test_burn() (gas: 112046)
 L2ToL1MessagePasserTest:test_initiateWithdrawal_fromContract() (gas: 67890)
 L2ToL1MessagePasserTest:test_initiateWithdrawal_fromEOA() (gas: 74851)
@@ -92,7 +94,7 @@ OVM_ETH_Test:test_transferFrom() (gas: 13008)
 OptimismMintableERC20_Test:test_bridge() (gas: 9785)
 OptimismMintableERC20_Test:test_burn() (gas: 52791)
 OptimismMintableERC20_Test:test_burnRevertsFromNotBridge() (gas: 13211)
-OptimismMintableERC20_Test:test_erc165_supportsInterface() (gas: 10999)
+OptimismMintableERC20_Test:test_erc165_supportsInterface() (gas: 7828)
 OptimismMintableERC20_Test:test_l1Token() (gas: 9779)
 OptimismMintableERC20_Test:test_l2Bridge() (gas: 9790)
 OptimismMintableERC20_Test:test_mint() (gas: 65687)

packages/contracts-bedrock/contracts/L1/L1StandardBridge.sol

@@ -118,9 +118,11 @@ contract L1StandardBridge is StandardBridge {
 
     /**
      * @custom:legacy
-     * @notice Deposits some amount of ETH into a target account 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.
+     * @notice Deposits some amount of ETH into a target account 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. ETH may be recoverable if the call can be
+     *         successfully replayed by increasing the amount of gas supplied to the call. If the
+     *         call will fail for any amount of gas, then the ETH will be locked permanently.
      *
      * @param _to          Address of the recipient on L2.
      * @param _minGasLimit Minimum gas limit for the deposit message on L2.

packages/contracts-bedrock/contracts/L1/L2OutputOracle.sol

@@ -73,7 +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.
+     * @param _sequencer The address of the _sequencer.
      */
     constructor(
         uint256 _submissionInterval,
@@ -81,7 +81,7 @@ contract L2OutputOracle is Ownable {
         bytes32 _genesisL2Output,
         uint256 _historicalTotalBlocks,
         uint256 _startingBlockTimestamp,
-        address sequencer
+        address _sequencer
     ) {
         require(
             _submissionInterval % _l2BlockTime == 0,
@@ -98,7 +98,7 @@ contract L2OutputOracle is Ownable {
         // solhint-disable-next-line not-rely-on-time
         STARTING_BLOCK_TIMESTAMP = _startingBlockTimestamp;
 
-        _transferOwnership(sequencer);
+        _transferOwnership(_sequencer);
     }
 
     /*********************************

packages/contracts-bedrock/contracts/L2/L2StandardBridge.sol

@@ -11,8 +11,9 @@ import { OptimismMintableERC20 } from "../universal/OptimismMintableERC20.sol";
  * @title L2StandardBridge
  * @notice The L2StandardBridge is responsible for transfering ETH and ERC20 tokens between L1 and
  *         L2. ERC20 tokens sent to L1 are escrowed within this contract.
- *         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
+ *         Note that this contract is not intended to support all variations of ERC20 tokens.
+ *         Examples of some token types that may not be properly supported by this contract include,
+ *         but are not limited to: tokens with transfer fees, rebasing tokens, and
  *         tokens with blocklists.
  *         TODO: ensure that this has 1:1 backwards compatibility
  */
@@ -100,15 +101,17 @@ contract L2StandardBridge is StandardBridge {
         uint256 _amount,
         uint32 _minGasLimit,
         bytes calldata _data
-    ) external payable virtual {
+    ) external payable virtual onlyEOA {
         _initiateWithdrawal(_l2Token, msg.sender, msg.sender, _amount, _minGasLimit, _data);
     }
 
     /**
      * @custom:legacy
-     * @notice Initiates a withdrawal from L2 to L1 to a target account 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.
+     * @notice Initiates a withdrawal from L2 to L1 to a target account 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. ETH may be recoverable if the call can be
+     *         successfully replayed by increasing the amount of gas supplied to the call. If the
+     *         call will fail for any amount of gas, then the ETH will be locked permanently.
      *
      * @param _l2Token     Address of the L2 token to withdraw.
      * @param _to          Recipient account on L1.
@@ -123,7 +126,6 @@ contract L2StandardBridge is StandardBridge {
         uint32 _minGasLimit,
         bytes calldata _data
     ) external payable virtual {
-        // TODO: add onlyEOA check on ETH withdrawals to match L1Bridge.depositETHTo?
         _initiateWithdrawal(_l2Token, msg.sender, _to, _amount, _minGasLimit, _data);
     }
 

packages/contracts-bedrock/contracts/test/L2StandardBridge.t.sol

@@ -4,6 +4,7 @@ pragma solidity 0.8.10;
 import { Bridge_Initializer } from "./CommonTest.t.sol";
 import { stdStorage, StdStorage } from "forge-std/Test.sol";
 import { CrossDomainMessenger } from "../universal/CrossDomainMessenger.sol";
+import { Lib_PredeployAddresses } from "../libraries/Lib_PredeployAddresses.sol";
 import { console } from "forge-std/console.sol";
 
 contract L2StandardBridge_Test is Bridge_Initializer {
@@ -43,6 +44,21 @@ contract L2StandardBridge_Test is Bridge_Initializer {
         assertEq(address(messagePasser).balance, 100);
     }
 
+    // withrdraw
+    // - requires amount == msg.value
+    function test_cannotWithdrawEthWithoutSendingIt() external {
+        assertEq(address(messagePasser).balance, 0);
+
+        vm.expectRevert("ETH withdrawals must include sufficient ETH value.");
+        vm.prank(alice, alice);
+        L2Bridge.withdraw(
+            address(Lib_PredeployAddresses.OVM_ETH),
+            100,
+            1000,
+            hex""
+        );
+    }
+
     // withdraw
     // - token is burned
     // - emits WithdrawalInitiated
@@ -65,6 +81,19 @@ contract L2StandardBridge_Test is Bridge_Initializer {
         assertEq(L2Token.balanceOf(alice), 0);
     }
 
+    function test_withdraw_onlyEOA() external {
+        // This contract has 100 L2Token
+        deal(address(L2Token), address(this), 100, true);
+
+        vm.expectRevert("Account not EOA");
+        L2Bridge.withdraw(
+            address(L2Token),
+            100,
+            1000,
+            hex""
+        );
+    }
+
     // withdrawTo
     // - token is burned
     // - emits WithdrawalInitiated w/ correct recipient

packages/contracts-bedrock/contracts/test/OptimismMintableERC20.t.sol

@@ -73,17 +73,13 @@ contract OptimismMintableERC20_Test is Bridge_Initializer {
         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));
-
     }
 }

packages/contracts-bedrock/contracts/universal/CrossDomainMessenger.sol

@@ -56,7 +56,10 @@ abstract contract CrossDomainMessenger is
 
     uint32 public constant MIN_GAS_CONSTANT_OVERHEAD = 100_000;
 
+    /// @notice Minimum amount of gas required prior to relaying a message.
     uint256 internal constant RELAY_GAS_REQUIRED = 45_000;
+
+    /// @notice Amount of gas held in reserve for accounting after relaying a message.
     uint256 internal constant RELAY_GAS_BUFFER = RELAY_GAS_REQUIRED - 5000;
 
     /*************