First Flight #2: Puppy Raffle

Summary

Because when refund is happened players.length does not decrease, it won't be possible to selectWinner.

Vulnerability Details

When someone refund they get paid and their corresponding place in players array become 0. But this does not reduce the length of the array. But prize and fee amounts are computed using players.length and entranceFee as shown below:

Because player refunded, there won't be enough balance to send both prize and also fee to the corresponding recipients. Hence all funds will be stuck in contract. Here is the POC for this scenario:

You can add the POC above to the PuppyRaffleTest.t.sol and the test will pass.

Users can then refund one by one in order to get their money back but selectWinner will be useless forever after even one refund call.

Impact

Protocol's main functionality is broken after refund call. Hence it is high.

Tools Used

Manual Review

Recommendations

Don't use players.length in both prize calculation and also in require statement of selectWinner function because it does not decrease after refund process. Loop can be useful with 0 address check in array but it will cost much and also it is DOS'able. Hence it is best if you use global length variable and increase it when players enter, decrease it when players refund.