First Flight #2: Puppy Raffle

Summary

Call of refund() function doesn't change number of players.
It just change the address of the player that refound with the address(0).

This behaviour breaks the selectWinner and withdrawFees function.

Vulnerability Details

When refundind the refund() funtion run the line:

players[playerIndex] = address(0);

players array still has the same size.

selectWinner() function then use players.length:

• as a requirement to run:

    require(players.length >= 4, "PuppyRaffle: Need at least 4 players");
  

• to claculate prizePool

• to claculate totalFees

    uint256 totalAmountCollected = players.length * entranceFee;
    uint256 prizePool = (totalAmountCollected * 80) / 100;
    uint256 fee = (totalAmountCollected * 20) / 100;
    totalFees = totalFees + uint64(fee);
  

and then withdrawFees() function use totalFees as a requirement:

    require(
        address(this).balance == uint256(totalFees),
        "PuppyRaffle: There are currently players active!"
    );

Impact

Contract refunds eth after calling the refund function but players array size doesn't decrease.

Let's supose each ticket costs 1 ether and that one player refund its ticket:

    uint256 totalAmountCollected = players.length * entranceFee;

totalAmountCollected calculation is wrong as it is higher than the amount of eth in the contract

    uint256 prizePool = (totalAmountCollected * 80) / 100;

under 5 players prizePool will be higher than the amount of eth in contract and selectWinner() will revert upon sending prizePool

• totalAmountCollected = 4*1ether = 4ether

• prizePool = 4ether*0.8 = 3.2ether

• ether in contract => 3ether

    (bool success, ) = winner.call{value: prizePool}(""); //@audit check if reentrancy possible
    require(success, "PuppyRaffle: Failed to send prize pool to winner");
  

from 5 players, prizePool will be equal or lower than the amount of eth in contract and selectWinner function will execute

• totalAmountCollected = 5*1ether = 5ether

• prizePool = 5ether*0.8 = 4ether

• ether in contract = 4ether

but the requirement:

    require(
        address(this).balance == uint256(totalFees),
        "PuppyRaffle: There are currently players active!"
    );

will fails as the amount of eth left in the contract will be inferior to 20% of totalAmountCollected

An easy way to break the selectWinner() function for a malicious user would be to enter and refund multiple times, increasing each time the size of players array.

Tools Used

Manual review

Recommendations

A solution can be to keep track of active players by adding a counter that increase for each new players added and decrease each time a player refunds.