Rock Paper Scissors
First Flight #38
Beginner FriendlySolidity
100 EXP
View results
Previous Next
Submission Details
Severity: medium
Valid

Possible DoS If Second Player Never Commits

0xsaii

Summary

Game Can Be Stuck Indefinitely If Second Player Never Commits

Vulnerability Details

In the commitMove function, once the first player commits, the game state transitions from Created to Committed. However, the cancelGame function only allows cancellation when the game is still in the Created state.

This means:

  • If playerA commits, and playerB never calls**** commitMove, the game stays stuck in the Committed state forever.

  • The game never reaches the reveal phase (revealDeadline is only set after both players commit).

  • There is no way for the creator or any player to cancel or recover from this state.

This opens up the game to griefing attacks where a malicious or unresponsive player joins a game and intentionally never commits, permanently stalling it.

Impact

Could result in a denial-of-service against active players.

Tools Used

Manual Review

Recommendations

  1. Set a deadline for the commit. For example, 2 hours.

    + uint256 public commitTimeout = 2 hours;

  2. Add a commitDealine variable to the Game struct:

    struct Game {
    address playerA; // Creator of the game
    address playerB; // Second player to join
    uint256 bet; // Amount of ETH bet
    uint256 timeoutInterval; // Time allowed for reveal phase
    uint256 revealDeadline; // Deadline for revealing moves
    uint256 creationTime; // When the game was created
    uint256 joinDeadline; // Deadline for someone to join the game
    + uint256 commitDeadline; // Deadline for committing moves
    uint256 totalTurns; // Total number of turns in the game
    uint256 currentTurn; // Current turn number
    bytes32 commitA; // Hashed move from player A
    bytes32 commitB; // Hashed move from player B
    Move moveA; // Revealed move from player A
    Move moveB; // Revealed move from player B
    uint8 scoreA; // Score for player A
    uint8 scoreB; // Score for player B
    GameState state; // Current state of the game
    }

  3. Whenever a player commits in the commitMove function, set a commitDeadline for both the first and subsequent turns:

    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)
    ) {
    // this is to check if they have not commited b4?
    // First turn, first commits
    require(game.playerB != address(0), "Waiting for player B to join");
    game.state = GameState.Committed;
    + game.commitDeadline = block.timestamp + commitTimeout; // Set commit deadline for the first turn
    } else {
    // Later turns or second player committing
    + if (game.commitDeadline == 0) {
    + game.commitDeadline = block.timestamp + commitTimeout;
    + }
    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 both players have committed, set the reveal deadline
    if (game.commitA != bytes32(0) && game.commitB != bytes32(0)) {
    game.revealDeadline = block.timestamp + game.timeoutInterval; //so revealDeadline and timeoutInterval are the same?
    }
    }

  4. Consider allowing both players to cancel a game if one of the players do not commit before the commit deadline:

    function cancelGame(uint256 _gameId) external {
    Game storage game = games[_gameId];
    - require(game.state == GameState.Created, "Game must be in created state");
    - require(msg.sender == game.playerA, "Only creator can cancel");
    - _cancelGame(_gameId);
    + // Case 1: Player A can cancel anytime in Created state
    + if (game.state == GameState.Created) {
    + require(
    + msg.sender == game.playerA,
    + "Only creator can cancel during Created state"
    + );
    + _cancelGame(_gameId);
    + return;
    + }
    ​
    + // Case 2: Either player can cancel if the other hasn't committed after timeout
    + if (
    + game.state == GameState.Committed &&
    + ((msg.sender == game.playerA && game.commitB == bytes32(0)) ||
    + (msg.sender == game.playerB && game.commitA == bytes32(0))) &&
    + block.timestamp > game.creationTime + commitTimeout
    + ) {
    + _cancelGame(_gameId);
    + return;
    + }
    +
    + revert("Cancel conditions not met");
    +

  5. Reset the commit deadline after each turn in the _determineWinner function:

    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)
    //if one of these conditions is true, do this right?
    ) {
    // 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.commitDeadline = 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);
    }
    }
    ​
Updates

Appeal created

m3dython Lead Judge 6 months ago
Submission Judgement Published
Validated
Assigned finding tags:

Player B cannot cancel a game if Player A becomes unresponsive after Player B joins

Protocol does not provide a way for Player B to exit a game and reclaim their stake if Player A stops participating

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.