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

[M-02] Refunded `address(0)` slots can be selected as winner, reverting `selectWinner()` for some callers

webrainsec

Description

When a player calls refund(), their slot in the players array is set to address(0). selectWinner() can pick this zeroed slot as the winner. The subsequent _safeMint(winner, tokenId) call reverts because ERC721 forbids minting to address(0). The entire transaction reverts, meaning the raffle cannot be resolved for that particular msg.sender/block combination.

Vulnerability Details

// src/PuppyRaffle.sol, line 103
players[playerIndex] = address(0); // refund zeroes the slot
​
// src/PuppyRaffle.sol, lines 128-130
uint256 winnerIndex =
uint256(keccak256(abi.encodePacked(msg.sender, block.timestamp, block.difficulty))) % players.length;
address winner = players[winnerIndex]; // can be address(0)
​
// src/PuppyRaffle.sol, line 153
_safeMint(winner, tokenId); // reverts for address(0)

Since the winner index depends on msg.sender, different callers produce different indices. Some callers will hit non-zero slots and succeed. But depending on how many refunds occurred, a large fraction of the array may be zeroed, making most callers unable to resolve the raffle.

Additionally, the prize calculation at line 131 uses players.length * entranceFee, which includes refunded slots. This means the math assumes ETH that was already returned, so the .call{value: prizePool} would also fail if the contract balance doesn't cover the inflated prize pool.

Risk

Likelihood:

  • Triggered whenever at least one player refunds before selectWinner() is called. Refunds are a core feature of the protocol (the README explicitly supports them), so this scenario is common.

Impact:

  • The raffle may be unresolvable for many callers. In the worst case (many refunds), nearly every possible winnerIndex maps to address(0), effectively deadlocking the raffle. The remaining players' entrance fees are stuck until a caller happens to produce a valid index.

PoC

No separate PoC. The vulnerability is visible from reading the code path: refund() sets address(0) → selectWinner() picks any index → _safeMint(address(0)) reverts per ERC721 spec.

Recommendations

Skip address(0) entries when selecting the winner:

uint256 winnerIndex =
uint256(keccak256(abi.encodePacked(msg.sender, block.timestamp, block.difficulty))) % players.length;
address winner = players[winnerIndex];
+ // Skip refunded (zeroed) slots
+ uint256 attempts = 0;
+ while (winner == address(0) && attempts < players.length) {
+ winnerIndex = (winnerIndex + 1) % players.length;
+ winner = players[winnerIndex];
+ attempts++;
+ }
+ require(winner != address(0), "PuppyRaffle: No active players");

Also fix the prize pool calculation to subtract refunded players, or track active player count separately.

Updates

Lead Judging Commences

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