Root + Impact

Description

PuppyRaffle.selectWinner() in src/PuppyRaffle.sol chooses a winner from the whole players array, including entries that have already been refunded and are now address(0). If one of those empty entries is chosen, the function cannot mint the puppy to it, so the whole winner-selection transaction reverts and no winner is chosen.

The relevant code is:

refund() leaves a blank entry behind instead of removing the player:

Nothing in selectWinner() checks whether winner is address(0), and nothing skips empty entries when the winner index is calculated. The index is simply keccak256(...) % players.length, so every slot in the array, including the blank ones, has an equal chance of being picked. OpenZeppelin's ERC721 _mint, which _safeMint calls, reverts when the recipient is the zero address, so minting to a refunded slot is impossible by design. The prize call to address(0) would succeed on its own, but the mint that follows it reverts, and because a revert undoes the whole transaction, the prize payment and the clearing of the players array are undone as well.

The same problem can arise without any refund at all: enterRaffle() does not reject address(0) as a player, so a zero address can be entered directly and later be selected in the same way. This finding is distinct from the prize-accounting issue in a separate submission, which is about the prize being larger than the balance; this finding is about who is selected, even when the prize amount itself would be affordable.

Risk

Likelihood: Medium - the failure needs a refunded slot and an unlucky random draw, but both are ordinary. Refunds are a normal feature, and the chance of picking a blank slot grows with every refund: with one refund in four players it is 1 in 4 on each attempt, and with half the players refunded it is 1 in 2.
Impact: Medium - each failed attempt wastes gas and delays the raffle, and repeated failures can keep the raffle from ever finishing while refunds accumulate. Players wait for a winner that is repeatedly blocked, and the prize and the puppy stay undistributed. The funds are not directly stolen here, but the raffle's core function, choosing a winner, is denied, and a griefer can increase the failure rate simply by entering and refunding many times.

Proof of Concept

Manual trace - no tests were run; this is a step-by-step reading of the code, with a worked example.

Worked example: suppose four players enter, one of them refunds, and the raffle duration then passes. The array is now [playerA, playerB, address(0), playerD] and players.length is 4.

1. Someone calls selectWinner(). The contract computes winnerIndex from the hash, modulo 4, so the result is 0, 1, 2, or 3 with equal probability.

2. If the result is 2, winner is address(0), the refunded slot.

3. The contract deletes the players array, then calls address(0) with the prize value. That call succeeds, because sending ether to the zero address does not revert on its own.

4. The contract then calls _safeMint(address(0), tokenId). OpenZeppelin's ERC721 _mint requires the recipient to be a non-zero address, so this call reverts.

5. The revert undoes the prize payment and the clearing of the players array. No winner is recorded, no puppy is minted, and the raffle is exactly where it started, except that the caller has paid gas for a failed transaction.

6. The next caller faces the same odds. With one blank slot in four, roughly one attempt in four fails in this way; if three of ten players have refunded, roughly three attempts in ten fail, and the raffle may need several attempts before a real player is finally selected.

Recommended Mitigation

Never let a blank entry be selected as the winner. Remove refunded players from the array, keep a separate list of active players, or check the winner before paying and minting.

That check alone only turns a silent revert into a clear one, so the better fix is to exclude refunded entries from selection altogether, for example by compacting the array on refund or by drawing the winner only from active players. Also reject address(0) in enterRaffle(), so a zero address can never be entered in the first place.