First Flight #2: Puppy Raffle

Summary

The PuppyRaffle::getActivePlayerIndex(address) returns 0 when the index of this player's address is not found, which is the same as if the player would have been found in the first element in the array. This can trick calling logic to think the address was found and then attempt to execute a PuppyRaffle::refund(uint256).

Vulnerability Details

The PuppyRaffle::refund() function requires the index of the player's address to preform the requested refund.

In order to have this index, PuppyRaffle::getActivePlayerIndex(address) must be used to learn the correct value.

The logic in this function returns 0 as the default, which is as stated in the @return NatSpec. However, this can create an issue when the calling logic checks the value and naturally assumes 0 is a valid index that points to the first element in the array. When the players array has at two or more players, calling PuppyRaffle::refund() with the incorrect index will result in a normal revert with the message "PuppyRaffle: Only the player can refund", which is fine and obviously expected.

On the other hand, in the event a user attempts to perform a PuppyRaffle::refund() before a player has been added the EvmError will likely cause an outrageously large gas fee to be charged to the user.

This test case can demonstrate the issue:

The results of running this one test show about 9 ETH in gas:

Additionally, in the very unlikely event that the first player to have entered attempts to preform a PuppyRaffle::refund() for another user who has not already entered the raffle, they will unwittingly refund their own entry. A scenario whereby this might happen would be if playerOne entered the raffle for themselves and 10 friends. Thinking that nonPlayerEleven had been included in the original list and has subsequently requested a PuppyRaffle::refund(). Accommodating the request, playerOne gets the index for nonPlayerEleven. Since the address does not exist as a player, 0 is returned to playerOne who then calls PuppyRaffle::refund(), thereby refunding their own entry.

Impact

1. Exorbitantly high gas fees charged to user who might inadvertently request a refund before players have entered the raffle.

2. Inadvertent refunds given based in incorrect playerIndex.

Tools Used

Manual Review and Foundry

Recommendations

1. Ideally, the whole process can be simplified. Since only the msg.sender can request a refund for themselves, there is no reason why PuppyRaffle::refund() cannot do the entire process in one call. Consider refactoring and implementing the PuppyRaffle::refund() function in this manner:

Which happens to take advantage of the existing and currently unused PuppyRaffle::_isActivePlayer() and eliminates the need for the index altogether.

2. Alternatively, if the existing process is necessary for the business case, then consider refactoring the PuppyRaffle::getActivePlayerIndex(address) function to return something other than a uint that could be mistaken for a valid array index.