Summary
Multiple opponents could join same game, overwriting previous joiners. Hereby previous opponent loses his funds - ETH/token.
Vulnerability Details
Functions RockPaperScissors::joinGameWithEth(), RockPaperScissors::joinGameWithToken() implement the conditions, allowing opponents to join already created games. There is no implemented check if the underlying game has valid opponent (playerB) joined already. This allows unbounded overwriting of already joined opponent, loosing his ETH/token bet. Only the join deadline expiration and game state transition to GameState.Committed will preserve the opponent.
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);
}
Impact
Мissing required check leads to:
opponent previously joined a game using RockPaperScissors::joinGameWithEth() is kicked out and he loses his ETH bet
opponent previously joined a game using RockPaperScissors::joinGameWithToken() is kicked out and he loses his token bet
Tools Used
Manual review
Foundry
PoC
Add the following test to RockPaperScissors.t.sol.
function testOverwriteAlreadyJoinedPlayer() public {
vm.prank(playerA);
gameId = game.createGameWithEth{value: BET_AMOUNT}(TOTAL_TURNS, TIMEOUT);
vm.startPrank(playerB);
vm.expectEmit(true, true, false, true);
emit PlayerJoined(gameId, playerB);
game.joinGameWithEth{value: BET_AMOUNT}(gameId);
vm.stopPrank();
vm.startPrank(playerC);
vm.expectEmit(true, true, false, true);
emit PlayerJoined(gameId, playerC);
game.joinGameWithEth{value: BET_AMOUNT}(gameId);
vm.stopPrank();
(address storedPlayerA, address storedPlayerC,,,,,,,,,,,,,, RockPaperScissors.GameState state) =
game.games(gameId);
assertEq(storedPlayerA, playerA);
assertEq(storedPlayerC, playerC);
assertEq(uint256(state), uint256(RockPaperScissors.GameState.Created));
}
Recommendations
Add the following check to RockPaperScissors::joinGameWithEth() and RockPaperScissors::joinGameWithToken() in order to avoid overwriting of already joined opponent.
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(playerB == address(0), "Second player already joined");
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(playerB == address(0), "Second player already joined");
require(block.timestamp <= game.joinDeadline, "Join deadline passed");
- require(game.bet == 0, "This game requires ETH bet");
+ require(game.bet == 0, "This game requires token bet");
require(winningToken.balanceOf(msg.sender) >= 1, "Must have winning token");
// Transfer token to contract
winningToken.transferFrom(msg.sender, address(this), 1);
game.playerB = msg.sender;
emit PlayerJoined(_gameId, msg.sender);
}
