First Flight #2: Puppy Raffle

Summary

A zero address winner can be picked and therefore block the selectWinner function because every time it is called the call will revert due to trying to send funds to a zero address.

Vulnerability Details

When picking a winner we use this formula: uint256(keccak256(abi.encodePacked(msg.sender, block.timestamp, block.difficulty))) % players.length. However after getting the winner's index we do not check if it is a zero address since when a user refunds, he is stored in the players array as address(0). Therefore when we call the function selectWinner and players[winnerIndex] == address(0) this function will be blocked because when we try to send the winner his prize by winner.call{value: prizePool}("") the call will revert (we are trying to call address(0)).

Impact

Not picking a proper winner and could possible DOS the selectWinner function.

POC

Here is a POC:

function testWinnerIsZeroAddress() 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(playerOne);
    puppyRaffle.refund(0);
    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);

    puppyRaffle.selectWinner();
}

In this test we see that the winnerIndex is the first player, but since the first player has refunded, that means that in the players array he is stored as address(0). Then we call the winner's address but the call reverts.

Tools Used

VS Code, Foundry, Manual Review

Recommendations

In the selectWinner function we should check if the winner picked is the address(0) and if it is, then we should pick a new winner.