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

`refund()` sets players to `address(0)` without shrinking array → `prizePool` over-calculates and `selectWinner()` fails to distribute funds

aiada

Description

  • Normal: After a player refunds, the players[] array should shrink so that selectWinner() calculates the prize pool based on actual remaining players.

  • Bug: refund() sets players[playerIndex] = address(0) — the array length stays the same. When selectWinner() runs, prizePool = (players.length * entranceFee * 80) / 100 still counts refunded players. The contract no longer holds enough ETH to pay the calculated prize pool, so winner.call{value: prizePool} reverts. Prize distribution fails, and funds are locked.

// src/PuppyRaffle.sol:103 — hole left in array
players[playerIndex] = address(0); //@> Array length unchanged
​
// src/PuppyRaffle.sol:131-152 — prize pool over-calculates
uint256 totalAmountCollected = players.length * entranceFee; //@> Counts address(0) slots!
uint256 prizePool = (totalAmountCollected * 80) / 100; //@> Overestimates — refunded ETH is gone
// ...
(bool success,) = winner.call{value: prizePool}(""); //@> REVERTS — insufficient balance
require(success, "PuppyRaffle: Failed to send prize pool to winner");

Risk

Likelihood:

  • Any refund triggers the condition — players.length stays the same while contract balance decreases

  • No attacker needed — normal protocol usage (players refunding before raffle ends) triggers it

  • With more refunds, the gap between calculated prizePool and actual balance widens

Impact:

  • selectWinner() reverts when the contract can't pay the calculated prize pool — prize distribution fails

  • All remaining funds are locked — no winner, no fee withdrawal

  • Core protocol functionality (selecting a winner) is permanently broken once refunds occur

Proof of Concept

4 players enter (4 ETH total). 1 player refunds. Array has 1 address(0) hole + 3 active players = length 4.

  • totalAmountCollected = 4 * 1 ETH = 4 ETH

  • prizePool = (4 * 80) / 100 = 3.2 ETH

  • Contract actual balance = 3 ETH (1 ETH refunded)

  • winner.call{value: 3.2 ETH}() → reverts (insufficient balance)

function testPrizePoolOverCalculatesAfterRefund() public playersEntered {
vm.warp(block.timestamp + duration + 1);
vm.roll(block.number + 1);
​
vm.prank(playerFour);
puppyRaffle.refund(3); // 1 refund → 3 ETH left, but length still 4
​
vm.expectRevert(); // prizePool = 3.2 > 3 ETH balance
puppyRaffle.selectWinner();
}

Run with:

forge test --match-test testPrizePoolOverCalculatesAfterRefund -vvvv

Recommended Mitigation

Use swap-and-pop to remove refunded players, keeping players.length accurate:

function refund(uint256 playerIndex) public {
address playerAddress = players[playerIndex];
require(playerAddress == msg.sender);
require(playerAddress != address(0));
​
+ // Swap-and-pop: remove the player, keeping array length accurate
+ players[playerIndex] = players[players.length - 1];
+ players.pop();
payable(msg.sender).sendValue(entranceFee);
- players[playerIndex] = address(0);
emit RaffleRefunded(playerAddress);
}
Updates

Lead Judging Commences

ai-first-flight-judge Lead Judge 20 days 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!