Puppy Raffle

AI First Flight #1

Beginner FriendlyFoundrySolidityNFT

EXP

View results

Previous Next

Submission Details

Impact: medium

Likelihood: low

Invalid

feeAddress is never checked for address(0), and withdrawFees can be called by anyone, so after an owner input error any account can permanently burn all accrued protocol fees

friedsocks233

Root + Impact

Description

The owner picks the wallet that receives the protocol's 20% cut, either in the constructor or later through changeFeeAddress. withdrawFees then sends the fees accrued in totalFees to that wallet once no players are active.
The constructor (L62) and changeFeeAddress (L168) store feeAddress without rejecting address(0), and withdrawFees (L157) has no caller restriction. withdrawFees sets totalFees to 0 (L160) and then sends the balance with a low-level call (L161). A value call to address(0) succeeds because no code lives there, so the require at L162 passes. Once feeAddress == address(0), any account can send the whole fee balance to address(0), where it is lost for good, before the owner can correct the address. As a secondary effect, the inherited OZ 3.4.0 renounceOwnership lets a bad feeAddress become permanent, because changeFeeAddress can no longer be called.

// src/PuppyRaffle.sol L60-L63 (constructor: no validation of _feeAddress)

constructor(uint256 _entranceFee, address _feeAddress, uint256 _raffleDuration) ERC721("Puppy Raffle", "PR") {

entranceFee = _entranceFee;

feeAddress = _feeAddress; // @>

raffleDuration = _raffleDuration;

// src/PuppyRaffle.sol L157-L163 (withdrawFees: callable by any account; zeroes accounting, then pushes ETH to 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 L167-L170 (changeFeeAddress: no validation of newFeeAddress)

function changeFeeAddress(address newFeeAddress) external onlyOwner {

feeAddress = newFeeAddress; // @>

emit FeeAddressChanged(newFeeAddress);

}

Risk

Likelihood:

The path starts only when the trusted owner makes an input error, passing address(0) as _feeAddress to the constructor or as newFeeAddress to changeFeeAddress. The README gives the owner this power, so it is a configuration mistake by a trusted role, not something an outsider can cause.
The burn needs the L158 check (address(this).balance == totalFees) to hold. That is true between rounds: after selectWinner has paid out and before new entrants arrive.
Once address(0) is in place, which anyone can see in the constructor arguments, the feeAddress() getter, or the FeeAddressChanged(address(0)) event, any account can trigger the burn with no privileges and no funds. The owner's chance to correct the mistake depends on getting changeFeeAddress mined before anyone else calls withdrawFees.

Impact:

All fees accrued in totalFees go to address(0) and cannot be recovered. In the PoC one 4-player round at a 1 ETH entrance fee burns 0.8 ETH. totalFees is reset to 0 at the same time, so the owner's later changeFeeAddress plus withdrawFees pays the intended wallet 0.
The loss is limited to the protocol's own fee revenue. Player entrance fees and the prize pool are not affected, which is why the impact is Medium rather than High.
Secondary: a non-payable feeAddress alone only makes withdrawFees revert, and nothing is lost because the totalFees = 0 write rolls back and one changeFeeAddress call recovers the funds. Fees become permanently locked only when the owner also calls the inherited renounceOwnership, which needs a second owner action.

Proof of Concept

test/PoC_PR-08.t.sol deploys PuppyRaffle, plays one 4-player round so the contract holds exactly the 0.8 ETH in fees, and covers each path. A baseline with a valid wallet pays that wallet. With address(0) set through the constructor or through changeFeeAddress (whose FeeAddressChanged event is checked), a non-owner's withdrawFees raises address(0)'s balance by 0.8 ETH, totalFees and the contract balance drop to 0, and the intended wallet gets 0 after the owner's correction. A non-payable feeAddress only reverts and is fully recoverable. After renounceOwnership the funds stay locked. Run it with: forge test --match-path test/PoC_PR-08.t.sol -vv

File test/PoC_PR-08.t.sol; run FOUNDRY_FFI=false forge test --match-path test/PoC_PR-08.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";

/// A fee recipient with no receive()/fallback: any plain ETH transfer to it reverts.

contract NoReceive {}

/// PR-08: feeAddress is never validated (constructor L62, changeFeeAddress L167-170) and

/// withdrawFees (L157-163) has no caller restriction. It zeroes totalFees (L160) and pushes the

/// fees with a low-level call (L161). A value CALL to address(0) succeeds (no code there), so with

/// feeAddress == address(0) ANY account can burn the accrued fees. Inherited OZ 3.4.0

/// renounceOwnership() makes a bad feeAddress permanent.

contract PoC_PR08 is Test {

event FeeAddressChanged(address newFeeAddress);

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

uint256 constant DURATION = 1 days;

uint256 constant EXPECTED_FEES = (4 * FEE * 20) / 100; // 0.8 ether from one 4-player round

address attacker = makeAddr("attacker");

address goodFeeWallet = makeAddr("goodFeeWallet");

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

function _deploy(address feeAddr) internal returns (PuppyRaffle r) {

r = new PuppyRaffle(FEE, feeAddr, DURATION); // owner = address(this)

}

/// One full 4-player round with code-less players, so the contract ends holding exactly totalFees.

function _playRound(PuppyRaffle r, uint160 seed) internal {

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

for (uint160 i = 0; i < 4; i++) {

ps[i] = address(uint160(0x10000000) + seed * 16 + i);

}

vm.deal(address(this), address(this).balance + 4 * FEE);

r.enterRaffle{value: 4 * FEE}(ps);

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

r.selectWinner();

assertEq(uint256(r.totalFees()), EXPECTED_FEES, "fees accrued");

assertEq(address(r).balance, EXPECTED_FEES, "contract holds exactly the fees");

}

// ---------------------------------------------------------------- baseline

/// Intended behaviour: a valid feeAddress receives the 20% cut when withdrawFees is called.

function test_baseline_validFeeAddressReceivesFees() public {

PuppyRaffle r = _deploy(goodFeeWallet);

_playRound(r, 1);

uint256 before = goodFeeWallet.balance;

vm.prank(attacker); // permissionless, but harmless when feeAddress is valid

r.withdrawFees();

assertEq(goodFeeWallet.balance - before, EXPECTED_FEES, "fee wallet paid");

assertEq(uint256(r.totalFees()), 0);

assertEq(address(r).balance, 0);

}

// ---------------------------------------------------------------- limb 1: address(0) burn

/// Constructor accepts address(0); a random account calls withdrawFees and the fees are burned.

function test_limb1_constructorZeroFeeAddress_anyoneBurnsFees() public {

PuppyRaffle r = _deploy(address(0)); // no revert: no validation at L62

assertEq(r.feeAddress(), address(0));

_playRound(r, 2);

uint256 zeroBefore = address(0).balance;

vm.prank(attacker); // not owner, not a player: no access control at L157

r.withdrawFees(); // call to address(0) succeeds -> require at L162 passes

console.log("fees burned to address(0):", address(0).balance - zeroBefore);

assertEq(address(0).balance - zeroBefore, EXPECTED_FEES, "fees sent to address(0)");

assertEq(uint256(r.totalFees()), 0, "accounting wiped");

assertEq(address(r).balance, 0, "contract holds nothing");

// Owner notices and fixes the address: nothing is left to recover.

r.changeFeeAddress(goodFeeWallet);

r.withdrawFees();

assertEq(goodFeeWallet.balance, 0, "fees irrecoverable");

}

/// changeFeeAddress(address(0)) is accepted and emits normally; anyone then burns the fees

/// before the owner's corrective transaction lands (simulated ordering: attacker first).

function test_limb1_changeFeeAddressToZero_burnBeatsOwnerFix() public {

PuppyRaffle r = _deploy(goodFeeWallet);

_playRound(r, 3);

vm.expectEmit(false, false, false, true, address(r));

emit FeeAddressChanged(address(0));

r.changeFeeAddress(address(0)); // owner typo: no require at L168

assertEq(r.feeAddress(), address(0));

uint256 zeroBefore = address(0).balance;

// Attacker's withdrawFees is mined before the owner's fix.

vm.prank(attacker);

r.withdrawFees();

// Owner's fix lands afterwards.

r.changeFeeAddress(goodFeeWallet);

r.withdrawFees();

console.log("fees burned before owner fix:", address(0).balance - zeroBefore);

assertEq(address(0).balance - zeroBefore, EXPECTED_FEES, "fees burned");

assertEq(goodFeeWallet.balance, 0, "intended fee wallet receives nothing");

assertEq(address(r).balance, 0);

}

// ---------------------------------------------------------------- limb 2: non-payable feeAddress

/// Non-payable feeAddress: withdrawFees reverts and the totalFees=0 write is rolled back.

/// Nothing is lost while the owner exists: one changeFeeAddress call recovers everything.

function test_limb2_nonPayableFeeAddress_revertsButRecoverable() public {

NoReceive bad = new NoReceive();

PuppyRaffle r = _deploy(address(bad));

_playRound(r, 4);

vm.prank(attacker);

vm.expectRevert(bytes("PuppyRaffle: Failed to withdraw fees"));

r.withdrawFees();

assertEq(uint256(r.totalFees()), EXPECTED_FEES, "totalFees=0 rolled back");

assertEq(address(r).balance, EXPECTED_FEES, "fees still held");

r.changeFeeAddress(goodFeeWallet);

r.withdrawFees();

assertEq(goodFeeWallet.balance, EXPECTED_FEES, "owner recovers in one call");

}

// ---------------------------------------------------------------- limb 3: renounce freezes it

/// After renounceOwnership, a non-payable feeAddress can never be corrected: every round's

/// fees accumulate in the contract with no exit.

function test_limb3_renounceOwnership_freezesBadFeeAddress() public {

NoReceive bad = new NoReceive();

PuppyRaffle r = _deploy(address(bad));

_playRound(r, 5);

r.renounceOwnership(); // inherited OZ 3.4.0, public onlyOwner

assertEq(r.owner(), address(0));

vm.expectRevert(bytes("Ownable: caller is not the owner"));

r.changeFeeAddress(goodFeeWallet);

vm.expectRevert(bytes("PuppyRaffle: Failed to withdraw fees"));

r.withdrawFees();

console.log("fees permanently stuck:", address(r).balance);

assertEq(address(r).balance, EXPECTED_FEES, "fees locked");

assertEq(uint256(r.totalFees()), EXPECTED_FEES);

}

Recommended Mitigation

--- a/src/PuppyRaffle.sol

+++ b/src/PuppyRaffle.sol

@@ -60,4 +60,5 @@

constructor(uint256 _entranceFee, address _feeAddress, uint256 _raffleDuration) ERC721("Puppy Raffle", "PR") {

+ require(_feeAddress != address(0), "PuppyRaffle: feeAddress is the zero address");

entranceFee = _entranceFee;

feeAddress = _feeAddress;

raffleDuration = _raffleDuration;

@@ -165,6 +166,12 @@

/// @notice only the owner of the contract can change the feeAddress

/// @param newFeeAddress the new address to send fees to

function changeFeeAddress(address newFeeAddress) external onlyOwner {

+ require(newFeeAddress != address(0), "PuppyRaffle: feeAddress is the zero address");

feeAddress = newFeeAddress;

emit FeeAddressChanged(newFeeAddress);

}

+ /// @notice disabled so that a misconfigured feeAddress can always be corrected

+ function renounceOwnership() public override onlyOwner {

+ revert("PuppyRaffle: renounceOwnership disabled");

+ }

Updates

Lead Judging Commences

ai-first-flight-judge Lead Judge about 1 hour ago

Submission Judgement Published

Invalidated

Reason: Incorrect statement

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!