First Flight #2: Puppy Raffle

Summary

The players array might have several empty value. The length of player does not equal to the amount of the players registered in the raffle activity since user might refund and quit.

Vulnerability Details

In the selectWinner function, the validation of players.length is useless. If there are originally N players registered but M players refund with N≥4 and M≥N-4, the number of active players participated in the activity is lower than 4.

Impact

The require statement that validates the length of players array becomes useless if the user can refund and quit.
And the calculation of totalAmountCollected is incorrect since it does not take the refund process into consideration. There might much less ether locked in the contract than the value of
totalAmountCollected if multiple users refund. This will lead to the revert of the function in the ether transfer to the winner in (bool success,) = winner.call{value: prizePool}("");
The contract will be never possible to distribute the ether to the winner and reset the start time of the ruffle, which result in the denial of service.

Tools Used

manual review and foundry test

Recommendations

Use a new variable, activePlayerAmount, to maintain the exact amount of the active players in the contest. Update the activePlayerAmount by increasing the length of newPlayers array, and decrease the value by one when user invokes refund function.