Skip to content
This repository has been archived by the owner on May 26, 2023. It is now read-only.

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

Open
github-actions bot opened this issue Feb 20, 2023 · 5 comments
Labels
Escalation Resolved This issue's escalations have been approved/rejected Has Duplicates A valid issue with 1+ other issues describing the same vulnerability Medium A valid Medium severity issue Reward A payout will be made for this issue

Comments

@github-actions
Copy link

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
Copy link
Contributor

Comment from Optimism


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

Reason: Yeah, this LGTM.

@sherlock-admin sherlock-admin added the Reward A payout will be made for this issue label Feb 21, 2023
@Allarious
Copy link

Allarious commented Feb 23, 2023

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.

@sherlock-admin
Copy link
Contributor

sherlock-admin commented Feb 23, 2023

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.

@sherlock-admin sherlock-admin added the Escalated This issue contains a pending escalation label Feb 23, 2023
@Evert0x
Copy link
Contributor

Evert0x commented Mar 2, 2023

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
Copy link
Contributor

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Escalation Resolved This issue's escalations have been approved/rejected Has Duplicates A valid issue with 1+ other issues describing the same vulnerability Medium A valid Medium severity issue Reward A payout will be made for this issue
Projects
None yet
Development

No branches or pull requests

4 participants