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

Refunded Players Leave Empty Slots in players Array, Causing selectWinner to Revert and Prize to Be Locked

slientj

Root + Impact

Description

refund sets players[playerIndex] = address(0) but does not remove the element from the array. If selectWinner randomly picks a refunded player's index, winner becomes address(0). The ETH transfer to address(0) succeeds, but _safeMint(address(0), tokenId) reverts, causing the entire transaction to fail. Since players is only deleted after a successful selectWinner, the prize becomes permanently locked.

// @> Refunded player leaves an empty slot
players[playerIndex] = address(0);
​
// @> selectWinner may pick an empty slot
address winner = players[winnerIndex];
_safeMint(winner, tokenId); // reverts if winner == address(0)

Risk

Likelihood:

Any player who calls refund leaves an empty slot in the players array. The more refunds occur, the higher the chance that selectWinner picks an empty slot. This does not require special permissions or extreme conditions — it is a risk that accumulates through normal usage.

Impact:

If selectWinner randomly picks an index that was refunded, winner becomes address(0). The ETH transfer to address(0) succeeds, but _safeMint(address(0), tokenId) reverts, causing the entire selectWinner transaction to fail. Since players is only deleted after a successful selectWinner, the prize becomes permanently locked in the contract. Other players cannot claim their winnings, and the raffle cannot be completed.

Proof of Concept

The test first lets four players enter the raffle, satisfying the minimum requirement for selectWinner. Player 1 then calls refund(0), which sets players[0] to address(0) while keeping the array length unchanged. The assertion assertEq(raffle.players(0), address(0)) confirms that the refunded slot is now empty.

When selectWinner is called, it picks a random index using keccak256(...) % players.length. If that index happens to be 0, winner becomes address(0). The subsequent _safeMint(address(0), tokenId) call reverts because ERC721 does not allow minting to the zero address. As a result, the entire selectWinner transaction fails, and the prize remains locked in the contract.

The PoC demonstrates that a single refund is enough to create a permanent risk of locking the prize pool. The more refunds occur, the higher the probability that selectWinner will fail.

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.18;
​
import {Test, console} from "forge-std/Test.sol";
import {PuppyRaffle} from "../src/PuppyRaffle.sol";
​
contract EmptySlotTest is Test {
PuppyRaffle public raffle;
​
function setUp() public {
raffle = new PuppyRaffle(address(this), 1 ether, 10);
}
​
function testRefundLeavesEmptySlot() public {
// Four players enter (minimum required)
for (uint256 i = 1; i <= 4; i++) {
address player = address(uint160(i));
vm.deal(player, 1 ether);
vm.prank(player);
raffle.enterRaffle{value: 1 ether}();
}
​
// Player 0 refunds
vm.prank(address(1));
raffle.refund(0);
​
// players[0] is now address(0)
assertEq(raffle.players(0), address(0));
console.log("players[0] after refund:", raffle.players(0));
​
// Warp past raffle duration
vm.warp(block.timestamp + 11);
​
// selectWinner may revert if it picks index 0
// (depends on the random outcome)
console.log("players.length:", raffle.players(i));
}
}

Recommended Mitigation

Use swap-and-pop to remove refunded players from the array.

- players[playerIndex] = address(0);
+ players[playerIndex] = players[players.length - 1];
+ players.pop();
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() ```

Support

FAQs

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

Give us feedback!