rvierdiiev - User can be temporary blocked to prove his transaction after removal of output from L2OutputOracle

[sherlock-admin]

rvierdiiev

medium

User can be temporary blocked to prove his transaction after removal of output from L2OutputOracle

Summary

User can be temporary blocked to prove his transaction after removal of output from L2OutputOracle. He will need to wait till provenWithdrawal.l2OutputIndex output will be posted to L2OutputOracle in order to prove it again as L2_ORACLE.getL2Output(provenWithdrawal.l2OutputIndex).outputRoot will revert.

Vulnerability Detail

When user withdraws from l2, he uses L2ToL1MessagePasser.initiateWithdrawal that creates WithdrawalTransaction for him with nonce inside. Then he needs to prove his transaction on l1 using OptimismPortal.proveWithdrawalTransaction function.

It's allowed for user to call this function for same transaction in case if he didn't do that yet or output for his provenWithdrawal.l2OutputIndex has changed. https://github.com/sherlock-audit/2023-03-optimism/blob/main/optimism/packages/contracts-bedrock/contracts/L1/OptimismPortal.sol#L266-L271

        require(
            provenWithdrawal.timestamp == 0 ||
                L2_ORACLE.getL2Output(provenWithdrawal.l2OutputIndex).outputRoot !=
                provenWithdrawal.outputRoot,
            "OptimismPortal: withdrawal hash has already been proven"
        );

In case if L2_ORACLE.getL2Output(provenWithdrawal.l2OutputIndex) is called for l2OutputIndex that is bigger than L2OutputOracle.latestOutputIndex(), then transaction will revert.

In case if some output is incorrect, then this output and all next outputs can be removed by challenger.

This makes next situation possible. All numbers here are just to simplify explanation. 1.suppose that last l2OutputIndex inside L2OutputOracle is currently 100(just example). 2.user has proved his withdrawal with provenWithdrawal.l2OutputIndex == 99. 3.10 outputs were removed by challenger then last l2OutputIndex inside L2OutputOracle now 90.

4. user can't finalize his withdraw, he needs to prove tx again with new l2OutputIndex. if user wants to prove his withdrawal again he will need to wait till all 99 outputs will be stored again(more 9) because of this check which will try to receive 99th output from L2OutputOracle which is not provided anymore.

       require(
           provenWithdrawal.timestamp == 0 ||
               L2_ORACLE.getL2Output(provenWithdrawal.l2OutputIndex).outputRoot !=
               provenWithdrawal.outputRoot,
           "OptimismPortal: withdrawal hash has already been proven"
       );

   5.As result user is temporarily blocked to prove his transaction. He can do that only, when 99th output index will be provided.

   Impact

   Temporal block of transaction proving.

   Code Snippet

   Provided above

   Tool used

Manual Review

Recommendation

In case if L2_ORACLE.latestOutputIndex() < provenWithdrawal.l2OutputIndex then allow user to prove his tx again.

        require(
            provenWithdrawal.timestamp == 0 ||
                (L2_ORACLE.latestOutputIndex() < provenWithdrawal.l2OutputIndex ||
                    L2_ORACLE.getL2Output(provenWithdrawal.l2OutputIndex).outputRoot 
                        != provenWithdrawal.outputRoot),
            "OptimismPortal: withdrawal hash has already been proven"
        );

[hrishibhat]

Sponsor comment: We need to check that the output root is inconsistent because otherwise in the provided solution, the user can prove multiple withdrawals so long as the latestOutputIndex is less than the output index.

[GalloDaSballo]

The older code would be forced to verify at the same index: https://github.com/ethereum-optimism/optimism/blame/2c0ad576f396766694ca5423bb35786fe0e97b06/packages/contracts-bedrock/contracts/L1/OptimismPortal.sol

The new code could use any valid index, but it can only be one

[rvierdiiev]

Escalate for 10 usdc.

I am not sure that i understood sponsor's response well. But in case if latestOutputIndex is less than the output index of withdrawal, that means that this withdrawal will never be executed, as its output index is now invalid and when execute withdrawal will be called then tx will revert. So i don't see any problems, that user now can prove it again. I don't understand how he can prove multiple withdrawals(or what that means). Because once he will prove withdrawal again, then its output index will be less or equal to latestOutputIndex, so he will not be able to pass the check again and reapprove same withdrawal again.

That's why i believe this is valid issue.

[sherlock-admin]

Escalate for 10 usdc.

I am not sure that i understood sponsor's response well. But in case if latestOutputIndex is less than the output index of withdrawal, that means that this withdrawal will never be executed, as its output index is now invalid and when execute withdrawal will be called then tx will revert. So i don't see any problems, that user now can prove it again. I don't understand how he can prove multiple withdrawals(or what that means). Because once he will prove withdrawal again, then its output index will be less or equal to latestOutputIndex, so he will not be able to pass the check again and reapprove same withdrawal again.

That's why i believe this is valid issue.

You've created a valid escalation for 10 USDC!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[hrishibhat]

Escalation rejected

the code works as expected If a user needs to re-prove needs to wait for 7 days, Additional Sponsor comment.

This is describing expected behavior.

[sherlock-admin]

Escalation rejected

the code works as expected If a user needs to re-prove needs to wait for 7 days, Additional Sponsor comment.

This is describing expected behavior.

This issue's escalations have been rejected!

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