Rock Paper Scissors

Summary

The RockPaperScissors.sol contract allows users to create and join Rock Paper Scissors games. The functions joinGameWithEth and joinGameWithToken are intended to allow a second player (Player B) to join a game initiated by Player A. However, these functions fail to adequately update the game's state or check if a second player has already joined. They only verify that the game.state is Created. Consequently, multiple users, or even the same user multiple times, can successfully call the join function for the same game ID before the joinDeadline passes. This leads to the game.playerB field being overwritten by the latest joiner, and additional stakes (ETH or Tokens) being sent to the contract without being properly accounted for in the game's logic, resulting in locked funds/tokens for the extra joiners.

Vulnerability Details

1. Affected Functions: joinGameWithEth, joinGameWithToken.

2. State Check: Both functions correctly check if the game is joinable using require(game.state == GameState.Created, "Game not open to join");.

3. Missing State Update: Upon a successful join, where game.playerB is set to msg.sender, neither function updates the game.state to a different value (e.g., Committed or a hypothetical Joined state). The state remains GameState.Created.

4. Missing Occupancy Check: Neither function includes a check to verify if game.playerB is already occupied (i.e., require(game.playerB == address(0), "Game already has player B");).

5. Exploitation: Because the game.state remains Created and there is no occupancy check, subsequent calls to joinGameWithEth or joinGameWithToken for the same _gameId (before the joinDeadline) will pass the initial require checks.

6. Consequences of Subsequent Joins:

   • The game.playerB = msg.sender; line overwrites the address of the previously joined Player B with the address of the new joiner.

   • The contract receives an additional stake (ETH via msg.value or a Token via transferFrom) from the new joiner. This extra stake is not reflected in the game.bet field (which only stores the initial bet amount) and is not accounted for in the payout logic (_finishGame, _handleTie, _cancelGame).

Proof Of Concept

The provided test cases below demonstrate the vulnerability:

Impact

• Locked Funds/Tokens: The primary impact is the permanent locking of funds (ETH) or tokens sent by any joiner after the first valid Player B. These extra stakes are received by the contract but are not tracked by game.bet and therefore cannot be refunded or paid out by _finishGame, _handleTie, or _cancelGame.

• Incorrect Game Participant: The first player to join as Player B is silently overwritten in the game state by the last player who successfully called the join function. The game proceeds with the wrong Player B according to the stored data.

• Incorrect Contract Balance: The contract's overall ETH or WinningToken balance becomes inflated with unaccounted-for stakes, potentially complicating administrative actions or balance checks.

• User Confusion & Griefing: Users might join a game believing they are Player B, only to be replaced without notification. Malicious users could potentially exploit this to disrupt games or attempt to lock funds (though their own funds get locked in the process).

Tools Used

• Manual Code Review

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

Recommendations

To fix this vulnerability, the join functions must prevent subsequent joins after the first Player B is successfully registered. This can be achieved in two main ways:

1. Add Occupancy Check (Recommended): Introduce a require statement at the beginning of both joinGameWithEth and joinGameWithToken to ensure game.playerB is currently empty. This is the most direct fix.

   function joinGameWithEth(uint256 _gameId) external payable {

   Game storage game = games[_gameId];

   ​

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

   + require(game.playerB == address(0), "Game already has player B"); // Add this check

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

   // ... rest of function ...

   }

   ​

   function joinGameWithToken(uint256 _gameId) external {

   Game storage game = games[_gameId];

   ​

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

   + require(game.playerB == address(0), "Game already has player B"); // Add this check

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

   // ... rest of function ...

   }

2. Update Game State: Alternatively, modify the join functions to change the game.state immediately after game.playerB is set. For instance, change it to GameState.Committed (though this might slightly alter the intended flow if commits aren't expected immediately).

   function joinGameWithEth(uint256 _gameId) external payable {

   // ... checks ...

   ​

   game.playerB = msg.sender;

   + // Optional: Change state if appropriate for game flow

   + // game.state = GameState.Committed; // Or a new 'Joined' state

   emit PlayerJoined(_gameId, msg.sender);

   }

   // Similar change in joinGameWithToken

   (Note: The occupancy check (Recommendation 1) is generally cleaner and more precisely addresses this specific vulnerability without potentially altering other state machine assumptions.)

Implementing Recommendation 1 will effectively prevent multiple players from joining the same game slot, resolving the vulnerability and preventing the associated fund locking and state corruption issues.