First Flight #2: Puppy Raffle

Vulnerability Details

totalAmountCollected is an important variable, other variables depend on it's value to carry the proper execution of the program. This variable is calculated by measuring the length of the players array. In the scenario that no single players gets refunded, nothing happens the program executes as normal, but if a single player gets refunded, the entranceFee is refunded to the player, and the players position in the Array is set to address(0), but the players array length doesn't change, so when calculating totalAmountCollected it would get an incorrect value, and making impossible to have a winner, and the funds of the players can only be accesed by getting refunded.

PoC

1. Copy the test into PuppyRaffleTest.t.sol

2. Run the test

   1. The test should fail

Impact

High - compromising the protocol's functionality

Recommendations

There's two possible solutions to this problem;

1. 'Sanitize' the players array every time a players get refunded

2. 'Sanitize' the players array in the selectWinner() function, and check for non Address(0) adresses to be greater or equal to 4.

I recommend for the second solution as it can consume less gas.

I would add some lines of code to selectWinner().

These will have the following execution flow;

Runs a for loop through the players array and counts the number of invalid addresses

• If the number is bigger than zero

  • create a new players array and populated with the valid addresses from the players array

  • change the players array to this 'cleaned' array.

  • execute selectWinner()

• If the number is zero

  • execute selectWinner()

After this modification we can re-run the test and it should pass this time