Rock Paper Scissors
First Flight #38
Beginner FriendlySolidity
100 EXP
View results
Previous Next
Submission Details
Severity: medium
Valid

[M-1] `RockPaperScissors::withdrawFees` require statement may leave ETH permanently locked in contract.

zacwilliamson

Description: The withdrawFees() function allows the admin to withdraw protocol fees up to the value of accumulatedFees. However, it does not account for any extra ETH that may have been sent to the contract accidentally or otherwise (e.g. through direct transfers or self-destructs).

function withdrawFees(uint256 _amount) external {
require(msg.sender == adminAddress, "Only admin can withdraw fees");
​
uint256 amountToWithdraw = _amount == 0 ? accumulatedFees : _amount;
@> require(amountToWithdraw <= accumulatedFees, "Insufficient fee balance");
​
accumulatedFees -= amountToWithdraw;
​
(bool success,) = adminAddress.call{value: amountToWithdraw}("");
require(success, "Fee withdrawal failed");
​
emit FeeWithdrawn(adminAddress, amountToWithdraw);
}

Impact: Any ETH not reflected in accumulatedFees becomes inaccessible and permanently locked.

Proof of Concept: Run the following test in RockPaperScissorsTest.t.sol...

  1. Send ETH directly to the contract (simulating accidental transfer)

  2. Assert that ETH was received by contract

  3. Admin tries to withdraw ETH, expecting failure

Test
function testCannotWithdrawExtraEthSentToContract() public {
// Step 1: Send ETH directly to the contract (simulating accidental transfer)
(bool success,) = address(game).call{value: 1 ether}(""); // send 1 ETH with empty call data
require(success, "ETH transfer failed");
​
// Step 2: Assert that ETH was received by contract
assertEq(address(game).balance, 1 ether);
assertEq(game.accumulatedFees(), 0); // no fees officially recorded
​
// Step 3: Admin tries to withdraw ETH, expecting failure
vm.expectRevert("Insufficient fee balance");
game.withdrawFees(1 ether);
}

Recommended Mitigation:

  1. Consider adding a parameter where admin can withdraw all fees through address(this).balance instead of relying on accumulatedFees.

Updates

Appeal created

m3dython Lead Judge 4 months ago
Submission Judgement Published
Validated
Assigned finding tags:

Orphaned ETH due to Unrestricted receive() or Canceled Game

ETH sent directly to the contract via the receive function or after a canceled game becomes permanently locked

m3dython Lead Judge 4 months ago
Submission Judgement Published
Validated
Assigned finding tags:

Orphaned ETH due to Unrestricted receive() or Canceled Game

ETH sent directly to the contract via the receive function or after a canceled game becomes permanently locked

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.