Rock Paper Scissors

Summary

The RockPaperScissors.sol contract allows a game to be cancelled almost immediately if one player commits their move but the opponent fails to commit theirs. This occurs because the revealDeadline is only set within the commitMove function after both players have submitted their commitments. If only one player commits, the revealDeadline remains at its default value of 0. The timeoutReveal function checks if block.timestamp > game.revealDeadline. Since block.timestamp is always greater than 0, this check passes instantly when revealDeadline is 0. Consequently, either player can call timeoutReveal immediately after the first player commits (and before the second player commits), triggering the logic path for "neither player revealed" (as the reveal phase never truly started), which results in _cancelGame being called. While this correctly refunds stakes, it allows a player (or even the first player) to force an immediate cancellation, potentially constituting a minor griefing or Denial of Service vector against completing the game.

Vulnerability Details

1. revealDeadline Initialization: The revealDeadline field in the Game struct defaults to 0.

2. commitMove Logic: The revealDeadline is only calculated and set if both game.commitA and game.commitB are non-zero:

   // Inside commitMove

   if (game.commitA != bytes32(0) && game.commitB != bytes32(0)) {

   game.revealDeadline = block.timestamp + game.timeoutInterval;

   }

   If only Player A commits, game.commitB remains 0, and game.revealDeadline stays 0.

3. timeoutReveal Logic: This function contains the check:

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

4. Exploitation Scenario:

   • Player A creates and Player B joins a game.

   • Player A calls commitMove. game.commitA is set, game.state becomes Committed, but game.revealDeadline remains 0.

   • Player B (or Player A) immediately calls timeoutReveal.

   • The check block.timestamp > game.revealDeadline (i.e., block.timestamp > 0) passes instantly.

   • Since neither game.moveA nor game.moveB has been set (the reveal phase didn't start), the condition !playerARevealed && !playerBRevealed becomes true.

   • timeoutReveal calls _cancelGame(_gameId).

Proof Of Concept

The test case below demonstrates the behavior

Impact

• Minor Griefing / Denial of Service: Allows either player (most likely Player B, but Player A could also change their mind) to force an immediate cancellation after Player A has committed, preventing the game from proceeding. This wastes Player A's gas spent on game creation and committing.

• User Experience: Can be confusing or frustrating if games are frequently cancelled immediately after the first commit.

Tools Used

• Manual Code Review

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

Recommendations

The current behavior, while perhaps slightly unintuitive, ensures games don't get stuck indefinitely if a player fails to commit. However, the immediacy of the cancellation via timeoutReveal might be considered undesirable. A more robust approach would involve a dedicated timeout for the commit phase itself.

1. Introduce a Commit Timeout (Recommended):

   • Add a new state variable to the Game struct, e.g., commitDeadline (uint256).

   • When Player A commits (and Player B has joined), set commitDeadline = block.timestamp + commitTimeoutDuration; (where commitTimeoutDuration is a new configurable or constant value, e.g., 1 hour).

   • Modify commitMove for Player B to require block.timestamp <= game.commitDeadline.

   • Create a new function timeoutCommit(uint256 _gameId):

     • Requires game.state == Committed.

     • Requires game.commitB == bytes32(0) (only B hasn't committed).

     • Requires block.timestamp > game.commitDeadline.

     • If conditions met, calls _cancelGame(_gameId).

   • Modify timeoutReveal: Add require(game.revealDeadline != 0, "Reveal deadline not set"); to ensure it only triggers after the reveal phase has actually begun (i.e., both committed).

2. Accept Current Behavior (Less Ideal): Acknowledge that the current mechanism, while allowing immediate cancellation via timeoutReveal if the second player doesn't commit, does resolve the stuck state and refunds players. Document this behavior clearly.

Implementing Recommendation 1 provides a more structured game flow with distinct timeouts for committing and revealing, preventing the immediate cancellation vector while still handling unresponsive players.