Rock Paper Scissors

Summary

The RockPaperScissors.sol contract contains two distinct issues related to asset handling that can lead to the permanent loss of funds for users:

1. Locked Ether via receive(): The contract includes a receive() external payable function, allowing it to accept direct Ether transfers sent without function data. However, the contract lacks any mechanism to account for or withdraw Ether received in this manner. The administrative withdrawFees function only targets the accumulatedFees balance derived from game operations. Consequently, any Ether sent directly to the contract becomes permanently locked and irrecoverable.

2. Lack of Token Rescue Function: The contract interacts with its specific WinningToken but lacks a generic function to handle or withdraw arbitrary ERC20 tokens that might be mistakenly sent to its address by users (including WinningToken itself sent outside of game functions). Without an administrative "rescue" function, any ERC20 tokens transferred directly to the contract address are permanently locked.

Vulnerability Details

1. Locked Ether via receive() Function:

• Mechanism: The receive() external payable { ... } function enables the contract address to be a recipient of raw Ether transfers.

• Accounting Gap: The contract's internal accounting only tracks Ether related to game bets (game.bet) and the resulting protocol fees (accumulatedFees). Ether received via the receive() function does not update either of these tracked values.

• Withdrawal Limitation: The withdrawFees(uint256 _amount) function allows the adminAddress to withdraw funds, but its logic is strictly tied to the accumulatedFees state variable. It calculates amountToWithdraw based on _amount and accumulatedFees, checks against accumulatedFees, and decrements accumulatedFees. It does not consider or allow withdrawal of the contract's total Ether balance (address(this).balance).

• Outcome: Ether sent directly increases address(this).balance but remains outside the scope of any withdrawal function, rendering it permanently locked.

2. Lack of Token Rescue Function:

• ERC20 Standard: The ERC20 standard allows users to transfer tokens to any valid Ethereum address, including contract addresses, using the transfer(address recipient, uint256 amount) function. Users may mistakenly send WinningToken or other unrelated ERC20 tokens directly to the RockPaperScissors contract address.

• Missing Functionality: The RockPaperScissors contract only contains logic to interact with WinningToken in specific ways related to game staking (transferFrom) and rewards/refunds (mint - which is itself flawed). It lacks a general-purpose administrative function (often named rescueTokens, emergencyWithdraw, or similar) that would allow a designated party (like the adminAddress) to transfer arbitrary ERC20 tokens out of the contract's balance to a specified recipient.

• Outcome: Any ERC20 tokens sent to the RockPaperScissors contract address using the standard transfer function become permanently trapped, as there is no function call that can initiate their withdrawal.

Proof Of Concept

Impact

• Permanent Loss of User Funds: Users who mistakenly send Ether directly to the contract or transfer arbitrary ERC20 tokens to it will permanently lose those assets.

• Irrecoverable Assets: There is no mechanism within the contract for the admin or users to recover these locked Ether or tokens.

• Contract Balance Bloat: The contract's ETH and token balances become inflated with unusable, inaccessible assets, potentially causing confusion.

• User Confusion and Trust Issues: Users losing funds due to simple mistakes like direct transfers can lead to frustration and loss of trust in the application.

Tools Used

• Manual Code Review

Recommendations

1. Handle Locked Ether:

• Option A (Recommended): Remove receive() Function: If the contract is not designed to receive direct Ether transfers for any specific purpose (which appears to be the case), the simplest and safest solution is to remove the receive() external payable { } function entirely. This will cause direct Ether transfers to the contract to revert, preventing funds from being locked in the first place.

• Option B (Less Ideal): Implement Withdrawal: If direct ETH receipt is somehow required, a separate accounting mechanism and withdrawal function for this ETH would need to be added, distinct from the game fee logic. This adds complexity and is likely unnecessary.

2. Handle Locked Tokens:

• Implement an Admin-Controlled Token Rescue Function: Add a new function restricted to the adminAddress (or owner) that allows the withdrawal of any ERC20 token held by the contract.

  import "@openzeppelin/contracts/token/ERC20/IERC20.sol";

  import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol"; // Recommended

  ​

  contract RockPaperScissors {

  using SafeERC20 for IERC20; // Use SafeERC20 library

  ​

  // ... existing code ...

  ​

  /**

  * @notice Allows admin to rescue mistakenly sent ERC20 tokens.

  * @param _tokenAddress The address of the ERC20 token contract.

  * @param _to The address to send the rescued tokens to.

  * @param _amount The amount of tokens to rescue.

  */

  function rescueTokens(address _tokenAddress, address _to, uint256 _amount) external {

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

  require(_to != address(0), "Rescue recipient cannot be zero address");

  ​

  IERC20 tokenInstance = IERC20(_tokenAddress);

  uint256 contractBalance = tokenInstance.balanceOf(address(this));

  require(_amount <= contractBalance, "Insufficient token balance in contract");

  ​

  // Use safeTransfer for robustness

  tokenInstance.safeTransfer(_to, _amount);

  ​

  // Optional: Emit an event

  // emit TokensRescued(msg.sender, _tokenAddress, _to, _amount);

  }

  ​

  // ... rest of contract ...

  }

  This function provides a necessary escape hatch for recovering tokens sent to the contract address by mistake, protecting users from permanent loss in such scenarios.