MiloTruck - L1 re-orgs could cause `FaultDisputeGame.move()` to be executed on the wrong parent claim

[sherlock-admin3]

MiloTruck

medium

L1 re-orgs could cause FaultDisputeGame.move() to be executed on the wrong parent claim

Summary

Since FaultDisputeGame.move() identifies the parent claim by its index in claimData, an L1 re-org could cause move() to be executed on the wrong parent claim.

Vulnerability Detail

In FaultDisputeGame, when users call move() to make a move against a parent claim, they specify _challengeIndex, which is the index of the parent claim in the claimData array:

FaultDisputeGame.sol#L226-L231

    function move(uint256 _challengeIndex, Claim _claim, bool _isAttack) public payable virtual {
        // INVARIANT: Moves cannot be made unless the game is currently in progress.
        if (status != GameStatus.IN_PROGRESS) revert GameNotInProgress();

        // Get the parent. If it does not exist, the call will revert with OOB.
        ClaimData memory parent = claimData[_challengeIndex];

Note that the parent claim is only identified by _challengeIndex, and nothing else. Claims are pushed to the end of the claimData array whenever move() is called:

FaultDisputeGame.sol#L296-L308

        // Create the new claim.
        claimData.push(
            ClaimData({
                parentIndex: uint32(_challengeIndex),
                // ...
            })
        );

Since the parent claim is only identified by _challengeIndex when calling move(), this makes move() vulnerable to L1 re-orgs. More specifically, it is possible for the following to occur:

• Assume the following unfinalized blocks:
  • Block 1: Proposer calls move() to submit a claim. Assume this new claim has position 8 and is pushed to claimData at index 5.
  • Block 2: Challenger calls move() with _challengeIndex = 5 to dispute the claim at position 8.
  • Block 3: Another party calls move() to submit an unrelated claim. Assume this claim has position 12.
• An L1 re-org occurs, placing block 3 before block 1. The new order of execution is:
  • Block 3 is executed. The claim at position 12 is pushed to the claimData array first. It ends up at index 5.
  • Block 1 is executed. This pushes the claim at position 8 to claimData at index 6.
  • Block 2 is executed, which calls move() with _challengeIndex = 5.
• The challenger ends up disputing the claim at position 12, instead of the claim at position 8 that he originally intended to dispute.

As seen from above, an L1 re-org can cause an honest challenger to end up disputing the wrong claim.

Impact

When an L1 re-org occurs, honest proposers and challengers could end up executing move() on the wrong claim. This causes them to lose funds as they end up paying bonds to submit invalid claims, which will then be contested.

Note that if a party's remaining game duration is less than the time it takes for an L1 block to finalize, it will not be possible to wait for the L1 block containing a previous move to finalize. For example:

• Assume GAME_DURATION is 8 days, which means each party gets 4 days of game duration.
• If a challenger's previous move has a duration of 4 days - 10 minutes, he only has 10 minutes to respond to a proposer's claim.
• It takes ~15 minutes for an L1 block to finalize.
• Therefore, since the challenger's remaining duration is less than the time needed for L1 block finality, the challenger cannot wait for the proposer's claim to be included in a finalized L1 block. Otherwise, move() will revert as his clock has run out.

Code Snippet

https://github.com/sherlock-audit/2024-02-optimism-2024/blob/f216b0d3ad08c1a0ead557ea74691aaefd5fd489/optimism/packages/contracts-bedrock/src/dispute/FaultDisputeGame.sol#L226-L231

https://github.com/sherlock-audit/2024-02-optimism-2024/blob/f216b0d3ad08c1a0ead557ea74691aaefd5fd489/optimism/packages/contracts-bedrock/src/dispute/FaultDisputeGame.sol#L296-L308

Tool used

Manual Review

Recommendation

Consider adding the parent claim's claim and position fields as a parameter:

- function move(uint256 _challengeIndex, Claim _claim, bool _isAttack) public payable virtual {
+ function move(uint256 _challengeIndex, Claim_parentClaim, Position _parentPos, Claim _claim, bool _isAttack) public payable virtual {

In move(), compare _parentClaim and _parentPos with the data stored in claimData[_challengeIndex]:

  // Get the parent. If it does not exist, the call will revert with OOB.
  ClaimData memory parent = claimData[_challengeIndex];
+ if (parent.claim.raw() != _parentClaim.raw() || parent.position.raw() != _parentPos.raw()) {
+     revert IncorrectParentClaim();
+ }

This ensures that move() will never be called on the wrong parent claim.

Duplicate of #201

[nevillehuang]

Invalid based on sherlock rules

20. Chain re-org and network liveness related issues are not considered valid.