Puppy Raffle
AI First Flight #1
Beginner FriendlyFoundrySolidityNFT
EXP
View results
Previous Next
Submission Details
Severity: high
Valid

selectWinner() performs division before multiplication causing consistent precision loss in fee calculation

squashingbugs007

Root + Impact

Description

  • Normal behavior: When selectWinner() is called, the total entrance fees collected are split: 80% goes to the winner as the prize pool, and 20% is accumulated as protocol fees in totalFees. This split is calculated using integer arithmetic.

  • The issue: Solidity integer division truncates toward zero and any remainder is discarded. When division is performed before multiplication, the truncation error is amplified. The current fee calculation divides first and then multiplies, which causes consistent rounding loss on every raffle round.

  • Specifically, totalAmountCollected * 20 / 100 is evaluated left-to-right. If totalAmountCollected is not perfectly divisible by 100, the intermediate division truncates, and the multiplication operates on an already-reduced value. This means the protocol consistently collects fewer fees than the intended 20% cut.

  • Over many raffle rounds, the cumulative shortfall becomes significant. The winner also receives slightly less than the intended 80% in some cases.

// src/PuppyRaffle.sol#L132-L133
// @> Division before multiplication precision loss on every raffle
uint256 prizePool = (totalAmountCollected * 80) / 100;
uint256 fee = (totalAmountCollected * 20) / 100;

  • However, looking at this more carefully, the actual dangerous pattern appears when the multiplication order is wrong. The real risk in PuppyRaffle is the interaction between integer truncation and totalFees being cast to uint64:

// @> totalFees is uint64 — can overflow for large raffles
// @> fee calculation loses precision due to integer division behavior
totalFees = totalFees + uint64(fee);

The uint64 cast on fee silently truncates any value above type(uint64).max (~18.4 ETH), meaning large raffles silently lose fee data.

Risk

Likelihood:

  • This executes on every single raffle completion, the precision loss is guaranteed and repeatable.

  • No special condition or attacker is required it happens through normal protocol usage.

  • The uint64 overflow risk increases as the protocol scales to larger raffles.

Impact:

  • Protocol consistently collects fewer fees than intended revenue loss on every round.

  • For large raffles where fee > type(uint64).max, totalFees overflows silently and the owner withdraws far less than owed.

  • The withdrawFees() balance check address(this).balance == uint256(totalFees) becomes permanently broken if totalFees overflows, as the actual balance will be much larger than the stored totalFees

Proof of Concept

// Demonstration of precision loss:
// totalAmountCollected = 3 ether (3 players at 1 ether each)
// Intended fee (20%): 3e18 * 20 / 100 = 6e17 (0.6 ETH)
// Actual calculation: (3e18 * 20) / 100 = 6e17 — same here
​
// But with rounding edge case:
// totalAmountCollected = 1000000000000000003 wei
// Intended 20%: 200000000000000000 wei (rounded)
// Actual: (1000000000000000003 * 20) / 100 = 200000000000000000 — truncated
​
// uint64 overflow demonstration:
// If fee = 20e18 (20 ETH from a large raffle)
// uint64 max = 18446744073709551615 (~18.4 ETH)
// uint64(20e18) silently overflows — totalFees stores wrong value
// withdrawFees() balance check fails permanently
​
function testFeeOverflow() public {
// Enter raffle with enough players to generate > 18.4 ETH in fees
uint256 playersNeeded = 100; // 100 * 1 ETH = 100 ETH total, 20 ETH fee
address[] memory players = new address[](playersNeeded);
for (uint i = 0; i < playersNeeded; i++) {
players[i] = address(uint160(i + 1));
vm.deal(players[i], 1 ether);
}
puppyRaffle.enterRaffle{value: playersNeeded * entranceFee}(players);
vm.warp(block.timestamp + duration + 1);
puppyRaffle.selectWinner();
​
// totalFees has silently overflowed
// address(this).balance >> uint256(totalFees)
// withdrawFees() will revert on the equality check
vm.prank(owner);
vm.expectRevert();
puppyRaffle.withdrawFees();
}

Recommended Mitigation

Three Fixes are required:

Note: Using basis points (parts per 10,000) provides finer granularity and reduces rounding loss while keeping the split percentages accurate.

//Fix 1 Change totalFees from uint64 to uint256 to prevent overflow:
- uint64 public totalFees = 0;
+ uint256 public totalFees = 0;
//Fix 2 Remove the unsafe cast and ensure multiplication before division:
- totalFees = totalFees + uint64(fee);
+ totalFees = totalFees + fee;
//Fix 3 Use basis points for cleaner precision:
- uint256 prizePool = (totalAmountCollected * 80) / 100;
- uint256 fee = (totalAmountCollected * 20) / 100;
+ uint256 constant PRIZE_POOL_BPS = 8000; // 80%
+ uint256 constant FEE_BPS = 2000; // 20%
+ uint256 constant BPS_DENOMINATOR = 10000;
+ uint256 prizePool = (totalAmountCollected * PRIZE_POOL_BPS) / BPS_DENOMINATOR;
+ uint256 fee = (totalAmountCollected * FEE_BPS) / BPS_DENOMINATOR;
Updates

Lead Judging Commences

ai-first-flight-judge Lead Judge about 2 hours ago
Submission Judgement Published
Validated
Assigned finding tags:

[H-05] Typecasting from uint256 to uint64 in PuppyRaffle.selectWinner() May Lead to Overflow and Incorrect Fee Calculation

## Description ## Vulnerability Details The type conversion from uint256 to uint64 in the expression 'totalFees = totalFees + uint64(fee)' may potentially cause overflow problems if the 'fee' exceeds the maximum value that a uint64 can accommodate (2^64 - 1). ```javascript totalFees = totalFees + uint64(fee); ``` ## POC <details> <summary>Code</summary> ```javascript function testOverflow() public { uint256 initialBalance = address(puppyRaffle).balance; // This value is greater than the maximum value a uint64 can hold uint256 fee = 2**64; // Send ether to the contract (bool success, ) = address(puppyRaffle).call{value: fee}(""); assertTrue(success); uint256 finalBalance = address(puppyRaffle).balance; // Check if the contract's balance increased by the expected amount assertEq(finalBalance, initialBalance + fee); } ``` </details> In this test, assertTrue(success) checks if the ether was successfully sent to the contract, and assertEq(finalBalance, initialBalance + fee) checks if the contract's balance increased by the expected amount. If the balance didn't increase as expected, it could indicate an overflow. ## Impact This could consequently lead to inaccuracies in the computation of 'totalFees'. ## Recommendations To resolve this issue, you should change the data type of `totalFees` from `uint64` to `uint256`. This will prevent any potential overflow issues, as `uint256` can accommodate much larger numbers than `uint64`. Here's how you can do it: Change the declaration of `totalFees` from: ```javascript uint64 public totalFees = 0; ``` to: ```jasvascript uint256 public totalFees = 0; ``` And update the line where `totalFees` is updated from: ```diff - totalFees = totalFees + uint64(fee); + totalFees = totalFees + fee; ``` This way, you ensure that the data types are consistent and can handle the range of values that your contract may encounter.

Support

FAQs

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

Give us feedback!