[High] Attacker can block other users L2 to L1 withdrawals in the OptimisimPortal and lock their funds by proving it to the wrong output index if sequencer send invalid L2 output root(future L2 blocks states) for that index

Summary

Users only allowed to re-prove their withdrawal only in the case that the output root for their specified output index has been updated in the proveWithdrawalTransaction(). attacker can use this to block users from proving and finalizing their withdrawals if sequencer send wrong L2 output root for a output index and users funds would be locked in the portal forever and users would lose funds. attacker can perform this attack to all the users that their withdrawal message wrongly included in the output root. This is a High issue for multiple reasons:

L2 output proposer is not responsible for the valid L2 outputs and the protocol has the multisig and challenge mechanism to make sure L2 outputs are valid and so there could be wrong L2 output from time to time that would give attacker opportunity to perform the attack and attacker can perform attack to multiple users and lock a lot of funds when the issue happens.
Sequencer is going to be decentralized in the future and other are going to send L2 outputs by locking some funds, so other can send wrong L2 output and block a lot of funds when they see big withdrawals.
User doesn't do anything wrong in this situation and user loss funds because of sequencer and L2 output proposer mistake (which can happen by mistake or intentionally) and attacker locks users funds when the issue happens.

Vulnerability Detail

Function proveWithdrawalTransaction() proves a withdrawal transaction. to withdraw funds from L2 to L1 users should prove their withdrawals in the OptimisimPortal and then wait for the delay time and then finalize their withdrawal. to prove a withdrawal user should specify L2 output index and withdrawal existence proof in the L2 output. code would perform the checks and if everything was valid then it would set withdraw message as proven. then after finalization period has elapsed user can call finalizeWithdrawalTransaction() and withdraw his funds. if in the finalization period the L2 output root has been challenged by multisig and then the L2 output root has been changed then user required to proof his withdrawal message again. but code won't allow user to change the L2 output index when this happens and user required to proof his withdrawal for the same output index but there may be cases when the withdrawal can be in other index after the multisig's challenge. The problem is in the line:

        // We generally want to prevent users from proving the same withdrawal multiple times
        // because each successive proof will update the timestamp. A malicious user can take
        // advantage of this to prevent other users from finalizing their withdrawal. However,
        // since withdrawals are proven before an output root is finalized, we need to allow users
        // to re-prove their withdrawal only in the case that the output root for their specified
        // output index has been updated.
        require(
            provenWithdrawal.timestamp == 0 ||
                (_l2OutputIndex == provenWithdrawal.l2OutputIndex &&
                    outputRoot != provenWithdrawal.outputRoot),
            "OptimismPortal: withdrawal hash has already been proven"
        );

the scenario that attacker can block a lot of users withdrawals are this:

USER1 sends a withdraw message to L2CrossDomainMessanger in the L2 chain and send 1000 ETH to his address in the L1.
L2CrossDomainMessanger would send message to L2ToL1MessageParser and contract would set message hash as sent message in the storage and it would be done in the block number 1000.
the batch sender would send the L2 blocks to the L1 up to block number 1000.
L2OutputOracle contract has 19 for output index and L2 Output Proposer want to calculates L2 chain state in the block number 995 and publish it to the L2OutputOracle contract (the index 19). but it would send L2 chain state in the block 1000 instead of 995 and it would send wrong L2 output root (root of block 1000) to the L2OutputOracle as L2 output index 19.
now user withdrawal can be prove for the L2 output root index 19 (which shows L2 state in the block 1000 by mistake). and attacker can use this opportunity and proof wrong withdrawals in the OptimisimPortal then all the withdrawals of User1 and other users that their withdrawal included in the output root by mistake would be proven in the OptimisimPortal.
after some time Multisig would detect that L2 output root for index 19 is wrong and it would challenge it and remove it from L2OutputOracle state.
the sequencer and L2 Output proposer would calculate the correct L2 output root for index 19 which is L2 state in block 995 and would send it to the L2OutputOracle and contract would set the new L2 Output root for the index 19.
the sequencer and L2 Output proposer would calculate the correct L2 Output for index 20 which is L2 state in block 1000 and would send it to the L2OutputOracle and contract would set the new L2 Output root for the index 20.
now User1 can't call finalizeWithdrawalTransaction() because L2 output root of the index 19 has been changed and User1 can't call proveWithdrawalTransaction() to proof his withdrawal again because he can only proof his withdrawal for L2 output index 19 output root but User1 withdrawal is not in that output root and it's in the L2 Output index 20 but code won't allow user to proof his withdrawal again for another index. the same issue would happen for all the users attacker performed the attack and all of the would lose their funds including L2StandardBridge and L2ERC721Bridge messages and funds too.
in the end because of the PROPOSER mistake (which is anticipated in the protocol and mitigated by CHALLENGER) attacker was able to block a lot of funds and users loss funds. code assumes that L2 output index that user specified is correct shouldn't be changed even after challenge but attacker can proof the withdraw message for wrong index if PROPOSER send wrong L2 output root.

Impact

Attacker can cause Users to lose funds if the PROPOSER send wrong L2 output root specially when PROPOSER sends output root that includes withdrawal from the next L2 blocks. as block generation is faster than sending L2 output root so sequencer and L2 output PROPOSER has access to the next L2 blocks and even after 1 week because of the challenge it would be possible to send L2 Output root for the past week. so if PROPOSER send future L2 state output root for old L2 output indexes in the L2OutputOracle then attacker can block all those future withdrawals. as PROPOSER is going to be decentralized in the future so the attack can be more critical in the future. Users would lose their funds without doing anything wrong even if they use L2CrossDomainMessanger or L2StandardBridge or L2ERC721Bridge..

Code Snippet

https://github.com/ethereum-optimism/optimism/blob/3f4b3c328153a8aa03611158b6984d624b17c1d9/packages/contracts-bedrock/contracts/L1/OptimismPortal.sol#L194-L199

Tool used

Manual Review

Recommendation

the correct logic is that allow proving a withdrawal if:

it isn't proved in the past (provenWithdrawal.timestamp == 0)
the past proof is not valid right now (provenWithdrawal.l2OutputIndex > L2_ORACLE.latestOutputIndex() or provenWithdrawal.outputRoot != L2_ORACLE.getL2Output(provenWithdrawal.l2OutputIndex).outputRoot)

in this case even if PROPOSER send wrong L2 output root attacker would never be able to block users withdrawal by proving the withdrawal to wrong index.

Duplicate of #53

sherlock-audit / 2023-01-optimism-judging

unforgiven - [High] Attacker can block other users L2 to L1 withdrawals in the OptimisimPortal and lock their funds by proving it to the wrong output index if sequencer send invalid L2 output root(future L2 blocks states) for that index #224