Rock Paper Scissors

Summary

The commitMove() function in the RockPaperScissors contract doesn't validate that the commit hash corresponds to a valid move (1-3 for Rock, Paper, Scissors). This allows players to commit to invalid moves, which will only be caught during the reveal phase, potentially leading to game disruption and wasted gas.

Vulnerability Details

In the commit-reveal pattern used by the contract, players first commit a hash of their move and a salt, then later reveal the actual move and salt. The issue is that the commitMove() function (lines 192-222) accepts any bytes32 hash without verifying that it corresponds to a valid move:

The validation only happens later in the revealMove() function (lines 230-257):

This means a player can commit to an invalid move (e.g., 0, 4, or any other value), and the issue will only be discovered during the reveal phase. At that point, the player will be unable to successfully reveal their move, potentially disrupting the game.

Impact

This vulnerability has several potential impacts:

1. Game Disruption: A player could intentionally commit to an invalid move, then be unable to reveal it, causing the game to stall until the reveal timeout is reached.

2. Wasted Gas: Players who accidentally commit invalid moves will waste gas on both the commit and attempted reveal transactions.

3. Forced Timeouts: A malicious player could exploit this to force games into timeout scenarios, potentially gaining an advantage if their opponent has already revealed a valid move.

4. Poor User Experience: Players who make genuine mistakes in the commit phase will have a confusing experience when they cannot successfully reveal their moves.

While this issue doesn't directly lead to fund loss, it can be exploited to manipulate game outcomes and waste resources, justifying a medium severity rating.

Tools Used

• Manual code review

Recommendations

Implement a pre-commitment validation mechanism that ensures the committed hash corresponds to a valid move. This can be done by requiring players to provide a zero-knowledge proof or by implementing a more structured commitment format.

Here's a suggested approach:

1. Modify the commitMove() function to accept the move and salt separately, then compute the hash internally:

   function commitMove(uint256 _gameId, uint8 _move, bytes32 _salt) 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");

   require(_move >= 1 && _move <= 3, "Invalid move");

   bytes32 commitHash = keccak256(abi.encodePacked(_move, _salt));

   // ... rest of the function ...

   }

2. Alternatively, if you want to maintain the current interface, implement a verification function that players can use to check their commit hash before submitting:

   function verifyCommitHash(uint8 _move, bytes32 _salt, bytes32 _commitHash) public pure returns (bool) {

   require(_move >= 1 && _move <= 3, "Invalid move");

   return keccak256(abi.encodePacked(_move, _salt)) == _commitHash;

   }

Either approach would help prevent invalid moves from being committed, improving the reliability and user experience of the game.