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.

1. The issue lies in the selectWinner function, the winner select calculation does not consider the presence of empty addresses within the array. Consequently, an empty address can be chosen as the winner, causing active players to be unfairly excluded from winning. To resolve this, the function should create a separate array that only includes non-empty addresses for selecting the winner.

   https://github.com/Cyfrin/2023-10-Puppy-Raffle/blob/069cf56ec051216ccab79b550fba5e2a188ade9c/src/PuppyRaffle.sol#L125-132

   Impact

   • Unfair Results: The presence of empty addresses allows the selection of an empty address as the winner, which is unjust for active participants.

   • Loss of Trust: Unfair outcomes undermine trust in the smart contract and the integrity of the raffle system.

   • Manipulation: Malicious actors may exploit this vulnerability by intentionally adding empty addresses to increase their chances of winning or disrupt the fairness of the raffle.

   Tools Used

   Manual Review

   Recommendations

   To address this vulnerability, the selectWinner function should be modified as follows:

   1. Create a new array, named activePlayers, to store the non-empty addresses from the players array.

   2. Iterate over the players array and populate the activePlayers array with the non-empty addresses.

   3. Calculate the winnerIndex using the size of the activePlayers array instead of the players array.

   4. Select the winner from the activePlayers array using the calculated winnerIndex.

Proof of Concept

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()