Loss of bonds and the game due to wrong assumption in resolveClaim()

Summary

Incorrect resolveClaim() logic can make the wrong team win and scoop up the bonds of the other team.

Vulnerability Detail

The game runs a chess clock for each time, at every moment any unanswered claim counts against the pending response team, until it's timer runs out and it can no longer perform moves. To resolve a claim, the code logic needs to make sure that it can no longer be answered by a counter-move, i.e. the opponent's timer has ran to zero.

When the timer runs out, a claim can be resolved. At this point it loops over all challenges to the claim and if any have their own challenges, it reverts. Otherwise, it treats the challenge as successful and marks the claim as countered.

for (uint256 i = 0; i < challengeIndicesLen; ++i) {
    uint256 challengeIndex = challengeIndices[i];
    // INVARIANT: Cannot resolve a subgame containing an unresolved claim
    if (subgames[challengeIndex].length != 0) revert OutOfOrderResolution();
    ClaimData storage claim = claimData[challengeIndex];
    // If the child subgame is uncountered and further left than the current left-most counter,
    // update the parent subgame's `countered` address and the current `leftmostCounter`.
    // The left-most correct counter is preferred in bond payouts in order to discourage attackers
    // from countering invalid subgame roots via an invalid defense position. As such positions
    // cannot be correctly countered.
    // Note that correctly positioned defense, but invalid claimes can still be successfully countered.
    if (claim.counteredBy == address(0) && leftmostCounter.raw() > claim.position.raw()) {
        countered = claim.claimant;
        leftmostCounter = claim.position;
    }
}

The critical issue is that it is incorrect to assume any unanswered challenges are definitely correct challenges and treat them as final. It could well be that these are freshly made challenges and the opposing team still has time to respond to them. The function only takes the target claim's timer into account, not its challenge's timer. Logically we don't give the team of the claim's side sufficient time to answer the expired team's challenge. This can be abused to force incorrect resolution of the game.

GAME_DURATION = 100

Dishonest team proposes a malicious root at T=0
Honest team responds with valid claim at T=5 , duration = 5
Dishonest team waits until T=48 and makes invalid claim, duration = 43
In proper functioning of contract, honest team should be able to respond at T=63, duration = 10 (they spend 5 seconds per response)
However, due to the bug, dishonest team can resolve the valid claim at T = 51. It counts duration = 5, +46 seconds = 51 seconds > half of GAME_DURATION. The valid claim resolves as countered, because the invalid claim challenging it is unanswered.

Impact

Incorrect resolution of a claim, directly leading to loss of bonds and malicious roots being approved.

Code Snippet

Tool used

Manual Review

Recommendation

When looping over the challenges for the claim being resolved, make sure those have expired as well, otherwise the honest team may need to respond to them.

Duplicate of #8

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

Trust - Loss of bonds and the game due to wrong assumption in resolveClaim() #198