Commit bceb2e56 authored by Maurelian's avatar Maurelian Committed by GitHub

Merge pull request #2763 from ethereum-optimism/m/oz-low-on-develop

parents 9aa8049c d78d7226
---
'@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
This diff is collapsed.
This diff is collapsed.
......@@ -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)
......@@ -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.
......
......@@ -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,
......@@ -80,7 +81,7 @@ contract L2OutputOracle is Ownable {
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);
}
/*********************************
......
......@@ -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"
);
......
......@@ -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);
}
}
......@@ -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(""));
}
/**
......
......@@ -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
......
......@@ -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));
}
}
......@@ -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
......
......@@ -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;
}
/**
......
......@@ -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,
......
......@@ -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";
......@@ -67,6 +68,15 @@ abstract contract StandardBridge {
bytes _data
);
/*************
* 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 *
*************/
......@@ -128,7 +138,7 @@ abstract contract StandardBridge {
* 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(""));
}
/**
......@@ -139,7 +149,8 @@ abstract contract StandardBridge {
}
/**
* @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,
......@@ -206,7 +217,7 @@ abstract contract StandardBridge {
emit ETHBridgeFinalized(_from, _to, _amount, _data);
(bool success, ) = _to.call{ value: _amount }(new bytes(0));
require(success, "TransferHelper::safeTransferETH: ETH transfer failed");
require(success, "ETH transfer failed.");
}
/**
......@@ -318,7 +329,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);
......@@ -376,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);
}
/**
......
// 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;
}
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment