search

sherlock-audit / 2023-01-optimism-judging

26 stars 10 forks source link

GalloDaSballo - Optimism Portal can run out of gas due to incorrect overhead estimation #138

Open github-actions[bot] opened 1 year ago

github-actions[bot] commented 1 year ago

GalloDaSballo

medium

Optimism Portal can run out of gas due to incorrect overhead estimation

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:

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
        );
rcstanciu commented 1 year ago

Comment from Optimism

Severity: medium

Reason: see 109

zobront commented 1 year ago

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:

For this issue, we see:

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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.