Rock Paper Scissors

Summary

A Denial of Service (DoS) vulnerability exists in the withdrawFees function of the RockPaperScissors contract. A malicious or careless admin can set the adminAddress (via setAdmin) to a contract designed to revert upon receiving Ether (e.g., through its receive/fallback function). When withdrawFees is subsequently called, the low-level .call{value: ...} to the reverting admin address fails. This failure causes the require(success, "Fee withdrawal failed") check within withdrawFees to trigger, reverting the entire withdrawal transaction. Consequently, the accumulated protocol fees become inaccessible and cannot be withdrawn by the currently configured admin address, effectively locking them from that specific admin. While the owner might be able to recover by setting a new admin, this presents a griefing vector for the admin role.

Vulnerability Details

The vulnerability exists within the withdrawFees() function located in src/RockPaperScissors.sol. This function is responsible for transferring accumulated protocol fees to the address stored in the adminAddress state variable.

The function utilizes a low-level external call for the ETH transfer and checks for its success:

The adminAddress can be updated via the setAdmin() function, typically callable by the current admin or potentially a separate owner role.

The vulnerability arises because an admin can intentionally or accidentally set the adminAddress to point to a contract whose receive() or fallback() function prevents the successful completion of the .call. This occurs if the receiving contract:

1. Explicitly reverts (e.g., receive() external payable { revert("Blocked"); }).

2. Consumes all forwarded gas (less likely with default .call behavior but possible).

If the external .call to adminAddress returns success = false due to such a revert or gas issue in the receiver, the require(success, "Fee withdrawal failed") check within withdrawFees() will trigger. This causes the entire withdrawFees transaction to revert.

As a result, if the adminAddress is set to such a non-cooperative contract, the withdrawFees function becomes unusable for withdrawing fees to that specific address. Any attempt to call withdrawFees will fail, preventing the extraction of accumulated fees via this mechanism as long as that address remains the admin.

PoC:

PoC Result:

Impact

The direct impact of this vulnerability is a Denial of Service (DoS) on the fee withdrawal mechanism. If the adminAddress is set to a contract that reverts upon receiving ETH, the withdrawFees function will consistently fail, preventing the withdrawal of any accumulated protocol fees by that admin.

• Temporarily Locked Fees: The fees remain locked in the contract and inaccessible via the withdrawFees function as long as the adminAddress points to the non-cooperative contract.

• Admin Griefing: A malicious admin could exploit this to deliberately prevent fee withdrawals. Alternatively, a careless admin could accidentally achieve the same result by setting an incompatible contract address.

• Owner Recovery (Mitigation): The overall impact is significantly mitigated if the contract owner (the address that initially deployed the RockPaperScissors contract, assuming ownership wasn't transferred and the owner role retains setAdmin capability) can call setAdmin. The owner could appoint a new, valid adminAddress, thereby restoring the functionality of withdrawFees and allowing the accumulated fees to eventually be recovered. If owner recovery is not possible, the fees would be permanently locked from withdrawal via this function.

• No Direct User Impact: This vulnerability primarily affects the protocol's ability to collect its fees via the designated admin and does not directly endanger player funds or game integrity.

Tools Used

Manuel code rview

Forge Foundry

Recommendations

The primary issue causing the Denial of Service is that a failure in the external ETH transfer to the adminAddress causes the entire withdrawFees function to revert due to the require(success, ...) check. To mitigate this specific DoS vector:

1. Consider Removing the require(success) Check in withdrawFees:

   • Modify the withdrawFees function by removing the line:

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

     - require(success, "Fee withdrawal failed");

     + // Optional: Emit an event if success is false

     + // if (!success) { emit FeeWithdrawalFailed(adminAddress, amountToWithdraw); }

     ​

• Effect: If the .call to adminAddress fails (returns success = false), the withdrawFees function will no longer revert. The state change (accumulatedFees -= amountToWithdraw) will still persist, but the ETH transfer for that specific attempt will simply fail silently (or with an event if added). The fees remain in the contract, available for withdrawal in a subsequent transaction (e.g., after the owner assigns a valid adminAddress).

• Rationale: This approach places the responsibility for providing a valid, payable address on the admin. If they provide an address that cannot receive ETH, the transfer fails, but it doesn't block the function execution or state updates. This prevents the DoS on the function call itself.

  2.Ensure Robust Owner Recovery for setAdmin:

• Verify that the setAdmin function includes necessary access control allowing a designated owner role (potentially separate from the operational adminAddress) to forcibly change the adminAddress if the current one becomes unusable or is set maliciously. This provides a crucial recovery path to restore fee withdrawal functionality.