Description:
The RockPaperScissors::timeoutReveal function can be called by playerA or playerB in the following scenarios where there is no genuine reveal timeout:
Game is in first turn and 1 of the 2 players have committed their move. game.state is GameState.Committed and game.revealDeadline is 0.
Game is in the second or later turn, 0 or 1 of the 2 players have committed their move, and it is past the revealDeadline of the previous turn. game.state is GameState.Committed and game.revealDeadline is equal to the one set in the previous turn.
In both cases none of the two players have revealed their move and calling the function will result in RockPaperScissors::_cancelGame being called, issuing a refund of the bets to both players.
The issue is caused by an inappropriate use of GameState.Committed throughout the game and a lack of access control in the timeoutReveal function.
Impact:
A player expecting to lose can prevent the other player from winning by cancelling the game before both moves are committed in the last turn of the game resulting in a refund of the bets.
Proof of Code:
Test for Scenario 1 - First turn, one player committed, none revealed:
function testAuditRevealTimeoutNoTimeoutFirstTurn() public {
gameId = createAndJoinGame();
bytes32 saltA = keccak256(abi.encodePacked("salt for player A"));
bytes32 commitA = keccak256(abi.encodePacked(uint8(RockPaperScissors.Move.Rock), saltA));
vm.prank(playerA);
game.commitMove(gameId, commitA);
(bool canTimeout, address winnerIfTimeout) = game.canTimeoutReveal(gameId);
assertTrue(canTimeout);
assertEq(winnerIfTimeout, address(0));
uint256 playerABalanceBefore = playerA.balance;
uint256 playerBBalanceBefore = playerB.balance;
vm.prank(playerB);
game.timeoutReveal(gameId);
(,,,,,,,,,,,,,,, RockPaperScissors.GameState state) = game.games(gameId);
assertEq(uint256(state), uint256(RockPaperScissors.GameState.Cancelled));
assertEq(playerA.balance - playerABalanceBefore, BET_AMOUNT);
assertEq(playerB.balance - playerBBalanceBefore, BET_AMOUNT);
}
Test for Scenario 2 - Last of 5 turns, playerA won 4 turns, one player committed, none revealed:
function testAuditRevealTimeoutNoTimeoutLastTurn() public {
vm.prank(playerA);
gameId = game.createGameWithEth{value: BET_AMOUNT}(5, TIMEOUT);
vm.prank(playerB);
game.joinGameWithEth{value: BET_AMOUNT}(gameId);
playTurn(gameId, RockPaperScissors.Move.Paper, RockPaperScissors.Move.Rock);
playTurn(gameId, RockPaperScissors.Move.Paper, RockPaperScissors.Move.Rock);
playTurn(gameId, RockPaperScissors.Move.Paper, RockPaperScissors.Move.Rock);
playTurn(gameId, RockPaperScissors.Move.Paper, RockPaperScissors.Move.Rock);
(,,,,,,,,,,,,, uint8 scoreA, uint8 scoreB,) = game.games(gameId);
assertEq(scoreA, 4);
assertEq(scoreB, 0);
bytes32 saltA = keccak256(abi.encodePacked("salt for turn 2"));
bytes32 commitA = keccak256(abi.encodePacked(uint8(RockPaperScissors.Move.Rock), saltA));
vm.prank(playerA);
game.commitMove(gameId, commitA);
vm.warp(block.timestamp + TIMEOUT + 1);
(bool canTimeout, address winnerIfTimeout) = game.canTimeoutReveal(gameId);
assertTrue(canTimeout);
assertEq(winnerIfTimeout, address(0));
uint256 playerABalanceBefore = playerA.balance;
uint256 playerBBalanceBefore = playerB.balance;
vm.prank(playerB);
game.timeoutReveal(gameId);
(,,,,,,,,,,,,,,, RockPaperScissors.GameState state) = game.games(gameId);
assertEq(uint256(state), uint256(RockPaperScissors.GameState.Cancelled));
assertEq(playerA.balance - playerABalanceBefore, BET_AMOUNT);
assertEq(playerB.balance - playerBBalanceBefore, BET_AMOUNT);
}
In both cases the test passes, meaning the player can call the timeoutReveal function resulting in the game being cancelled.
Recommended Mitigation:
The game should only be in the Committed state if both players have committed in the current turn and both players have not yet revealed their move. Additional access control should be set up in the timeoutReveal function:
For example, not allowing the function to be called if game.revealDeadline is 0 and resetting the game.revealDeadline to 0 at the end of each turn.
function timeoutReveal(uint256 _gameId) external {
Game storage game = games[_gameId];
require(msg.sender == game.playerA || msg.sender == game.playerB, "Not a player in this game");
require(game.state == GameState.Committed, "Game not in reveal phase");
require(block.timestamp > game.revealDeadline, "Reveal phase not timed out yet");
+ require(game.revealDeadline > 0, "Reveal deadline has not been set.");
}
function _determineWinner(uint256 _gameId) internal {
Game storage game = games[_gameId];
+ game.revealDeadline = 0;
}
