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

selectWinner() computes the pot from players.length, which still counts refunded (zeroed) slots, causing prize/fee over-accounting and allowing the prize to be sent to address(0)

0xj3d1

Root + Impact

Description

  • When a player refunds, refund() sets their slot to address(0) but does not shrink the array — players.length is unchanged and the refunded ETH has already left the contract.

  • selectWinner() derives the payout from players.length * entranceFee and selects winner = players[winnerIndex]. After refunds, players.length overstates the funds actually held, so prizePool + fee is computed against money that is no longer there; and winnerIndex can land on a zeroed slot, sending the prize to address(0).

function refund(uint256 playerIndex) public {
...
@> players[playerIndex] = address(0); // length unchanged, ETH already gone
}
...
function selectWinner() external {
...
@> uint256 winnerIndex = uint256(keccak256(...)) % players.length; // may index a zeroed slot
address winner = players[winnerIndex];
@> uint256 totalAmountCollected = players.length * entranceFee; // counts refunded slots
uint256 prizePool = (totalAmountCollected * 80) / 100;
...
@> (bool success,) = winner.call{value: prizePool}(""); // winner may be address(0)
}

Risk

Likelihood:

  • Occurs in any round where at least one player refunds before selectWinner() is called — a normal, intended user action.

  • Occurs as a payout-to-zero whenever the pseudo-random index lands on a refunded slot.

Impact:

  • The computed prize and fee exceed the ETH actually collected, so the winner transfer can pull from accumulated fees / other rounds' balance, or revert and stall the draw.

  • If a zeroed slot is selected, the prize ETH is sent to address(0) and is irrecoverable, and the NFT mint to address(0) reverts, stalling the raffle.

Proof of Concept

The following test enters four players, refunds one, and shows the contract balance drops to 3 ETH while selectWinner() still computes the pot from the unchanged players.length of 4 — and players[0] is now address(0), so it can be selected as winner.

function test_refund_desyncs_pot_accounting() public {
address[] memory players = new address[](4);
players[0] = playerOne;
players[1] = playerTwo;
players[2] = playerThree;
players[3] = playerFour;
puppyRaffle.enterRaffle{value: entranceFee * 4}(players);
​
// playerOne refunds: balance drops to 3 ETH, but players.length stays 4
uint256 idx = puppyRaffle.getActivePlayerIndex(playerOne);
vm.prank(playerOne);
puppyRaffle.refund(idx);
​
assertEq(address(puppyRaffle).balance, entranceFee * 3);
// selectWinner still computes pot as 4 * entranceFee and players[0] == address(0)
​
vm.warp(block.timestamp + duration + 1);
vm.roll(block.number + 1);
// depending on winnerIndex this either over-pays from fees or targets address(0)
puppyRaffle.selectWinner();
}

Recommended Mitigation

Account for active players and the real balance rather than the raw array length, and skip zeroed slots when selecting a winner. A cleaner refactor removes players from the array on refund (swap-and-pop) so length stays accurate:

- uint256 totalAmountCollected = players.length * entranceFee;
+ uint256 totalAmountCollected = address(this).balance; // or a tracked activePlayerCount * entranceFee
Updates

Lead Judging Commences

ai-first-flight-judge Lead Judge about 22 hours ago
Submission Judgement Published
Validated
Assigned finding tags:

[H-01] Potential Loss of Funds During Prize Pool Distribution

## Description In the `selectWinner` function, when a player has refunded and their address is replaced with address(0), the prize money may be sent to address(0), resulting in fund loss. ## Vulnerability Details In the `refund` function if a user wants to refund his money then he will be given his money back and his address in the array will be replaced with `address(0)`. So lets say `Alice` entered in the raffle and later decided to refund her money then her address in the `player` array will be replaced with `address(0)`. And lets consider that her index in the array is `7th` so currently there is `address(0)` at `7th index`, so when `selectWinner` function will be called there isn't any kind of check that this 7th index can't be the winner so if this `7th` index will be declared as winner then all the prize will be sent to him which will actually lost as it will be sent to `address(0)` ## Impact Loss of funds if they are sent to address(0), posing a financial risk. ## Recommendations Implement additional checks in the `selectWinner` function to ensure that prize money is not sent to `address(0)`

Support

FAQs

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

Give us feedback!