search

sherlock-audit / 2023-01-optimism-judging

24 stars 10 forks source link

cmichel - Withdrawal transactions can get stuck if output root is reproposed #53

Open github-actions[bot] opened 1 year ago

github-actions[bot] commented 1 year ago

cmichel

high

Withdrawal transactions can get stuck if output root is reproposed

Summary

Withdrawal transactions may never be executed if the L2 output root for the block, for which the withdrawal was proven, is challenged and reproposed.

Vulnerability Detail

Withdrawal transactions can be reproven in the case that the output root for their previously proven output index has been updated. This can happen if the L2 output root was removed by the challenger. However, to circumvent malicious users from reproving messages all the time and resetting the withdrawal countdown, reproving can only be done on the same L2 block number (and if the output root changed).

If the challenger deletes the block with the withdrawal transaction and the proposer proposes a different block that does not have the withdrawal transaction, the withdrawal transaction can never be finalized - even if a future block includes the legitimate withdrawal transaction again, as reproving it is bound to the old provenWithdrawals[withdrawalHash].l2OutputIndex.

Impact

Legitimate withdrawal transactions will never be finalized if the proposed block was challenged and replaced with a different one not having the withdrawal transaction. As this call fails on the "lowest level", the OptimismPortal, these transactions also cannot be replayed or be issued refunds. In case the withdrawal transaction was a token bridge transfer, the tokens are stuck on the other chain and cannot be recovered by the user.

Code Snippet

https://github.com/ethereum-optimism/optimism/blob/3f4b3c328153a8aa03611158b6984d624b17c1d9/packages/contracts-bedrock/contracts/L1/OptimismPortal.sol#L196-L197 https://github.com/ethereum-optimism/optimism/blob/3f4b3c328153a8aa03611158b6984d624b17c1d9/packages/contracts-bedrock/contracts/L1/L2OutputOracle.sol#L128

Tool used

Manual Review

Recommendation

Loosen the restriction of reproving: Allow reproving under a new L2 output index whenever the output root of the proven output index changes. This still balances the other concern of malicious users reproving transactions to reset the withdrawal countdown well as in the case where the output root changed, the withdrawal needs to be proved again anyway to be finalized.

- require(
-     provenWithdrawal.timestamp == 0 ||
-         (_l2OutputIndex == provenWithdrawal.l2OutputIndex &&
-             outputRoot != provenWithdrawal.outputRoot),
-     "OptimismPortal: withdrawal hash has already been proven"
- );
+ require(
+   provenWithdrawal.timestamp == 0 || 
+     L2_ORACLE.getL2Output(provenWithdrawal.l2OutputIndex) != provenWithdrawal.outputRoot,
+   "OptimismPortal: withdrawal hash has already been proven"
+ );
rcstanciu commented 1 year ago

Comment from Optimism

Description: cannot reprove if you would need to do so on a different output index

Reason: Yeah, this LGTM.

Allarious commented 1 year ago

Escalate for 60 USDC

While this issue is mostly discussed here as something that can happen among honest actors, this can be an attack vector from the proposer role to users that disrupts the liveness of the system. This attack can lock the funds of many transactions and I believe this should be labelled as high.

The Attack

A proposer can block many transactions by submitting a faulty output root to the oracle and proving the transaction before the actual transaction reaches the L2. This can cause the total lock of funds and there is no way to recover the funds after.

Justification of this issue being high

The variations of this attack

sherlock-admin commented 1 year ago

Escalate for 60 USDC

While this issue is mostly discussed here as something that can happen among honest actors, this can be an attack vector from the proposer role to users that disrupts the liveness of the system. This attack can lock the funds of many transactions and I believe this should be labelled as high.

The Attack

A proposer can block many transactions by submitting a faulty output root to the oracle and proving the transaction before the actual transaction reaches the L2. This can cause the total lock of funds and there is no way to recover the funds after.

Justification of this issue being high

  • The proposer is not a trusted role, and the challenger role can not stop this since provenWithdrawals are already written to L1. Furthermore, Faulty proofs should not be able to lock funds up and they should just make temporary disturbance in the system.
  • Can block many incoming transactions at any time, proposer can wait as long as he wants to maximize the attack value.
  • There is no longer a guarantee for the liveness of the system since there is no guarantee anymore that withdrawal transactions from L1 can be executed.
  • This attack can be escalated if proposer and the sequencer are working together. Which means that user of the system who deposits into L2 has to trust Optimism not to lock up their funds (at the start of the project), which makes a centralized authority for L2.
  • Since the proposer role is going to be decentralized in the future, this can have catastrophic outcomes.

The variations of this attack

  • Proposer can target several transactions in L1 mempool or receive them from the sequencer on L2 to lock up. He can attack as many transaction before the outputroot is challenged.
  • Proposer can submit an outputroot where all the leaves of the tree is set to true, in this case, everyone in the network will be able to front-run each other to lock up each others transactions. (Such output root can be built with O(log(n))
  • If the proposer unknowingly sends an incorrect output root due to a faulty off-chain program, an attacker could exploit this by using it to obstruct all the incorrectly included withdrawal proofs, even if the proposer is not intentionally acting malicious.

You've created a valid escalation for 60 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 commented 1 year ago

Escalation rejected.

Although the statements in the justifications section seem all correct to me, we take into account that the proposer role will be controlled by Optimism, this restriction makes the attack extremely unlikely as it requires a private key to be leaked.

Because Optimism documented plans to decentralize this role we keep it medium severity.

sherlock-admin commented 1 year ago

Escalation rejected.

Although the statements in the justifications section seem all correct to me, we take into account that the proposer role will be controlled by Optimism, this restriction makes the attack extremely unlikely as it requires a private key to be leaked.

Because Optimism documented plans to decentralize this role we keep it medium severity.

This issue's escalations have been rejected!

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