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

`PuppyRaffle::refund` replaces an index with address(0) which can cause the function `PuppyRaffle::selectWinner` to always revert

maroutis

Summary

PuppyRaffle::refund is supposed to refund a player and remove him from the current players. But instead, it replaces his index value with address(0) which is considered a valid value by solidity. This can cause a lot issues because the players array length is unchanged and address(0) is now considered a player.

Vulnerability Details

players[playerIndex] = address(0);
​
@> uint256 totalAmountCollected = players.length * entranceFee;
(bool success,) = winner.call{value: prizePool}("");
require(success, "PuppyRaffle: Failed to send prize pool to winner");
_safeMint(winner, tokenId);

If a player refunds his position, the function PuppyRaffle::selectWinner will always revert. Because more than likely the following call will not work because the prizePool is based on a amount calculated by considering that that no player has refunded his position and exit the lottery. And it will try to send more tokens that what the contract has :

uint256 totalAmountCollected = players.length * entranceFee;
uint256 prizePool = (totalAmountCollected * 80) / 100;
​
(bool success,) = winner.call{value: prizePool}("");
require(success, "PuppyRaffle: Failed to send prize pool to winner");

However, even if this calls passes for some reason (maby there are more native tokens that what the players have sent or because of the 80% ...). The call will thankfully still fail because of the following line is minting to the zero address is not allowed.

_safeMint(winner, tokenId);

Impact

The lottery is stoped, any call to the function PuppyRaffle::selectWinnerwill revert. There is no actual loss of funds for users as they can always refund and get their tokens back. However, the protocol is shut down and will lose all it's customers. A core functionality is exposed. Impact is high

Proof of concept

To execute this test : forge test --mt testWinnerSelectionRevertsAfterExit -vvvv

function testWinnerSelectionRevertsAfterExit() public playersEntered {
vm.warp(block.timestamp + duration + 1);
vm.roll(block.number + 1);
// There are four winners. Winner is last slot
vm.prank(playerFour);
puppyRaffle.refund(3);
​
// reverts because out of Funds
vm.expectRevert();
puppyRaffle.selectWinner();
​
vm.deal(address(puppyRaffle), 10 ether);
vm.expectRevert("ERC721: mint to the zero address");
puppyRaffle.selectWinner();
​
}

Tools Used

  • foundry

Recommendations

Delete the player index that has refunded.

- players[playerIndex] = address(0);
​
+ players[playerIndex] = players[players.length - 1];
+ players.pop()
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!