Rock Paper Scissors

Summary

The RockPaperScissors contract is vulnerable to a cross-function reentrancy attack through the cancelGame function. This vulnerability allows a malicious player (Player A) to potentially steal funds from other players (Player Bs) by exploiting the lack of reentrancy protection and the nested if structure when ETH is refunded.

Vulnerability Details

The vulnerability exists in the interaction between the cancelGame function and the internal _cancelGame function it calls:

1. In cancelGame (lines 319-326), the function only checks:

   • If the game is in the Created state

   • If the caller is the game creator (Player A)

2. In _cancelGame (lines 548-574), the contract:

   • Sets the game state to Cancelled (line 551)

   • Refunds ETH to Player A using a low-level call (line 556)

   • Then refunds ETH to Player B if they exist (line 560)

3. The nested if statement in _cancelGame is particularly problematic:

   if (game.bet > 0) {

   (bool successA,) = game.playerA.call{value: game.bet}("");

   require(successA, "Transfer to player A failed");

   ​

   if (game.playerB != address(0)) {

   (bool successB,) = game.playerB.call{value: game.bet}("");

   require(successB, "Transfer to player B failed");

   }

   }

   This sequential execution allows Player A to receive their refund first and potentially reenter before Player B gets their refund.

4. The critical issue is that when a reentrant call is made to cancelGame for another game, it only checks if that game is in the Created state, not whether it's already been cancelled in the same transaction.

Attack Scenario

1. Attacker (Player A) creates multiple games with ETH bets

2. Other players (Player Bs) join these games with matching bets

3. Before any moves are committed, Attacker calls cancelGame() on Game #1

4. When receiving the ETH refund, Attacker reenters and calls cancelGame() on Games #2, #3, etc.

5. Due to the nested if structure, the Attacker can potentially manipulate the call sequence to steal Player B's refunds

Impact

This vulnerability has a high severity impact as it allows a malicious player to:

1. Steal ETH from other players who have joined their games

2. Drain the contract of funds meant for legitimate refunds

3. Manipulate game cancellations to their advantage

The financial impact is directly proportional to the number and size of active games with ETH bets. In a production environment with multiple games and significant betting amounts, this could lead to substantial financial losses for other players.

Tools Used

• Manual code review

Recommendations

To fix this vulnerability, implement the following changes:

1. Add a reentrancy guard using OpenZeppelin's ReentrancyGuard:

   import "@openzeppelin/contracts/security/ReentrancyGuard.sol";

   ​

   contract RockPaperScissors is ReentrancyGuard {

   // ...

   function cancelGame(uint256 _gameId) external nonReentrant {

   // existing code

   }

   }

2. Implement proper state validation in the _cancelGame function:

   function _cancelGame(uint256 _gameId) internal {

   Game storage game = games[_gameId];

   // Check if game is already in a terminal state

   require(game.state != GameState.Finished && game.state != GameState.Cancelled,

   "Game already finished or cancelled");

   game.state = GameState.Cancelled;

   // Rest of the function

   }

3. Restructure the nested if statements to avoid sequential execution:

   // Refund ETH to players

   if (game.bet > 0) {

   // Store refund amounts in memory

   uint256 refundA = game.bet;

   uint256 refundB = game.playerB != address(0) ? game.bet : 0;

   // Update state before external calls

   game.bet = 0;

   // Execute refunds after all state changes

   if (refundA > 0) {

   (bool successA,) = game.playerA.call{value: refundA}("");

   require(successA, "Transfer to player A failed");

   }

   if (refundB > 0) {

   (bool successB,) = game.playerB.call{value: refundB}("");

   require(successB, "Transfer to player B failed");

   }

   }

4. Consider using a pull-over-push pattern for refunds:

   • Store refund amounts in a mapping

   • Let players withdraw their funds with a separate function

   • This eliminates reentrancy concerns entirely