Rock Paper Scissors

Summary

The RockPaperScissors.sol contract contains a state management flaw affecting multi-turn games. After a turn successfully completes (both players reveal on time) and it's not the final turn, the _determineWinner function resets game variables (moveA, moveB, commitA, commitB) and sets the game.state back to Committed in preparation for the next turn. However, it fails to invalidate or clear the revealDeadline associated with the just completed turn. If the revealDeadline for the completed turn passes after this state reset, the timeoutReveal function can be called. It incorrectly interprets the state (Committed, deadline passed, moves == None) as a scenario where neither player revealed for the previous turn, leading it to erroneously call _cancelGame. This allows a player to unfairly trigger a full game cancellation and refund after a turn has legitimately concluded.

Vulnerability Details

1. Turn Completion (Non-Final): In a multi-turn game, when both players successfully call revealMove for turn N, the _determineWinner function is triggered.

2. State Reset for Next Turn: Inside _determineWinner, if game.currentTurn < game.totalTurns, the following state changes occur to prepare for turn N+1:

   • game.currentTurn is incremented.

   • game.commitA and game.commitB are reset to bytes32(0).

   • game.moveA and game.moveB are reset to Move.None.

   • game.state is set back to GameState.Committed.

3. Persistent Deadline: Crucially, the game.revealDeadline which was set when both players committed for turn N is not reset or cleared during this state transition.

4. Timeout Window: A window opens after the state reset for turn N+1 but before the next turn significantly progresses. During this window, the revealDeadline for turn N may pass (block.timestamp > game.revealDeadline).

5. timeoutReveal Called: If timeoutReveal(_gameId) is called during this window:

   • The check require(game.state == GameState.Committed, ...) passes because the state was reset for turn N+1.

   • The check require(block.timestamp > game.revealDeadline, ...) passes because the deadline for turn N has expired.

   • The status checks bool playerARevealed = game.moveA != Move.None; and bool playerBRevealed = game.moveB != Move.None; both evaluate to false because the moves were reset to Move.None by _determineWinner.

   • Consequently, the condition else if (!playerARevealed && !playerBRevealed) becomes true.

6. Incorrect Action: timeoutReveal incorrectly concludes that neither player revealed for turn N and calls the internal function _cancelGame(_gameId).

Proof Of Concept

The provided test case below demonstrates the vulnerability

Impact

• Unfair Game Termination: Allows a player (potentially one who is losing or anticipates losing) to force the cancellation of a game after a turn has been legitimately won or lost.

• Denial of Service (Game Progression): Prevents the game from reaching its natural conclusion based on the agreed number of turns and player skill/luck.

• Griefing: Enables malicious players to disrupt ongoing multi-turn games and force refunds, wasting opponents' time and potentially gas fees.

• User Trust Erosion: Players expect games to proceed according to the rules; unexpected cancellations after successful turns undermine confidence in the platform's fairness and reliability. (Note: This specific bug leads to refunds, not direct fund theft, but disrupts the game).

Tools Used

• Manual Code Review

• Foundry/Forge (for Test Execution and PoC verification)

Recommendations

The root cause is the failure to invalidate the previous turn's revealDeadline when resetting the state for the next turn.

1. Clear revealDeadline on Turn Progression: Modify the _determineWinner function. Within the if (game.currentTurn < game.totalTurns) block, after resetting other variables, explicitly reset the deadline:

   if (game.currentTurn < game.totalTurns) {

   // Reset for next turn

   game.currentTurn++;

   game.commitA = bytes32(0);

   game.commitB = bytes32(0);

   game.moveA = Move.None;

   game.moveB = Move.None;

   + game.revealDeadline = 0; // Clear the deadline from the previous turn

   game.state = GameState.Committed;

   } else {

   // End game logic...

   }

   This ensures that once a turn is processed, its associated deadline can no longer trigger the timeoutReveal logic.

2. Add Deadline Check in timeoutReveal (Optional Robustness): As an additional safeguard, consider adding a check in timeoutReveal to ensure the deadline being checked is non-zero. This prevents potential issues if the deadline was somehow already zero.

   function timeoutReveal(uint256 _gameId) external {

   // ... other checks ...

   require(game.state == GameState.Committed, "Game not in reveal phase");

   + require(game.revealDeadline != 0, "Reveal deadline not set"); // Add this check

   require(block.timestamp > game.revealDeadline, "Reveal phase not timed out yet");

   // ... rest of function ...

   }

Implementing Recommendation 1 is essential to fix the vulnerability. Recommendation 2 adds an extra layer of defense.