In OptimismPortal.finalizeWithdrawalTransaction(), the condition to revert the txn is incorrect.

[code423n4]

Lines of code

https://github.com/ethereum-optimism/optimism/blob/a48e53c100e6ac024f45be7bdec94ad35fe5cd1c/packages/contracts-bedrock/contracts/L1/OptimismPortal.sol#L405-L406
https://github.com/ethereum-optimism/optimism/blob/a48e53c100e6ac024f45be7bdec94ad35fe5cd1c/packages/contracts-bedrock/contracts/L1/OptimismPortal.sol#L417-L419

Vulnerability details

Impact

Medium Impact : Loss of withdrawn funds

Proof of Concept

In OptimismPortal.finalizeWithdrawalTransaction(), the condition to revert the txn is incorrect.

A call is made to SafeCall.callWithMinGas()

bool success = SafeCall.callWithMinGas(_tx.target, _tx.gasLimit, _tx.value, _tx.data);

https://github.com/ethereum-optimism/optimism/blob/a48e53c100e6ac024f45be7bdec94ad35fe5cd1c/packages/contracts-bedrock/contracts/L1/OptimismPortal.sol#L405-L406

and returned success value checked in following IF condition:

if (success == false && tx.origin == Constants.ESTIMATION_ADDRESS) {
    revert("OptimismPortal: withdrawal failed");
}

https://github.com/ethereum-optimism/optimism/blob/a48e53c100e6ac024f45be7bdec94ad35fe5cd1c/packages/contracts-bedrock/contracts/L1/OptimismPortal.sol#L417-L419

Here I believe, developer meant to use || instead of && in IF condition, either condition resulting true would revert the txn.

So, if success == false, then there is no need to check value of tx.origin, because txns should fail/revert anyway.

Tools Used

Manual

Recommended Mitigation Steps

Change the code to :

if (success == false || tx.origin == Constants.ESTIMATION_ADDRESS) {
    revert("OptimismPortal: withdrawal failed");
}

Assessed type

Other

[c4-judge]

0xleastwood marked the issue as primary issue

[c4-judge]

0xleastwood marked the issue as satisfactory

[c4-judge]

0xleastwood marked the issue as selected for report

[antojoseph]

The report doesn't explain if there is a vulnerability caused by using && over || . Please provide a POC if there is a vulnerability

[c4-sponsor]

antojoseph marked the issue as sponsor disputed

[itsmetechjay]

Reached out to warden via DM to request POC.

[itsmetechjay]

POC provided by warden:

The function finalizeWithdrawalTransaction is external, and user can choose the amount of gas the function should use.

Now as per the comments in code, the line

bool success = SafeCall.callWithMinGas(_tx.target, _tx.gasLimit, _tx.value, _tx.data);

could fail every time under following conditions

/* The amount of gas provided to the execution context of the target is at least the gas limit specified by the user. If there is not enough gas in the current context to accomplish this, callWithMinGas will revert. */

(https://github.com/ethereum-optimism/optimism/blob/a48e53c100e6ac024f45be7bdec94ad35fe5cd1c/packages/contracts-bedrock/contracts/L1/OptimismPortal.sol#L402C1-L404C66)

and

/* Perform a low level call without copying any returndata. This function will revert if the call cannot be performed with the specified minimum gas. */

(https://github.com/ethereum-optimism/optimism/blob/a48e53c100e6ac024f45be7bdec94ad35fe5cd1c/packages/contracts-bedrock/contracts/libraries/SafeCall.sol#L103-L105)

Additionally, there are certain more scenarios I assume sponsor/devs should consider -

• If the execution of the callWithMinGas function consumes all the gas specified by _tx.gasLimit before completing, then the transaction will be reverted, and success will be set to false.

• If the contract being called (_tx.target) reverts during its execution, the callWithMinGas function will return false, and success will be set to false. This could happen if the contract encounters an error condition or if an explicit revert statement is triggered within the contract.

• If _tx.target does not refer to a valid contract address on the Ethereum network, the call will fail, and the callWithMinGas function will return false. This could happen if _tx.target is an incorrect or non-existent address.

Now coming back to the IF condition -

bool success = SafeCall.callWithMinGas(_tx.target, _tx.gasLimit, _tx.value, _tx.data);

..

if (success == false && tx.origin == Constants.ESTIMATION_ADDRESS) {
    revert("OptimismPortal: withdrawal failed");
}

In case if the success is returned as false and tx.origin is not set to the ESTIMATION_ADDRESS, then the function execution would complete and won't reverting.

That is, the finalizeWithdrawalTransaction function would complete execution even if success is returned as false

If callWithMinGas reverts for any reason (failure to trigger the call to the target contract), the function should revert 100% of the time without checking any other condition - which does not seem to happen (Because && is used, and it that makes the final boolean result depended on the value of : tx.origin == Constants.ESTIMATION_ADDRESS)

Similarly, by looking at code, I assume devs intented to revert function execution when caller was the ESTIMATION_ADDRESS.

That's why I believe the correct condition would be to use || instead of && operator.

Mitigation:

if (success == false || tx.origin == Constants.ESTIMATION_ADDRESS) {
    revert("OptimismPortal: withdrawal failed");
}

[0xleastwood]

I agree with the warden, using || over && seems to make more sense.

[JeffCX]

With full respect to judge's expertise,

I think this is a intended design decision in the documentation

https://github.com/ethereum-optimism/optimism/blob/382d38b7d45bcbf73cb5e1e3f28cbd45d24e8a59/specs/withdrawals.md#handling-successfully-verified-messages-that-fail-when-relayed

If the execution of the relayed call fails in the target contract, it is unfortunately not possible to determine whether or not it was 'supposed' to fail, and whether or not it should be 'replayable'. For this reason, and to minimize complexity, we have not provided any replay functionality, this may be implemented in external utility contracts if desired.

Also, this exact finding is reported in previous auditing and the discussed extensively

sherlock-audit/2023-01-optimism-judging#148

The issue is pointing out the general lack of replayability on the portal, the risks of the portal are clearly described and this counts as a known risk.

Moreover, you can see in this PR that is close to complete that Optimism did a ton of work to keep that lack of replayability in the portal: ethereum-optimism/optimism#5017

So I think this finding is QA finding

Judge has the final authority, I will do no more dispute and respect judge's final decision

[rvierdiiev]

I agree with @JeffCX
This is intended design. OptimismPortal should not revert when relaying was not successful. That's why we have CDM which gives some guarantees to replay failed tx.

[anupsv]

This is not an issue as it follows the spec.

[0xleastwood]

As per the provided info, transactions which fail should not revert but instead be handled by the CDM. Marking as invalid.

[c4-judge]

0xleastwood marked the issue as unsatisfactory:
Invalid