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

Refund Results in Fees Getting Permanently Stuck

pacelli

Summary

After a player uses refund to exit the raffle, the players array still account for the same number of players, this will break the prize distribution and fee allocation during the selection of the winner.

Vulnerability Details

Let's assume five players enter the raffle and after some time a player decides to use refund to exit. The function will send the ether back to the player but in line 103 the function does not reduce the array, it only replaces the address with the zero address:

players[playerIndex] = address(0);

This means the raffle still has five players but the balance of the contract was reduced from 5 to 4 ether.

After the raffle ends, someone calls selectWinner and here is where the impact of the bug can be seen. The amount of prize and fees are calculated as such:

uint256 totalAmountCollected = players.length * entranceFee;
uint256 prizePool = (totalAmountCollected * 80) / 100;
uint256 fee = (totalAmountCollected * 20) / 100;

Because the players array still accounts for five player the values of these variables are:

  • totalAmountCollected is equal to 5 ether

  • prizePool will be 4 ether

  • fee will be 1 ether

The actual balance in the contract is 4 ether, everything will be sent to the winner and the totalFees will be set as 1 ether despite having no remaining balance.

The will make withdrawFees to revert because the check in this function will never pass.

require(address(this).balance == uint256(totalFees), "PuppyRaffle: There are currently players active!");

Even after new rounds the balance of the contract will never equal the value of totalFees because after new rounds the new fees are added to it.

totalFees = totalFees + uint64(fee);

Proof of Concept

Paste and run the following code snippet in PuppyRaffleTest.t.sol to see the effect of the bug.

Code snippet
modifier playerEnterRaffle() {
address[] memory players = new address[](5);
players[0] = playerOne;
players[1] = playerTwo;
players[2] = playerThree;
players[3] = playerFour;
players[4] = address(5);
puppyRaffle.enterRaffle{value: entranceFee * 5}(players);
_;
}
​
function testFeesGetStuck() public playerEnterRaffle() {
// player refunds
vm.prank(playerTwo, playerTwo);
puppyRaffle.refund(1);
​
// end raffle
vm.warp(block.timestamp + duration);
vm.roll(block.number + 1);
​
puppyRaffle.selectWinner();
​
// due to incorrect accounting of players after a refund, fees cannot be withdrawn
vm.expectRevert("PuppyRaffle: There are currently players active!");
puppyRaffle.withdrawFees();
}

Impact

Fees stuck in the contract.

Tools Used

VS Code and Foundry.

Recommendations

Instead of only replacing the address of the player, switch its position to the last and use the pop array method to remove it from the game, this change will result in correct math for the prize and fee allocation.

diff --git a/src/PuppyRaffle.orig.sol b/src/PuppyRaffle.sol
index 9e20456..f25ec83 100644
--- a/src/PuppyRaffle.orig.sol
+++ b/src/PuppyRaffle.sol
@@ -102,7 +102,8 @@ contract PuppyRaffle is ERC721, Ownable {
payable(msg.sender).sendValue(entranceFee);
- players[playerIndex] = address(0);
+ players[playerIndex] = players[players.length - 1];
+ players.pop();
emit RaffleRefunded(playerAddress);
}
Updates

Lead Judging Commences

Hamiltonite Lead Judge almost 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.