Rock Paper Scissors

Summary

The RockPaperScissors contract contains a receive() function that allows it to accept ETH sent directly to it, but provides no mechanism to track or withdraw this ETH. This creates an accounting discrepancy and permanently locks any ETH sent outside of the game functions.

Vulnerability Details

The contract includes a receive() function that allows it to accept ETH sent directly:

However, unlike ETH sent through the game functions (which is tracked in game.bet and accumulatedFees), ETH sent directly to the contract:

1. Is not tracked in any state variable

2. Is not associated with any game

3. Is not included in the accumulatedFees

4. Has no withdrawal mechanism

This means that any ETH sent directly to the contract becomes permanently locked with no way to retrieve it.

The contract's accounting system only tracks:

• Game bets: game.bet for each game

• Protocol fees: accumulatedFees for admin withdrawal

This creates a situation where:

Impact

This vulnerability has several severe impacts:

1. Permanent Fund Loss: Any ETH sent directly to the contract (accidentally or intentionally) is permanently locked with no way to recover it.

2. Accounting Discrepancy: The contract's actual ETH balance will not match the sum of tracked game bets and fees, making it difficult to audit and verify the contract's financial state.

3. Contract Insolvency Risk: If the accounting discrepancy becomes significant, it could lead to confusion about the contract's financial state and potentially disrupt operations.

4. Griefing Attack: A malicious actor could intentionally send small amounts of ETH directly to the contract to create accounting discrepancies and confusion.

5. Protocol Fee Bypass: ETH sent directly doesn't contribute to protocol fees, potentially reducing protocol revenue.

This is classified as a high severity issue because it can lead to permanent loss of user funds with no recovery mechanism.

Tools Used

• Manual code review

• Static analysis of the contract's ETH handling mechanisms

• Analysis of contract accounting system

Recommendations

Implement one of the following solutions:

1. Remove the receive() function to prevent direct ETH transfers:

   // Remove this function entirely

   receive() external payable {

   // Allow contract to receive ETH

   }

2. Revert direct ETH transfers by implementing a check in the receive() function:

   receive() external payable {

   revert("Direct ETH transfers not allowed");

   }

3. Track direct ETH transfers by adding them to the accumulated fees:

   receive() external payable {

   // Add direct transfers to accumulated fees

   accumulatedFees += msg.value;

   emit DirectEthReceived(msg.sender, msg.value);

   }

4. Implement a rescue function that allows the admin to withdraw untracked ETH:

   function rescueEth() external {

   require(msg.sender == adminAddress, "Only admin can rescue ETH");

   uint256 trackedEth = 0;

   // Calculate total ETH that should be in games

   for (uint256 i = 0; i < gameCounter; i++) {

   Game storage game = games[i];

   if (game.bet > 0 && (game.state == GameState.Created || game.state == GameState.Committed)) {

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

   trackedEth += game.bet * 2;

   } else {

   trackedEth += game.bet;

   }

   }

   }

   // Add accumulated fees

   trackedEth += accumulatedFees;

   // Calculate untracked ETH

   uint256 untrackedEth = address(this).balance - trackedEth;

   // Send untracked ETH to admin

   if (untrackedEth > 0) {

   (bool success,) = adminAddress.call{value: untrackedEth}("");

   require(success, "ETH transfer failed");

   emit UntrackedEthRescued(untrackedEth);

   }

   }

Option 1 or 2 is recommended as they prevent the issue entirely, while options 3 and 4 provide mechanisms to handle direct ETH transfers if they need to be allowed for some reason.