Rock Paper Scissors

Summary

The commitMove function within the RockPaperScissors.sol contract includes a require statement intended to ensure the state related to the previous game turn (specifically, the revealed moves) is cleared before allowing a commit for the subsequent turn. However, the associated revert message, "Moves already committed for this turn", is inaccurate and misleading. This message incorrectly suggests that the check is preventing a double-commit within the current turn, a condition which is actually handled by separate checks later in the function. This discrepancy does not represent a security vulnerability but reduces code clarity and can significantly hinder debugging and user understanding of transaction failures.

Vulnerability Details

1. Affected Function: RockPaperScissors.commitMove

2. Code Snippet:

   // Inside commitMove, within the 'else' block for subsequent commits

   else {

   require(game.state == GameState.Committed, "Not in commit phase");

   // Check if previous turn's moves were cleared (reset by _determineWinner)

   require(game.moveA == Move.None && game.moveB == Move.None, "Moves already committed for this turn"); // <-- Misleading Message

   }

3. Intended Purpose of Check: This specific require statement (game.moveA == Move.None && game.moveB == Move.None) is designed to act as a safeguard. It ensures that before a player can commit a move for turn N+1, the revealed moves (moveA, moveB) from the completed turn N have been reset to their default Move.None state by the _determineWinner function.

4. Actual Check for Double Commits: The prevention of committing twice within the same turn is correctly handled later in the function by these checks:

   if (msg.sender == game.playerA) {

   require(game.commitA == bytes32(0), "Already committed"); // Correct check

   // ...

   } else {

   require(game.commitB == bytes32(0), "Already committed"); // Correct check

   // ...

   }

5. Message Discrepancy: The revert message "Moves already committed for this turn" associated with the check on moveA/moveB is therefore incorrect. It describes the condition checked by the later commitA/commitB requirements, not the condition it is actually checking (which relates to the cleared state from the previous turn).

Proof Of Concept

While the current logic in _determineWinner correctly resets moveA and moveB, making this specific require statement unlikely to trigger in normal operation, the misleading nature of the message can be understood conceptually:

1. Hypothetical State: Assume a multi-turn game where Turn N has just concluded. Due to a hypothetical bug or modification in _determineWinner, game.moveA was not reset to Move.None, but game.state was set to Committed and game.currentTurn incremented to N+1.

2. Action: Player A attempts to call commitMove for Turn N+1.

3. Expected Revert Reason: The transaction should revert because the precondition game.moveA == Move.None is false, indicating the previous turn's state wasn't properly cleared.

4. Actual Revert Message: The transaction reverts with the message "Moves already committed for this turn".

5. Confusion: A developer or user seeing this message would likely assume Player A had already submitted commitA for Turn N+1, which is not the actual reason for the revert in this hypothetical scenario. The true reason (uncleared state from Turn N) is obscured.

Impact

• Reduced Code Clarity: The inaccurate message makes the code harder to understand and reason about.

• Increased Debugging Time: Developers encountering this revert (especially if the condition could be met due to future code changes or unforeseen interactions) would be misled, potentially spending significant time investigating the wrong cause (double commits) instead of the actual state issue (uncleared previous moves).

• User Confusion: Off-chain applications or users interpreting revert reasons would be confused about why their commit failed, potentially leading to incorrect assumptions or support requests.

• Maintainability Issues: Misleading messages make future maintenance and refactoring more error-prone.

The severity is considered Low / Informational as it does not directly impact fund security or core game fairness logic but affects developer experience and code quality.

Tools Used

• Manual Code Review

Recommendations

The revert string associated with the check should be updated to accurately reflect the condition being verified.

1. Modify Revert Message: Change the message in the commitMove function:

   else {

   require(game.state == GameState.Committed, "Not in commit phase");

   // Check if previous turn's moves were cleared (reset by _determineWinner)

   - require(game.moveA == Move.None && game.moveB == Move.None, "Moves already committed for this turn");

   + require(game.moveA == Move.None && game.moveB == Move.None, "Previous turn not resolved"); // Example fix

   + // Alternate suggestion: require(game.moveA == Move.None && game.moveB == Move.None, "Moves not cleared from previous turn");

   }

2. Suggested Messages: Use a clearer message such as:

   • "Previous turn not resolved"

   • "Moves not cleared from previous turn"

   • "Waiting for previous turn state reset"

Implementing this change will improve code clarity and make debugging potential issues related to game state transitions more straightforward.