Summary

The RockPaperScissors contract fails to reset the revealDeadline variable between game rounds, creating a critical vulnerability in the core gameplay mechanism. This flaw allows a malicious player to gain an unfair advantage by revealing their move before their opponent can commit, rendering the opponent unable to participate in the round, and then exploiting the timeout mechanism to automatically win the game and claim the entire prize pool.

Vulnerability Details

After a round completes, the _determineWinner function resets several game state variables but crucially fails to reset the revealDeadline:

This vulnerability can be exploited through three critical flaws working together:

1. In the revealMove function, there is no check to ensure both players have committed before allowing reveals:

1. In the commitMove function, once a player has revealed, the other player is blocked from committing:

1. The timeoutReveal function automatically declares a winner if only one player has revealed:

Impact

This vulnerability has severe consequences for the game's integrity:

• Complete Breakdown of Game Fairness: A malicious player can bypass the core commit-reveal mechanism intended to prevent players from seeing each other's moves before committing.

• Automatic Win Exploitation: Attackers can guarantee wins without actual gameplay by blocking their opponents from participating.

• Financial Loss: Victims lose their bet amounts to attackers who exploit this vulnerability.

• Unwinnable Scenario: The victim player is placed in a position where they cannot defend against the attack by any means.

• Protocol Trust Degradation: This exploitable vulnerability undermines trust in the protocol and renders the game unplayable.

Tools Used

• Manual review

• Foundry

Proof of Concept

The following steps demonstrate how this vulnerability can be exploited:

1. Complete the first round of the game normally (both players commit and reveal)

2. When the next round begins, Player A quickly commits their move

3. Player A immediately reveals their move (which is allowed due to the lingering revealDeadline)

4. Player B attempts to commit but is blocked by the check require(game.moveA == Move.None && game.moveB == Move.None, "Moves already committed for this turn");

5. Player A waits for the reveal deadline to pass

6. Player A calls timeoutReveal() which automatically declares them the winner, since they have revealed and Player B could not

Below is a Foundry test that proves this vulnerability exists:

Recommendations

To fix this vulnerability, implement all of the following changes:

1. Reset the revealDeadline between rounds

   function _determineWinner(uint256 _gameId) internal {

   // ... existing code ...

   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.state = GameState.Committed;

   game.revealDeadline = 0; // Add this line to reset the deadline

   }

   // ... existing code ...

   }

2. Ensure both players have committed before allowing reveals

   function revealMove(uint256 _gameId, uint8 _move, bytes32 _salt) external {

   Game storage game = games[_gameId];

   require(msg.sender == game.playerA || msg.sender == game.playerB, "Not a player in this game");

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

   require(block.timestamp <= game.revealDeadline, "Reveal phase timed out");

   require(game.commitA != bytes32(0) && game.commitB != bytes32(0), "Both players must commit first");

   require(_move >= 1 && _move <= 3, "Invalid move");

   // ... rest of the function ...

   }

3. Allow commits even if one player has already revealed

   function commitMove(uint256 _gameId, bytes32 _commitHash) external {

   // ... existing code ...

   // Remove or modify this check to allow commits when moves are revealed

   // require(game.moveA == Move.None && game.moveB == Move.None, "Moves already committed for this turn");

   // Use a more specific check for each player

   if (msg.sender == game.playerA) {

   require(game.commitA == bytes32(0), "Already committed");

   game.commitA = _commitHash;

   } else {

   require(game.commitB == bytes32(0), "Already committed");

   game.commitB = _commitHash;

   }

   // ... rest of the function ...

   }

4. Consider adding a commit phase timeout as well, to prevent game stalling if a player never commits.