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

withdrawFees Permanently Blocked by Direct ETH Transfer

nezar_alhammadi

Root + Impact

Description

The withdrawFees() function uses a strict equality check between the contract's ETH balance and the totalFees variable to guard against withdrawal while players are active:

function withdrawFees() external {
// @> strict equality: balance must equal EXACTLY totalFees
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");
}

This check assumes the only way ETH enters the contract is through enterRaffle(). However, there are two additional ways ETH can be forced into a contract that bypass all payable restrictions:

  1. selfdestruct: Any contract can call selfdestruct(address(puppyRaffle)) to forcefully send ETH to the raffle contract. This cannot be rejected.

  2. Pre-funding before deployment: ETH can be sent to the contract address before it is deployed using a CREATE2-predictable address.

Once any amount of extra ETH enters the contract — even 1 wei — the strict equality address(this).balance == uint256(totalFees) can never be satisfied again. This permanently bricks withdrawFees(), trapping all accumulated fees in the contract forever with no recovery mechanism.


Risk

Likelihood:

  • Any attacker with any amount of ETH (as little as 1 wei) can execute this attack using selfdestruct

  • The attack is irreversible — once executed, there is no way to restore the balance equality

  • A griefing attacker gains nothing financially but can permanently destroy the protocol's fee revenue

  • A competing protocol operator could perform this attack to harm a rival

  • The cost to the attacker is negligible (just gas + 1 wei)

Impact:

  • withdrawFees() is permanently bricked — the feeAddress can never withdraw any fees again

  • All ETH tracked by totalFees is trapped in the contract with no recovery path

  • The protocol owner loses 100% of all future and past fee revenue

  • There is no emergency function, no owner override, and no upgrade mechanism to fix this

  • If the totalFees overflow bug (separate finding) is also triggered, the combination makes the situation even worse

Proof of Concept

// Attack contract — costs only gas + 1 wei
contract Attacker {
function attack(address payable puppyRaffle) external payable {
// Send 1 wei via selfdestruct — cannot be rejected by PuppyRaffle
// After this: address(puppyRaffle).balance = totalFees + 1 wei
// withdrawFees() will now always revert forever
selfdestruct(puppyRaffle);
}
}
​
// Proof:
// Before attack: balance = 4 ETH, totalFees = 0.8 ETH → withdrawFees() works
// After attack: balance = 4 ETH + 1 wei, totalFees = 0.8 ETH → ALWAYS REVERTS

Formal Verification Evidence (Certora Prover):

The Certora Prover rule withdrawFees_always_possible was formally verified and returned VIOLATED, providing mathematical proof that withdrawFees() reverts whenever nativeBalances[contract] != totalFees:

​
Rule: withdrawFees\_always\_possible
Property: withdrawFees succeeds when totalFees > 0
Result: VIOLATED — reverts when balance != totalFees
Prover output: prover.certora.com/output/3088449/d25650413fc1466280b1d4664a294a41

Recommended Mitigation

Replace the strict equality check with a greater-than-or-equal comparison, and withdraw only the tracked totalFees amount rather than the entire balance:

function withdrawFees() external {
- require(address(this).balance == uint256(totalFees),
- "PuppyRaffle: There are currently players active!");
+ 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");
}

This change allows withdrawFees() to succeed even if extra ETH has been forced into the contract, while still correctly sending only the tracked fee amount to feeAddress. Any excess ETH from selfdestruct attacks remains in the contract but does not block fee withdrawal.

Updates

Lead Judging Commences

ai-first-flight-judge Lead Judge about 12 hours 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!