First Flight #2: Puppy Raffle
First Flight #2
Beginner FriendlyFoundryNFT
100 EXP
View results
Previous Next
Submission Details
Severity: high
Valid

The refund function should not leave refunded players with zero addresses in the players array because it messes up the selectWinner function in multiple ways

happyformerlawyer

Summary

If a player wants to refund their entry, the refund function returns their funds and then changes their address in the players array to 0 but does not remove them from the array. This creates problems in the selectWinner function because it depends on players.length in multiple ways. selectWinner picks a winner using a modulo of players.length which means that a refunded player could be selected. Also, a selection could be made with less than 4 players even though selectWinner intends for there to be at least 4 players. Also selectWinner will incorrectly calculate the winnings as players.length * entranceFee which doesn't account for refunds.

Vulnerability Details

refund does not remove refunded players from the array:

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);
}

selectWinner picks a winner using a modulo of players.length (which means that a refunded player could be selected or that a selection could be made with less than 4 players and also select Winner incorrectly calculates the winnings as players.length * entranceFee which doesn't account for refunds.

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();

Impact

The protocol could send more money than it intends to the winner if someone had requested a refund during that lottery. They could also select the index of a player who got a refund and then send the winnings to a 0 address (since that is what the refund function set their address too). This is effectively burning the funds. Also, they could run the lottery with less than 4 players even though that is not what they intend.

Tools Used

Manual review

Recommendations

Instead of just zeroing out the address of someone who wants a refund, instead move the last player in the array to the spot of the person who asked for a refund and then use .pop to remove the last item in the array. This will shorten the length of the array to only the active players and fix all these problems.

Updates

Lead Judging Commences

Hamiltonite Lead Judge over 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

refund-doesnt-reduce-players-array-size-causing-protocol-to-freeze

zero address can win the raffle

Funds are locked to no one. If someone gets the refund issue, they also got this issue. IMPACT: High Likelihood: High

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.