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

withdrawFees requires balance == totalFees exactly — anyone can selfdestruct ETH in to break the equality and permanently lock all fees

hopeh

Description

withdrawFees releases fees only when the contract's entire ETH balance exactly equals the tracked totalFees:

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

The strict equality address(this).balance == uint256(totalFees) assumes the only ETH in the contract is the accounted fees. But a contract's balance can be increased without any of its functions being called, via selfdestruct(address(puppyRaffle)) from another contract (or by being the recipient of a coinbase/pre-computed address transfer). Such forced ETH is not reflected in totalFees.

Once even 1 wei of unaccounted ETH is forced in, address(this).balance > totalFees permanently, and the equality can never be satisfied again. withdrawFees reverts on every call, and the legitimately accrued fees are locked in the contract forever — there is no alternative withdrawal path and no owner override.

This is a griefing/denial-of-service on the fee-withdrawal mechanism: an attacker spends a tiny amount of ETH (destroyed via selfdestruct) to permanently deny the protocol its fees.

Risk

Impact: Medium. All accrued protocol fees become permanently unwithdrawable. No attacker profit, but a total, irreversible loss of the fee revenue to the protocol, triggerable by anyone.

Likelihood: Medium. Trivial to execute (deploy a tiny contract holding dust and selfdestruct it to the raffle), but it destroys the griefer's own small amount of ETH and yields them no gain, so it is a spite/denial vector rather than a profit motive.

Proof of Concept

contract ForceFeeder {
constructor(address payable target) payable {
selfdestruct(target); // pushes ETH into `target` with no function call
}
}
​
function test_forcedEthBricksWithdrawFees() public {
// ... run a raffle so totalFees > 0 and balance == totalFees (normally withdrawable) ...
assertEq(address(puppyRaffle).balance, uint256(puppyRaffle.totalFees()));
​
// Force 1 wei in via selfdestruct
new ForceFeeder{value: 1}(payable(address(puppyRaffle)));
​
// Now balance != totalFees forever
assertGt(address(puppyRaffle).balance, uint256(puppyRaffle.totalFees()));
vm.expectRevert("PuppyRaffle: There are currently players active!");
puppyRaffle.withdrawFees(); // permanently bricked
}

Expected: fees can always be withdrawn once accrued. Actual: any forced ETH permanently breaks the equality check and locks the fees.

Recommended Mitigation

Do not tie withdrawal to the contract's total balance. Withdraw the tracked totalFees amount directly, independent of any extra ETH:

function withdrawFees() external {
uint256 feesToWithdraw = totalFees;
totalFees = 0;
(bool success,) = feeAddress.call{value: feesToWithdraw}("");
require(success, "PuppyRaffle: Failed to withdraw fees");
}

This releases exactly the accounted fees and is immune to forced-ETH balance manipulation. (Restricting withdrawFees to onlyOwner/feeAddress is also advisable, though not required to fix this issue.)

Updates

Lead Judging Commences

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