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

Strict balance equality in `withdrawFees()` lets anyone permanently lock protocol fees with force-fed ETH

mohdhuzaifah0786

Root + Impact

Description

  • Normal behavior: Once no players are active, withdrawFees() should transfer the accumulated totalFees to the feeAddress.

  • Specific issue: The withdrawal is gated by require(address(this).balance == uint256(totalFees), ...) — an exact equality. ETH can be forced into any contract without calling its functions (via selfdestruct, or as a coinbase/pre-deploy recipient), and such ETH is not tracked by totalFees. A single wei force-fed into the contract makes balance permanently unequal to totalFees, so the check can never pass again and the fees are locked forever.

solidity
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");
}

Risk

Likelihood:

  • Reason 1: Occurs whenever anyone deploys a tiny contract funded with ≥1 wei and selfdestructs it with PuppyRaffle as the target, which requires no permissions.

  • Reason 2: Occurs as a side effect any time the contract receives ETH outside enterRaffle (e.g. block reward to a pre-computed address), permanently breaking the equality.

Impact:

  • Impact 1: withdrawFees() reverts permanently and all accrued protocol fees become unrecoverable.

  • Impact 2: A griefer can brick fee collection for a negligible cost (1 wei plus gas), with no way for the owner to recover.

Proof of Concept

This PoC runs one full round so totalFees equals the contract balance (the normal, withdrawable state). It then force-feeds 1 wei via a self-destructing helper, breaking the equality, and shows withdrawFees() now reverts — the fees are stuck.
​
Add to test/PuppyRaffleTest.t.sol and run forge test --mt test_lock_fees -vvv.
​
solidity
function test_lock_fees() public {
address[] memory players = new address[](4);
for (uint160 i = 0; i < 4; i++) players[i] = address(i + 1);
puppyRaffle.enterRaffle{value: entranceFee * 4}(players);
vm.warp(block.timestamp + duration + 1);
vm.roll(block.number + 1);
puppyRaffle.selectWinner(); // now balance == totalFees (withdrawable)
​
// Force-feed 1 wei so balance != totalFees, forever
ForceFeeder feeder = new ForceFeeder{value: 1 wei}();
feeder.attack(payable(address(puppyRaffle)));
​
vm.expectRevert("PuppyRaffle: There are currently players active!");
puppyRaffle.withdrawFees();
}
​
contract ForceFeeder {
constructor() payable {}
function attack(address payable target) external { selfdestruct(target); }
}
​
The withdrawFees() call reverts on the equality check, confirming the fees are permanently locked by 1 wei of unsolicited ETH.

Recommended Mitigation

Do not tie a withdrawal to the raw contract balance. Track owed fees in totalFees and use a >= comparison (or drop the balance check entirely and simply send totalFees), so force-fed ETH cannot block the withdrawal. This still prevents withdrawing while a raffle is mid-round, because totalFees is only credited when a round is settled.
​
function withdrawFees() external {
- require(address(this).balance == uint256(totalFees), "PuppyRaffle: There are currently players active!");
+ require(players.length == 0, "PuppyRaffle: There are currently players active!");
uint256 feesToWithdraw = totalFees;
totalFees = 0;
(bool success,) = feeAddress.call{value: feesToWithdraw}("");
require(success, "PuppyRaffle: Failed to withdraw fees");
}
Updates

Lead Judging Commences

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