First Flight #2: Puppy Raffle

Summary

HIGH-2: The "refund" function does not change the "totalAmountCollected" variable when refunding a user with the entrance fee, causing a discrepancy in the calculation of the prizePool and fee variables. The variable is also calculated late in the contract process.

Vulnerability Details

The "totalAmountCollected" variable is responsible for holding the entrance fee for all players. But the contract does not reflect that when a player withdraws from the raffle (called refund function), this variable needs to be lowered by the entrance fee to reflect the actual balance of the contract.

Impact

This causes the base for calculating the prizePool and fee staying high despite some players may have withdrawn their refunds. A potential attack may consist of entering a dozen addresses, paying the entrance fee, then withdrawing all but one of the addresses and receiving the huge amount that the contract may not even have in reality.

Registering 20 players with 1000 wei in fee results in the contract having 20 players registered and 20000 wei in balance. Withdrawing 19 of these players, the real balance is 1000 wei due to refunds, but the variable totalAmoutCollected will during its calculation in the "selectWinner" function reach 20000. This causes the prizePool be calculated as 16000 wei, and the fee as 4000 wei. But this is a balance the contract does not have therefore causing it to fail.

Tools Used

Static analysis, mathematical proof

Recommendations

Making the totalAmountCollected a storage variable that is incremented each time new players enter the raffle and decremented when the players asks for refund would pose a better approach to handling the contract balance tracking.

Create static variable totalAmountCollected as unit256.

In the enterRaffle function, add the following line:
totalAmountCollected += players.length * entranceFee;

In the refund function, add the following line before sending the value to the player:
totalAmountCollected -= entranceFee;

Delete the calculation of the totalAmountCollected from the selectWinner function.

Before assigning the "raffleStartTime" variable (that should be as mentioned in M-3 issue at the very end of the selectWinner function), reassign the totalAmountCollected to zero.