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

selectWinner() counts refunded zero-address slots in totalAmountCollected, inflating the prize pool beyond the contract's actual balance

virgilbb

Root + Impact

Description

  • PuppyRaffle tracks players in a dynamic array and lets them exit via refund(), which zeroes their slot (players[playerIndex] = address(0)) but does not remove the entry from the array. The contract balance decreases by entranceFee for every refund issued.

  • selectWinner() computes totalAmountCollected = players.length * entranceFee, using the raw array length rather than the count of non-zero (active) entries. When one or more players have refunded, totalAmountCollected exceeds the ETH actually held by the contract, causing the winner transfer to attempt to send more ETH than exists and always revert — permanently blocking raffle completion.

function selectWinner() external {
// ...
uint256 totalAmountCollected = players.length * entranceFee; // @> counts zeroed (refunded) slots
uint256 prizePool = (totalAmountCollected * 80) / 100; // @> inflated beyond real balance
uint256 fee = (totalAmountCollected * 20) / 100;
totalFees = totalFees + uint64(fee); // @> fee also over-counted
​
// ...
(bool success,) = winner.call{value: prizePool}(""); // @> reverts if balance < prizePool
require(success, "PuppyRaffle: Failed to send prize pool to winner");
}

Risk

Likelihood:

  • Any player who calls refund() before the raffle draw sets up the condition; a single refund among four players is enough to make prizePool exceed the contract balance and cause a permanent revert on selectWinner().

Impact:

  • The raffle draw is bricked for all remaining participants — their entrance fees are locked in the contract with no path to withdrawal (no admin rescue function exists), and the raffle can never conclude.

Proof of Concept

Three players enter the raffle, one refunds, and then selectWinner() is called after the duration elapses. The winner call reverts because prizePool (80% of 4 × entranceFee) exceeds the actual contract balance (3 × entranceFee).

function testInflatedPrizePoolReverts() public {
// Four players enter
address alice = address(0x1);
address bob = address(0x2);
address carol = address(0x3);
address dave = address(0x4);
​
address[] memory entrants = new address[](4);
entrants[0] = alice;
entrants[1] = bob;
entrants[2] = carol;
entrants[3] = dave;
​
uint256 fee = raffle.entranceFee();
raffle.enterRaffle{value: fee * 4}(entrants);
​
// Alice refunds — balance drops to 3 * fee, but players.length stays 4
uint256 aliceIdx = raffle.getActivePlayerIndex(alice);
vm.prank(alice);
raffle.refund(aliceIdx);
​
assertEq(address(raffle).balance, fee * 3);
​
// Advance time past raffle duration
vm.warp(block.timestamp + raffle.raffleDuration() + 1);
​
// selectWinner() calculates prizePool = (4 * fee * 80) / 100 = 3.2 * fee
// but contract only holds 3 * fee — the ETH send reverts
vm.expectRevert("PuppyRaffle: Failed to send prize pool to winner");
raffle.selectWinner();
}

The test confirms that a single refund before draw time permanently prevents selectWinner() from completing, trapping the remaining players' funds.

Recommended Mitigation

Track the count of active (non-refunded) entries separately and base prize pool calculations on that value rather than players.length.

+ uint256 activePlayers;
+ for (uint256 i = 0; i < players.length; i++) {
+ if (players[i] != address(0)) activePlayers++;
+ }
- uint256 totalAmountCollected = players.length * entranceFee;
+ uint256 totalAmountCollected = activePlayers * entranceFee;

Alternatively, maintain a separate activePlayerCount state variable that is incremented in enterRaffle and decremented in refund, eliminating the loop overhead.

Updates

Lead Judging Commences

ai-first-flight-judge Lead Judge about 5 hours 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() ```

Support

FAQs

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

Give us feedback!