Root + Impact

Description

PuppyRaffle.selectWinner() in src/PuppyRaffle.sol works out the prize and the fees from players.length, even though refunded players are left in that array as address(0). The refunded players have already taken their entrance fee back, so the contract holds less ether than the calculation assumes, and the prize it tries to pay can be larger than the balance available.

The vulnerable code is:

refund() does not shrink the array or reduce any total. It only replaces the refunded entry with address(0):

So players.length counts every player who ever entered, including everyone who has already been refunded. The prize is 80% of the entrance fees of all of them, while the contract only still holds the fees of the players who have not refunded. There is no function that can remove a refunded entry, compact the array, or correct the total, so once the numbers cross the threshold there is no way to fix a raffle round in progress.

Risk

Likelihood: High - refunding is a normal, documented feature of the contract, and no attack is needed. Any raffle in which more than 20% of the players refund before the winner is chosen will hit this problem, which is an entirely ordinary outcome if players change their minds or if the raffle runs for a while.
Impact: High - when the prize exceeds the balance, selectWinner() reverts on every call and the raffle can never finish. No winner is chosen, no puppy is minted, and the prize is never paid. The remaining players can only get their money back by refunding one by one; if they do not notice, or if they wait for a winner that can never be chosen, their funds stay locked in the contract with no recovery function.

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 the entrance fee is 1 ether and four players enter, so the contract holds 4 ether.

1. One player calls refund() for their index. They receive 1 ether back, their slot becomes address(0), and the contract balance falls to 3 ether. players.length is still 4.

2. The raffle duration passes and someone calls selectWinner().

3. The contract computes totalAmountCollected = 4 * 1 ether = 4 ether, prizePool = (4 ether * 80) / 100 = 3.2 ether, and fee = 0.8 ether.

4. It then tries to send prizePool, 3.2 ether, to the winner, but the contract only holds 3 ether. A call that tries to send more ether than the contract holds reverts, so selectWinner() reverts and the require(success, ...) is never even reached cleanly.

5. Calling selectWinner() again changes nothing: the balance is still 3 ether, players.length is still 4, and the prize is still 3.2 ether, so it reverts again, every time. The threshold is simple: the prize is 80% of the original total, so the raffle is bricked whenever refunds exceed 20% of the players. With five players, two refunds are enough; with ten players, three refunds are enough.

6. The only way out is for the remaining three players to refund individually. If any of them does not, their entrance fee remains in a contract that can never select a winner.

Note that the fee is overstated in the same way: totalFees is increased by 20% of the original total, including fees on entrance fees that have already been refunded, so the recorded fees are also higher than the money actually held.

Recommended Mitigation

Base the prize and the fee on the money the contract actually holds for active players, not on players.length. Count only non-zero entries, or track an active player count that is reduced on refund, and derive the prize from that count.

Alternatively, remove refunded players from the array, or keep a running total of collected fees that is reduced when a refund is paid, so the prize can never exceed the balance available.