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

Ghost slots in PuppyRaffle::selectWinner() inflate prize pool beyond actual balance, permanently freezing the raff

georgibuilds

Root + Impact

Description

  • PuppyRaffle::refund() zeroes a player's slot in the players
    array but never decreases players.length. PuppyRaffle::selectWinner()
    calculates totalAmountCollected using players.length * entranceFee,
    counting zeroed slots as active players. After any refund, the
    computed prizePool exceeds the actual contract balance, causing
    winner.call{value: prizePool} to revert and permanently freezing
    the raffle with all remaining ETH locked inside.

function refund(uint256 playerIndex) public {
...
// @> Zeroes slot but players.length stays the same
players[playerIndex] = address(0);
}
​
function selectWinner() external {
...
// @> Counts ghost slots as active players
uint256 totalAmountCollected = players.length * entranceFee;
uint256 prizePool = (totalAmountCollected * 80) / 100;
...
// @> Reverts when prizePool > actual balance
(bool success,) = winner.call{value: prizePool}("");
require(success, "PuppyRaffle: Failed to send prize pool to winner");
}

Risk

Likelihood:

  • Triggered any time a player refunds before selectWinner()

  • Normal protocol usage — no attacker required

  • Probability increases with more refunds per round

Impact:

  • selectWinner() permanently reverts

  • Raffle is frozen — no winner, no NFT, no reset

  • All remaining ETH locked inside contract forever

  • Legitimate players cannot recover funds

Proof of Concept

Attack Path:

  1. 4 players enter — contract holds 4 ETH

  2. 2 players refund — contract holds 2 ETH
    players.length still = 4 (ghost slots)

  3. selectWinner() calculates:
    totalAmountCollected = 4 * 1 ETH = 4 ETH (wrong)
    prizePool = 4 ETH * 80% = 3.2 ETH

  4. Contract only holds 2 ETH

  5. winner.call{value: 3.2 ETH} → OutOfFunds

  6. Transaction reverts — raffle permanently frozen

  7. 2 ETH locked inside forever

function test_ghost_slots() public {
address[] memory players = new address[](4);
players[0] = makeAddr("Alice");
players[1] = makeAddr("Bob");
players[2] = makeAddr("Carol");
players[3] = makeAddr("Dave");
​
vm.deal(players[0], entranceFee * 4);
vm.prank(players[0]);
puppyRaffle.enterRaffle{value: entranceFee * 4}(players);
​
vm.prank(players[0]);
puppyRaffle.refund(0);
vm.prank(players[1]);
puppyRaffle.refund(1);
​
vm.warp(block.timestamp + raffleDuration + 1);
​
console.log("Contract balance:", address(puppyRaffle).balance);
console.log("Expected prize pool:", 4 * entranceFee * 80 / 100);
​
vm.expectRevert();
puppyRaffle.selectWinner();
}

Recommended Mitigation

Track active player count separately using a dedicated
variable that decrements on refund. Use this variable
instead of players.length for prize pool calculation,
ensuring the computed prize never exceeds actual balance.

+ uint256 public activePlayerCount;
​
function enterRaffle(address[] memory newPlayers) public payable {
+ activePlayerCount += newPlayers.length;
}
​
function refund(uint256 playerIndex) public {
+ activePlayerCount -= 1;
players[playerIndex] = address(0);
}
​
function selectWinner() external {
- uint256 totalAmountCollected = players.length * entranceFee;
+ uint256 totalAmountCollected = activePlayerCount * entranceFee;
}
Updates

Lead Judging Commences

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