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

Addition of `totalFees` in `PuppyRaffle::selectWinner` can cause overflow, fees paid to `feeAddress` drastically reduced

stakog

Summary

The variable totalFees is responsible for accumulating fees for the owner across different raffles, but since it's not protected from overflowing, it will reset to 0 after accumulating approximately 18.45 ether.

Vulnerability Details

When a user calls PuppyRaffle::selectWinner, the function will choose a winner, calculate the total entrance fees and then cut 20% of them and send them to feeAddress, which we can consider as the owner of the raffle contract. The other 80% goes to the winner.

The 20% cut is added to an uint64 totalFees variable, which accumulated the 20% fee from all raffles since last time withdrawFees() was called.
Since totalFees is an uint64, it means it can hold a maximum of about 18.45 ether. Addition of extra fees will cause totalFees to reset to 0 and start over.

This means that from the 93rd participant registering, totalFees will reset and start over, reducing the fees to ~0.35 (this is also assuming we ignore the unsafe casting of fee, which is another issue).
This overflow cycle will continue as more players join, but anyway the 20% cut for the owner is capped at ~18.45 ether, and may be as low as 0 ether.

Impact

This vulnerability means a big loss in fee payments for the owner. Also, it doesn't require to craft any special attack. This overflow will occur once enough players join, which is a pretty small number of only 93.

Here is a PoC showing the value of totalFees before and after an overflow:

function testTotalFeesOverflow() public {
​
address payable attacker = address(666);
​
// Create the array of participants
// Do it in two groups to not trigger another issue (uint64 casting of `fee`)
address[] memory newPlayersGroup1 = new address[](46);
for (uint256 i = 0; i < newPlayersGroup1.length; i++){
newPlayersGroup1[i] = address(i+100);
}
​
address[] memory newPlayersGroup2 = new address[](47);
for (uint256 i = 0; i < newPlayersGroup2.length; i++){
newPlayersGroup2[i] = address(i+100);
}
// Enter the raffle with fee required, first group
puppyRaffle.enterRaffle{value: entranceFee * newPlayersGroup1.length}(newPlayersGroup1);
​
// Assert that totalFees still zero
uint256 totalFeesBefore = puppyRaffle.totalFees();
assertEq(totalFeesBefore, 0 ether);
​
// Run the raffle for first group
vm.startPrank(attacker);
vm.warp(block.timestamp + duration + 1); // wait
vm.roll(block.number + 1);
puppyRaffle.selectWinner();
vm.stopPrank();
​
// Assert totalFees is 20% of group 1 entrance fees
uint256 totalFeesAfter1 = puppyRaffle.totalFees();
assertLt(totalFeesAfter1, 9.5 ether); // totalFees is ~9.4 ether
​
​
// Enter the raffle with fee required, second group
puppyRaffle.enterRaffle{value: entranceFee * newPlayersGroup2.length}(newPlayersGroup2);
​
// Run the raffle for second group
vm.startPrank(attacker);
vm.warp(block.timestamp + duration + 1); // wait
vm.roll(block.number + 1);
puppyRaffle.selectWinner();
vm.stopPrank();
​
// Assert that totalFees is small and not 93*1*0.2 = 18.6 ether as expected, since an uint64 can hold max of ~18.45 ether
uint256 totalFeesAfter = puppyRaffle.totalFees();
assertLt(totalFeesAfter, 0.2 ether); // totalFees ~0.15 ether
}

Tools Used

  • Foundry

Recommendations

Use larger type. The contract actually uses an uint256 for the fee variable which accumulates the 20% cut for a single raffle, while totalFees accumulates across multiple raffles, yet it has a smaller data type. It doesn't make much sense, so use uint256 totalFees.

Updates

Lead Judging Commences

Hamiltonite Lead Judge about 2 years ago
Submission Judgement Published
Validated
Assigned finding tags:

overflow-uint64

Support

FAQs

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

Give us feedback!