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

After someone refunded, the raffle cannot start, and players array contain address(0)

WangSecurity

Summary

If one of the players refunded, the raffle cannot start, it reverts during transfer of funds to the winner, so entrance fees may be frozen in the protocol.

Vulnerability Details

If 4 players entered, one of them refunded, it means there are only three active players and address(0) (which is also an issue), so it doesn't revert on the check that there must be at least 4 players to select a winner (function selectWinner()).
The test:

function testRaffleDoesntStartAfterPlayerHaveRefunded() public playersEntered {
vm.prank(playerOne);
puppyRaffle.refund(0); //playerOne refunds his ticket
​
vm.warp(block.timestamp + duration + 1); //skip the time to select a winner
vm.roll(block.number + 1);
​
puppyRaffle.selectWinner(); // selecting a winner
assertEq(puppyRaffle.previousWinner(), playerThree); // actually just guessed who is a winner, wanted to know if we even get one or not.
}

It reverts with "PuppyRaffle: Failed to send prize pool to winner"

Impact

Impact is considered high, the contract will stop working, it won't be able to select winner, or collect fees, because there are active players (address(0) at least won't ever leave that beautiful raffle!), but players can refund as well.

Tools Used

Foundry

Recommendations

Actually delete players from an array of players and not replace them with address(0). In function refund() Replace players[playerIndex] = address(0); with:

// we rewrite every player in the array starting with the player we want to remove (playerIndex), yes there are other ways to delete and don't leave gaps, but this way also preserves the order of players entered.
for (uint i = playerIndex; i<players.length-1; i++){
players[i] = players[i+1];
}
delete players[players.length-1];
players.pop();

Here is the test, which shows that this is working:

function testRaffleStartsAfterPlayerHaveRefunded() public playersEntered {
address[] memory players = new address[](1); // adding a fifth address, so there are four after one of them refunds
players[0] = address(5);
puppyRaffle.enterRaffle(players);
​
vm.prank(playerOne); // playerOne refunds
puppyRaffle.refund(0);
​
vm.warp(block.timestamp + duration + 1); // skip the time to select winner
vm.roll(block.number + 1);
​
puppyRaffle.selectWinner(); // selecting winner
assertEq(puppyRaffle.previousWinner(), address(5)); // the winner is selected and it is indeed player five!
}
Updates

Lead Judging Commences

Hamiltonite Lead Judge about 2 years 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.

Give us feedback!