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

Fee withdrawal DoS

lordbolton

Root + Impact

Description

The PuppyRaffle::withdrawFees function uses a strict equality check (==) to verify that the contract only contains fee money before allowing withdrawal. This creates a permanent DoS vulnerability where anyone can send a tiny amount of ETH directly to the contract (via selfdestruct or as a coinbase recipient), making the equality check always fail and permanently preventing fee withdrawal.

function withdrawFees() external {
// @> Strict equality check - vulnerable to DoS!
@> 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 Problem:

The check assumes contract balance == totalFees means no active raffle. However:

  1. Anyone can send ETH directly to the contract via selfdestruct or force-sending

  2. Contract can receive block rewards as coinbase recipient

  3. This makes address(this).balance > totalFees

  4. The equality check == fails forever

  5. Fees can never be withdrawn

Risk

Likelihood: Medium - Requires attacker to deliberately send ETH to contract, but costs minimal amount (1 wei is enough).

Impact: High - Permanent loss of all accumulated protocol fees. Owner cannot recover funds.

Proof of Concept

// SPDX-License-Identifier: MIT
pragma solidity ^0.7.6;
​
contract FeeWithdrawalDoS {
/**
* @notice Attack by self-destructing and sending ETH to PuppyRaffle
* @dev Sending even 1 wei breaks the equality check forever
*/
function attack(address puppyRaffle) external payable {
// Self-destruct sends all ETH to target, bypassing receive/fallback
selfdestruct(payable(puppyRaffle));
}
}

Attack Steps:

  1. Setup:

    • PuppyRaffle deployed

    • Several raffles completed

    • totalFees = 10 ETH

    • contract.balance = 10 ETH

    • Fee withdrawal works: 10 == 10 ✓

  2. Attack:

    • Attacker deploys FeeWithdrawalDoS with 1 wei

    • Calls attack(puppyRaffleAddress)

    • Contract self-destructs, forcing 1 wei to PuppyRaffle

    • Now: contract.balance = 10 ETH + 1 wei

    • But: totalFees = 10 ETH

  3. Result:

    • Owner calls withdrawFees()

    • Check: 10 ETH + 1 wei == 10 ETH ❌ FALSE

    • Transaction reverts with "There are currently players active!"

    • Fees permanently locked - no way to withdraw

Cost of Attack: 1 wei + gas fees (~30k gas) = negligible

Tools Used

Manual review

Recommended Mitigation

Use >= instead of == to allow withdrawal when contract has at least the fee amount:

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

Why This Works:

  • If balance >= totalFees, there's no active raffle (or extra ETH was sent)

  • Fee withdrawal succeeds regardless of extra ETH sent to contract

  • Any excess ETH beyond totalFees remains in contract (minor, but acceptable)

  • Cannot be DoS'd by force-sending ETH

Alternative: Track player deposits separately:

+ uint256 public totalDeposits;
​
function enterRaffle(address[] memory newPlayers) public payable {
require(msg.value == entranceFee * newPlayers.length, "PuppyRaffle: Must send enough to enter raffle");
+ totalDeposits += msg.value;
// ... rest of function
}
function withdrawFees() external {
- require(address(this).balance == uint256(totalFees), "PuppyRaffle: There are currently players active!");
+ require(totalDeposits == 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");
}

Best Practice: Use >= comparison as it's the simplest fix that prevents the DoS attack.

Updates

Lead Judging Commences

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