First Flight #2: Puppy Raffle

Summary

By the time raffle time ends and we call selectWinner() the 'players' array may not be accurate and could lead to too much ETH being sent to the winner.

Vulnerability Details

During the raffle period any player could have requested their refund. In doing so the entry at their index in the array is set to 0. But otherwise the array stays the same including the it's length. Therefore, when selecting winner we could have an array with several zero values and an inaccurate length. Here is a scenario:

1. 10 players (a-j) enter the raffle and we have a 'players' array that looks like this:

[a,b,c,d,e,f,g,h,i,j], length 10

2. Player C and D (index 2 and 3) decide to get refunds. The array now looks like:
   [a,b,0,0,e,f,g,h,i,j], length 10

3. The raffle ends and somebody calls 'selectWinner()'. There 8 players left but the contract thinks there are 10 due to the value of players.length. We use this length value to compute the total winnings which are now larger than if the array returned a length of 8 which is the actual number of players left.

Impact

High - There is a good chance people occasionally request refunds. This can lead to the winner getting too much ETH and then the withdrawFees() function breaks due to inaccurate accounting of 'totalFees' variable. This may also result in there not being enough ETH to cover the false value of the winner's earnings. This means nobody will get their funds!

Tools Used

Manual inspection

Recommendations

There are several methods by which we can keep better accounting of the number of players. You may want to augment or create new complex data types.

A simple solution could be to keep an internal 'playerCount' integer state variable and update it appropriately anytime a player is added or refunded. Then use that to calculate how to divvy up the winnings.