csanuragjain - Challenger can override the 7 day finalization period

[github-actions[bot]]

csanuragjain

medium

Challenger can override the 7 day finalization period

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)
            })
        );

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

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

4. 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

5. 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);
    }

6. 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");

[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.