First Flight #2: Puppy Raffle

Summary

When we calculate the winner's prize, we take into account all of the users that participated in the raffle, no matter if they withdrew their funds or not. This is wrong because we are trying to send the winner an amount that we don't have.

Vulnerability Details

When we select the winner we get this as the total amount collected:

uint256 totalAmountCollected = players.length * entranceFee;

However this is not correct since some users may have withdrawn their entranceFee and since we set the player's index to be address(0) when he refunds, the players.length doesn't change. Meaning if one gets a refund we will have less funds than the calculated totalAmountCollected.

Impact

This leaves the winner with no prize because if enough people get a refund the contract won't have enough money to pay the winner and the call will revert.

POC

Here is a POC showing the vulnerability:

function testRefundAmountIsNotCorrect() public {
    //Lets say we have 4 people in the raffle
    address[] memory players = new address[](4);
    players[0] = playerOne;
    players[1] = playerTwo;
    players[2] = playerThree;
    players[3] = playerFour;

    puppyRaffle.enterRaffle{value: entranceFee * players.length}(players);

    vm.startPrank(playerTwo);
    puppyRaffle.refund(1);
    vm.stopPrank();
    vm.startPrank(playerThree);
    puppyRaffle.refund(2);
    vm.stopPrank();

    vm.warp(block.timestamp + duration + 1);

    uint256 winnerIndex =
        uint256(keccak256(abi.encodePacked(msg.sender, block.timestamp, block.difficulty))) % players.length;

    console.log("The winner is: ", winnerIndex);
    console.log("Winner's prize: ", ((players.length * entranceFee) * 80) / 100);
    console.log("Actual raffle funds: ", address(puppyRaffle).balance);

    puppyRaffle.selectWinner();
}

If we run this test we see that it reverts with the reason: PuppyRaffle: Failed to send prize pool to winner. This means that the winner.call{value: prizePool}("") didn't succeed. It is because of the reason I described above.
The winner picked is the player with index 0 and his prize is 3200000000000000000 but the raffle only has 2000000000000000000.

Tools Used

VS Code, Foundry, Manual Review

Recommendations

To calculate the totalAmountCollected loop through the players array and if players[i] == address(0) ignore that line and don't take into consideration his entranceFee.