OZ Low and Informational audit fixes (on develop)

.changeset/gorgeous-berries-behave.md

@@ -0,0 +1,9 @@
+---
+'@eth-optimism/contracts-bedrock': patch
+---
+
+OZ Audit fixes with a Low or informational severity:
+
+- Hardcode constant values
+- Require that msg.value == \_amount on ETH withdrawals
+- use \_from in place of msg.sender when applicable in internal functions

packages/contracts-bedrock/.gas-snapshot

@@ -26,13 +26,13 @@ L1BlockNumberTest:test_receive() (gas: 25436)
 L1CrossDomainMessenger_Test:testCannot_L1MessengerPause() (gas: 10909)
 L1CrossDomainMessenger_Test:test_L1MessengerMessageVersion() (gas: 8366)
 L1CrossDomainMessenger_Test:test_L1MessengerPause() (gas: 31882)
-L1CrossDomainMessenger_Test:test_L1MessengerRelayMessageSucceeds() (gas: 61175)
+L1CrossDomainMessenger_Test:test_L1MessengerRelayMessageSucceeds() (gas: 61239)
 L1CrossDomainMessenger_Test:test_L1MessengerRelayMessageToSystemContract() (gas: 44859)
 L1CrossDomainMessenger_Test:test_L1MessengerRelayShouldRevertIfPaused() (gas: 41631)
 L1CrossDomainMessenger_Test:test_L1MessengerSendMessage() (gas: 172148)
 L1CrossDomainMessenger_Test:test_L1MessengerTwiceSendMessage() (gas: 1254131)
 L1CrossDomainMessenger_Test:test_L1MessengerXDomainSenderReverts() (gas: 10566)
-L1CrossDomainMessenger_Test:test_L1MessengerxDomainMessageSenderResets() (gas: 58471)
+L1CrossDomainMessenger_Test:test_L1MessengerxDomainMessageSenderResets() (gas: 58535)
 L1StandardBridge_Test:test_depositERC20() (gas: 452873)
 L1StandardBridge_Test:test_depositERC20To() (gas: 454650)
 L1StandardBridge_Test:test_depositETH() (gas: 247054)
@@ -49,13 +49,13 @@ L1StandardBridge_Test:test_receive() (gas: 391794)
 L2CrossDomainMessenger_Test:testCannot_L2MessengerPause() (gas: 10843)
 L2CrossDomainMessenger_Test:test_L2MessengerMessageVersion() (gas: 8410)
 L2CrossDomainMessenger_Test:test_L2MessengerPause() (gas: 31837)
-L2CrossDomainMessenger_Test:test_L2MessengerRelayMessageSucceeds() (gas: 57474)
+L2CrossDomainMessenger_Test:test_L2MessengerRelayMessageSucceeds() (gas: 57538)
 L2CrossDomainMessenger_Test:test_L2MessengerRelayMessageToSystemContract() (gas: 24567)
 L2CrossDomainMessenger_Test:test_L2MessengerRelayShouldRevertIfPaused() (gas: 41599)
 L2CrossDomainMessenger_Test:test_L2MessengerSendMessage() (gas: 119659)
 L2CrossDomainMessenger_Test:test_L2MessengerTwiceSendMessage() (gas: 133096)
 L2CrossDomainMessenger_Test:test_L2MessengerXDomainSenderReverts() (gas: 10588)
-L2CrossDomainMessenger_Test:test_L2MessengerxDomainMessageSenderResets() (gas: 54795)
+L2CrossDomainMessenger_Test:test_L2MessengerxDomainMessageSenderResets() (gas: 54859)
 L2OutputOracleTest:testCannot_appendCurrentTimestamp() (gas: 18627)
 L2OutputOracleTest:testCannot_appendEmptyOutput() (gas: 16756)
 L2OutputOracleTest:testCannot_appendFutureTimestamp() (gas: 18708)
@@ -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: 93169)
+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: 352626)
-L2StandardBridge_Test:test_withdrawTo() (gas: 353477)
+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)
@@ -89,17 +91,18 @@ OVM_ETH_Test:test_metadata() (gas: 15608)
 OVM_ETH_Test:test_mint() (gas: 10621)
 OVM_ETH_Test:test_transfer() (gas: 10726)
 OVM_ETH_Test:test_transferFrom() (gas: 13008)
-OptimismMintableERC20_Test:test_bridge() (gas: 9850)
+OptimismMintableERC20_Test:test_bridge() (gas: 9785)
 OptimismMintableERC20_Test:test_burn() (gas: 52791)
 OptimismMintableERC20_Test:test_burnRevertsFromNotBridge() (gas: 13211)
+OptimismMintableERC20_Test:test_erc165_supportsInterface() (gas: 7828)
 OptimismMintableERC20_Test:test_l1Token() (gas: 9779)
-OptimismMintableERC20_Test:test_l2Bridge() (gas: 9768)
-OptimismMintableERC20_Test:test_mint() (gas: 65732)
-OptimismMintableERC20_Test:test_mintRevertsFromNotBridge() (gas: 13213)
-OptimismMintableERC20_Test:test_remoteToken() (gas: 9762)
+OptimismMintableERC20_Test:test_l2Bridge() (gas: 9790)
+OptimismMintableERC20_Test:test_mint() (gas: 65687)
+OptimismMintableERC20_Test:test_mintRevertsFromNotBridge() (gas: 13235)
+OptimismMintableERC20_Test:test_remoteToken() (gas: 9784)
 OptimismMintableTokenFactory_Test:test_bridge() (gas: 9707)
-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: 11413)
@@ -161,4 +164,4 @@ SequencerFeeVault_Test:test_constructor() (gas: 7611)
 SequencerFeeVault_Test:test_minWithdrawalAmount() (gas: 5429)
 SequencerFeeVault_Test:test_receive() (gas: 17280)
 SequencerFeeVault_Test:test_revertWithdraw() (gas: 9245)
-SequencerFeeVault_Test:test_withdraw() (gas: 147274)
+SequencerFeeVault_Test:test_withdraw() (gas: 147297)

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

@@ -119,6 +119,10 @@ 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. 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,14 +73,15 @@ 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,
         uint256 _l2BlockTime,
         bytes32 _genesisL2Output,
         uint256 _historicalTotalBlocks,
         uint256 _startingBlockTimestamp,
-        address sequencer
+        address _sequencer
     ) {
         require(
             _submissionInterval % _l2BlockTime == 0,
@@ -97,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/L1/OptimismPortal.sol

@@ -67,6 +67,16 @@ contract OptimismPortal is ResourceMetering {
      */
     address public l2Sender = DEFAULT_L2_SENDER;
 
+    /**
+     * @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;
+
     /**
      * @notice A list of withdrawal hashes which have been successfully finalized.
      */
@@ -84,10 +94,11 @@ contract OptimismPortal is ResourceMetering {
     /**
      * @notice Accepts value so that users can send ETH directly to this contract and have the
      *         funds be deposited to their address on L2. This is intended as a convenience
-     *         function for EOAs. Contracts should call the depositTransaction() function directly.
+     *         function for EOAs. Contracts should call the 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(""));
     }
 
     /**
@@ -221,7 +232,7 @@ contract OptimismPortal is ResourceMetering {
         // target contract is at least the gas limit specified by the user. We can do this by
         // enforcing that, at this point in time, we still have gaslimit + buffer gas available.
         require(
-            gasleft() >= _gasLimit + 20000,
+            gasleft() >= _gasLimit + FINALIZE_GAS_BUFFER,
             "OptimismPortal: insufficient gas to finalize withdrawal"
         );
 

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

@@ -11,6 +11,11 @@ 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.
+ *         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
  */
 contract L2StandardBridge is StandardBridge {
     /**
@@ -96,13 +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. 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.
@@ -168,10 +177,11 @@ contract L2StandardBridge is StandardBridge {
     ) 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);
         } else {
             _initiateBridgeERC20(_l2Token, l1Token, _from, _to, _amount, _minGasLimit, _data);
         }
-        emit WithdrawalInitiated(l1Token, _l2Token, msg.sender, _to, _amount, _data);
+        emit WithdrawalInitiated(l1Token, _l2Token, _from, _to, _amount, _data);
     }
 }

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

@@ -39,6 +39,19 @@ 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 *
+     *************/
+
     /**
      * @notice Includes the message hashes for all withdrawals
      */
@@ -53,7 +66,7 @@ contract L2ToL1MessagePasser {
      * @notice Allows users to withdraw ETH by sending directly to this contract.
      */
     receive() external payable {
-        initiateWithdrawal(msg.sender, 100000, bytes(""));
+        initiateWithdrawal(msg.sender, RECEIVE_DEFAULT_GAS_LIMIT, bytes(""));
     }
 
     /**

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

@@ -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 OptimismMintableERC20_Test is Bridge_Initializer {
     event Mint(address indexed _account, uint256 _amount);
@@ -64,4 +66,20 @@ contract OptimismMintableERC20_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));
+
+        bytes4 iface2 = L2Token.l1Token.selector ^ L2Token.mint.selector ^ L2Token.burn.selector;
+        assertEq(iface2, type(IL1Token).interfaceId);
+        assert(L2Token.supportsInterface(iface2));
+
+        bytes4 iface3 = L2Token.remoteToken.selector ^ L2Token.mint.selector ^ L2Token.burn.selector;
+        assertEq(iface3, type(IRemoteToken).interfaceId);
+        assert(L2Token.supportsInterface(iface3));
+    }
 }

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

@@ -54,7 +54,13 @@ 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;
+
+    /// @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;
 
     /*************
      * Variables *
@@ -209,12 +215,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

packages/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;
     }
 
     /**

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

@@ -41,6 +41,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,