search

sherlock-audit / 2024-02-optimism-2024-judging

6 stars 4 forks source link

darkbit - Guardian can override accepted valid withdrawals after PROOF_MATURITY_DELAY_SECONDS and DISPUTE_GAME_FINALITY_DELAY_SECONDS passed #169

Closed sherlock-admin4 closed 6 months ago

sherlock-admin4 commented 7 months ago

darkbit

medium

Guardian can override accepted valid withdrawals after PROOF_MATURITY_DELAY_SECONDS and DISPUTE_GAME_FINALITY_DELAY_SECONDS passed

Summary

All withdrawals are finalized after a PROOF_MATURITY_DELAY_SECONDS and DISPUTE_GAME_FINALITY_DELAY_SECONDS window (finalization period). After this duration transaction are confirmed and user can surely withdraw their balance. But if guardian changes the respectedGameType then those valid withdrawals can no longer be finalized.

Vulnerability Detail

This is similar to this finding in previous audits. This is the POC for this issue:

  1. User A create a withdraw transaction (L2->L2) in block number X.
  2. Someone creates a dispute game Y with corresponding outputRoot.
  3. User A prove his tx by calling proveWithdrawalTransaction() and proving his transaction against game Y.
  4. Dispute game Y finished with state DEFENDER_WINS.
  5. After PROOF_MATURITY_DELAY_SECONDS and DISPUTE_GAME_FINALITY_DELAY_SECONDS seconds User A is sure that his transaction is finalized and he can call finalizeWithdrawalTransaction() and receive his funds any time he wants.
  6. After some time due to some incident or update the Guardian changes the respectedGameType value.
  7. Now User A can't finalize his withdrawal even so his withdraw was accepted.
  8. In fact all the proven old withdrawal txs that was accepted can't be finalized and users have to prove them again. This is incorrect and nullifies the network guarantee.

PROOF_MATURITY_DELAY_SECONDS and DISPUTE_GAME_FINALITY_DELAY_SECONDS period is the time for raising any dispute and if finalization period is crossed then no matter what, transaction should be considered confirmed

Impact

Withdrawal will fail for confirmed transaction

Code Snippet

https://github.com/sherlock-audit/2024-02-optimism-2024/blob/f216b0d3ad08c1a0ead557ea74691aaefd5fd489/optimism/packages/contracts-bedrock/src/L1/OptimismPortal2.sol#L500-L501 https://github.com/sherlock-audit/2024-02-optimism-2024/blob/f216b0d3ad08c1a0ead557ea74691aaefd5fd489/optimism/packages/contracts-bedrock/src/L1/OptimismPortal2.sol#L512-L515

Tool used

Manual Review

Recommendation

In function checkWithdrawal() when game type is not equal to current game type code should check game.resolvedAt() + DISPUTE_GAME_FINALITY_DELAY_SECONDS < respectedGameTypeUpdatedAt and if it's true it should allow this transaction to finalized.

Duplicate of #119