Rock Paper Scissors

Summary

The commitMove() function in the RockPaperScissors contract allows players to commit moves when the game is still in the Created state, before Player B has officially joined. This inconsistency in state management, combined with the missing state updates in join functions, creates potential race conditions and unexpected behavior.

Vulnerability Details

The issue is in the commitMove() function (lines 192-222), which explicitly allows committing moves in either the Created or Committed states:

There are several issues with this implementation:

1. The function allows committing moves when the game is in the Created state, which should only be possible after Player B has joined.

2. While there is a check to ensure Player B has joined (require(game.playerB != address(0))), this is only applied for the first turn and first commits. For subsequent commits, the function only checks that the game is in the Committed state.

3. As identified in a previous issue, the join functions (joinGameWithEth() and joinGameWithToken()) do not update the game state to Committed when Player B joins, creating an inconsistency in the state management.

4. The commitMove() function itself updates the game state to Committed when the first player commits a move, which is inconsistent with the expected game flow where the state should be updated when Player B joins.

Impact

This inconsistent state management can lead to several issues:

1. Race Conditions: Player A could commit a move before Player B has officially joined, leading to unexpected behavior.

2. Inconsistent Game State: The game state doesn't accurately reflect the current stage of the game, making it harder to reason about the contract's behavior.

3. Potential for Exploits: Malicious players could potentially exploit these inconsistencies to gain advantages or disrupt games.

4. Contract Logic Confusion: The contract relies on checking the game state in multiple functions, and inconsistent state management could lead to unexpected behavior in other parts of the contract.

While this issue doesn't directly lead to fund loss, it creates confusion and potential race conditions that could impact the user experience and contract reliability.

Tools Used

• Manual code review

• Static analysis of the contract's state management

• Logical analysis of game flow

Recommendations

1. Modify the commitMove() function to only allow commits when the game is in the Committed state:

   function commitMove(uint256 _gameId, bytes32 _commitHash) 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 commit phase");

   ​

   // ... [rest of the function] ...

   }

2. Ensure that the join functions update the game state to Committed when Player B joins, as recommended in the previous issue.

3. Remove the state update from the commitMove() function, as the game should already be in the Committed state when players are committing moves.

4. Add clear documentation about the expected state transitions in the contract to ensure consistent implementation across all functions.

These changes would ensure a more consistent and predictable state management flow throughout the contract, reducing the potential for race conditions and unexpected behavior.