First Flight #2: Puppy Raffle

Summary

The determination of the selected winner (represented by the winnerIndex variable in PuppyRaffle::selectWinner method), totalAmountCollected, prizePool and totalFees in PuppyRaffle::selectWinner fail to take into account that there can be zero addresses in PuppyRaffle::players array resulting from the refunding of some players prior to the call to selectWinner.

Vulnerability Details

Assuming there was some refunds before the call to selectWinner. This leads to a cascade of effects gravest among which are:

• The selected winner (represented by winnerIndex) can be address(0). Knowing we can't send ether to this address, this reverts the current transaction thereby protecting the ether but unfairly disadvantaging the current transaction author as he can't call selectWinner without it reverting unless he artificially increases the length of PuppyRaffle::players array by entering the raffle with another address thereby changing what would have been the winner of the previous transaction.

• If the selected winner (winnerIndex of the selected winner) by luck does not point to address(0), then totalAmountCollected will be greater than what was effectively collected for the current raffle as it fails to account for the refunded players. This cascades into the prizePool and totalFees collected being greater than what really is.
  Due to the fact that, the prizePool is paid out to the winner in the current call to selectWinner but not the totalFees, what this effectively means is that :

• For raffles wherein less than 1/5th of the players were refunded, the prizePool paid to the winner eats into the protol collected fees totalFees of the current raffle thereby messing up with the accounting of the totalFees leading to an inabilitiy of withdrawing the collected fees in PuppyRaffle::withdrawFees because of the strict equality check at the first line of this method.

• For raffles wherein more than 1/5th of the players were refunded, the inability to payout the selected winner regardless of whether he's not address(0). This is because there isn't enough ether in the current raffle to payout such a high prizePool except if there was some residual ether in the contract resulting form the collected fees from prior raffle rounds.

POC

```solidity
    function testRefundBreaksSelectingAWinner() public playersEntered {
        vm.warp(block.timestamp + duration + 1);
        vm.roll(block.number + 1);

        uint playerFourIndex = puppyRaffle.getActivePlayerIndex(playerFour);
        vm.prank(playerFour);
        puppyRaffle.refund(playerFourIndex);
        vm.expectRevert("PuppyRaffle: Failed to send prize pool to winner");
        puppyRaffle.selectWinner();
    }

    function testRefundBlocksWithrawalOfFees() public playersEntered {
        vm.warp(block.timestamp + duration + 1);
        vm.roll(block.number + 1);

        address[] memory newPlayers = new address[](1);
        newPlayers[0] = address(20);
        puppyRaffle.enterRaffle{ value: entranceFee }(newPlayers);
        uint256 expectedPrizeAmount = ((entranceFee * 4) * 80) / 100;
        uint playerOneIndex = puppyRaffle.getActivePlayerIndex(playerOne);
        vm.prank(playerOne);
        puppyRaffle.refund(playerOneIndex);
        console.log("new balance: ", address(feeAddress).balance);
        puppyRaffle.selectWinner();
        vm.expectRevert("PuppyRaffle: There are currently players active!");
        puppyRaffle.withdrawFees();
    }
```

In the terminal we run the following commands one after the other.

forget test --mt testRefundBreaksSelectingAWinner -vvv

forget test --mt testRefundBlocksWithdrawalOfFees -vvv

Impact

Not accounting for refunds during the winner selection can lead to the fees collected by the protocol to be stolen or to not be withdrawable through the PuppyRaffle::withdrawFees method.

Tools Used

Manual review

Recommendations

• Either we determine the winnerIndex based only on the total number of the real actual participants of the current raffle round

• or better yet, we change the data type of PuppyRaffle::players to use an enumerable sate. OpenZeppelin EnumerableSet has the add/remove methods which can be used to add/remove entries from the set and the length method which returns the real size of the set which is an effective count of the active players in the set and will not include the players that were added and removed afterwards.

and