Rock Paper Scissors

Summary

The joinGameWithEth() function in the RockPaperScissors contract receives ETH from Player B but doesn't explicitly track this ETH in the contract's state variables. This creates an accounting discrepancy and potential for fund loss, especially in game cancellation or refund scenarios.

Vulnerability Details

In the joinGameWithEth() function (lines 155-165), Player B sends ETH to match the bet amount set by Player A:

The function is correctly marked as payable and checks that the sent amount matches the required bet. However, it doesn't update any state variable to track that this ETH has been received. The ETH is implicitly stored in the contract's balance, but there's no accounting for it in the game's state.

This contrasts with the token-based game creation and joining, where tokens are explicitly transferred to the contract with transferFrom():

Impact

This lack of explicit ETH accounting can lead to several issues:

1. Accounting Discrepancy: The contract's ETH balance might not match the sum of all game bets, making it difficult to audit and verify the contract's financial state.

2. Potential Fund Loss: If a game is canceled or ends in a draw, there's no clear record of how much ETH should be returned to Player B. This could lead to incorrect refund amounts or even complete loss of funds.

3. Reentrancy Vulnerability Enhancement: Since the ETH tracking isn't explicit, it could complicate reentrancy protection and make it harder to implement secure fund management.

4. Contract Insolvency Risk: Over time, if the contract fails to properly account for all ETH received, it could become insolvent and unable to pay out winnings or refunds.

Tools Used

• Manual code review

• Static analysis of the contract's ETH handling

• Logical analysis of game flow and fund management

Recommendations

1. Update the joinGameWithEth() function to explicitly track the received ETH:

   function joinGameWithEth(uint256 _gameId) external payable {

   Game storage game = games[_gameId];

   ​

   require(game.state == GameState.Created, "Game not open to join");

   require(game.playerA != msg.sender, "Cannot join your own game");

   require(block.timestamp <= game.joinDeadline, "Join deadline passed");

   require(msg.value == game.bet, "Bet amount must match creator's bet");

   ​

   game.playerB = msg.sender;

   game.totalBet = game.bet * 2; // Explicitly track the total bet amount

   game.state = GameState.Committed; // Update game state

   emit PlayerJoined(_gameId, msg.sender);

   }

2. Add a totalBet field to the Game struct to track the total ETH held for each game:

   struct Game {

   // Existing fields...

   uint256 totalBet; // Total ETH held for this game

   // Other fields...

   }

3. Update all functions that handle ETH (like _finishGame() and _cancelGame()) to use this explicit accounting:

   function _finishGame(Game storage game, address winner) private {

   // ...

   if (game.bet > 0) {

   // Use the explicitly tracked bet amount

   (bool sent, ) = winner.call{value: game.totalBet}("");

   require(sent, "Failed to send Ether");

   }

   // ...

   }

4. Consider implementing a pull-over-push pattern for ETH withdrawals to further enhance security and avoid potential reentrancy issues.

These changes would ensure proper accounting of ETH throughout the contract, reducing the risk of fund loss and improving the contract's financial transparency.