First Flight #2: Puppy Raffle

Summary

When the player calls PuppyRaffle#refund() method, their address in the players array gets overwritten with address(0). However, the length of the array remains the same. This is problematic as the PuppyRaffle contract relies on the length of players array in several situations. In the worst scenario this issue may lead to the protocol's permanent DOS and loss of ETH.

Vulnerability Details

When the player decided to exit the raffle, they may execute the refund() method which looks like this:

The address of the player gets overwritten with address(0). However, this does not change the length of the array. It simply means that the value at the playerIndex will be address(0), but players.length will remain unmodified.
This is a root cause of several issues in the contract, which are as follows:

1. Full DOS

Please consider the following scenario:

i) Alice and Bob have entered the PuppyRaffle. There are now two players in the raffle and the players.length = 2

ii) Both Alice and Bob have withdrawn their ETH using refund() method. The players.length is still equal to 2 - there are 2 elements inside the players array, both of which are equal to address(0)

The protocol in the current state is fully DOS'ed, because:

a) No other player can enter the raffle. This is due to the fact that the enterRaffle method checks for the duplicates inside the players array:

If any duplicates are present inside the players array, the method will revert. In the current state the address(0) is already duplicated in the array. Therefore, noone will be able to enter the raffle.

b) The raffle can't be resolved via selectWinner()method, as it has the following check:

The selectWinner() method is the only method that resets the array players to an empty array via delete players statement. Therefore the contract is now fully DOS'ed.

2. Loss of tokens

Please consider the aforementioned scenario, only this time 2 or more additional players have entered the raffle. The raffle can be now resolved with selectWinner() method, but it is possible address(0) will be selected as the winner. The prizePool will be successfully sent to address(0), and therefore - burnt and lost forever.

3. Incorrect value sent to the winner / Tokens getting stuck forever

The scenario from point 2 has an additional problem. This is due to the fact that the prizePool and fee are calculated as follows:

As mentioned before, even if some player has refunded the money, the players.length has not changed. Therefore the totalAmountCollected will be incorrect, as it implicitly assumes no player has refunded the money. Subsequently, the prizePool and fee will also be incorrect. This may lead to one of two scenarios:

a) The ETH balance of the contract will be sufficient to execute the transfer (due to the fact that there are some fees from previous raffle rounds that have not been claimed yet). As a result of that the winner will receive ETH, altough more than intended - the surplus will be taken from the unclaimed fees. This also means that the remainder of unclaimed fees will be stuck forever in the contract, because of this equality check in withdrawFees() method:

b) The ETH balance is not sufficient to execute the transfer - in this scenario the selectWinner() call will fail as the contract's balance will not be sufficient to transfer the (wrongly calculated) prizePool to the winner. The only way to increase the PuppyRaffle contract's balance is via enterRaffle(), the only payable method. However, this method increases the players.length and can't be used to mitigate that problem as it impacts the prizePool calculation. As a result, the tokens will be stuck in the contract forever. New players can still enter the raffle, but it can never be successully resolved.

Impact

DOS, tokens getting stuck forever, wrong amount of ETH sent to the winner

Tools Used

Manual review

Recommendations

Inside the refund() method, remove the player from the players array properly.