Description
The joinGameWithEth function allows any user to join an existing game by supplying the correct gameId and bet amount. However, there is no check to ensure that the playerB slot has not already been filled. This allows any user to repeatedly overwrite the playerB address for a given gameId, leading to corrupted or inconsistent game state.
Since playerB is set unconditionally, this behavior breaks the expected one-to-one mapping between players and games. It also introduces the risk of race conditions where multiple users can attempt to join the same game at the same time — the last transaction mined wins, overwriting the others silently.
function joinGameWithEth(bytes32 gameId, Hand _hand) external payable {
Game storage game = games[gameId];
require(game.playerA != address(0), "Game does not exist");
require(msg.value == game.betAmount, "Incorrect bet amount");
game.playerB = msg.sender;
game.handB = _hand;
emit GameJoined(gameId, msg.sender);
}
There is no check like require(game.playerB == address(0)), so this function allows playerB to be overwritten any number of times before playerA reveals their hand.
Impact
State Corruption and Game Hijacking: A malicious user can repeatedly overwrite the playerB field, breaking the integrity of the game logic.
Loss of Funds: Original playerB will loose their staked ETH as there is no method to get the funds back after being overwritten.
Proof of Concepts
function testPlayerBOverwriteInJoinGameWithEth() public {
address playerC = makeAddr("playerC");
vm.deal(playerC, 10 ether);
vm.startPrank(playerA);
uint256 gameId = game.createGameWithEth{value: BET_AMOUNT}(TOTAL_TURNS, TIMEOUT);
vm.stopPrank();
vm.startPrank(playerB);
game.joinGameWithEth{value: BET_AMOUNT}(gameId);
vm.stopPrank();
(address storedPlayerA, address storedPlayerB,,,,,,,,,,,,,, RockPaperScissors.GameState state) =
game.games(gameId);
assertEq(storedPlayerA, playerA);
assertEq(storedPlayerB, playerB);
vm.startPrank(playerC);
game.joinGameWithEth{value: BET_AMOUNT}(gameId);
vm.stopPrank();
(address storedPlayerA2, address storedPlayerB2,,,,,,,,,,,,,, RockPaperScissors.GameState state2) =
game.games(gameId);
assertEq(storedPlayerA2, playerA);
assertEq(storedPlayerB2, playerC);
assertEq(uint256(state2), uint256(RockPaperScissors.GameState.Created));
}
1. Player A creates a game (gameId = 0xabc...)
2. Player B joins with 1 ETH
3. Player C calls joinGameWithEth with same gameId, also sending 1 ETH
Result: game.playerB now equals Player C, not B
Recommended mitigation
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 joined");
require(game.playerA != msg.sender, "Cannot join your own game");
require(block.timestamp <= game.joinDeadline, "Join deadline passed");
// @audit - should check if game was created using createGameWithEth
// can join game created using createGameWithToken without spending ETH or Tokens
// require(game.bet > 0, "Game Was Created With Token");
require(msg.value == game.bet, "Bet amount must match creator's bet"); // must be 1ETH
game.playerB = msg.sender;
// game.GameState = Joined
emit PlayerJoined(_gameId, msg.sender);
}
