KingNFT - ETH withdrawn from L2 to L1 may stuck in OptimismPortal contract

[github-actions]

KingNFT

medium

ETH withdrawn from L2 to L1 may stuck in OptimismPortal contract

Summary

finalizeWithdrawalTransaction function in OptimismPortal contract doesn't support failed message replay, which may cause ETH stuck in the OptimismPortal contract.

Vulnerability Detail

As shown in OptimismPortal contract of L324
https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/packages/contracts-bedrock/contracts/L1/OptimismPortal.sol#L324

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

the finalizeWithdrawalTransaction function will not revert even on failed sub call (SafeCall.call) for the withdrawal transaction if tx.origin != Constants.ESTIMATION_ADDRESS.

On the other hand, as shown on L308
https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/packages/contracts-bedrock/contracts/L1/OptimismPortal.sol#L308

the transaction has been flaged finalized. So failed message can not be replayed, which may causes ETH stuck in the OptimismPortal contract.

Impact

ETH withdrawn from L2 to L1 may stuck in OptimismPortal contract

Code Snippet

    function finalizeWithdrawalTransaction(Types.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 == Constants.DEFAULT_L2_SENDER,
            "OptimismPortal: can only trigger one withdrawal per transaction"
        );

        // Grab the proven withdrawal from the `provenWithdrawals` map.
        bytes32 withdrawalHash = Hashing.hashWithdrawal(_tx);
        ProvenWithdrawal memory provenWithdrawal = provenWithdrawals[withdrawalHash];

        // A withdrawal can only be finalized if it has been proven. We know that a withdrawal has
        // been proven at least once when its timestamp is non-zero. Unproven withdrawals will have
        // a timestamp of zero.
        require(
            provenWithdrawal.timestamp != 0,
            "OptimismPortal: withdrawal has not been proven yet"
        );

        // As a sanity check, we make sure that the proven withdrawal's timestamp is greater than
        // starting timestamp inside the L2OutputOracle. Not strictly necessary but extra layer of
        // safety against weird bugs in the proving step.
        require(
            provenWithdrawal.timestamp >= L2_ORACLE.startingTimestamp(),
            "OptimismPortal: withdrawal timestamp less than L2 Oracle starting timestamp"
        );

        // A proven withdrawal must wait at least the finalization period before it can be
        // finalized. This waiting period can elapse in parallel with the waiting period for the
        // output the withdrawal was proven against. In effect, this means that the minimum
        // withdrawal time is proposal submission time + finalization period.
        require(
            _isFinalizationPeriodElapsed(provenWithdrawal.timestamp),
            "OptimismPortal: proven withdrawal finalization period has not elapsed"
        );

        // Grab the OutputProposal from the L2OutputOracle, will revert if the output that
        // corresponds to the given index has not been proposed yet.
        Types.OutputProposal memory proposal = L2_ORACLE.getL2Output(
            provenWithdrawal.l2OutputIndex
        );

        // Check that the output root that was used to prove the withdrawal is the same as the
        // current output root for the given output index. An output root may change if it is
        // deleted by the challenger address and then re-proposed.
        require(
            proposal.outputRoot == provenWithdrawal.outputRoot,
            "OptimismPortal: output root proven is not the same as current output root"
        );

        // Check that the output proposal has also been finalized.
        require(
            _isFinalizationPeriodElapsed(proposal.timestamp),
            "OptimismPortal: output proposal finalization period has not elapsed"
        );

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

        // Reverting here is useful for determining the exact gas cost to successfully execute the
        // sub call to the target contract if the minimum gas limit specified by the user would not
        // be sufficient to execute the sub call.
        if (success == false && tx.origin == Constants.ESTIMATION_ADDRESS) {
            revert("OptimismPortal: withdrawal failed");
        }
    }

Tool used

Manual Review

Recommendation

Allow replaying failed withdrawal messages

[rcstanciu]

Comment from Optimism

---

Description: ETH withdrawn from L2 to L1 may stuck in OptimismPortal contract

Reason: This is a non-issue, and it is the intended behavior of the finalizeWithdrawalTransaction function. It is up to the initiator of the finalizeWithdrawalTransaction function to ensure that the WithdrawalTransaction has a sufficient minimum gas limit to finalize their withdrawal on L1. If the user specifies a minimum gas limit that does not cover the cost of finalization, it is expected behavior that their withdrawal will not be able to be finalized.

[ydspa]

Escalate for 22 USDC

1. Gas consumption may be heavily dependent on the current state of the smart contract, e.g. there is a for loop on dynamic state array, and the 7-day waiting period is a relatively long time during which changes to the state may significantly impact gas consumption. As a result, regardless of how users set the gas limit, there is no guarantee that the gas is 100% enough .
2. Subcall failures are not always due to insufficient gas, but can also be caused by changes in the smart contract state during the 7-day waiting period. For example, a custom subcall with sub-subcall to Uniswap for cross-chain exchange may fail due to slippage protection at the first execution of finalizeWithdrawalTransaction. In this case, it would be necessary to allow users to retry the failed transaction too.

Therefore, i insist the fact that funds may be permanently lost due to factors out of users' control is a problem. In a well-designed system, users should be able to recover from these scenes, but this issue can result in irreversible financial losses.

In addition, we can also see disallowing replay along with no revert on failed subcall is a really bad design at the view of security, it opens doors for many known and unknown attack vectors, such as
#109
#87
#96
These type of attacks will all be unavailable if replay is supported or revert immediately on failed subcall.

[sherlock-admin]

Escalate for 22 USDC

1. Gas consumption may be heavily dependent on the current state of the smart contract, e.g. there is a for loop on dynamic state array, and the 7-day waiting period is a relatively long time during which changes to the state may significantly impact gas consumption. As a result, regardless of how users set the gas limit, there is no guarantee that the gas is 100% enough .
2. Subcall failures are not always due to insufficient gas, but can also be caused by changes in the smart contract state during the 7-day waiting period. For example, a custom subcall with sub-subcall to Uniswap for cross-chain exchange may fail due to slippage protection at the first execution of finalizeWithdrawalTransaction. In this case, it would be necessary to allow users to retry the failed transaction too.

Therefore, i insist the fact that funds may be permanently lost due to factors out of users' control is a problem. In a well-designed system, users should be able to recover from these scenes, but this issue can result in irreversible financial losses.

In addition, we can also see disallowing replay along with no revert on failed subcall is a really bad design at the view of security, it opens doors for many known and unknown attack vectors, such as
#109
#87
#96
These type of attacks will all be unavailable if replay is supported or revert immediately on failed subcall.

You've created a valid escalation for 22 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.

[0xdeadbeef0x]

Escalate for 41 USDC

I want to strengthen the claim that this is not a user error but can be caused by an state change in the target contract. I have provided a good scenario in the description in my issue which is a duplicate of this: #25

Please consider adding my issue as a duplicate if the escalation is accepted.

[sherlock-admin]

Escalate for 41 USDC

I want to strengthen the claim that this is not a user error but can be caused by an state change in the target contract. I have provided a good scenario in the description in my issue which is a duplicate of this: #25

Please consider adding my issue as a duplicate if the escalation is accepted.

You've created a valid escalation for 41 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.

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

[sherlock-admin]

Escalation rejected.

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

This issue's escalations have been rejected!

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