Summary
As refund() method resets address of player to address(0) without decreasing the array size. It can lead to winner getting selected as address(0) leading to reverting of transaction.
Vulnerability Details
The refund() method replaces the address of the player with address(0).
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);
}
Now, when the selectWinner() is called, it calculates the winner and send the rewards tokens to it. There is a high possibility of the winner being address(0) and unexpected reverting of transaction.
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];
uint256 totalAmountCollected = players.length * entranceFee;
uint256 prizePool = (totalAmountCollected * 80) / 100;
uint256 fee = (totalAmountCollected * 20) / 100;
totalFees = totalFees + uint64(fee);
uint256 tokenId = totalSupply();
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);
}
POC Test:
function testSelectWinnerReverts() public playersEntered {
uint256 balanceBefore = address(playerFour).balance;
vm.warp(block.timestamp + duration + 1);
vm.roll(block.number + 1);
uint256 expectedPayout = ((entranceFee * 4) * 80 / 100);
vm.prank(playerFour);
puppyRaffle.refund(3);
vm.expectRevert("PuppyRaffle: Failed to send prize pool to winner");
puppyRaffle.selectWinner();
}
Impact
The selectWinner() will revert unexpectedly.
Recommendations
Decrease the size of players array as user calls refund().
if(players.length > 1) {
players[playerIndex] = players[players.length - 1];
players.pop();
} else {
delete players;
}
