Rock Paper Scissors
First Flight #38
Beginner FriendlySolidity
100 EXP
View results
Previous Next
Submission Details
Severity: high
Valid

joinGameWithEth Allows Overwriting of playerB in RockPaperScissors.sol

roccomania

Summary

The joinGameWithEth function in the RockPaperScissors.sol contract allows any user to become playerB in a game that is in the Created state, even if another user has already successfully joined as playerB. This allows the second joiner to overwrite the first, potentially disrupting the game and loss of fund.

Vulnerability Details

The joinGameWithEth function performs the necessary checks:

  • Verifies the game exists (game.playerA != address(0)).

  • Ensures the game is accepting players (game.state == GameState.Created).

  • Checks if the join deadline has passed (block.timestamp <= game.joinDeadline).

  • Confirms the caller is not Player A (msg.sender != game.playerA).

  • Validates the correct ETH amount is sent (msg.value == game.bet).

If these checks pass, it sets game.playerB = msg.sender. However, crucially, it does not change the game.state from GameState.Created.

Because the state remains Created, another user can call joinGameWithEth before the joinDeadline expires and before either player calls commitMove (which would change the state). This second caller will pass the same checks and will overwrite the game.playerB address set by the first joiner.

// In RockPaperScissors.sol
​
function joinGameWithEth(uint256 _gameId) external payable {
Game storage game = games[_gameId];
​
require(game.playerA != address(0), "Game does not exist");
require(game.state == GameState.Created, "Game not accepting players"); // State check passes for multiple callers
require(block.timestamp <= game.joinDeadline, "Join deadline passed");
require(msg.sender != game.playerA, "Player A cannot join as Player B");
require(msg.value == game.bet, "Incorrect ETH amount sent"); // Assumes bet > 0 for ETH games
​
game.playerB = msg.sender; // playerB can be overwritten by a subsequent call
// Missing state change here! e.g., game.state = GameState.Committed;
​
emit PlayerJoined(_gameId, msg.sender);
}
​
function commitMove(uint256 _gameId, bytes32 _moveCommit) external {
// ... checks ...
if (game.state == GameState.Created && game.playerB != address(0)) {
game.state = GameState.Committed; // State only changes *here*, too late to prevent playerB overwrite
game.revealDeadline = uint64(block.timestamp + timeoutInterval);
}
// ... rest of commit logic ...
}
​

Impact

This vulnerability allows a malicious user or simply a concurrent user to replace the intended playerB. This disrupts the game setup process. The originally joined playerB might expect to play but finds they have been replaced.

While the ETH sent by the first joiner might eventually be reclaimable via timeoutJoin if the game never starts properly, the user is denied participation in that specific game instance. It can be used as a griefing vector to prevent specific players from joining games.

Tools Used

  • Manual code review.

Recommendations

To fix this vulnerability, the joinGameWithEth function should immediately transition the game state out of Created once playerB is successfully set. Changing the state to Committed seems appropriate, as the next step is for players to commit their moves.

Add the following line within joinGameWithEth after setting game.playerB:

function joinGameWithEth(uint256 _gameId) external payable {
Game storage game = games[_gameId];
​
require(game.playerA != address(0), "Game does not exist");
require(game.state == GameState.Created, "Game not accepting players");
require(block.timestamp <= game.joinDeadline, "Join deadline passed");
require(msg.sender != game.playerA, "Player A cannot join as Player B");
require(msg.value == game.bet, "Incorrect ETH amount sent");
​
game.playerB = msg.sender;
+ // Transition state immediately to prevent another player from joining
+ game.state = GameState.Committed;
+ // Set the reveal deadline now that both players are locked in
+ game.revealDeadline = uint64(block.timestamp + timeoutInterval);
​
emit PlayerJoined(_gameId, msg.sender);
}
​
Updates

Appeal created

m3dython Lead Judge about 2 months ago
Submission Judgement Published
Validated
Assigned finding tags:

Absence of State Change on Join Allows Player B Hijacking

Game state remains Created after a player joins

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.