Summary
When a player requests a refund by calling the 'PuppyRaffle::refund' function, it puts address(0) in the players array of that players index. This means that address(0) could potentially win the lottery once selectWinner is called. If this address wins it would cause the function to revert with a 'ERC721: mint to the zero address' error.
Vulnerability Details
Because the refund function sets the index of the refunding player to address(0), it can potentially win the raffle when selectWinner is called. If address(0) wins the raffle the function will revert with an error of 'ERC721: mint to the zero address'. This is due to the _saftMint function being called, it comes with built in protections to stop the token from being minted to the zero address.
function refund(uint256 playerIndex) public {
address playerAddress = players[playerIndex];
require(playerAddress == msg.sender, "PuppyRaffle: Only the player can refund");
require(playerAddress != address(0), "PuppyRaffle: Player already refunded, or is not active");
payable(msg.sender).sendValue(entranceFee);
@> players[playerIndex] = address(0);
emit RaffleRefunded(playerAddress);
}
function selectWinner() external {
require(block.timestamp >= raffleStartTime + raffleDuration, "PuppyRaffle: Raffle not over");
require(players.length >= 4, "PuppyRaffle: Need at least 4 players");
uint256 winnerIndex =
uint256(keccak256(abi.encodePacked(msg.sender, block.timestamp, block.difficulty))) % players.length;
@> address winner = players[winnerIndex];
@> _safeMint(winner, tokenId);
}
Impact
This test returns as true, proving that the zero address could be selected as the winner when one or more players have refunded from the raffle.
modifier playersEnteredFive() {
address[] memory players = new address[](5);
players[0] = playerOne;
players[1] = playerTwo;
players[2] = playerThree;
players[3] = playerFour;
players[4] = playerFive;
puppyRaffle.enterRaffle{value: entranceFee * 5}(players);
_;
}
function testAddressZeroCanWin() public playersEnteredFive {
vm.warp(block.timestamp + duration + 1);
vm.roll(block.number + 1);
uint256 indexOfPlayerThree = puppyRaffle.getActivePlayerIndex(playerThree);
vm.prank(playerThree);
puppyRaffle.refund(indexOfPlayerThree);
vm.expectRevert("ERC721: mint to the zero address");
puppyRaffle.selectWinner();
}
[PASS] testAddressZeroCanWin() (gas: 231293)
Tools Used
-Foundry
Recommendations
If the logic of the refund function cannot change, then it would be suggested in the selectWinner function to check if address(0) has been selected as the winner, if it has, then revert with a custom error to avoid confusion.
+ error PuppyRaffle__AddressZeroCannotWin();
function selectWinner() external {
require(block.timestamp >= raffleStartTime + raffleDuration, "PuppyRaffle: Raffle not over");
require(players.length >= 4, "PuppyRaffle: Need at least 4 players");
uint256 winnerIndex =
uint256(keccak256(abi.encodePacked(msg.sender, block.timestamp, block.difficulty))) % players.length;
address winner = players[winnerIndex];
+ if(winner == address(0)) {
+ revert PuppyRaffle__AddressZeroCannotWin();
+ }
uint256 totalAmountCollected = players.length * entranceFee;
uint256 prizePool = (totalAmountCollected * 80) / 100;
uint256 fee = (totalAmountCollected * 20) / 100;
totalFees = totalFees + uint64(fee);
uint256 tokenId = totalSupply();
// We use a different RNG calculate from the winnerIndex to determine rarity
uint256 rarity = uint256(keccak256(abi.encodePacked(msg.sender, block.difficulty))) % 100;
if (rarity <= COMMON_RARITY) {
tokenIdToRarity[tokenId] = COMMON_RARITY;
} else if (rarity <= COMMON_RARITY + RARE_RARITY) {
tokenIdToRarity[tokenId] = RARE_RARITY;
} else {
tokenIdToRarity[tokenId] = LEGENDARY_RARITY;
}
delete players;
raffleStartTime = block.timestamp;
previousWinner = winner;
(bool success,) = winner.call{value: prizePool}("");
require(success, "PuppyRaffle: Failed to send prize pool to winner");
_safeMint(winner, tokenId);
}
