Rock Paper Scissors

Summary

In both joinGameWithEth() and joinGameWithToken() functions, the game state is not updated from Created to Committed after a player joins. This is inconsistent with the game flow described in the README and could lead to unexpected behavior, including potential race conditions and confusion for players.

Vulnerability Details

The issue exists in two functions:

1. In joinGameWithEth() (lines 153-164), after Player B joins a game with ETH:

   function joinGameWithEth(uint256 _gameId) external payable {

   Game storage game = games[_gameId];

   ​

   require(game.state == GameState.Created, "Game not open to join");

   require(game.playerA != msg.sender, "Cannot join your own game");

   require(block.timestamp <= game.joinDeadline, "Join deadline passed");

   require(msg.value == game.bet, "Bet amount must match creator's bet");

   ​

   game.playerB = msg.sender;

   emit PlayerJoined(_gameId, msg.sender);

   // Missing: game.state = GameState.Committed;

   }

2. Similarly, in joinGameWithToken() (lines 170-185), after Player B joins a game with tokens:

   function joinGameWithToken(uint256 _gameId) external {

   // ... [validation code] ...

   ​

   // Transfer token to contract

   winningToken.transferFrom(msg.sender, address(this), 1);

   ​

   game.playerB = msg.sender;

   emit PlayerJoined(_gameId, msg.sender);

   // Missing: game.state = GameState.Committed;

   }

According to the game flow described in the README and the contract's state machine design, the game should transition from Created to Committed once Player B joins. However, this state transition is missing in both join functions.

Instead, the state is only updated to Committed in the commitMove() function (line 201) when the first player makes a move, but only if both commitA and commitB are empty (first turn, first commits).

Impact

This issue has several potential impacts:

1. Race Conditions: Multiple players could attempt to join the same game if the state isn't immediately updated after the first player joins.

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. Unexpected Behavior: Players might be confused about whether they can still join a game that appears to be in the Created state even though it already has two players.

4. Contract Logic Inconsistency: 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

Recommendations

Update both join functions to set the game state to Committed after a player successfully joins:

1. For joinGameWithEth():

   function joinGameWithEth(uint256 _gameId) external payable {

   Game storage game = games[_gameId];

   ​

   require(game.state == GameState.Created, "Game not open to join");

   require(game.playerA != msg.sender, "Cannot join your own game");

   require(block.timestamp <= game.joinDeadline, "Join deadline passed");

   require(msg.value == game.bet, "Bet amount must match creator's bet");

   ​

   game.playerB = msg.sender;

   game.state = GameState.Committed; // Add this line

   emit PlayerJoined(_gameId, msg.sender);

   }

2. For joinGameWithToken():

   function joinGameWithToken(uint256 _gameId) external {

   // ... [validation code] ...

   ​

   // Transfer token to contract

   winningToken.transferFrom(msg.sender, address(this), 1);

   ​

   game.playerB = msg.sender;

   game.state = GameState.Committed; // Add this line

   emit PlayerJoined(_gameId, msg.sender);

   }

3. Update the commitMove() function to handle the case where the game is already in the Committed state after a player joins, but before any moves are committed.