First Flight #2: Puppy Raffle

Summary

We are using a nested for loop to check for duplicate player entries in the enterRaffle() function. This is wildly inefficient and can lead to a tremendous waste of gas and even a potential DOS given enough players

Vulnerability Details

A general rule in computer programming is to avoid nested loops when possible as it guarantees at least an O(n^2) runtime. For example if the players array has 10 elements and for each of those elements we iterate over the array again then this gives us 100 (10 x 10) computations.

If we can find a way to only iterate over the array once we can reduce this cost by 90%!

Impact

Medium - While this may waste a ton of gas it will not lead to a loss of deposited funds.

Tools Used

Manual Inspection

Recommendations

This recommendation may take up more storage but the extra cost to do so is significantly smaller than the existing solution. What we can do is maintain a separate mapping to tell us if a player has been added or not:

Anytime a new player is added or removed we can set this mapping like so:

Now when we initially add a player to the 'players[]' array we can check this first and get rid of the double for loop altogether

BONUS: This isPlaying mapping approach also reduces the runtime of the _isActivePlayer() function which we can now reduce to this...

BONUS #2: When do the savings end?!?!?! This approach also lets us refactor the getActivePlayerIndex() function like so...

this way we don't have to iterate over the entire array if we already know the player does not exist.