Skip to content
This repository has been archived by the owner on May 26, 2023. It is now read-only.

sherlock-audit/2023-01-optimism-judging

BranchesTags

Folders and files

NameName
Last commit message
Last commit date

Latest commit

 

History

26 Commits

.data

.data

 
 

.github/workflows

.github/workflows

 
 

001-H

001-H

 
 

002-H

002-H

 
 

003-H

003-H

 
 

004-H

004-H

 
 

005-M

005-M

 
 

006-M

006-M

 
 

007-M

007-M

 
 

008-M

008-M

 
 

009-M

009-M

 
 

010-M

010-M

 
 

011-M

011-M

 
 

012-M

012-M

 
 

013-M

013-M

 
 

014-M

014-M

 
 

015-L

015-L

 
 

016-L

016-L

 
 

017-L

017-L

 
 

018-L

018-L

 
 

019-L

019-L

 
 

020-L

020-L

 
 

021-L

021-L

 
 

022-L

022-L

 
 

023-L

023-L

 
 

024-L

024-L

 
 

025-L

025-L

 
 

026-L

026-L

 
 

027-L

027-L

 
 

028-L

028-L

 
 

029-L

029-L

 
 

030-L

030-L

 
 

031-L

031-L

 
 

032-L

032-L

 
 

033-L

033-L

 
 

034-L

034-L

 
 

035-L

035-L

 
 

false

false

 
 

003.md

003.md

 
 

005.md

005.md

 
 

007.md

007.md

 
 

008.md

008.md

 
 

009.md

009.md

 
 

010.md

010.md

 
 

013.md

013.md

 
 

015.md

015.md

 
 

018.md

018.md

 
 

019.md

019.md

 
 

020.md

020.md

 
 

021.md

021.md

 
 

022.md

022.md

 
 

023.md

023.md

 
 

024.md

024.md

 
 

025.md

025.md

 
 

028.md

028.md

 
 

029.md

029.md

 
 

030.md

030.md

 
 

031.md

031.md

 
 

032.md

032.md

 
 

033.md

033.md

 
 

034.md

034.md

 
 

035.md

035.md

 
 

036.md

036.md

 
 

037.md

037.md

 
 

038.md

038.md

 
 

039.md

039.md

 
 

040.md

040.md

 
 

041.md

041.md

 
 

042.md

042.md

 
 

043.md

043.md

 
 

044.md

044.md

 
 

045.md

045.md

 
 

046.md

046.md

 
 

047.md

047.md

 
 

048.md

048.md

 
 

049.md

049.md

 
 

050.md

050.md

 
 

052.md

052.md

 
 

054.md

054.md

 
 

056.md

056.md

 
 

059.md

059.md

 
 

060.md

060.md

 
 

061.md

061.md

 
 

064.md

064.md

 
 

065.md

065.md

 
 

067.md

067.md

 
 

068.md

068.md

 
 

069.md

069.md

 
 

070.md

070.md

 
 

072.md

072.md

 
 

073.md

073.md

 
 

074.md

074.md

 
 

075.md

075.md

 
 

076.md

076.md

 
 

077.md

077.md

 
 

078.md

078.md

 
 

079.md

079.md

 
 

081.md

081.md

 
 

082.md

082.md

 
 

083.md

083.md

 
 

Repository files navigation

Issue H-1: Malicious user can finalize other’s withdrawal with less than specified gas limit, leading to loss of funds

Source: #109

Found by

obront

Summary

Transactions to execute a withdrawal from the Optimism Portal can be sent with 5122 less gas than specified by the user, because the check is performed a few operations prior to the call. Because there are no replays on this contract, the result is that a separate malicious user can call finalizeWithdrawalTransaction() with a precise amount of gas, cause the withdrawer’s withdrawal to fail, and permanently lock their funds.

Vulnerability Detail

Withdrawals can be initiated directly from the L2ToL1MessagePasser contract on L2. These withdrawals can be withdrawn directly from the OptimismPortal on L1. This path is intended to be used only by users who know what they are doing, presumably to save the gas of going through the additional more “user-friendly” contracts.

One of the quirks of the OptimismPortal is that there is no replaying of transactions. If a transaction fails, it will simply fail, and all ETH associated with it will remain in the OptimismPortal contract. Users have been warned of this and understand the risks, so Optimism takes no responsibility for user error.

However, there is an issue in the implementation of OptimismPortal that a withdrawal transaction can be executed with 5122 gas less than the user specified. In many cases, this could cause their transaction to revert, without any user error involved. Optimism is aware of the importance of this property being correct when they write in the comments:

We want to maintain the property that the amount of gas supplied to the call to the 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.

This property is not maintained because of the gap between the check and the execution.

The check is as follows, where FINALIZE_GAS_BUFFER == 20_000:

require(
    gasleft() >= _tx.gasLimit + FINALIZE_GAS_BUFFER,
    "OptimismPortal: insufficient gas to finalize withdrawal"
);

After this check, we know that the current execution context has at least 20,000 more gas than the gas limit. However, we then proceed to spend gas by (a) assigning the l2Sender storage variable, which uses 2900 gas because it’s assigning from a non-zero value, and (b) perform some additional operations to prepare the contract for the external call.

The result is that, by the time the call is sent with gasleft() - FINALIZE_GAS_BUFFER gas, gasleft() is 5122 lower than it was in the initial check.

Mathematically, this can be expressed as:

  • gasAtCheck >= gasLimit + 20000
  • gasSent == gasAtCall - 20000
  • gasAtCall == gasAtCheck - 5122

Rearranging, we get gasSent >= gasLimit + 20000 - 5122 - 20000, which simplifies to gasSent >= gasLimit - 5122.

Impact

For any withdrawal where a user sets their gas limit within 5122 of the actual gas their execution requires, a malicious user can call finalizeWithdrawalTransaction() on their behalf with enough gas to pass the check, but not enough for execution to succeed.

The result is that the withdrawing user will have their funds permanently locked in the OptimismPortal contract.

Proof of Concept

To test this behavior in a sandboxed environment, you can copy the following proof of concept.

Here are three simple contracts that replicate the behavior of the Portal, as well as an external contract that uses a predefined amount of gas.

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;

library SafeCall {
    /**
     * @notice Perform a low level call without copying any returndata
     *
     * @param _target   Address to call
     * @param _gas      Amount of gas to pass to the call
     * @param _value    Amount of value to pass to the call
     * @param _calldata Calldata to pass to the call
     */
    function call(
        address _target,
        uint256 _gas,
        uint256 _value,
        bytes memory _calldata
    ) internal returns (bool) {
        bool _success;
        assembly {
            _success := call(
                _gas, // gas
                _target, // recipient
                _value, // ether value
                add(_calldata, 0x20), // inloc
                mload(_calldata), // inlen
                0, // outloc
                0 // outlen
            )
        }
        return _success;
    }
}

contract GasUser {
    uint[] public s;

    function store(uint i) public {
        for (uint j = 0; j < i; j++) {
            s.push(1);
        }
    }
}

contract Portal {
    address l2Sender;

    struct Transaction {
        uint gasLimit;
        address sender;
        address target;
        uint value;
        bytes data;
    }

    constructor(address _l2Sender) {
        l2Sender = _l2Sender;
    }

    function execute(Transaction memory _tx) public {
        require(
            gasleft() >= _tx.gasLimit + 20000,
            "OptimismPortal: insufficient gas to finalize withdrawal"
        );

        // Set the l2Sender so contracts know who triggered this withdrawal on L2.
        l2Sender = _tx.sender;

        // Trigger the call to the target contract. We use SafeCall because we don't
        // care about the returndata and we don't want target contracts to be able to force this
        // call to run out of gas via a returndata bomb.
        bool success = SafeCall.call(
            _tx.target,
            gasleft() - 20000,
            _tx.value,
            _tx.data
        );
    }
}

Here is a Foundry test that calls the Portal with various gas values to expose this vulnerability:

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;

import "forge-std/Test.sol";
import "../src/Portal.sol";

contract PortalGasTest is Test {
    Portal public c;
    GasUser public gu;

    function setUp() public {
        c = new Portal(0x000000000000000000000000000000000000dEaD);
        gu = new GasUser();
    }

    function testGasLimitForGU() public {
        gu.store{gas: 44_602}(1);
        assert(gu.s(0) == 1);
    }

    function _executePortalWithGivenGas(uint gas) public {
        c.execute{gas: gas}(Portal.Transaction({
            gasLimit: 44_602,
            sender: address(69),
            target: address(gu),
            value: 0,
            data: abi.encodeWithSignature("store(uint256)", 1)
        }));
    }

    function testPortalCatchesGasTooSmall() public {
        vm.expectRevert(bytes("OptimismPortal: insufficient gas to finalize withdrawal"));
        _executePortalWithGivenGas(65681);
    }

    function testPortalSucceedsWithEnoughGas() public {
        _executePortalWithGivenGas(70803);
        assert(gu.s(0) == 1);
    }

    function testPortalBugWithInBetweenGasLow() public {
        _executePortalWithGivenGas(65682);
        
        // It now reverts because the array has a length of 0.
        vm.expectRevert();
        gu.s(0);
    }

    function testPortalBugWithInBetweenGasHigh() public {
        _executePortalWithGivenGas(70802);
        
        // It now reverts because the array has a length of 0.
        vm.expectRevert();
        gu.s(0);
    }
}

Summarizing the results of this test:

  • We verify that the call to the target contract succeeds with 44,602 gas, and set that as gasLimit for all tests.
  • When we send 65,681 or less gas, it’s little enough to be caught by the require statement.
  • When we send 70,803 or more gas, the transaction will succeed.
  • When we send any amount of gas between these two values, the require check is passed but the transaction fails.

Code Snippet

https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/packages/contracts-bedrock/contracts/L1/OptimismPortal.sol#L310-L329

https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/packages/contracts-bedrock/contracts/libraries/SafeCall.sol#L17-L36

Tool used

Manual Review

Recommendation

Instead of using one value for FINALIZE_GAS_BUFFER, two separate values should be used that account for the gas used between the check and the call.

Discussion

rcstanciu

Comment from Optimism

Description: Malicious user can finalize other’s withdrawal with less than specified gas limit, leading to loss of funds

Reason: This issue allows for attackers to permissionlessly brick a withdrawal transaction that specifies a minimum gas limit < required_gas + 5122. Most users will simply simulate the L1 transaction to identify the minimum gas needed for their withdrawal tx without including any padding, implying a high likelihood that this will affect a majority of our withdrawal transactions. Great find.

Action: In order to fix this issue, we need to factor in the gas used between the check on L313 of OptimismPortal.sol and the call to the destination on L324. Exploit PoC: here

Issue H-2: Withdrawals with high gas limits can be bricked by a malicious user, permanently locking funds

Source: #96

Found by

Allarious, obront, ladboy233, Barichek

Summary

Transactions to execute a withdrawal from the Optimism Portal require the caller to send enough gas to cover gasLimit specified by the withdrawer.

Because the EVM limits the total gas forwarded on to 63/64ths of the total gasleft() (and silently reduces it to this value if we try to send more) there are situations where transactions with high gas limits will be vulnerable to being reverted.

Because there are no replays on this contract, the result is that a malicious user can call finalizeWithdrawalTransaction() with a precise amount of gas, cause the withdrawer’s withdrawal to fail, and permanently lock their funds.

Vulnerability Detail

Withdrawals can be withdrawn from L2's L2ToL1MessagePasser contract to L1's OptimismPortal contract. This is a less "user-friendly" withdrawal path, presumably for users who know what they are doing.

One of the quirks of the OptimismPortal is that there is no replaying of transactions. If a transaction fails, it will simply fail, and all ETH associated with it will remain in the OptimismPortal contract. Users have been warned of this and understand the risks, so Optimism takes no responsibility for user error.

In order to ensure that failed transactions can only happen at the fault of the user, the contract implements a check to ensure that the gasLimit is sufficient:

require(
    gasleft() >= _tx.gasLimit + FINALIZE_GAS_BUFFER,
    "OptimismPortal: insufficient gas to finalize withdrawal"
);

When the transaction is executed, the contract requests to send along all the remaining gas, minus the hardcoded FINALIZE_GAS_BUFFER for actions after the call. The goal is that this will ensure that the amount of gas forwarded on is at least the gas limit specified by the user.

Optimism is aware of the importance of this property being correct when they write in the comments:

“We want to maintain the property that the amount of gas supplied to the call to the 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.”

The issue is that the EVM specifies the maximum gas that can be sent to an external call as 63/64ths of the gasleft(). For very large gas limits, this 1/64th that remains could be greater than the hardcoded FINALIZE_GAS_BUFFER value. In this case, less gas would be forwarded along than was directed by the contract.

Here is a quick overview of the math:

  • We need X gas to be sent as a part of the call.
  • This means we need X * 64 / 63 gas to be available at the time the function is called.
  • However, the only check is that we have X + 20_000 gas a few operations prior to the call (which guarantees that we have X + 14878 at the time of the call).
  • For any situation where X / 64 > 14878 (in other words, when the amount of gas sent is greater than 952_192), the caller is able to send an amount of gas that passes the check, but doesn't forward the required amount on in the call.

Impact

For any withdrawal with a gas limit of at least 952,192, a malicious user can call finalizeWithdrawalTransaction() with an amount of gas that will pass the checks, but will end up forwarding along less gas than was specified by the user.

The result is that the withdrawing user can have their funds permanently locked in the OptimismPortal contract.

Proof of Concept

To test this behavior in a sandboxed environment, you can copy the following proof of concept.

Here are three simple contracts that replicate the behavior of the Portal, as well as an external contract that uses a predefined amount of gas.

(Note that we added 5122 to the gas included in the call to correct for the other bug we submitted, as this issue remains even when the other bug is patched.)

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;

library SafeCall {
    /**
     * @notice Perform a low level call without copying any returndata
     *
     * @param _target   Address to call
     * @param _gas      Amount of gas to pass to the call
     * @param _value    Amount of value to pass to the call
     * @param _calldata Calldata to pass to the call
     */
    function call(
        address _target,
        uint256 _gas,
        uint256 _value,
        bytes memory _calldata
    ) internal returns (bool) {
        bool _success;
        assembly {
            _success := call(
                _gas, // gas
                _target, // recipient
                _value, // ether value
                add(_calldata, 0x20), // inloc
                mload(_calldata), // inlen
                0, // outloc
                0 // outlen
            )
        }
        return _success;
    }
}

contract GasUser {
    uint[] public s;

    function store(uint i) public {
        for (uint j = 0; j < i; j++) {
            s.push(1);
        }
    }
}

contract Portal {
    address l2Sender;

    struct Transaction {
        uint gasLimit;
        address sender;
        address target;
        uint value;
        bytes data;
    }

    constructor(address _l2Sender) {
        l2Sender = _l2Sender;
    }

    function execute(Transaction memory _tx) public {
        require(
            gasleft() >= _tx.gasLimit + 20000,
            "OptimismPortal: insufficient gas to finalize withdrawal"
        );

        // Set the l2Sender so contracts know who triggered this withdrawal on L2.
        l2Sender = _tx.sender;

        // Trigger the call to the target contract. We use SafeCall because we don't
        // care about the returndata and we don't want target contracts to be able to force this
        // call to run out of gas via a returndata bomb.
        bool success = SafeCall.call(
            _tx.target,
            gasleft() - 20000 + 5122, // fix for other bug
            _tx.value,
            _tx.data
        );
    }
}

Here is a Foundry test that calls the Portal with various gas values to expose this vulnerability:

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;

import "forge-std/Test.sol";
import "../src/Portal.sol";

contract PortalGasTest is Test {
    Portal public c;
    GasUser public gu;

    function setUp() public {
        c = new Portal(0x000000000000000000000000000000000000dEaD);
        gu = new GasUser();
    }

    function testGasLimitForGU() public {
        gu.store{gas: 11_245_655}(500);
        assert(gu.s(499) == 1);
    }

    function _executePortalWithGivenGas(uint gas) public {
        c.execute{gas: gas}(Portal.Transaction({
            gasLimit: 11_245_655,
            sender: address(69),
            target: address(gu),
            value: 0,
            data: abi.encodeWithSignature("store(uint256)", 500)
        }));
    }

    function testPortalCatchesGasTooSmall() public {
        vm.expectRevert(bytes("OptimismPortal: insufficient gas to finalize withdrawal"));
        _executePortalWithGivenGas(11_266_734);
    }

    function testPortalSucceedsWithEnoughGas() public {
        _executePortalWithGivenGas(11_433_180);
        assert(gu.s(499) == 1);
    }

    function testPortalBugWithInBetweenGasLow() public {
        _executePortalWithGivenGas(11_266_735);
        
        // It now reverts because the array has a length of 0.
        vm.expectRevert();
        gu.s(0);
    }

    function testPortalBugWithInBetweenGasHigh() public {
        _executePortalWithGivenGas(11_433_179);
        
        // It now reverts because the array has a length of 0.
        vm.expectRevert();
        gu.s(0);
    }
}

As you can see:

  • We verify that the call to the target contract succeeds with 11,245,655 gas, and set that as gasLimit for all tests. This is the X from our formula above.
  • This means that we need 11_245_655 * 64 / 63 = 11_424_157 gas available at the time the call is made.
  • The test uses 9023 gas before it makes our call, so we can see that if we send 11_424_157 + 9_023 = 11_433_180 gas, the test passes.
  • Similarly, if we send 11_266_734 gas, the total gas will be small enough to fail the require check.
  • But in the sweet spot between these values, we have enough gas to pass the require check, but when we get to the call, the amount of gas requested is more than 63/64ths of the total, so the EVM sends less than we asked for. As a result, the transaction fails.

Code Snippet

https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/packages/contracts-bedrock/contracts/L1/OptimismPortal.sol#L310-L329

https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/packages/contracts-bedrock/contracts/libraries/SafeCall.sol#L17-L36

Tool used

Manual Review

Recommendation

Change the check to account for this 63/64 rule:

require(
    gasleft() >= (_tx.gasLimit + FINALIZE_GAS_BUFFER) * 64 / 63,
    "OptimismPortal: insufficient gas to finalize withdrawal"
);

Discussion

rcstanciu

Comment from Optimism

Description: Withdrawals with high gas limits can be bricked by a malicious user, permanently locking funds

Reason: The issue is that we do not account for the 63/64ths rule specified in EIP-150 when checking the remaining gas available for executing a withdrawal transaction. This means that a malicious user can permissionlessly brick a withdrawal that has a minimum gas limit > 952,192 by forcing the OptimismPortal to forward an incorrect amount of gas, causing the withdrawal transaction to fail unexpectedly.

Action: In order to fix this issue, we need to factor in the 63/64 rule specified in EIP-150 when checking the remaining gas available for executing a withdrawal transaction on L313 of the OptimismPortal contract. See Zach's suggestion. (VERY IMPORTANT NOTE: This issue exists independently of #109.)

Issue H-3: Causing users lose their fund during finalizing withdrawal transaction

Source: #87

Found by

HE1M

Summary

A malicious user can make users lose their fund during finalizing their withdrawal. This is possible due to presence of reentrancy guard on the function relayMessage.

Vulnerability Detail

  • Bob (a malicious user) creates a contract (called AttackContract) on L1.
// SPDX-License-Identifier: MIT
pragma solidity 0.8.0;

struct WithdrawalTransaction {
   uint256 nonce;
   address sender;
   address target;
   uint256 value;
   uint256 gasLimit;
   bytes data;
}

interface IOptimismPortal {
   function finalizeWithdrawalTransaction(WithdrawalTransaction memory _tx)
       external;
}

contract AttackContract {
   bool public donotRevert;
   bytes metaData;
   address optimismPortalAddress;

   constructor(address _optimismPortal) {
       optimismPortalAddress = _optimismPortal;
   }

   function enableRevert() public {
       donotRevert = true;
   }

   function setMetaData(WithdrawalTransaction memory _tx) public {
       metaData = abi.encodeWithSelector(
           IOptimismPortal.finalizeWithdrawalTransaction.selector,
           _tx
       );
   }

   function attack() public {
       if (!donotRevert) {
           revert();
       } else {
           optimismPortalAddress.call(metaData);
       }
   }
}

        if (!donotRevert) {
            revert();
        }
  • Then, Bob calls the function enableRevert to set donotRevert to true. So that if later the function attack() is called again, it will not revert.
    function enableRevert() public {
        donotRevert = true;
    }
  • Then, Bob notices that Alice is withdrawing large amount of fund from L2 to L1. Her withdrawal transaction is proved but she is waiting for the challenge period to be finished to finalize it.
  • Then, Bob calls the function setMetaData on the contract AttackContract with the following parameter:
    • _tx = Alice's withdrawal transaction
  • By doing so, the metaData will be equal to finalizeWithdrawalTransaction.selector + Alice's withdrawal transaction.
   function setMetaData(WithdrawalTransaction memory _tx) public {
        metaData = abi.encodeWithSelector(
            IOptimismPortal.finalizeWithdrawalTransaction.selector,
            _tx
        );
    }
  • Now, after the challenge period is passed, and before the function finalizeWithdrawalTransaction is called by anyone (Alice), Bob calls the function relayMessage with the required data to retry his previous failed message again.
  • This time, since donotRevert is true, the call to function attack() will not revert, instead the body of else clause will be executed.
        else {
            optimismPortalAddress.call(metaData);
        }

In summary the attack is as follows:

  1. Bob creates a malicious contract on L1 called AttackContract.
  2. Bob sends a message from L2 to L1 to call the function AttackContract.attack on L1.
  3. On L1 side, after the challenge period is passed, the function AttackContract.attack will be called.
  4. Message relay on L1 will be unsuccessful, because the function AttackContract.attack reverts. So, Bob's message will be flagged as failed message.
  5. Bob sets AttackContract.donotRevert to true.
  6. Bob waits for an innocent user to request withdrawal transaction.
  7. Bob waits for the innocent user's withdrawal transaction to be proved.
  8. Bob sets meta data in his malicious contract based on the innocent user's withdrawal transaction.
  9. Bob waits for the challenge period to be passed.
  10. After the challenge period is elapsed, Bob retries to relay his failed message again.
  11. CrossDomainMessenger.relayMessage will call the AttackContract.attack, then it calls OptimismPortal.finalizeWithdrawalTransaction to finalize innocent user's withdrawal transaction. Then, it calls CrossDomainMessenger.relayMessage, but it will be unsuccessful because of reentrancy guard.
  12. After finalizing the innocent user's withdrawal transaction, Bob's message will be flagged as successful.
  13. So, innocent user's withdrawal transaction is flagged as finalized, while it is not.

Impact

By doing this attack it is possible to prevent users from withdrawing their fund. Moreover, they lose their fund because withdrawal is flagged as finalized, but the withdrawal sent to L1CrossDomainMessanger was not successful.

Code Snippet

Tool used

Manual Review

Recommendation

Maybe it is better to use the following code instead of: https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/packages/contracts-bedrock/contracts/L1/OptimismPortal.sol#L324-L329

        try IL1CrossDomainMessanger.relayMessage(...) {} catch Error(string memory reason) {
            if (
                keccak256(abi.encodePacked(reason)) ==
                keccak256(abi.encodePacked("ReentrancyGuard: reentrant call"))
            ) {
                revert("finalizing should be reverted");
            }
        }

Discussion

rcstanciu

Comment from Optimism

Description: Causing users lose their fund during finalizing withdrawal transaction

Reason: This is a very creative exploit that takes advantage of the L1CrossDomainMessenger's reentrancy guard on relayMessage. By creating an attack contract that reverts the first time a message is relayed by the OptimismPortal to the L1CrossDomainMessenger, but can be toggled to call finalizeWithdrawalTransaction for a different withdrawal transaction when the attacker replays it via the L1CrossDomainMessenger, the attacker can force the honest withdrawal's call to the L1CrossDomainMessenger to revert, effectively bricking it. PoC: here

Action: If a withdrawal transaction fails, we should check to see if the _target of the WithdrawalTransaction is the L1CrossDomainMessenger. If so, check if the call's revert data is the reentrancy guard message. If it is, we should revert the finalizeWithdrawalTransaction message and NOT mark the withdrawal as finalized.

Issue M-1: Batcher frames are incorrectly decoded leading to consensus split

Source: #279

Found by

obront

Summary

There is an error in the implementation of how frames are decoded, which will allow invalid frames to be accepted. This can allow a malicious sequencer to cause a consensus split between op-node implementations and the (correct) reference implementation.

Vulnerability Detail

Optimism implements a highly efficient derivation scheme based on compression of L2 data that is sent to an L1 address. The spec clearly defines how this works. Channels are split into frames, each one encoded as defined here. Frames will be aggregated by the derivation driver.

Docs state that:

All data in a frame is fixed-size, except the frame_data. The fixed overhead is 16 + 2 + 4 + 1 = 23 bytes. Fixed-size frame metadata avoids a circular dependency with the target total data length, to simplify packing of frames with varying content length.

Specifically:

is_last is a single byte with a value of 1 if the frame is the last in the channel, 0 if there are frames in the channel. Any other value makes the frame invalid (it must be ignored by the rollup node).

Clearly, is_last is mandatory as per the specs. However, if we look at the code it will accept a frame even if is_last is not supplied.

Decoding of the frame is done in frame.go, in the UnmarshalBinary function. After reading the frame data, only the last byte remains.

if isLastByte, err := r.ReadByte(); err != nil && err != io.EOF {
	return fmt.Errorf("error reading final byte: %w", err)
} else if isLastByte == 0 {
	f.IsLast = false
	return err
} else if isLastByte == 1 {
	f.IsLast = true
	return err
} else {
	return errors.New("invalid byte as is_last")
}

If the ByteReader object is empty, reading the next byte will return an EOF and the error clause is skipped. The result of ReadByte when an error occurs is undefined, however in all Go setups we've tested isLastByte is zero. This means it sets f.IsLast = false and returns the EOF.

Back in ParseFrames which calls UnmarshalBinary, the EOF is ignored and the frame is accepted:

for buf.Len() > 0 {
	var f Frame
	if err := (&f).UnmarshalBinary(buf); err != io.EOF && err != nil {
		return nil, err
	}
	frames = append(frames, f)
}

So, it is demonstrated that an invalid frame is accepted by the Optimism implementation, provided the frame is the last one in the frames buffer. The impact is that a malicious sequencer can cause a consensus split between correct implementations and the reference implementation. It has been defined by the rules as Medium severity:

- Causing a consensus failure
- Explanation: There is one sequencer op-node submitting transaction batches to L1, but many verifier op-nodes will read these batches and check the results of its execution. The sequencer and verifiers must remain in consensus, even in the event of an L1 reorg.

All that is needed is to send in different frame packages two frames of a channel, omit the is_last byte in the first frame and make sure it is the last frame in the package.

Impact

Malicious sequencer can easily cause a consensus split by taking advantage of the incorrect frame reading logic in op-node.

Code Snippet

https://github.com/ethereum-optimism/optimism/blob/407f97b9d13448b766624995ec824d3059d4d4f6/op-node/rollup/derive/frame.go#L93

Tool used

Manual Review

Recommendation

In UnmarshalBinary, return a non-EOF error when is_last byte does not exist.

Discussion

rcstanciu

Comment from Optimism

Description: A malicious or buggy batcher could submit a frame that the op-node would accept that a spec compliant node would not due to a bug in our parsing code.

Reason: This would cause a consensus failure between different clients.

Action: We need to fix this code.

zobront

Escalate for 250 USDC

This submission concerns a specific consensus failure due to parsing of batcher frames.

It has been wrongly duped to #53, which concerns a contract level issue with posted output roots for withdrawals.

Optimism has acknowledged the validity of the issue and this is certainly just a misclick, as the issues are entirely unrelated.

sherlock-admin

Escalate for 250 USDC

This submission concerns a specific consensus failure due to parsing of batcher frames.

It has been wrongly duped to #53, which concerns a contract level issue with posted output roots for withdrawals.

Optimism has acknowledged the validity of the issue and this is certainly just a misclick, as the issues are entirely unrelated.

You've created a valid escalation for 250 USDC!

To remove the escalation from consideration: Delete your comment. To change the amount you've staked on this escalation: Edit your comment (do not create a new comment).

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

Evert0x

Escalation accepted and resolving incorrect duplication state

sherlock-admin

Escalation accepted and resolving incorrect duplication state

This issue's escalations have been accepted!

Contestants' payouts and scores will be updated according to the changes made on this issue.

Issue M-2: Censorship resistance is undermined and bridging of assets can be DOSed at low cost

Source: #277

Found by

obront, Robert, cergyk

Summary

All L1->L2 transactions go through OptimismPortal's depositTransaction function. It is wrapped through the metered modifier. The goal is to create a gas market for L1->L2 transactions and not allow L1 TXs to fill up L2 batches (as the gas for deposit TX in L2 is payed for by the system), but the mechanism used makes it too inexpensive for a malicious user to DOS and censor deposits.

Vulnerability Detail

It is possible for a malicious actor to snipe arbitrary L1->L2 transactions in the mempool for far too cheaply. This introduces two impacts:

  1. Undermines censorship resistance guarantees by Optimism
  2. Griefs users who simply want to bridge assets to L2

The core issue is the check in ResourceMetering.sol:

// Make sure we can actually buy the resource amount requested by the user.
params.prevBoughtGas += _amount;
require(
    int256(uint256(params.prevBoughtGas)) <= MAX_RESOURCE_LIMIT,
    "ResourceMetering: cannot buy more gas than available gas limit"
);

Note that params.prevBoughtGas is reset per block. This means attacker can view a TX in the mempool and wrap up the following flashbot bundle:

  1. Attacker TX to depositTransaction, with gasLimit = 8M (MAX_RESOURCE_LIMIT)
  2. Victim TX to depositTransaction

The result is that attacker's transaction will execute and victim's TX would revert. It is unknown how this affects the UI and whether victim would be able to resubmit this TX again easily, but regardless it's clearly griefing user's attempt to bridge an asset. Note that a reverted TX is different from an uncompleted TX from a UX point of view.

From a censorship resistance perspective, there is nothing inherently preventing attack to continually use this technique to block out all TXs, albert gas metering price will rise as will be discussed.

Now we can demonstrate the cost of the attack to be low. Gas burned by the modifier is calculated as:

// Determine the amount of ETH to be paid.
uint256 resourceCost = _amount * params.prevBaseFee;
...
uint256 gasCost = resourceCost / Math.max(block.basefee, 1000000000);

params.prevBaseFee is initialized at 1e9 and goes up per block by a factor of 1.375 when gas market is drained, while going down by 0.875 when gas market wasn't used at all.

If we take the initial value, resourceCost = 8e6 * 1e9 = 8e15. If we assume tip is negligible to block.basefee, L1 gas cost in ETH equals resourceCost (divide by basefee and multiply by basefee). Therefore, cost of this snipe TX is:

8e15 / 1e18 (ETH decimals) * 1600 (curr ETH price) = $12.80

The result is an extremely low price to pay, and even taking into account extra tips for frontrunning, is easily achievable.

In practice prevBaseFee will represent the market price for L2 gas. If it goes lower than initial value, DOSing will become cheaper, while if it goes higher it will become more expensive. The key problem is that the attacker's cost is too similar to the victim's cost. If victim is trying to pass a 400k TX, attacker needs to buy a 7.6M of gas. This gap is too small and the resulting situation is that for DOS to be too expensive for attacker, TX would have to be far too expensive for the average user.

Impact

Censorship resistance is undermined and bridging of assets can be DOSed at low cost.

Code Snippet

https://github.com/ethereum-optimism/optimism/blob/407f97b9d13448b766624995ec824d3059d4d4f6/packages/contracts-bedrock/contracts/L1/ResourceMetering.sol#L133

Tool used

Manual Review

Recommendation

It is admittedly difficult to balance the need for censorship resistance with the prevention of L2 flooding via L1 TXs. However, the current solution which will make a victim's TX revert at hacker's will is inadequate and will lead to severe UX issues for users.

Recommendation would be to not require gas spending to be under MAX_RESOURCE_LIMIT and solve it in L2 sequencing. It's already close enough to the L1 gas limit so that it can't be really abused.

Discussion

rcstanciu

Comment from Optimism

Description: Deposit griefing by front running and filling up the MAX_RESOURCE_LIMIT

Reason: DOS

Issue M-3: Panic when decoding a malformed deposit transaction JSON string

Source: #276

Found by

shw

Summary

When decoding a deposit transaction JSON string without the "gas" field, a panic/runtime error is triggered due to a nil pointer dereference.

Vulnerability Detail

The op-geth/core/types/transaction_marshalling.go file defines how transactions are encoded and decoded from JSON format. In the UnmarshalJSON() function, from L283 to L315, a new logic is added for the deposit transaction type, DepositTxType. The bug happens at L293, where the dec.Gas field is dereferenced without checking it against nil first. As a result, if the provided deposit transaction JSON string does not have the "gas" field, the nil pointer dereference will trigger a runtime error and crash the program.

For a PoC: Add the following test case to op-geth/core/types/transaction_test.go and run cd op-geth && go test -run TestDecodeJSON -v ./core/types

func TestDecodeJSON(t *testing.T) {
    // the "gas" field does not exist
    var data = []byte("{\"type\":\"0x7e\",\"nonce\":null,\"gasPrice\":null,\"maxPriorityFeePerGas\":null,\"maxFeePerGas\":null,\"value\":\"0x1\",\"input\":\"0x616263646566\",\"v\":null,\"r\":null,\"s\":null,\"to\":null,\"sourceHash\":\"0x0000000000000000000000000000000000000000000000000000000000000000\",\"from\":\"0x0000000000000000000000000000000000000001\",\"isSystemTx\":false,\"hash\":\"0xa4341f3db4363b7ca269a8538bd027b2f8784f84454ca917668642d5f6dffdf9\"}")
    var parsedTx = &Transaction{}
    _ = json.Unmarshal(data, &parsedTx) // will panic here
}

Impact

A possible exploit scenario is targeting an ethclient compiled based on op-geth. For example, according to op-geth/ethclient/ethclient.go, the BlockByHash() API makes a RPC eth_getBlockByHash request to an RPC endpoint and expect to receive a json.RawMessage data that represents the queried block. In the getBlock() function, the JSON raw message is then decoded into a rpcBlock, containing a list of rpcTransactions with type of Transaction. Therefore, a malicious RPC endpoint can construct a deposit transaction without the "gas" field in a block and return the block to the ethclient. The ethclient will fail to decode the received block and crash due to this bug.

Marking this issue as medium severity according to previous similar audit findings from Sigma Prime.

Code Snippet

Please refer to op-geth/core/types/transaction_marshalling.go#L293, op-geth/ethclient/ethclient.go#L77-L79 and op-geth/ethclient/ethclient.go#L118-L126. https://github.com/ethereum-optimism/op-geth/blob/985086bf2a5c61e76a8ce7c74ac029660751e260/core/types/transaction_marshalling.go#L293

Tool used

Manual Review

Recommendation

Check whether dec.Gas is nil before dereferencing it:

if dec.Gas == nil {
    return errors.New("missing required field 'gas' in transaction")
}

Discussion

rcstanciu

Comment from Optimism

Description: Panic in transaction unmarshalling code

Reason: This is a legit bug in our transaction unmarshalling code.

Action: Check for nil gas before unmarshalling

Issue M-4: [High] Function MigrateWithdrawal() may set gas limit so high for old withdrawals when migrating them by mistake and they can't be relayed in the L1 and users funds would be lost

Source: #235

Found by

unforgiven

Summary

Function MigrateWithdrawal() in migrate.go will turn a LegacyWithdrawal into a bedrock style Withdrawal. it should set a min gas limit value for the withdrawals. to calculate a gas limit contract overestimates it and if the value goes higher than L1 maximum gas in the block then the withdraw can't be relayed in the L1 and users funds would be lost while the withdraw could be possible before the migration it won't be possible after it.

Vulnerability Detail

This is MigrateWithdrawal() code:

// MigrateWithdrawal will turn a LegacyWithdrawal into a bedrock
// style Withdrawal.
func MigrateWithdrawal(withdrawal *LegacyWithdrawal, l1CrossDomainMessenger *common.Address) (*Withdrawal, error) {
	// Attempt to parse the value
	value, err := withdrawal.Value()
	if err != nil {
		return nil, fmt.Errorf("cannot migrate withdrawal: %w", err)
	}

	abi, err := bindings.L1CrossDomainMessengerMetaData.GetAbi()
	if err != nil {
		return nil, err
	}

	// Migrated withdrawals are specified as version 0. Both the
	// L2ToL1MessagePasser and the CrossDomainMessenger use the same
	// versioning scheme. Both should be set to version 0
	versionedNonce := EncodeVersionedNonce(withdrawal.Nonce, new(big.Int))
	// Encode the call to `relayMessage` on the `CrossDomainMessenger`.
	// The minGasLimit can safely be 0 here.
	data, err := abi.Pack(
		"relayMessage",
		versionedNonce,
		withdrawal.Sender,
		withdrawal.Target,
		value,
		new(big.Int),
		withdrawal.Data,
	)
	if err != nil {
		return nil, fmt.Errorf("cannot abi encode relayMessage: %w", err)
	}

	// Set the outer gas limit. This cannot be zero
	gasLimit := uint64(len(data)*16 + 200_000)

	w := NewWithdrawal(
		versionedNonce,
		&predeploys.L2CrossDomainMessengerAddr,
		l1CrossDomainMessenger,
		value,
		new(big.Int).SetUint64(gasLimit),
		data,
	)
	return w, nil
}

As you can see it sets the gas limit as gasLimit := uint64(len(data)*16 + 200_000) and contract set 16 gas per data byte but in Ethereum when data byte is 0 then the overhead intrinsic gas is 4 and contract overestimate the gas limit by setting 16 gas for each data. this can cause messages with big data(which calculated gas is higher than 30M) to not be relay able in the L1 because if transaction gas set lower than calculated gas then OptimisimPortal would reject it and if gas set higher than calculated gas then miners would reject the transaction. while if code correctly estimated the required gas the gas limit could be lower by the factor of 4. for example a message with about 2M zeros would get gas limit higher than 30M and it won't be withdrawable in the L1 while the real gas limit is 8M which is relayable.

Impact

some withdraw messages from L2 to L1 that could be relayed before the migration can't be relayed after the migration because of the wrong gas estimation.

Code Snippet

https://github.com/ethereum-optimism/optimism/blob/3f4b3c328153a8aa03611158b6984d624b17c1d9/op-chain-ops/crossdomain/migrate.go#L83-L88

Tool used

Manual Review

Recommendation

calculate gas estimation correctly, 4 for 0 bytes and 16 for none zero bytes.

Discussion

rcstanciu

Comment from Optimism

Description: Incorrect gas limit calculation for migrated withdrawals

Reason: This is a legitimate issue.

Action: Implement correct gas estimation

0xunforgiven

Escalate for 111 USDC

This should labeled as High because some of old legit withdrawals(L2 -> L1) won't be executed in the L1 after the bedrock migration. Those withdrawals belongs to L2CrossDomainMessages which promised to be delivered in L1.

Users may lose funds and there is no limit for it. there may be users who wants to withdraw 1M funds with L2StandardBridge and because of this issue their funds would be lost even so the L2StandardBridge is supposed to deliver the funds! The withdrawals with large amount of data isn't that rare, There may be other protocols or users in L2 which uses the bridge to store data in the L1 and send withdrawals with large data to the L2CrossDomainMessenger and because of the issue their withdrawal message won't be finalized in L1 after migration.

It is true that this would only happen to some of the withdrawal messages but as we have other gas related issues that blocks withdrawal of limited number of withdrawals(#96 which is only happen to withdrawals that would revert if gas was 5K lower and #109 which happened to withdrawals with higher than ~1M gas) and they considered as High so to be fair This issue should be High too.

This is direct fund loss if it happens to user ETH bridges and also cause L2CrossDomainMessenger's messages to not finalized and be replayable in L1 if it happens to it's withdrawal messages.

sherlock-admin

Escalate for 111 USDC

This should labeled as High because some of old legit withdrawals(L2 -> L1) won't be executed in the L1 after the bedrock migration. Those withdrawals belongs to L2CrossDomainMessages which promised to be delivered in L1.

Users may lose funds and there is no limit for it. there may be users who wants to withdraw 1M funds with L2StandardBridge and because of this issue their funds would be lost even so the L2StandardBridge is supposed to deliver the funds! The withdrawals with large amount of data isn't that rare, There may be other protocols or users in L2 which uses the bridge to store data in the L1 and send withdrawals with large data to the L2CrossDomainMessenger and because of the issue their withdrawal message won't be finalized in L1 after migration.

It is true that this would only happen to some of the withdrawal messages but as we have other gas related issues that blocks withdrawal of limited number of withdrawals(#96 which is only happen to withdrawals that would revert if gas was 5K lower and #109 which happened to withdrawals with higher than ~1M gas) and they considered as High so to be fair This issue should be High too.

This is direct fund loss if it happens to user ETH bridges and also cause L2CrossDomainMessenger's messages to not finalized and be replayable in L1 if it happens to it's withdrawal messages.

You've created a valid escalation for 111 USDC!

To remove the escalation from consideration: Delete your comment. To change the amount you've staked on this escalation: Edit your comment (do not create a new comment).

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

Evert0x

Escalation rejected as there is insufficient evidence for high severity.

sherlock-admin

Escalation rejected as there is insufficient evidence for high severity.

This issue's escalations have been rejected!

Watsons who escalated this issue will have their escalation amount deducted from their next payout.

Issue M-5: contract with only IOptimismMintableERC20 interface is not compatible with StandardBridge

Source: #220

Found by

Barichek, lemonmon

Summary

If a custom contract implements only the IOptimismMintableERC20, but no the ILegacyMintableERC20, the contract is not compatible with the StandardBridge, as the bridge uses the l1Token function from the legacy interface

Vulnerability Detail

The comment in the IOptimismMintableERC20 suggests that one can make a custom implementation of OptimismMintableERC20 using the interface IOptimismMintableERC20.

https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/packages/contracts-bedrock/contracts/universal/IOptimismMintableERC20.sol#L8-L10

Also, the StandardBridge, which uses the OptimismMintableERC20 has _isOptimismMintableERC20 function, which checks whether the given token address is implementing OptimismMintableERC20. The function will be true if either of ILegacyMintableERC20 or IOptimismMintableERC20 is implemented. it means that if a token implements only one of the interfaces, it will return true.

https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/packages/contracts-bedrock/contracts/universal/StandardBridge.sol#L446-L450

However, if the given token passes the _isOptimismMintableERC20, the legacy function l1Token will be called on the token. If the token does not implement the legacy interface, the call will fail.

https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/packages/contracts-bedrock/contracts/L2/L2StandardBridge.sol#L170

Therefore, the token which only implements IOptimismMintableERC20, but not the ILegacyMintableERC20, is not compatible with StandardBridge.

Impact

Any custom contract without l1Token function will not be compatible with StandardBridge

Code Snippet

https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/packages/contracts-bedrock/contracts/universal/IOptimismMintableERC20.sol#L8-L10

https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/packages/contracts-bedrock/contracts/universal/StandardBridge.sol#L446-L450

https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/packages/contracts-bedrock/contracts/L2/L2StandardBridge.sol#L170

Tool used

Manual Review

Recommendation

It is unclear it is intended behavior. If the _isOptimismMintableERC20 function returns true only when the both of interfaces are implemented, the token with only the IOptimismMintableERC20 will be treated as if they are not the optimism mintable function, without failing.

Issue M-6: Crafted p2p spam can render nodes permanently unable to process L2 blocks

Source: #177

Found by

usmannk

Summary

The constraints in BuildBlocksValidator are meant to prevent p2p spam from bogging down the network. However they are not sufficient for stopping spam, but are strong enough to render nodes unable to catch up.

Vulnerability Detail

The p2p.BuildBlocksValidator function has several constraints that must be met before a gossiped block is considered valid. If a block is considered valid then it is gossiped to peers and ingested into the current node.

The constraints on a gossiped block block are, in order:

  • block must be <10MB
  • block must be compressed as valid snappy data
  • block must be signed by the sequencer
  • block must be a valid SSZ encoded block
  • block.timestamp must within 60 seconds in the past and 5 seconds in the future
  • block.BlockHash must be correctly calculated
  • block.BlockNumber must not be in the set of the previous 100 unique block numbers that reached this step

These constraints are applied here:

https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/op-node/p2p/gossip.go#L208

However, it is often the case that there are over 100 valid L2 blocks emitted per minute. For example, the 253 blocks between block 71404110 and 71404363 were emitted within 60 seconds.

In this example, an attacker would take the 200 blocks from 71404110-71404310 and replay them in order to a node for 60 seconds. Every block would be valid because they each pass all constraints and the 100 block LRU cache for marking seen blocks would be evicted twice over during the process.

After 60 seconds, block 71404310 + 1 would be invalid to this node because its timestamp would be too old. At this point, for any new L2 block the node would either:

  • mark it as invalid because it is too old
  • or ingest it and throw it away because it is waiting for block 71404311

As a result, the node will no longer be able to successfully process any L2 blocks from the p2p network.

Furthermore, because all 200 blocks are marked as valid several times, the node would gossip all of these blocks (potentially several times) to its peers who would suffer the same effect and impact their peers.

Impact

Attackers may halt targeted nodes.

Further knock-on potential halting of the node's peers, peers of peers, and so on.

Code Snippet

Tool used

Manual Review

Recommendation

Increase the size of the blockHeightLRU cache to more effectively catch and reject previously seen nodes. It would be reasonable to set it to 1000 instead of 100. Even at 1000 the cache should occupy less than 500kb in memory.

Discussion

rcstanciu

Comment from Optimism

Description: P2P buffer is too small

Reason: This is a valid attack, wherein the attacker replays blocks and eclipses a node from the P2P network.

Action: Increase buffer size

Issue M-7: LES (Light Ethereum Subprotocol) doesn't forward the transaction to the sequencer

Source: #175

Found by

Koolex

Summary

LES (Light Ethereum Subprotocol) doesn't forward the transaction to the sequencer when receiving it over RPC.

Vulnerability Detail

When a user submits a transaction to op-geth node (validator/verfier mode), the node sends the transaction to the sequencer, if no error, it adds it to the tx pool.

func (b *EthAPIBackend) SendTx(ctx context.Context, tx *types.Transaction) error {
	if b.eth.seqRPCService != nil {
		data, err := tx.MarshalBinary()
		if err != nil {
			return err
		}
		if err := b.eth.seqRPCService.CallContext(ctx, nil, "eth_sendRawTransaction", hexutil.Encode(data)); err != nil {
			return err
		}
	}
	return b.eth.txPool.AddLocal(tx)
}

https://github.com/ethereum-optimism/op-geth/blob/optimism-history/eth/api_backend.go#L253-L264

However, when LES, It only adds the transaction to the tx pool.

func (b *LesApiBackend) SendTx(ctx context.Context, signedTx *types.Transaction) error {
	return b.eth.txPool.Add(ctx, signedTx)
}

https://github.com/ethereum-optimism/op-geth/blob/optimism-history/les/api_backend.go#L193-L195

Note: Sequencer http flag is configured only if we're running in verifier mode.

Impact

  • Transction isn't sent to the sequencer and will never be processed (submitted to L1).
  • Inconsistency among op-geth nodes validators/verifiers and the sequencer.
  • Additionally, from UX perspective, it is misleading as the user would think the transaction was submitted "successfully".

Code Snippet

Check above.

Tool used

Manual Review

Recommendation

Match this RPC change in the LES RPC. As it seems to be overlooked.

Ref: https://op-geth.optimism.io/

Discussion

rcstanciu

Comment from Optimism

Description: LES doesn't forward transactions to the sequencer

Reason: We don't support LES, so this is out-of-scope.

Action: No action.

koolexcrypto

Escalate for 53 USDC

I kindly ask you to reconsider the assessment due to the following reasons:

  1. In the contest description, it lists the components that are in scope:

    The key components of the system can be found in our monorepo at commit 3f4b3c3281. L1 Contracts L2 Contracts (AKA Predeploys) op-node op-geth (in its own repo)

    op-geth is listed as it is, and it is never mentioned anywhere in the docs that LES is out of scope. As a participant, I spent quite some time to look for bugs in LES. otherwise, I would have shifted my focus on some other parts of the component considering the short time of the contest. On a personal level, it feels a bit unfair.

  2. If you go over LES directory in the source code. You could see the code was adapted to match Optimism specs. This actually gave me a confirmation that it is in scope. Here are some examples:

sherlock-admin

Escalate for 53 USDC

I kindly ask you to reconsider the assessment due to the following reasons:

  1. In the contest description, it lists the components that are in scope:

The key components of the system can be found in our monorepo at commit 3f4b3c3281. L1 Contracts L2 Contracts (AKA Predeploys) op-node op-geth (in its own repo)

op-geth is listed as it is, and it is never mentioned anywhere in the docs that LES is out of scope. As a participant, I spent quite some time to look for bugs in LES. otherwise, I would have shifted my focus on some other parts of the component considering the short time of the contest. On a personal level, it feels a bit unfair.

  1. If you go over LES directory in the source code. You could see the code was adapted to match Optimism specs. This actually gave me a confirmation that it is in scope. Here are some examples:

You've created a valid escalation for 53 USDC!

To remove the escalation from consideration: Delete your comment. To change the amount you've staked on this escalation: Edit your comment (do not create a new comment).

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

Evert0x

Escalation accepted.

Labeling as medium severity.

This was an oversight on Optimism's part and there are markers that would suggest it should be in scope.

sherlock-admin

Escalation accepted.

Labeling as medium severity.

This was an oversight on Optimism's part and there are markers that would suggest it should be in scope.

This issue's escalations have been accepted!

Contestants' payouts and scores will be updated according to the changes made on this issue.

Issue M-8: Optimism Portal can run out of gas due to incorrect overhead estimation

Source: #138

Found by

GalloDaSballo

Summary

In contrast to CrossDomainMessenger which has a 5k gas buffer, the Optimism Portal doesn't, meaning all its relayed calls will have 5k+ less gas than intended.

This forces integrations (e.g. Bridges) to spend more gas by default, because of a logic flaw.

For this reason am filing the finding as Medium Severity:

  • Programming Mistake (Math is incorrect)
  • Call forwards less gas than intended and can revert because of it

Vulnerability Detail

CrossDomainMessenger will compute the gas-required, adding a 5k gas buffer https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/packages/contracts-bedrock/contracts/universal/CrossDomainMessenger.sol#L318-L324

It will then pass the remainder of the gas, minus the buffer as it's assumed to have been spent for the SSTORE

Optimism Portal on the other hand will not do that

Impact

By only checking that

gasleft() >= _tx.gasLimit + FINALIZE_GAS_BUFFER /// @audit At least gasLimit + buffer

The check will ensure that before the SSTORE + Call the buffer is available

However, the following SSTORE will consider 5k+ (5k for SSTORE, hundreds of gas for overhead)

This will leave the SafeCall with less gas than intended

            gasleft() - FINALIZE_GAS_BUFFER, /// @audit gasLeft will be < tx.gasLimit because we've subtracted the same constant

SafeCall will have less gas than intended, meaning that every integrator that estimates their TXs accurately will have their tx revert.

Code Snippet

https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/packages/contracts-bedrock/contracts/L1/OptimismPortal.sol#L310-L328

Tool used

Manual Review

Recommendation

Recompute the buffer to add the extra 5k + the overhead of the SSTORE (in the few hundreds of gas)

To ensure the call has enough gas, you may also consider swapping positions between the SSTORE and the require

        // Set the l2Sender so contracts know who triggered this withdrawal on L2.
        l2Sender = _tx.sender;

        require(
            gasleft() >= _tx.gasLimit + FINALIZE_GAS_BUFFER,
            "OptimismPortal: insufficient gas to finalize withdrawal"
        );

        // Trigger the call to the target contract. We use SafeCall because we don't
        // care about the returndata and we don't want target contracts to be able to force this
        // call to run out of gas via a returndata bomb.
        bool success = SafeCall.call(
            _tx.target,
            gasleft() - FINALIZE_GAS_BUFFER,
            _tx.value,
            _tx.data
        );

Discussion

rcstanciu

Comment from Optimism

Severity: medium

Reason: see 109

zobront

Escalate for 250 USDC

This report should not be a duplicate of #109.

For an issue to be judged as High, it requires the Watson to (a) understand the exploit and (b) provide a report with an adequate burden of proof for a High Severity Finding. This report does neither.

While the same underlying root cause is identified as #109, the Watson does not see the possibility that this could be used to brick legitimate withdrawals. Instead, it's seen merely as something that might happen accidentally. As a result, the Watson files the report as a Medium Severity, which is appropriate for what he found.

As a example, if you look at #109, you'll see:

  • an explicit exploit vector that justified High Severity
  • calculations for exactly how much extra gas is needed
  • a full, runnable POC
  • submitted as a high, accepted as a high

For this issue, we see:

  • thinks it can only happen by accident
  • no detailed explanation or gas calculation
  • no POC
  • submitted as only a medium
  • optimism's comment is that it was accepted as just a medium

I understand that Optimism's team simply lumped them together because they address the same issue. But based on Sherlock's rules, issues of different severity should not be dup'd together. For that reason, we believe this should be dedup'd from #109 and live as its own issue.

sherlock-admin

Escalate for 250 USDC

This report should not be a duplicate of #109.

For an issue to be judged as High, it requires the Watson to (a) understand the exploit and (b) provide a report with an adequate burden of proof for a High Severity Finding. This report does neither.

While the same underlying root cause is identified as #109, the Watson does not see the possibility that this could be used to brick legitimate withdrawals. Instead, it's seen merely as something that might happen accidentally. As a result, the Watson files the report as a Medium Severity, which is appropriate for what he found.

As a example, if you look at #109, you'll see:

  • an explicit exploit vector that justified High Severity
  • calculations for exactly how much extra gas is needed
  • a full, runnable POC
  • submitted as a high, accepted as a high

For this issue, we see:

  • thinks it can only happen by accident
  • no detailed explanation or gas calculation
  • no POC
  • submitted as only a medium
  • optimism's comment is that it was accepted as just a medium

I understand that Optimism's team simply lumped them together because they address the same issue. But based on Sherlock's rules, issues of different severity should not be dup'd together. For that reason, we believe this should be dedup'd from #109 and live as its own issue.

You've created a valid escalation for 250 USDC!

To remove the escalation from consideration: Delete your comment. To change the amount you've staked on this escalation: Edit your comment (do not create a new comment).

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

Evert0x

Escalation accepted.

Labeling as a unique medium (instead of duplicate high)

It's clear that the Watson didn't understand the full impact of the issue.

sherlock-admin

Escalation accepted.

Labeling as a unique medium (instead of duplicate high)

It's clear that the Watson didn't understand the full impact of the issue.

This issue's escalations have been accepted!

Contestants' payouts and scores will be updated according to the changes made on this issue.

Issue M-9: Migration can be bricked by sending a message directly to the LegacyMessagePasser

Source: #105

Found by

0xdeadbeef, obront, Bobface, unforgiven, xiaoming90

Summary

The migration process halts and returns an error if any of the withdrawal data breaks from the specified format. However, the data for this migration comes from every call that has been made to the LegacyMessagePasser (0x00) address, and it is possible to send a transaction that would violate the requirements. The result is that the migration process would be bricked and need to be rebuilt, with some difficult technical challenges that we'll outline below.

Vulnerability Detail

Withdrawal data is saved in l2geth whenever a call is made to the LegacyMessagePasser address:

if addr == dump.MessagePasserAddress {
    statedumper.WriteMessage(caller.Address(), input)
}

This will save all the calls that came via the L2CrossDomainMessenger. The expected format for the data is encoded in the L2CrossDomainMessenger. It encodes the calldata to be executed on the L1 side as: abi.encodeWithSignature("relayMessage(...)", target, sender, message, nonce)

The migration process expects the calldata to follow this format, and expects the call to come from L2CrossDomainMessenger, implemented with the following two checks:

selector := crypto.Keccak256([]byte("relayMessage(address,address,bytes,uint256)"))[0:4]
if !bytes.Equal(data[0:4], selector) {
    return fmt.Errorf("invalid selector: 0x%x", data[0:4])
}

msgSender := data[len(data)-len(predeploys.L2CrossDomainMessengerAddr):]
if !bytes.Equal(msgSender, predeploys.L2CrossDomainMessengerAddr.Bytes()) {
    return errors.New("invalid msg.sender")
}

The migration process will be exited and the migration will fail if this assumption is violated.

However, since the function on the LegacyMessagePasser is public, it can also be called directly with arbitrary calldata:

function passMessageToL1(bytes memory _message) external {
    sentMessages[keccak256(abi.encodePacked(_message, msg.sender))] = true;
}

This allows us to submit calldata that would violate both of these checks and cause the migration to panic and fail.

While it may seem easy to filter these withdrawals out and rerun the migration, this solution would not work either. That's because, later in the process, we check that easy storage slot in the LegacyMessagePasser contract has a corresponding withdrawal in the migration:

for slot := range slotsAct {
    _, ok := slotsInp[slot]
    if !ok {
        return nil, fmt.Errorf("unknown storage slot in state: %s", slot)
    }
}

The result is that the Optimism team would need to unwind the migration, develop a new migration process to account for this issue, and remigrate with an untested system.

Impact

Exploitation of this bug would lead to significant challenges for the Optimism team, needing to run a less tested migration process (which could lead to further issues), and a significant amount of FUD in pausing a partially completed migration partway through. We think that the ability to unexpectedly shut down the migration causes enough structural damage as well as second-order financial damage to warrant high severity.

Code Snippet

https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/l2geth/core/vm/evm.go#L207-L209

https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/op-chain-ops/crossdomain/legacy_withdrawal.go#L58-L66

https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/packages/contracts/contracts/L2/predeploys/OVM_L2ToL1MessagePasser.sol#L29-L34

Tool used

Manual Review

Recommendation

Rather than throwing an error if withdrawal data doesn't meet the requirements, save a list of these withdrawals and continue. Include this list when prechecking withdrawals to ensure that they are included in the storage slot matching process, but not included in withdrawals to be transferred to the new system.

Special note

After coming up with this attack, we've noticed that someone has done exactly what we described and sent a message directly to the MessagePasser! Obviously this TX has nothing to do with us and we want to make sure Optimism is absolutely safe during migration. Furthermore, this TX should be traced and if a contestant is linked to this then they should clearly be disqualified from being rewarded.

Issue M-10: Challenger can override the 7 day finalization period

Source: #75

Found by

unforgiven, csanuragjain

Summary

All withdrawals are finalized after a 7 days window (finalization period). After this duration transaction are confirmed and user can surely withdraw their balance. But due to lack of check, challenger can delete a l2Output which is older than 7 days meaning withdrawals will stop working for even confirmed transaction

Vulnerability Detail

  1. Proposer has proposed L2 output for a _l2BlockNumber which creates entries on l2Outputs using the proposeL2Output. Assume this creates a new l2Output at index X
l2Outputs.push(
            Types.OutputProposal({
                outputRoot: _outputRoot,
                timestamp: uint128(block.timestamp),
                l2BlockNumber: uint128(_l2BlockNumber)
            })
        );

  1. proveWithdrawalTransaction has been called for user linked to this l2Output

  2. Finalization period(7 day) is over after proposal and Users is ready to call finalizeWithdrawalTransaction to withdraw their funds

  3. Since confirmation is done, User A is sure that he will be able to withdraw and thinks to do it after coming back from his holidays

  4. Challenger tries to delete the index X (Step 1), ideally it should not be allowed as already confirmed. But since there is no such timeline check so the l2Output gets deleted

function deleteL2Outputs(uint256 _l2OutputIndex) external {
        require(
            msg.sender == CHALLENGER,
            "L2OutputOracle: only the challenger address can delete outputs"
        );

        // Make sure we're not *increasing* the length of the array.
        require(
            _l2OutputIndex < l2Outputs.length,
            "L2OutputOracle: cannot delete outputs after the latest output index"
        );

        uint256 prevNextL2OutputIndex = nextOutputIndex();

        // Use assembly to delete the array elements because Solidity doesn't allow it.
        assembly {
            sstore(l2Outputs.slot, _l2OutputIndex)
        }

        emit OutputsDeleted(prevNextL2OutputIndex, _l2OutputIndex);
    }
  1. User comes back and now tries to withdraw but the withdraw fails since the l2Output index X does not exist anymore. This is incorrect and nullifies the network guarantee.

Note: In case of a separate output root could be proven then user withdrawal will permanently stuck. Ideally if such anomaly could not be caught within finalization period then user should be allowed to withdraw

Impact

Withdrawal will fail for confirmed transaction

Code Snippet

https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/packages/contracts-bedrock/contracts/L1/L2OutputOracle.sol#L128-L148

Tool used

Manual Review

Recommendation

Add below check in

require(getL2Output(_l2OutputIndex).timestamp<=FINALIZATION_PERIOD_SECONDS, "Output already confirmed");

Discussion

rcstanciu

Comment from Optimism

Description: deleteL2Outputs delays finalization

Reason: This is know and expected behavior. If it did not delay finalization, new outputs would not be subject to the finalization window

csanuragjain

Escalate for 30 USDC

Finalization period is the time for raising any dispute and if finalization period is crossed then no matter what, transaction should be considered confirmed (Refer https://github.com/ethereum-optimism/optimism/blob/develop/specs/withdrawals.md).

The issue here is that Challenger is able to bypass the 7 days finalization period. Post 7 day finalization period, transaction should come in confirmed state but seems like Challenger can delete the l2OutputRoot even for confirmed state transaction. This is incorrect and will bring confirmed transaction into unconfirmed state again even when they completed the finalization period.

Challenger should be disallowed to delete any L2Output where getL2Output(_l2OutputIndex).timestamp>FINALIZATION_PERIOD_SECONDS

sherlock-admin

Escalate for 30 USDC

Finalization period is the time for raising any dispute and if finalization period is crossed then no matter what, transaction should be considered confirmed (Refer https://github.com/ethereum-optimism/optimism/blob/develop/specs/withdrawals.md).

The issue here is that Challenger is able to bypass the 7 days finalization period. Post 7 day finalization period, transaction should come in confirmed state but seems like Challenger can delete the l2OutputRoot even for confirmed state transaction. This is incorrect and will bring confirmed transaction into unconfirmed state again even when they completed the finalization period.

Challenger should be disallowed to delete any L2Output where getL2Output(_l2OutputIndex).timestamp>FINALIZATION_PERIOD_SECONDS

You've created a valid escalation for 30 USDC!

To remove the escalation from consideration: Delete your comment. To change the amount you've staked on this escalation: Edit your comment (do not create a new comment).

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

Evert0x

Escalation accepted.

Accepting escalation and labeling issue as medium as challenger is able to delete finalized withdrawals.

sherlock-admin

Escalation accepted.

Accepting escalation and labeling issue as medium as challenger is able to delete finalized withdrawals.

This issue's escalations have been accepted!

Contestants' payouts and scores will be updated according to the changes made on this issue.

Issue M-11: Gasless ETH bridging from L1 to L2

Source: #71

Found by

tnch

Summary

Vulnerability Detail

Users can submit deposit transactions on L1 using the depositTransaction function of the OptimismPortal contract. Among other parameters, the user must specify the gas limit.

The function does not enforce any minimum gas limit to pay for the submission of the transaction on L2. Therefore, it is possible to submit deposit gasless transactions (i.e., with a gas limit of zero). These gasless transactions trigger a number of interesting consequences on the system.

First, because the gas limit is set to zero, the deposit transaction won't accumulate any gas on the prevBoughtGas variable on L1. Therefore, they're not constrained by the MAX_RESOURCE_LIMIT setting.

Second, the resource and gas cost on L1 will be zero. And therefore they will never trigger the gas burning mechanism on L1. Users don't have to pay for the consumed L2 resources in advance.

Third, when the transaction is executed on L2, it will be run with a gas limit of zero. Unsurprisingly, execution will fail. This is a reasonable behavior. Although under certain circumstances, it will still allow the user to alter L2 state. In particular, if a user submits on L1 a deposit transaction with zero gas limit and positive msg.value, it will record a mint of the corresponding ETH on L2, even if the transaction fails. Thus effectively allowing users to bridge ETH from L1 to L2 in a gasless fashion, without paying in advance for the L2 resources consumed.

Fourth, it is worth noting that in op-geth, a comment states that "Failed deposits must still be included. Unless we cannot produce the block at all due to the gas limit". However, these gasless deposits bypass the block gas limit, and in this aspect guarantee inclusion. Because they consume no gas at all.

Here's a real example of gasless ETH bridging. In this L1 tx, the account 0x612c calls depositTransaction with msg.value of 1 wei and gasLimit set to zero. The transaction executes successfuly. Some seconds later, the corresponding L2 tx is executed. It fails, but it increases account's ETH balance on L2 by 1 wei.

I should mention that the specs do explain the behavior of failed deposits, explicitly stating that the mint on L2 will succeed and the nonce will be increased. However, it assumes the costs of the operation (even if failed) have already been paid on L1. This issue would prove the assumption incorrect.

Impact

Gasless bridging of ETH allows users to only pay for the L1 execution of their ETH deposit transactions. They allow users to spend L2-bridging resources without paying for them in advance on L1.

Also, the issue renders the expected way of bridging ETH pointless. Because the default for an ETH deposit to L2 uses a gas limit of 100000 units.

Code Snippet

Tool used

Manual review

Recommendation

Enforce on L1 a minimum gas limit that the users must pay for to successfully begin invocation on L2. I would suggest exploring the possibility to at least charge the intrinsic gas cost of the transaction on L1, so as to ensure the user is paying for the minimum gas a transaction must have to even be considered on L2.

Discussion

rcstanciu

Comment from Optimism

Description: Gasless ETH bridging from L1 to L2

Reason: Enables a dos and circumvents the resource metering logic.

0xunforgiven

Escalate for 51 USDC

shouldn't be Medium as the protocol representatives said in the walkthrough streams: "If you pay gas to consume resource in the L2, then it's not an issue" and here user need to pay L1 gas and it's not effective to DOS L2 by paying L1 gas which has high price. in any case even if user set 0 gas limit when calling deposits it can create 3500 deposits in one L1 block because calling portal consumes a lot of gas already. This can't be Dos because it would require a lot of L1 gas to perform DOS and adding 21K minimum gas doesn't change anything as user need to pay more than 21K gas to call optimism portal. and it doesn't circumvents the resource metering logic because the function call uses 21K gas in L1.

sherlock-admin

Escalate for 51 USDC

shouldn't be Medium as the protocol representatives said in the walkthrough streams: "If you pay gas to consume resource in the L2, then it's not an issue" and here user need to pay L1 gas and it's not effective to DOS L2 by paying L1 gas which has high price. in any case even if user set 0 gas limit when calling deposits it can create 3500 deposits in one L1 block because calling portal consumes a lot of gas already. This can't be Dos because it would require a lot of L1 gas to perform DOS and adding 21K minimum gas doesn't change anything as user need to pay more than 21K gas to call optimism portal. and it doesn't circumvents the resource metering logic because the function call uses 21K gas in L1.

You've created a valid escalation for 51 USDC!

To remove the escalation from consideration: Delete your comment. To change the amount you've staked on this escalation: Edit your comment (do not create a new comment).

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

Evert0x

Escalation rejected.

It's a serious issue for Optimism, it could allow the creation of 4 MB deposit blocks. Although the attack is expensive for the attacker it is also costly to the sequencer to process such blocks without compensation.

sherlock-admin

Escalation rejected.

It's a serious issue for Optimism, it could allow the creation of 4 MB deposit blocks. Although the attack is expensive for the attacker it is also costly to the sequencer to process such blocks without compensation.

This issue's escalations have been rejected!

Watsons who escalated this issue will have their escalation amount deducted from their next payout.

Issue M-12: Deposits from L1 to L2 using L1CrossDomainMessenger will fail and will not be replayable when L2CrossDomainMessenger is paused

Source: #57

Found by

rvierdiiev, Bnke0x0, 0xdeadbeef

Summary

Deposits from L1 to L2 using L1CrossDomainMessenger will fail and will not be replayable when L2CrossDomainMessenger is paused.

Vulnerability Detail

Both L1CrossDomainMessenger and L2CrossDomainMessenger extend CrossDomainMessenger contract that can be paused.

When CrossDomainMessenger is paused then only relayMessage function will revert as it uses whenNotPaused modifier. It's still possible to call sendMessage when contract is paused.

In known issues section you can find following:

3.If the L1CrossDomainMessenger is paused, withdrawals sent to it will fail and not be replayable.

However there is another problem with pausing. In case if L2CrossDomainMessenger is paused and no matter if L1CrossDomainMessenger is pause or not(as sendMessage function is callable when contract is paused), then all deposits that are coming from L1 to L2 through the L1CrossDomainMessenger will fail on L2 side, as relayMessage will revert, because is paused. As result, it will be not possible for depositor to replay that deposit anymore as no information about the call is saved. All sent value will be minted to aliased L2CrossDomainMessenger address.

Example. 1.User wants to send himself some eth from L1 to L2 through L1CrossDomainMessenger. 2.When new TransactionDeposited event was fired by OptimismPortal, L2CrossDomainMessenger on L2 was paused for some reasons. 3.TransactionDeposited was explored by special service that sent tx to L2 node. 4.Sent value amount is minted to aliased L2CrossDomainMessenger address and L2CrossDomainMessenger.relayMessage is called which reverts. 5.Depositor lost his eth as he is not able to replay tx.

Impact

Lose of funds for depositor.

Code Snippet

https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/packages/contracts-bedrock/contracts/universal/CrossDomainMessenger.sol#L212-L343

Tool used

Manual Review

Recommendation

Pause CrossDomainMessenger on both sides at same time(L1 and L2) and restrict call of sendMessage function when contract is paused.

Discussion

rcstanciu

Comment from Optimism

Description: lock ups on l1->l2 pausing

Issue M-13: Withdrawal transactions can get stuck if output root is reproposed

Source: #53

Found by

Allarious, Barichek, HE1M, cmichel, unforgiven

Summary

Withdrawal transactions may never be executed if the L2 output root for the block, for which the withdrawal was proven, is challenged and reproposed.

Vulnerability Detail

Withdrawal transactions can be reproven in the case that the output root for their previously proven output index has been updated. This can happen if the L2 output root was removed by the challenger. However, to circumvent malicious users from reproving messages all the time and resetting the withdrawal countdown, reproving can only be done on the same L2 block number (and if the output root changed).

If the challenger deletes the block with the withdrawal transaction and the proposer proposes a different block that does not have the withdrawal transaction, the withdrawal transaction can never be finalized - even if a future block includes the legitimate withdrawal transaction again, as reproving it is bound to the old provenWithdrawals[withdrawalHash].l2OutputIndex.

Impact

Legitimate withdrawal transactions will never be finalized if the proposed block was challenged and replaced with a different one not having the withdrawal transaction. As this call fails on the "lowest level", the OptimismPortal, these transactions also cannot be replayed or be issued refunds. In case the withdrawal transaction was a token bridge transfer, the tokens are stuck on the other chain and cannot be recovered by the user.

Code Snippet

https://github.com/ethereum-optimism/optimism/blob/3f4b3c328153a8aa03611158b6984d624b17c1d9/packages/contracts-bedrock/contracts/L1/OptimismPortal.sol#L196-L197 https://github.com/ethereum-optimism/optimism/blob/3f4b3c328153a8aa03611158b6984d624b17c1d9/packages/contracts-bedrock/contracts/L1/L2OutputOracle.sol#L128

Tool used

Manual Review

Recommendation

Loosen the restriction of reproving: Allow reproving under a new L2 output index whenever the output root of the proven output index changes. This still balances the other concern of malicious users reproving transactions to reset the withdrawal countdown well as in the case where the output root changed, the withdrawal needs to be proved again anyway to be finalized.

- require(
-     provenWithdrawal.timestamp == 0 ||
-         (_l2OutputIndex == provenWithdrawal.l2OutputIndex &&
-             outputRoot != provenWithdrawal.outputRoot),
-     "OptimismPortal: withdrawal hash has already been proven"
- );
+ require(
+   provenWithdrawal.timestamp == 0 || 
+     L2_ORACLE.getL2Output(provenWithdrawal.l2OutputIndex) != provenWithdrawal.outputRoot,
+   "OptimismPortal: withdrawal hash has already been proven"
+ );

Discussion

rcstanciu

Comment from Optimism

Description: cannot reprove if you would need to do so on a different output index

Reason: Yeah, this LGTM.

Allarious

Escalate for 60 USDC

While this issue is mostly discussed here as something that can happen among honest actors, this can be an attack vector from the proposer role to users that disrupts the liveness of the system. This attack can lock the funds of many transactions and I believe this should be labelled as high.

The Attack

A proposer can block many transactions by submitting a faulty output root to the oracle and proving the transaction before the actual transaction reaches the L2. This can cause the total lock of funds and there is no way to recover the funds after.

Justification of this issue being high

  • The proposer is not a trusted role, and the challenger role can not stop this since provenWithdrawals are already written to L1. Furthermore, Faulty proofs should not be able to lock funds up and they should just make temporary disturbance in the system.
  • Can block many incoming transactions at any time, proposer can wait as long as he wants to maximize the attack value.
  • There is no longer a guarantee for the liveness of the system since there is no guarantee anymore that withdrawal transactions from L1 can be executed.
  • This attack can be escalated if proposer and the sequencer are working together. Which means that user of the system who deposits into L2 has to trust Optimism not to lock up their funds (at the start of the project), which makes a centralized authority for L2.
  • Since the proposer role is going to be decentralized in the future, this can have catastrophic outcomes.

The variations of this attack

  • Proposer can target several transactions in L1 mempool or receive them from the sequencer on L2 to lock up. He can attack as many transaction before the outputroot is challenged.
  • Proposer can submit an outputroot where all the leaves of the tree is set to true, in this case, everyone in the network will be able to front-run each other to lock up each others transactions. (Such output root can be built with O(log(n))
  • If the proposer unknowingly sends an incorrect output root due to a faulty off-chain program, an attacker could exploit this by using it to obstruct all the incorrectly included withdrawal proofs, even if the proposer is not intentionally acting malicious.

sherlock-admin

Escalate for 60 USDC

While this issue is mostly discussed here as something that can happen among honest actors, this can be an attack vector from the proposer role to users that disrupts the liveness of the system. This attack can lock the funds of many transactions and I believe this should be labelled as high.

The Attack

A proposer can block many transactions by submitting a faulty output root to the oracle and proving the transaction before the actual transaction reaches the L2. This can cause the total lock of funds and there is no way to recover the funds after.

Justification of this issue being high

  • The proposer is not a trusted role, and the challenger role can not stop this since provenWithdrawals are already written to L1. Furthermore, Faulty proofs should not be able to lock funds up and they should just make temporary disturbance in the system.
  • Can block many incoming transactions at any time, proposer can wait as long as he wants to maximize the attack value.
  • There is no longer a guarantee for the liveness of the system since there is no guarantee anymore that withdrawal transactions from L1 can be executed.
  • This attack can be escalated if proposer and the sequencer are working together. Which means that user of the system who deposits into L2 has to trust Optimism not to lock up their funds (at the start of the project), which makes a centralized authority for L2.
  • Since the proposer role is going to be decentralized in the future, this can have catastrophic outcomes.

The variations of this attack

  • Proposer can target several transactions in L1 mempool or receive them from the sequencer on L2 to lock up. He can attack as many transaction before the outputroot is challenged.
  • Proposer can submit an outputroot where all the leaves of the tree is set to true, in this case, everyone in the network will be able to front-run each other to lock up each others transactions. (Such output root can be built with O(log(n))
  • If the proposer unknowingly sends an incorrect output root due to a faulty off-chain program, an attacker could exploit this by using it to obstruct all the incorrectly included withdrawal proofs, even if the proposer is not intentionally acting malicious.

You've created a valid escalation for 60 USDC!

To remove the escalation from consideration: Delete your comment. To change the amount you've staked on this escalation: Edit your comment (do not create a new comment).

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

Evert0x

Escalation rejected.

Although the statements in the justifications section seem all correct to me, we take into account that the proposer role will be controlled by Optimism, this restriction makes the attack extremely unlikely as it requires a private key to be leaked.

Because Optimism documented plans to decentralize this role we keep it medium severity.

sherlock-admin

Escalation rejected.

Although the statements in the justifications section seem all correct to me, we take into account that the proposer role will be controlled by Optimism, this restriction makes the attack extremely unlikely as it requires a private key to be leaked.

Because Optimism documented plans to decentralize this role we keep it medium severity.

This issue's escalations have been rejected!

Watsons who escalated this issue will have their escalation amount deducted from their next payout.

Issue S-1: Batch validation logic is ordered differently to specification

Source: #283

Found by

obront

Summary

Optimism L2 block derivation has a long list of validation steps performed on incoming blocks. They are described in the spec page, but the code implements the validations in a different order than the spec lays out.

Vulnerability Detail

There is an error in the order which is described in the spec versus the one in the implementation.

According to the specs, the following check comes first:

batch.timestamp > batch_origin.time + max_sequencer_drift -> drop: i.e. a batch that does not adopt the next L1 within time will be dropped, in favor of an empty batch that can advance the L1 origin. This enforces the max L2 timestamp rule.

After it comes this check:

batch.timestamp < batch_origin.time -> drop: enforce the min L2 timestamp rule.

However, in code in batches.go executes in reverse:

if batch.Batch.Timestamp < batchOrigin.Time {
	log.Warn("batch timestamp is less than L1 origin timestamp", "l2_timestamp", batch.Batch.Timestamp, "l1_timestamp", batchOrigin.Time, "origin", batchOrigin.ID())
	return BatchDrop
}
// If we ran out of sequencer time drift, then we drop the batch and produce an empty batch instead,
// as the sequencer is not allowed to include anything past this point without moving to the next epoch.
if max := batchOrigin.Time + cfg.MaxSequencerDrift; batch.Batch.Timestamp > max {
	log.Warn("batch exceeded sequencer time drift, sequencer must adopt new L1 origin to include transactions again", "max_time", max)
	return BatchDrop
}

In practice this should not lead to any issues because both are drop rules and are right next to each other.

Impact

The code does not align with the ordering laid out in the specification.

Code Snippet

https://github.com/ethereum-optimism/optimism/blob/407f97b9d13448b766624995ec824d3059d4d4f6/op-node/rollup/derive/batches.go#L98

Tool used

Manual Review

Recommendation

Change the code so that it is in line with the specification.

Issue S-2: Specification of the new base fee calculation is inconsistent with the code

Source: #266

Found by

shw

Summary

Specification of the new base fee calculation is inconsistent with the code.

Vulnerability Detail

The "Guaranteed Gas Fee Market" specification provides a pseudocode explaining how a new base fee is calculated, which has two inconsistencies with the actual code implementation:

  1. When multiple blocks are skipped, the new base fee is applied with a 7/8 reduction per block. The code uses the updated newBaseFee (see ResourceMetering.sol#L115) while the specification uses the un-updated prev_basefee.
  2. The maximum new base fee in the specification is UINT_64_MAX_VALUE but is set to type(uint128).max in the code.

Impact

The specification does not match the code.

Code Snippet

https://github.com/ethereum-optimism/optimism/blob/3f4b3c328153a8aa03611158b6984d624b17c1d9/packages/contracts-bedrock/contracts/L1/ResourceMetering.sol#L115

Tool used

Manual Review

Recommendation

Fix either the specification or the code.

Issue S-3: Incorrect code comments in the StandardBridge.sol contract

Source: #264

Found by

unforgiven, tsvetanovv, shw

Summary

Incorrect code comments in the StandardBridge.sol contract.

Vulnerability Detail

The code comments for the bridgeERC20 and bridgeERC20To functions in the universal/StandardBridge.sol contract do not match the actual code. The comments say the bridge returns tokens to the sender if the bridging fails, but the refund logic has been removed since PR#3535.

Impact

Specfication error only.

Code Snippet

https://github.com/ethereum-optimism/optimism/blob/3f4b3c328153a8aa03611158b6984d624b17c1d9/packages/contracts-bedrock/contracts/universal/StandardBridge.sol#L219-L222 https://github.com/ethereum-optimism/optimism/blob/3f4b3c328153a8aa03611158b6984d624b17c1d9/packages/contracts-bedrock/contracts/universal/StandardBridge.sol#L251-L254

Tool used

Manual Review

Recommendation

Remove the outdated code comments.

Issue S-4: Incorrect owner check

Source: #263

Found by

csanuragjain

Summary

The depositTransaction of OptimismPortal can directly be called by user instead of intermediate contract. This means from address wont be aliased. But this is not considered in CrossDomainOwnable contract which plainly undoL1ToL2Alias the caller

Vulnerability Detail

  1. Assume depositTransaction is called by User A directly. Since no intermediary contract so no aliasing is done
  2. On L2 side, if _checkOwner is checked
function _checkOwner() internal view override {
        require(
            owner() == AddressAliasHelper.undoL1ToL2Alias(msg.sender),
            "CrossDomainOwnable: caller is not the owner"
        );
    }
  1. This will try undoL1ToL2Alias on User A address and then match with owner which is incorrect since User A address was never aliased on L1

Impact

The owner check might fail for genuine transaction

Code Snippet

https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/packages/contracts-bedrock/contracts/L2/CrossDomainOwnable.sol#L21

Tool used

Manual Review

Recommendation

This check need to be revised. If the transaction came directly from tx.origin (without any intermediary contract) then no need of removing aliasing

Discussion

csanuragjain

Escalate for 28 USDC

This is mentioning a valid issue where CrossDomainOwnable.sol#L21 is always assuming call to depositTransaction coming from contract (as undo Alias is always done). If depositTransaction is called by a user instead then CrossDomainOwnable will fail to check properly and not allow a genuine user

sherlock-admin

Escalate for 28 USDC

This is mentioning a valid issue where CrossDomainOwnable.sol#L21 is always assuming call to depositTransaction coming from contract (as undo Alias is always done). If depositTransaction is called by a user instead then CrossDomainOwnable will fail to check properly and not allow a genuine user

You've created a valid escalation for 28 USDC!

To remove the escalation from consideration: Delete your comment. To change the amount you've staked on this escalation: Edit your comment (do not create a new comment).

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

Evert0x

Escalation accepted and labeling as specification issue

sherlock-admin

Escalation accepted and labeling as specification issue

This issue's escalations have been accepted!

Contestants' payouts and scores will be updated according to the changes made on this issue.

Issue S-5: Block number is not an input to proveWithdrawalTransaction in OptimismPortal

Source: #233

Found by

ck

Summary

According to the Withdrawals spec, a block number is one of the submitted inputs to the OptimismPortal which is not the case.

Vulnerability Detail

The documentation says:

"A relayer submits the required inputs to the OptimismPortal contract. The relayer need not be the same entity which initiated the withdrawal on L2. These inputs include the withdrawal transaction data, inclusion proofs, and a block number. The block number must be one for which an L2 output root exists, which commits to the withdrawal as registered on L2."

In the contract, the _l2OutputIndex is the one used to check for existence of the outputRoot and not the block number.

   function proveWithdrawalTransaction(
        Types.WithdrawalTransaction memory _tx,
        uint256 _l2OutputIndex,
        Types.OutputRootProof calldata _outputRootProof,
        bytes[] calldata _withdrawalProof
    ) external {
        // Prevent users from creating a deposit transaction where this address is the message
        // sender on L2. Because this is checked here, we do not need to check again in
        // `finalizeWithdrawalTransaction`.
        require(
            _tx.target != address(this),
            "OptimismPortal: you cannot send messages to the portal contract"
        );

        // Get the output root and load onto the stack to prevent multiple mloads. This will
        // revert if there is no output root for the given block number.
        bytes32 outputRoot = L2_ORACLE.getL2Output(_l2OutputIndex).outputRoot;

        // Verify that the output root can be generated with the elements in the proof.
        require(
            outputRoot == Hashing.hashOutputRootProof(_outputRootProof),
            "OptimismPortal: invalid output root proof"
        );

Impact

Misleading spec

Code Snippet

https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/packages/contracts-bedrock/contracts/L1/OptimismPortal.sol#L160-L182

Tool used

Manual Review

Recommendation

The specification should refer to _l2OutputIndex as one of the inputs instead of 'block number'.

Issue S-6: system_config: incorrect variable name and missing config update type

Source: #213

Found by

lemonmon

Summary

The specs for system config contains incorrect information and incorrect names for its contents.

  1. The names for overhead and scalar are falsely stated as l1FeeOverhead and l1FeeScalar in multiple occasions.
  2. The list of the configuration update types is missing the unsafe block signer update

Vulnerability Detail

In the SystemConfig contract, there are public variables overhead and scalar:

https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/packages/contracts-bedrock/contracts/L1/SystemConfig.sol#L51-L59

Below the variables overhead and scalar are incorrectly named as l1FeeOverhead and l1FeeScalar in multiple occasions:

https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/specs/system_config.md?plain=1#L34 https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/specs/system_config.md?plain=1#L73 https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/specs/system_config.md?plain=1#L86-L90

In the above snippet: Line 89: the names of overhead and scalar It also is missing the type 3:

  • type 3: unsafeBlockSigner overwrite, as address payload.

The corresponding update code snippets from SystemConfig are below:

https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/packages/contracts-bedrock/contracts/L1/SystemConfig.sol#L25-L30 https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/packages/contracts-bedrock/contracts/L1/SystemConfig.sol#L169-L175

Impact

factually incorrect information

  • incorrect name of variables
  • missing config update type

As the name of variables double as interface to fetch the value, anybody uses the incorrect name in the specs to fetch the values will fail. Also the specs do not list the possible config update types, so the users may not know that the unsafeBlockSigner can be updated.

Code Snippet

https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/packages/contracts-bedrock/contracts/L1/SystemConfig.sol#L51-L59 https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/specs/system_config.md?plain=1#L34 https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/specs/system_config.md?plain=1#L73 https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/specs/system_config.md?plain=1#L86-L90 https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/packages/contracts-bedrock/contracts/L1/SystemConfig.sol#L25-L30 https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/packages/contracts-bedrock/contracts/L1/SystemConfig.sol#L169-L175

Tool used

Manual Review

Recommendation

Correct the names of the variables and add the missing config update type

Issue S-7: Address collision in cross - chain contract creation (breaks tooling)

Source: #204

Found by

Bahurum, Ro

Summary

When creating a contract from L1 (mainnet) to L2 (optimism) the nonce of the sender is kept at 0 in the context of the transaction receipt and thus the existing infrastructure (Etherscan) will incorrectly interpret the address of the new contract (creating a collision).

Vulnerability Detail

It is possible to create a contract in Layer 2 from Layer 1 by sending a transaction to the optimism portal. This is the function of interest:

 function depositTransaction(
        address _to,
        uint256 _value,
        uint64 _gasLimit,
        bool _isCreation,
        bytes memory _data
    ) public payable metered(_gasLimit)

To create a contract the "_to" address needs to be set to "address(0x0), " the "_isCreation" needs to be set to "true" and the bytecode needs to be passed in the "_data" field.

  • The contract is then created through the "create" opcode, therefore the address is computed by hashing the nonce and the address of the sender.

If the nonce is kept at 0 during some time of the execution, the new contract address will always be the same (nonce is always 0).

NOTE: In reality, the new contract is created with the proper nonce (and the nonce is updated), but the receipt displays it wrongly and during a point in the execution is also kept at 0 so it breaks Etherscan, Blockscout and the rest of the block exploreres.

Impact

Infrastructure tooling like Etherscan is the source of truth for most users, therefore having inconsistent data between these tools and Optimism is not recommended.

This bug causes existing tooling like Etherscan and Blockscout to display wrong data in the following ways:

  1. The first contract created will always appear as the new contract in contract creation. See this link for reference: https://goerli-optimism.etherscan.io/address/0x5d7ee88447367b9212fa655dc863a1e83f7e189d

When the account 0x7d.. creates a new contract from Layer 1, the address "0x5d.." will always appear as the new contract created.

  1. It incorrectly displays contracts as EOA's See this address on etherscan goerli optimism: https://goerli-optimism.etherscan.io/address/0x0a1c3c13c35275d030ff9fb660946cf2fca74ece It appears to be a regular EOA, while in reality, it is a contract. Run the following command to verify:
cast code 0x0a1c3c13c35275d030ff9fb660946cf2fca74ece --rpc-url https://goerli.optimism.io

Code Snippet

https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/packages/contracts-bedrock/contracts/L1/OptimismPortal.sol#L358

Tool used

Manual Review

Recommendation

The nonce should be set to the sender's nonce from the op-node.

Discussion

rcstanciu

Comment from Optimism

Description: Address collision in cross - chain contract creation (breaks tooling)

Reason: This is correct, but is unexpected behavior external to the scope of the code. Thus, this is a low issue.

Action: Increment the nonce in the op-node to make transaction receipts use the correct nonce for contract creation on L1 -> L2 bridge.

rodrigoherrerai

Escalate for 250 USDC

This bug clearly breaks the specifications from Optimism. The debate here is if this is marked as a Medium or as a spec.

It could be marked as medium because it breaks EVM complete equivalence (as shown below):

Bedrock’s design aims for EVM equivalence, meaning that smart contract developers should be able to safely deploy the same contracts to Optimism as they would on Ethereum, without concern for differences in the execution.

Here are concrete examples where this bug causes the specifications to differ:

Just to recap, this issue is manifested when creating contracts from L1 -> L2. The nonce is not incremented and thus the contract created in the transaction receipt will always be the same (nonce is always 0).

When creating a contract, the address is computed as follows:

computed_address = keccak256(rlp.encode([address, nonce]))
    canonical_address = computed_address[-20:]
    padded_address = left_pad_zero_bytes(canonical_address, 20)
    return Address(padded_address)

If the nonce is kept at 0 during the receipt, then the new contract address will always be the same. https://github.com/ethereum/execution-specs/blob/master/src/ethereum/london/utils/address.py#L42

This issue breaks the following specifications:

1:

Nonce Handling Despite the lack of signature validation, we still increment the nonce of the from account when a deposit transaction is executed. In the context of a deposit-only roll up, this is not necessary for transaction ordering or replay prevention, however it maintains consistency with the use of nonces during contract creation. It may also simplify integration with downstream tooling (such as wallets and block explorers).

Link: https://github.com/ethereum-optimism/optimism/blob/develop/specs/deposits.md#nonce-handling

Quote: "however it maintains consistency with the use of nonces during contract creation. It may also simplify integration with downstream tooling (such as wallets and block explorers)."

This issue completely breaks the integration with downstream tooling:

  • Etherscan and the rest of the block explorers show contract accounts without code (as EOA's):

This is an account with code (contract) on Optimism goerli: 0x0a1c3c13c35275d030ff9fb660946cf2fca74ece

But Etherscan and the rest of the block explorers display it as an EOA: Screenshot 2023-02-23 at 10 12 06

  • Etherscan and the rest of the block explorers incorrectly show the contract creation as the first contract will always appear to be the contract created:

Take this transaction for example:

Screenshot 2023-02-23 at 10 53 34

It appears as if the contract created was "0x5d7..", while in reality it is incorrect. Note that the nonce also appears as "0".

It breaks complete "EVM Equivalence"

From the Optimism blog post:

…What is EVM equivalence? In short: EVM equivalence is complete compliance with the Ethereum yellow paper, the formal definition of the protocol. By definition, L1 Ethereum software must comply with this specification.

This means that — down to the very deepest depths — the existing Ethereum stack will now integrate with the L2 system, too. Every debugger. Every toolchain. Every node implementation. We believe that any L2 offering any EVM experience will have to meet this bar — anything less is unacceptable.

Quote: "This means that — down to the very deepest depths — the existing Ethereum stack will now integrate with the L2 system, too. Every debugger. Every toolchain. Every node implementation. We believe that any L2 offering any EVM experience will have to meet this bar — anything less is unacceptable." Link: https://medium.com/ethereum-optimism/introducing-evm-equivalence-5c2021deb306

Apart from breaking downstream tooling like block explorers and debuggers, it can also break frontend / backend applications that rely on the transaction receipt as the source of truth.

For example, when the address 0x7dF608C479d2D2a7df677fEEc6f2535Eb268da01 creates a contract from L1 to L2, the contract address in the receipt will always be the same, because the nonce is kept at 0:

{
  to: null,
  from: '0x7dF608C479d2D2a7df677fEEc6f2535Eb268da01',
  contractAddress: '0x5d7Ee88447367b9212FA655dc863A1e83f7e189D',
...

sherlock-admin

Escalate for 250 USDC

This bug clearly breaks the specifications from Optimism. The debate here is if this is marked as a Medium or as a spec.

It could be marked as medium because it breaks EVM complete equivalence (as shown below):

Bedrock’s design aims for EVM equivalence, meaning that smart contract developers should be able to safely deploy the same contracts to Optimism as they would on Ethereum, without concern for differences in the execution.

Here are concrete examples where this bug causes the specifications to differ:

Just to recap, this issue is manifested when creating contracts from L1 -> L2. The nonce is not incremented and thus the contract created in the transaction receipt will always be the same (nonce is always 0).

When creating a contract, the address is computed as follows:

computed_address = keccak256(rlp.encode([address, nonce]))
    canonical_address = computed_address[-20:]
    padded_address = left_pad_zero_bytes(canonical_address, 20)
    return Address(padded_address)

If the nonce is kept at 0 during the receipt, then the new contract address will always be the same. https://github.com/ethereum/execution-specs/blob/master/src/ethereum/london/utils/address.py#L42

This issue breaks the following specifications:

1:

Nonce Handling Despite the lack of signature validation, we still increment the nonce of the from account when a deposit transaction is executed. In the context of a deposit-only roll up, this is not necessary for transaction ordering or replay prevention, however it maintains consistency with the use of nonces during contract creation. It may also simplify integration with downstream tooling (such as wallets and block explorers).

Link: https://github.com/ethereum-optimism/optimism/blob/develop/specs/deposits.md#nonce-handling

Quote: "however it maintains consistency with the use of nonces during contract creation. It may also simplify integration with downstream tooling (such as wallets and block explorers)."

This issue completely breaks the integration with downstream tooling:

  • Etherscan and the rest of the block explorers show contract accounts without code (as EOA's):

This is an account with code (contract) on Optimism goerli: 0x0a1c3c13c35275d030ff9fb660946cf2fca74ece

But Etherscan and the rest of the block explorers display it as an EOA: Screenshot 2023-02-23 at 10 12 06

  • Etherscan and the rest of the block explorers incorrectly show the contract creation as the first contract will always appear to be the contract created:

Take this transaction for example:

Screenshot 2023-02-23 at 10 53 34

It appears as if the contract created was "0x5d7..", while in reality it is incorrect. Note that the nonce also appears as "0".

It breaks complete "EVM Equivalence"

From the Optimism blog post:

…What is EVM equivalence? In short: EVM equivalence is complete compliance with the Ethereum yellow paper, the formal definition of the protocol. By definition, L1 Ethereum software must comply with this specification.

This means that — down to the very deepest depths — the existing Ethereum stack will now integrate with the L2 system, too. Every debugger. Every toolchain. Every node implementation. We believe that any L2 offering any EVM experience will have to meet this bar — anything less is unacceptable.

Quote: "This means that — down to the very deepest depths — the existing Ethereum stack will now integrate with the L2 system, too. Every debugger. Every toolchain. Every node implementation. We believe that any L2 offering any EVM experience will have to meet this bar — anything less is unacceptable." Link: https://medium.com/ethereum-optimism/introducing-evm-equivalence-5c2021deb306

Apart from breaking downstream tooling like block explorers and debuggers, it can also break frontend / backend applications that rely on the transaction receipt as the source of truth.

For example, when the address 0x7dF608C479d2D2a7df677fEEc6f2535Eb268da01 creates a contract from L1 to L2, the contract address in the receipt will always be the same, because the nonce is kept at 0:

{
  to: null,
  from: '0x7dF608C479d2D2a7df677fEEc6f2535Eb268da01',
  contractAddress: '0x5d7Ee88447367b9212FA655dc863A1e83f7e189D',
...

You've created a valid escalation for 250 USDC!

To remove the escalation from consideration: Delete your comment. To change the amount you've staked on this escalation: Edit your comment (do not create a new comment).

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

Evert0x

Escalation accepted.

Comment provides sufficient evidence for a specification issue.

Labeling #176 as duplicate as it describes the same issue

sherlock-admin

Escalation accepted.

Comment provides sufficient evidence for a specification issue.

Labeling #176 as duplicate as it describes the same issue

This issue's escalations have been accepted!

Contestants' payouts and scores will be updated according to the changes made on this issue.

Issue S-8: Specification Inconsistency in Legacy ERC20 Functions Reverting

Source: #200

Found by

Robert

Summary

Documentation says that after Bedrock migration all methods interacting with state on the LegacyERC20 contract will now revert https://github.com/ethereum-optimism/optimism/blob/develop/specs/predeploys.md#legacyerc20eth. Functions that changed state were already reverting and view functions are being updated to still work.

Vulnerability Detail

Just not working as described.

Impact

Low

Code Snippet

https://github.com/ethereum-optimism/optimism/blob/407f97b9d13448b766624995ec824d3059d4d4f6/packages/contracts-bedrock/contracts/legacy/LegacyERC20ETH.sol#L35

Tool used

Manual Review

Recommendation

Remove comment about this reverting--maybe change to it saying they're being updated (such as balance using address.balance).

Issue S-9: deposits: the sourceHash of L1 attributes deposited

Source: #162

Found by

Bahurum, lemonmon

Summary

The calculation of sourceHash for L1 attributes deposited is incorrect.

Although, it is a very small difference, because of the misplaced blacket, it means a different thing with a different result from the actual calculation.

Vulnerability Detail

According to the specs, the sourceHash of L1 attributes deposited is calculated based on:

https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/specs/deposits.md?plain=1#L92

It means the l1BlockHash will be hashed alone, before it is hashed with other values. However, l1BlockHash and seqNumber should be hashed together, as the actual calculation in the deposit_source.go:

https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/op-node/rollup/derive/deposit_source.go#L35-L46

Therefore, the line should be corrected as following:

-  `keccak256(bytes32(uint256(1)), keccak256(l1BlockHash), bytes32(uint256(seqNumber)))`.
+  `keccak256(bytes32(uint256(1)), keccak256(l1BlockHash, bytes32(uint256(seqNumber))))`.

Impact

factually incorrect information

The calculation of sourceHash in the specs will give a different result from the actual code.

Code Snippet

https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/specs/deposits.md?plain=1#L92 https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/op-node/rollup/derive/deposit_source.go#L35-L46

Tool used

Manual Review

Recommendation

correct the calculation

Issue S-10: overview, withdrawals: DepositFeed does not exist

Source: #161

Found by

ck, Ro, lemonmon

Summary

There is no DepositFeed contract. The implementation of Deposit contract would be OptimismPortal. There are some multiple occasions of incorrect information, for example, stating that Optimism Portal inherits from DepositFeed contract.

Also, using "Deposit Contract" and DepositFeed contract interchangeably may confuse the reader.

Vulnerability Detail

https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/specs/overview.md?plain=1#L52

There is no DepositFeed contract. It should be OptimismPortal contract.

 - The `OptimismPortal` contract emits `TransactionDeposited` events, which the rollup driver reads in order to process

https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/specs/overview.md?plain=1#L109

Here as well, the DepositFeed contract should be OptimismPortal contract.

call the `depositTransaction` method on the `OptimismPortal` contract. This in turn emits `TransactionDeposited` events,

https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/specs/overview.md?plain=1#L144

Here as well, the DepositFeed contract should be OptimismPortal contract.

deposits initiated via the `OptimismPortal` contract on L1. All L2 blocks can also contain _sequenced transactions_, i.e.

https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/specs/withdrawals.md?plain=1#L133-L135

The OptimismPortal inherits Initializable, ResourceMetering and Semver and there is no DepositFeed contract in the inheritance tree.

Impact

Factually wrong specs

Code Snippet

https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/specs/overview.md?plain=1#L52 https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/specs/overview.md?plain=1#L109 https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/specs/overview.md?plain=1#L144 https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/specs/withdrawals.md?plain=1#L133-L135

Tool used

Manual Review

Recommendation

Use "Deposit Contract" or OptimismPortal depending on the context, instead of DepositFeed contract.

Issue S-11: Confusion in gap size

Source: #146

Found by

ustas, 0x1337, supernova

Summary

According to the comments

Reserve extra slots in the storage layout for future upgrades.
     *         A gap size of 41 was chosen here, so that the first slot used in a child contract
     *         would be a multiple of 50.

Vulnerability Detail

But actually gap is provided for 42 instead of 41 mentioned above . This can lead to presumptions on the minds of the dev that the first slot of the child contract is a multiple of 50 , when it is not .

Impact

Code Snippet

https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/packages/contracts-bedrock/contracts/universal/CrossDomainMessenger.sol#L132-L137

Tool used

Manual Review

Recommendation

Clear the collision in the comments and the actual code .

Issue S-12: The CrossDomainMessenger process is explained incorrectly in the spec

Source: #108

Found by

obront, 0xdeadbeef, lemonmon

Summary

The process for using the L1CrossDomainMessenger has not been updated in the spec, and it still explains the pre-Bedrock process rather than the updated process.

Vulnerability Detail

The spec explains:

When going from L2 into L1, the user must call relayMessage on the L1CrossDomainMessenger to finalize the withdrawal. This function can only be called after the finalization window has passed.

This is no longer the process. In Bedrock:

  • the user proves their withdrawal right away
  • the user proves their withdrawal on OptimismPortal, not CrossDomainMessenger
  • the user executes their withdrawal on OptimismPortal, not by calling relayMessage (or any other function) on CrossDomainMessenger

Impact

The spec is still showing the old withdrawal process, and doesn't accurately reflect the new process that will exist in Bedrock.

Code Snippet

https://github.com/ethereum-optimism/optimism/blob/develop/specs/messengers.md#message-passing

https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/packages/contracts-bedrock/contracts/L1/OptimismPortal.sol#L160-L344

Tool used

Manual Review

Recommendation

Update the explanation of the CrossDomainMessenger in the spec to explain the new withdrawal process.

Issue S-13: Withdrawing process in spec does not include two-step withdrawals

Source: #107

Found by

rvierdiiev, obront, lemonmon

Summary

The withdrawal process in the Introduction of the spec displays the old, single-step withdrawal process, not the new, two-step one.

Vulnerability Detail

The Introduction section of the spec explains withdrawals with the following image:

https://github.com/ethereum-optimism/optimism/blob/develop/specs/assets/user-withdrawing-to-l1.svg

This image seems to have been drawn for the old withdrawal system, including steps:

4. Wait for block hash to finalize
5. Send execute withdrawal transaction

In the new system, the user submits the proof right away, then waits at least 7 days for the proof (regardless of when block hash finalizes), and then makes an additional call to execute the transaction.

Impact

The spec is still showing the old withdrawal process, and doesn't accurately reflect the new process that will exist in Bedrock.

Code Snippet

https://github.com/ethereum-optimism/optimism/blob/develop/specs/introduction.md#withdrawing

https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/packages/contracts-bedrock/contracts/L1/OptimismPortal.sol#L160-L344

Tool used

Manual Review

Recommendation

Update the diagram in the spec Introduction to explain the new two-step withdrawal process.

Issue S-14: Deposits are not guaranteed to be reflected within sequencing window

Source: #106

Found by

obront

Summary

There is an inconsistency between the spec and the code regarding guarantees for deposits.

Vulnerability Detail

In the spec, it states:

Deposits are guaranteed to be reflected in the L2 state within the sequencing window.

However, until fraud proofs are implemented, there are no guarantees that this will actually be the case.

Impact

Users may expect that there are guarantees in the system that ensure their deposits will be processed within a given number of blocks, but these guarantees do not exist yet.

Code Snippet

https://github.com/ethereum-optimism/optimism/blob/develop/specs/overview.md#l1-components

https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/packages/contracts-bedrock/contracts/L1/L2OutputOracle.sol#L160-L210

Tool used

Manual Review

Recommendation

Remove this language from the spec until fraud proofs are live.

Discussion

rcstanciu

Comment from Optimism

Description: Deposits are not guaranteed without fault proofs

Reason: This is actually guaranteed, and part of the protocol definition for batch derivation.

zobront

Escalate for 250 USDC

This issue is a valid Low severity, and should be included.

Optimism's response was:

Reason: This is actually guaranteed, and part of the protocol definition for batch derivation.

However, that isn't the case. In my conversation with Maurelian over DMs during the contest, he explained: "basically, you could say that the protocol guarantees this, based on the execution rules in the sequencer. However that 'guarantee' is based on the assumption that we have a fault proof working, which would enforce those execution rules."

Since we don't have a fault proof working, we do not have a guarantee.

sherlock-admin

Escalate for 250 USDC

This issue is a valid Low severity, and should be included.

Optimism's response was:

Reason: This is actually guaranteed, and part of the protocol definition for batch derivation.

However, that isn't the case. In my conversation with Maurelian over DMs during the contest, he explained: "basically, you could say that the protocol guarantees this, based on the execution rules in the sequencer. However that 'guarantee' is based on the assumption that we have a fault proof working, which would enforce those execution rules."

Since we don't have a fault proof working, we do not have a guarantee.

You've created a valid escalation for 250 USDC!

To remove the escalation from consideration: Delete your comment. To change the amount you've staked on this escalation: Edit your comment (do not create a new comment).

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

Evert0x

Escalation accepted.

The specification is correct as it related the L2 state ( not the L1 representation for the L2 state ) but the context of the specification can be improved.

sherlock-admin

Escalation accepted.

The specification is correct as it related the L2 state ( not the L1 representation for the L2 state ) but the context of the specification can be improved.

This issue's escalations have been accepted!

Contestants' payouts and scores will be updated according to the changes made on this issue.

Issue S-15: Information passed to L1Block.sol is not delayed by 10 blocks

Source: #103

Found by

obront

Summary

There is an inconsistency between the spec and the code regarding the timing of L1 block info being submitted to the L2 contract L1Block.sol.

Vulnerability Detail

In the spec, it states:

Currently the L1 information is delayed by ten block confirmations (~2.5 minutes) to minimize the impact of reorgs. This value may be reduced in the future.

However, in the node's derivation code, the L1 block information is included along with the deposits for each block as it's processed, with no delays.

This can further be verified by watching L1 as well as the L1Block contract, and observing that, for a given block, the information is posted to Optimism instantly.

Impact

Spec doesn't accurately reflect the reality of what the code is doing.

Code Snippet

https://community.optimism.io/docs/developers/bedrock/differences/#l1block

https://github.com/ethereum-optimism/optimism/blob/6c6d142d7bb95faa11066aab5d8aed7187abfe38/op-node/rollup/derive/attributes.go#L56-L68

Tool used

Manual Review

Recommendation

Remove this language from the spec or adjust the code to match.

Issue S-16: L2OutputOracle outputs are removed by challenger, not proposer

Source: #98

Found by

tnch, obront, lemonmon

Summary

There is an inconsistency between the spec and reality regarding which user is able to delete the L2 outputs to roll back the chain.

Vulnerability Detail

In the spec, it states:

The proposer may also delete multiple output roots by calling the deleteL2Outputs() function and specifying the index of the first output to delete, this will also delete all subsequent outputs.

It goes on to explicitly state that this will be the same role as the sequencer:

Note regarding future work: In the initial version of the system, the proposer will be the same entity as the sequencer, which is a trusted role. In the future proposers will need to submit a bond in order to post L2 output roots, and some or all of this bond may be taken in the event of a faulty proposal.

However, the code specifies a different user, called CHALLENGER, who has this permission:

require(
    msg.sender == CHALLENGER,
    "L2OutputOracle: only the challenger address can delete outputs"
);

Looking at the CHALLENGER address, it appears that it is actually a multisig, separate from both the sequencer and proposer.

Impact

The spec is incorrect in defining which user has the ability to roll back the chain.

Code Snippet

https://github.com/ethereum-optimism/optimism/blob/f30376825c82f62b846590487fe46b7435213d37/specs/proposals.md#proposing-l2-output-commitments

https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/packages/contracts-bedrock/contracts/L1/L2OutputOracle.sol#L129-L132

Tool used

Manual Review

Recommendation

Adjust the language in the spec to make clear that the CHALLENGER is a separate role controlled by a multisig.

Issue S-17: Spec: Wrong description of withdrawal process

Source: #94

Found by

ck, 0xdeadbeef, lemonmon, cmichel, obront, unforgiven

Summary

Wrong description of the withdrawal process in the spec.

Vulnerability Detail

The withdrawals.md describes the second step of the withdrawal process as:

  1. The OptimismPortal contract retrieves the output root for the given block number from the L2OutputOracle's getL2OutputAfter() function, and performs the remainder of the verification process internally.

The getL2OutputAfter call is never performed. This function does not even exist, most likely, it was referring to getL2OutputIndexAfter. However, even this function is not called. What happens in the withdrawal process is that getL2Output is called to retrieve the output root for the given block number (index).

Impact

The withdrawal process should be clearly documented in the spec. Currently, it's referring to a non-existant function.

Code Snippet

https://github.com/ethereum-optimism/optimism/blob/3f4b3c328153a8aa03611158b6984d624b17c1d9/specs/withdrawals.md?plain=1#L67

Tool used

Manual Review

Recommendation

Fix the spec by referring to getL2Output(l2OutputIndex) instead.

Issue S-18: Spec: Wrong OptimismPortal interface

Source: #93

Found by

cmichel, lemonmon

Summary

Wrong OptimismPortal interface in the specs.

Vulnerability Detail

The withdrawals.md specification file shows a wrong L2ToL1MessagePasser interface:

  • function proveWithdrawalTransaction(Types.WithdrawalTransaction memory _tx, uint256 _l2BlockNumber, Types.OutputRootProof calldata _outputRootProof, bytes[] calldata _withdrawalProof) external uses the wrong _l2BlockNumber parameter. The parameter should be named _l2OutputIndex like in the OptimismPortal code. The difference is that the L2OutputOracle pushes the blocks to an array, and the startingBlockNumber is its first element, so there's a shift from a L2 output index to its L2 block number. Using the block number as described in the spec will make any proveWithdrawalTransaction calls fail.
interface OptimismPortal {

    event WithdrawalFinalized(bytes32 indexed);

    function l2Sender() returns(address) external;

    function proveWithdrawalTransaction(
        Types.WithdrawalTransaction memory _tx,
        uint256 _l2BlockNumber, // @audit this is NOT the block number, it's the `_l2OutputIndex`.
        Types.OutputRootProof calldata _outputRootProof,
        bytes[] calldata _withdrawalProof
    ) external;

    function finalizeWithdrawalTransaction(
        Types.WithdrawalTransaction memory _tx
    ) external;
}

Impact

Users usually go to the docs & specification to see how to integrate a project. Integrating Optimisim's OptimismPortal based on the specification will lead to errors as it uses the block number, different from the required block index in L2OutputOracle's array.

Code Snippet

https://github.com/ethereum-optimism/optimism/blob/3f4b3c328153a8aa03611158b6984d624b17c1d9/specs/withdrawals.md?plain=1#L149

Tool used

Manual Review

Recommendation

Use the correct interface by fixing the mentioned issues.

Issue S-19: Spec: Wrong L2ToL1MessagePasser interface

Source: #92

Found by

cmichel, lemonmon

Summary

Wrong L2ToL1MessagePasser interface in the specs.

Vulnerability Detail

The withdrawals.md specification file shows a wrong L2ToL1MessagePasser interface:

  • function nonce() view external returns (uint256); does not exist. It should be function messageNonce() view external returns (uint256);
interface L2ToL1MessagePasser {
    event MessagePassed(
        uint256 indexed nonce, // this is a global nonce value for all withdrawal messages
        address indexed sender,
        address indexed target,
        uint256 value,
        uint256 gasLimit,
        bytes data,
        bytes32 withdrawalHash
    );

    event WithdrawerBalanceBurnt(uint256 indexed amount);

    function burn() external;

    function initiateWithdrawal(address _target, uint256 _gasLimit, bytes memory _data) payable external;
   // @audit it's called messageNonce
    function nonce() view external returns (uint256);

    function sentMessages(bytes32) view external returns (bool);
}

Impact

Users usually go to the docs & specification to see how to integrate a project. Integrating Optimisim's L2ToL1MessagePasser based on the specification will lead to errors.

Code Snippet

https://github.com/ethereum-optimism/optimism/blob/3f4b3c328153a8aa03611158b6984d624b17c1d9/specs/withdrawals.md?plain=1#L106

Tool used

Manual Review

Recommendation

Use the correct interface by fixing the mentioned issues.

Issue S-20: Spec: Wrong L2OutputOracle interface

Source: #91

Found by

cmichel, lemonmon

Summary

Wrong L2OutputOracle interface in the specs.

Vulnerability Detail

The proposals.md specification file shows a wrong L2OutputOracle interface:

  • function getNextBlockNumber() public view returns (uint256) does not exist. It should be function nextBlockNumber() public view returns (uint256)
/**
 * @notice The number of the first L2 block recorded in this contract.
 */
uint256 public startingBlockNumber;

/**
 * @notice The timestamp of the first L2 block recorded in this contract.
 */
uint256 public startingTimestamp;

/**
 * @notice Accepts an L2 outputRoot and the timestamp of the corresponding L2 block. The
 * timestamp must be equal to the current value returned by `nextTimestamp()` in order to be
 * accepted.
 * This function may only be called by the Proposer.
 *
 * @param _l2Output      The L2 output of the checkpoint block.
 * @param _l2BlockNumber The L2 block number that resulted in _l2Output.
 * @param _l1Blockhash   A block hash which must be included in the current chain.
 * @param _l1BlockNumber The block number with the specified block hash.
*/
  function proposeL2Output(
      bytes32 _l2Output,
      uint256 _l2BlockNumber,
      bytes32 _l1Blockhash,
      uint256 _l1BlockNumber
  )

/**
 * @notice Deletes all output proposals after and including the proposal that corresponds to
 *         the given output index. Only the challenger address can delete outputs.
 *
 * @param _l2OutputIndex Index of the first L2 output to be deleted. All outputs after this
 *                       output will also be deleted.
 */
function deleteL2Outputs(uint256 _l2OutputIndex) external

/**
 * @notice Computes the block number of the next L2 block that needs to be checkpointed.
 */
 // @audit is called `function nextBlockNumber() public view returns (uint256)`
function getNextBlockNumber() public view returns (uint256)

Impact

Users usually go to the docs & specification to see how to integrate a project. Integrating Optimisim's L2OutputOracle based on the specification will lead to errors.

Code Snippet

https://github.com/ethereum-optimism/optimism/blob/3f4b3c328153a8aa03611158b6984d624b17c1d9/specs/proposals.md?plain=1#L142

Tool used

Manual Review

Recommendation

Use the correct interface by fixing the mentioned issues.

Issue S-21: Spec: Wrong CrossDomainMessenger interface

Source: #90

Found by

Bahurum, cmichel, lemonmon

Summary

Wrong CrossDomainMessenger interface in the specs.

Vulnerability Details

The messengers.md specification file shows a wrong CrossDomainMessenger interface:

  • function otherMessenger() view external returns (address); does not exist. It should be function OTHER_MESSENGER() view external returns (address);
interface CrossDomainMessenger {
    event FailedRelayedMessage(bytes32 indexed msgHash);
    event RelayedMessage(bytes32 indexed msgHash);
    event SentMessage(address indexed target, address sender, bytes message, uint256 messageNonce, uint256 gasLimit);

    function MESSAGE_VERSION() view external returns (uint16);
    function messageNonce() view external returns (uint256);
    // @audit is OTHER_MESSENGER
    function otherMessenger() view external returns (address);
    function failedMessages(bytes32) view external returns (bool);
    function relayMessage(uint256 _nonce, address _sender, address _target, uint256 _value, uint256 _minGasLimit, bytes memory _message) payable external;
    function sendMessage(address _target, bytes memory _message, uint32 _minGasLimit) payable external;
    function successfulMessages(bytes32) view external returns (bool);
    function xDomainMessageSender() view external returns (address);
}

Impact

Users usually go to the docs & specification to see how to integrate a project. Integrating Optimisim's CrossDomainMessenger based on the specification will lead to errors.

Code Snippet

https://github.com/ethereum-optimism/optimism/blob/3f4b3c328153a8aa03611158b6984d624b17c1d9/specs/messengers.md?plain=1#L38

Tool used

Manual Review

Recommendation

Use the correct interface by fixing the mentioned issues.

Issue S-22: Spec: Wrong Deposited Transaction Type encoding

Source: #89

Found by

cmichel, tnch, shw

Summary

Wrong Deposited Transaction Type encoding in the specs.

Vulnerability Details

The deposits.md specification file says that the deposited transaction type is encoded with "the following fields (rlp encoded in the order they appear here)":

However, they are missing the isSystemTransaction bool from the encoding and the order is also different (data after gasLimit), see:

/**
 * @notice RLP encodes the L2 transaction that would be generated when a given deposit is sent
 *         to the L2 system. Useful for searching for a deposit in the L2 system. The
 *         transaction is prefixed with 0x7e to identify its EIP-2718 type.
 *
 * @param _tx User deposit transaction to encode.
 *
 * @return RLP encoded L2 deposit transaction.
 */
function encodeDepositTransaction(Types.UserDepositTransaction memory _tx)
    internal
    pure
    returns (bytes memory)
{
    bytes32 source = Hashing.hashDepositSource(_tx.l1BlockHash, _tx.logIndex);
    bytes[] memory raw = new bytes[](8);
    raw[0] = RLPWriter.writeBytes(abi.encodePacked(source));
    raw[1] = RLPWriter.writeAddress(_tx.from);
    raw[2] = _tx.isCreation ? RLPWriter.writeBytes("") : RLPWriter.writeAddress(_tx.to);
    raw[3] = RLPWriter.writeUint(_tx.mint);
    raw[4] = RLPWriter.writeUint(_tx.value);
    raw[5] = RLPWriter.writeUint(uint256(_tx.gasLimit));
    raw[6] = RLPWriter.writeBool(false);
    raw[7] = RLPWriter.writeBytes(_tx.data);
    return abi.encodePacked(uint8(0x7e), RLPWriter.writeList(raw));
}

Impact

Users go to the specification to see how to integrate a project. Integrating according to this spec will be wrong.

Code Snippet

https://github.com/ethereum-optimism/optimism/blob/3f4b3c328153a8aa03611158b6984d624b17c1d9/specs/deposits.md?plain=1#L55-L68 https://github.com/ethereum-optimism/optimism/blob/3f4b3c328153a8aa03611158b6984d624b17c1d9/packages/contracts-bedrock/contracts/libraries/Encoding.sol#L22

Tool used

Manual Review

Recommendation

Use the correct encoding from the encodeDepositTransaction function above.

Issue S-23: Spec: Wrong StandardBridge interface

Source: #88

Found by

cmichel, lemonmon

Summary

Wrong StandardBridge interface in the specs.

Vulnerability Details

The bridges.md specification file shows a wrong StandardBridge interface:

  • ERC20BridgeFinalized event is defined twice, this is an invalid interface as compilation will fail with "DeclarationError: Event with same name and parameter types defined twice."
  • The function parameter _extraData is defined as bytes memory _extraData but the StandardBridge uses call-data. The encodings are incompatible.
interface StandardBridge {
    // @audit ERC20BridgeFinalized event defined twice, this is an invalid interface as compilation will fail with "DeclarationError: Event with same name and parameter types defined twice."
    event ERC20BridgeFinalized(address indexed localToken, address indexed remoteToken, address indexed from, address to, uint256 amount, bytes extraData);
    event ERC20BridgeFinalized(address indexed localToken, address indexed remoteToken, address indexed from, address to, uint256 amount, bytes extraData);
    event ERC20BridgeInitiated(address indexed localToken, address indexed remoteToken, address indexed from, address to, uint256 amount, bytes extraData);
    event ETHBridgeFinalized(address indexed from, address indexed to, uint256 amount, bytes extraData);
    event ETHBridgeInitiated(address indexed from, address indexed to, uint256 amount, bytes extraData);
    // @audit everywhere it is `bytes calldata _extraData`, like 5 times
    function bridgeERC20(address _localToken, address _remoteToken, uint256 _amount, uint32 _minGasLimit, bytes memory _extraData) external;
    function bridgeERC20To(address _localToken, address _remoteToken, address _to, uint256 _amount, uint32 _minGasLimit, bytes memory _extraData) external;
    function bridgeETH(uint32 _minGasLimit, bytes memory _extraData) payable external;
    function bridgeETHTo(address _to, uint32 _minGasLimit, bytes memory _extraData) payable external;
    function deposits(address, address) view external returns (uint256);
    function finalizeBridgeERC20(address _localToken, address _remoteToken, address _from, address _to, uint256 _amount, bytes memory _extraData) external;
    function finalizeBridgeETH(address _from, address _to, uint256 _amount, bytes memory _extraData) payable external;
    function messenger() view external returns (address);
    function otherBridge() view external returns (address);
}

Impact

Users usually go to the docs & specification to see how to integrate a project. Integrating Optimisim's bridge based on the specification will lead to errors.

Code Snippet

https://github.com/ethereum-optimism/optimism/blob/3f4b3c328153a8aa03611158b6984d624b17c1d9/specs/bridges.md?plain=1#L24

Tool used

Manual Review

Recommendation

Use the correct interface by fixing the mentioned issues.

Issue S-24: Incorrectness in computing block signing hash allows cross-chain replay attacks

Source: #67

Found by

Koolex, usmannk

Summary

Incorrectness in computing block signing hash allows cross-chain replay attacks

Vulnerability Detail

The sequencer signs over a message: keccak256(domain ++ chain_id ++ payload_hash). The chain_id is included to prevent replaying the same message over another chain. However, SigningHash function fails to ensure the chain_id is included in the message.

func SigningHash(domain [32]byte, chainID *big.Int, payloadBytes []byte) (common.Hash, error) {
	var msgInput [32 + 32 + 32]byte
	// domain: first 32 bytes
	copy(msgInput[:32], domain[:])
	// chain_id: second 32 bytes
	if chainID.BitLen() > 256 {
		return common.Hash{}, errors.New("chain_id is too large")
	}
	chainID.FillBytes(msgInput[32:64])
	// payload_hash: third 32 bytes, hash of encoded payload
	copy(msgInput[32:], crypto.Keccak256(payloadBytes))

	return crypto.Keccak256Hash(msgInput[:]), nil
}

If you look at this line:

copy(msgInput[32:], crypto.Keccak256(payloadBytes))

https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/op-node/p2p/signer.go#L36

It is supposed to copy the encoded payload hash to the third 32 bytes of msgInput. However, it start from 32nd byte which would overwrite the second 32 bytes allocated for the chain_id.

Impact

Any block signed by the sequencer in any chain is valid for other chains. For example, a malicious verifier can pick a message signed for a test chain and gossip it out for P2P on main chain.

Code Snippet

Please create a file gossip_crosschain_test.go under op-node/p2p directory and add the following code:

package p2p

import (
	// "encoding/binary"
	l "log"
	"testing"
	"math/big"

	"github.com/stretchr/testify/require"
	"github.com/ethereum-optimism/optimism/op-node/rollup"
)

func TestSigningHash(t *testing.T) {

	cfg1 := &rollup.Config{
		L2ChainID:         big.NewInt(100),
	}
	cfg2 := &rollup.Config{
		L2ChainID:         big.NewInt(101),
	}

	payloadBytes := []byte("arbitraryData")
	hash, err := SigningHash(SigningDomainBlocksV1,cfg1.L2ChainID,payloadBytes)
	if err != nil {
		l.Println("Error while hashing", err)
	}
	l.Println("hash : ", hash)

	hash2, err2 := SigningHash(SigningDomainBlocksV1,cfg2.L2ChainID,payloadBytes)
	if err2 != nil {
		l.Println("Error while hashing", err2)
	}
	l.Println("hash2: ", hash2)

	require.NotEqual(t, hash, hash2)
}

Run the test as follows:

go test -run=TestSigningHashCrossChain ./p2p -v

The test will fail as the two hashes match:

=== RUN   TestSigningHash
2023/01/28 15:57:41 hash :  0x35af5b09dcb95c4ebac34a932a84a70d6aa97dece830972ce9d0affb7cbaea30
2023/01/28 15:57:41 hash2:  0x35af5b09dcb95c4ebac34a932a84a70d6aa97dece830972ce9d0affb7cbaea30
    gossip_attack_test.go:35: 
        	Error Trace:	/share/2023-01-optimism-koolexcrypto/optimism/op-node/p2p/gossip_attack_test.go:35
        	Error:      	Should not be: 0x35af5b09dcb95c4ebac34a932a84a70d6aa97dece830972ce9d0affb7cbaea30
        	Test:       	TestSigningHash
--- FAIL: TestSigningHash (0.00s)
FAIL
FAIL	github.com/ethereum-optimism/optimism/op-node/p2p	0.634s
FAIL

Tool used

Manual Review

Recommendation

Just replace this

   copy(msgInput[32:], crypto.Keccak256(payloadBytes))

with

  copy(msgInput[64:], crypto.Keccak256(payloadBytes))

Run the test above again and it should pass successfully.

Discussion

rcstanciu

Comment from Optimism

Reason: The chain ID is indeed being clobbered by the payloadBytes. DDoS risk is low, however, so this is more of a low than a medium.

koolexcrypto

Escalate for 150 USDC

Speaking generally, cross-chain replay was always a critical/high issue. However, in this case here, I believe it should be considered as a medium (at least) since DDoS risk can not be disregarded. Although Optimism's judge acknowledged the DDoS risk, I disagree with the low severity assessment for the following reasons:

  1. As you might agree, It is difficult to measure the extent of DDoS attack given the short time of the contest. nevertheless, the issue could cause unpredictable bugs causing serious impacts in future.
  2. Since it's planned to decentralise the sequencer ultimately by supporting multiple concurrent sequencers (check refs below), the risk is even higher. Imagine the following (note: this is very simplified just to explain the impact roughly):

    • Assume DDoS_risk is the risk level of making the network too busy. seq(x) is sequencer x. count is how many sequencers we have. impact(seq(x)) is the potential impact caused by sequencer x. T(x) is the count of blocks signed by sequencer x in other chains (e.g. Optimism Goerli). The higher the T, the higher the impact. If T = 0, there is basically no impact

      Let $impact(seq(x)) = f(T(x))$ ,then

      DDoS_risk = $\sum_{n = 1}^{count} impact(seq(n))$

As you can notice, once we have multiple sequencers, the risk gets higher.

  1. It's also worth mentioning that if it happens that a sequencer produces blocks for Optimism and a forked chain, then one can gossip (in Optimism) those blocks signed for the forked chain. Have a look at this forked chain (VMeta3 chain): https://github.com/VMETA3/vmeta3-chain/blob/master/op-node/p2p/signer.go#L25-L35 It has exactly the same code.

  2. Even though the code was audited multiple times, the issue was still overlooked since (at least) May 16, 2022. This shows somewhat the error could possibly stay there till after decentralising the sequencer. Check: https://github.com/ethereum-optimism/optimism/commits/2006a2f5ed921b1069446abbae805a037541dcab/op-node/p2p/signer.go

Moreover, as per Optimism specs:

The signature is a secp256k1 signature, and signs over a message: keccak256(domain ++ chain_id ++ payload_hash)

Still, the issue was marked as non-reward even though it goes under the "Specification errors" category described in the contest regardless of the reasoning above.

References/Links:

Please note that there is a duplicate of the issue and the escalation should be applied to both Issue #125. The issue is also escalated.

sherlock-admin

Escalate for 150 USDC

Speaking generally, cross-chain replay was always a critical/high issue. However, in this case here, I believe it should be considered as a medium (at least) since DDoS risk can not be disregarded. Although Optimism's judge acknowledged the DDoS risk, I disagree with the low severity assessment for the following reasons:

  1. As you might agree, It is difficult to measure the extent of DDoS attack given the short time of the contest. nevertheless, the issue could cause unpredictable bugs causing serious impacts in future.
  2. Since it's planned to decentralise the sequencer ultimately by supporting multiple concurrent sequencers (check refs below), the risk is even higher. Imagine the following (note: this is very simplified just to explain the impact roughly):

    • Assume DDoS_risk is the risk level of making the network too busy. seq(x) is sequencer x. count is how many sequencers we have. impact(seq(x)) is the potential impact caused by sequencer x. T(x) is the count of blocks signed by sequencer x in other chains (e.g. Optimism Goerli). The higher the T, the higher the impact. If T = 0, there is basically no impact

      Let $impact(seq(x)) = f(T(x))$ ,then

      DDoS_risk = $\sum_{n = 1}^{count} impact(seq(n))$

As you can notice, once we have multiple sequencers, the risk gets higher.

  1. It's also worth mentioning that if it happens that a sequencer produces blocks for Optimism and a forked chain, then one can gossip (in Optimism) those blocks signed for the forked chain. Have a look at this forked chain (VMeta3 chain): https://github.com/VMETA3/vmeta3-chain/blob/master/op-node/p2p/signer.go#L25-L35 It has exactly the same code.

  2. Even though the code was audited multiple times, the issue was still overlooked since (at least) May 16, 2022. This shows somewhat the error could possibly stay there till after decentralising the sequencer. Check: https://github.com/ethereum-optimism/optimism/commits/2006a2f5ed921b1069446abbae805a037541dcab/op-node/p2p/signer.go

Moreover, as per Optimism specs:

The signature is a secp256k1 signature, and signs over a message: keccak256(domain ++ chain_id ++ payload_hash)

Still, the issue was marked as non-reward even though it goes under the "Specification errors" category described in the contest regardless of the reasoning above.

References/Links:

Please note that there is a duplicate of the issue and the escalation should be applied to both Issue #125. The issue is also escalated.

You've created a valid escalation for 150 USDC!

To remove the escalation from consideration: Delete your comment. To change the amount you've staked on this escalation: Edit your comment (do not create a new comment).

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

Evert0x

Escalation accepted.

Accepting as a specification issue.

Although it's an issue that needs to be addressed, the likelihood that it will have an impact is so insignificant that it's considered a non-medium severity issue, labeling as spec issue because of section in escalation.

sherlock-admin

Escalation accepted.

Accepting as a specification issue.

Although it's an issue that needs to be addressed, the likelihood that it will have an impact is so insignificant that it's considered a non-medium severity issue, labeling as spec issue because of section in escalation.

This issue's escalations have been accepted!

Contestants' payouts and scores will be updated according to the changes made on this issue.

Issue S-25: Owner has multiple meanings when proxy is involved

Source: #27

Found by

keccak123

Summary

The word "owner" is overloaded and refers to more than one address in the proxies used in optimism bedrock.

Vulnerability Detail

The proxy contract used for several contracts, including L1CrossDomainMessenger, confuses the meaning of owner in the code and in the spec. In the proxy contract, there is OWNER_KEY, but this storage slot actually stores the admin of the proxy and is retrieved by calling admin. This meaning of owner is more confusing because L1CrossDomainMessenger, the implementation contract behind the proxy, inherits OwnableUpgradeable and has an owner function, a transferOwnership function, and an onlyOwner modifier.

The overloading of "owner" makes all documentation with the word "owner" confusing, and sometimes contradictory, in the specifications and code natspec. One specific example of this contradiction is from the predeploy spec

ProxyAdmin Address: 0x4200000000000000000000000000000000000018

The ProxyAdmin is the owner of all of the proxy contracts set at the predeploys. It is itself behind a proxy. The owner of the ProxyAdmin will have the ability to upgrade any of the other predeploy contracts.

The first time the word "owner" is used, it actually refers to the proxy admin. This is seen by calling admin on L2CrossDomainMessenger: cast call 0x4200000000000000000000000000000000000007 "admin()(address)" --rpc-url https://goerli.optimism.io -> 0x4200000000000000000000000000000000000018. Using the same admin meaning for owner, like the first time the word is used, returns the ProxyAdmin address, which does not make sense in the context of what the spec is trying to explain: cast call 0x4200000000000000000000000000000000000018 "admin()(address)" --rpc-url https://goerli.optimism.io -> 0x4200000000000000000000000000000000000018. But the second time the word "owner" is used, it refers to the owner behind the proxy, in the implementation contract, which is seen in this cast call: cast call 0x4200000000000000000000000000000000000018 "owner()(address)" --rpc-url https://goerli.optimism.io -> 0xf80267194936da1E98dB10bcE06F3147D580a62e. This explanation of the spec should replace the first time the word "owner" is used with the word "admin".

Impact

The meaning of owner is ambiguous in many contracts because there owner can refer to the return value of admin or the return value of owner.

Code Snippet

The natspec for changeAdmin and admin in the proxy shows how the word owner is used to refer to the proxy admin, confusing the two terms https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/packages/contracts-bedrock/contracts/universal/Proxy.sol#L111-L126

The natspec and variable name in L1CrossDomainMessenger uses owner to refer to a different value https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/packages/contracts-bedrock/contracts/L1/L1CrossDomainMessenger.sol#L38

The predeploy spec for ProxyAdmin summarizes this confusion by using the word "owner" two times to refer to different values https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/specs/predeploys.md#proxyadmin

Tool used

Manual Review

Recommendation

Disambiguate the terms admin and owner. The proxyadmin spec explanation should replace the first time the word "owner" is used with the word "admin". Consider renaming OWNER_KEY in the proxy contract to ADMIN_KEY and remove the word owner from the proxy contract.

About

No description, website, or topics provided.

Resources

Readme
Activity
Custom properties

Stars

20 stars

Watchers

8 watching

Forks

6 forks
Report repository

Releases

No releases published

Packages

No packages published

Contributors 3

  •  
  •  
  •  

Languages