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

totalAmountCollected value is incorrectly calculated inside PuppyRaffle::selectWinner()

shikhar229169

Summary

The totalAmountCollected variable value is wrongly calculated. It uses the ````total length of players * entrance fees``` to calculate the total amount collected which will produce wrong result if some people called the refund function, the total players will decrease but still the players array length will remain the same and as players array length is used to calculate the total amount collected it will lead to reverting of the function call and selectWinner function can never be called as the total amount calculated will be more than the balance of contract.

Vulnerability Details

It is not always true that the total active players are actually always equal to length of players array because if a person calls the PuppyRaffle::refund() function they will become inactive and are not active in the Raffle, thus they should not be counted while calculating the totalAmountCollected but as the length of players array will remain the same and is used to calculate totalAmountCollected therefore it is wrongly calculated.

So, suppose if 10 players entered the raffle and fees was 1 ETH, so contract balance = 10 ETH.
Now if 3 person leaves then active players = 7 and contract balance will become 7 ETH.
Therefore if we call selectWinner at this instance the totalAmountCollected should be 7 ETH, but it will give 10 ETH, as it uses the length of players array to calculate that as it will remain unchanged.
And now 80% of 10 ETH will be sent to winner which is 8 ETH and as contract balance is 7 ETH, so this will always revert and select winner can never be called.

Below test is the demonstration for my finding, it can be used in PuppyRaffleTest.t.sol

function test_SelectWinner_Reverts_DueToWrongCalculationOfTotalAmountCollected() public {
uint256 count = 10;
uint256 START_BALANCE = 10 ether;
address[] memory gang = new address[](count);
​
for (uint160 i = 1; i <= 10; ++i) {
gang[i - 1] = address(i);
}
​
address attacker = makeAddr("attacker");
hoax(attacker, count * entranceFee);
puppyRaffle.enterRaffle{value: count * entranceFee}(gang);
​
// At this point PuppyRaffle have 10 active players and 10 eth
​
// 3 people leaves the Raffle
for (uint160 i = 1; i <= 3; ++i) {
hoax(address(i), START_BALANCE);
puppyRaffle.refund(i - 1);
}
​
vm.warp(block.timestamp + duration + 1);
vm.roll(block.number + 1);
​
// At this moment PuppyRaffle have 7 active players and 7 eth
​
// this will incorrectly calculate totalAmountCollected as 10 eth but it should be 7 eth
// as it uses (players.length * entranceFee) to calculate it and it will be equal to 10 eth
// but it should be 7 eth as total active players are 7
// so this will try to send 80% of 10 eth to winner but as balance is 7 eth so it will revert
vm.expectRevert("PuppyRaffle: Failed to send prize pool to winner");
puppyRaffle.selectWinner();
}

Impact

This will have a negative impact on our lottery system as selectWinner function can't be called in the above case due to wrong calculation of totalAmountCollected.

Tools Used

Manual Review

Recommendations

  • Use a counter variable for total active players and increment it when a player entered a raffle while decrementing it when one calls refund().
    Therefore,

totalAmountCollected = totalPlayersCounter * entranceFees

  • But if we want to calculate in the similar way by players array length, then when a user calls the refund() function, replace the address at their idx with the address at last most idx and pop the last address out of players array.

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.