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

`totalFees` Declared as `uint64` Causes Silent Truncation of Accumulated Fees — Protocol Revenue Lost

h3xu

totalFees Declared as uint64 Causes Silent Truncation of Accumulated Fees — Protocol Revenue Lost

Description

PuppyRaffle tracks accumulated rake fees in a uint64 public totalFees variable. In selectWinner(), the raffle's 20% fee share is calculated as a uint256 and then explicitly cast to uint64 before being added to totalFees. Because uint64 can hold at most ≈18.44 ETH (i.e., 2^64 - 1 = 18446744073709551615 wei), any round where the 20% fee exceeds this value silently wraps, causing the feeAddress to receive far less than the protocol is owed.

At entranceFee = 1 ETH and a round with 93 players: fee = (93 × 1e18 × 20) / 100 = 18.6e18. uint64(18.6e18) = 18.6e18 mod 2^64 ≈ 0.156e18 — the protocol loses 18.44 ETH per round silently.

uint64 public totalFees = 0; // @> only 64-bit capacity
​
function selectWinner() external {
// ...
uint256 totalAmountCollected = players.length * entranceFee;
uint256 fee = (totalAmountCollected * 20) / 100; // uint256
@> totalFees = totalFees + uint64(fee); // Silent truncation if fee > type(uint64).max
// ...
}

Risk

Likelihood: High

  • This triggers automatically during a normal raffle round with sufficient players — no attack required.

  • As raffle popularity grows or entranceFee increases, the threshold is easily crossed.

  • The contract does NOT emit an event or revert on truncation; the loss is invisible to the protocol owner.

Impact: High

  • The feeAddress (protocol's revenue address) receives a drastically reduced fee — potentially only a few wei — while honest players' fees remain locked in the contract (since totalFees is now incorrect).

  • This also causes the withdrawFees() invariant guard (balance == totalFees) to diverge further, potentially locking all fee withdrawal.

Severity: High

Proof of Concept

The following inline arithmetic demonstrates that in a round with 93 players at 1 ETH entrance fee, the 20% protocol fee (~18.6 ETH) exceeds type(uint64).max (18.44 ETH), causing the cast to silently truncate the value. A Foundry test asserting totalFees after selectWinner() would catch this mismatch. The protocol loses 18.44 ETH per round without any error or event.

// Demonstrate truncation in a unit test
// Enter 93 players at 1 ETH each → totalAmountCollected = 93 ETH
// fee = 93e18 * 20 / 100 = 18.6e18 (uint256)
// uint64(18.6e18) = 18.6e18 mod 2^64 = ~159374246e9 ≈ 0.000000159 ETH
// totalFees after one round = 0.000000159 ETH, not 18.6 ETH
// protocol has silently lost ~18.44 ETH in one round
​
uint256 fee = (93 * 1 ether * 20) / 100;
// fee = 18_600_000_000_000_000_000
// type(uint64).max = 18_446_744_073_709_551_615
// uint64(fee) = fee - type(uint64).max - 1 = 153_255_926_290_448_384 (~0.153 ETH)
assert(uint64(fee) != fee); // TRUE — truncation occurred

Recommended Mitigation

Upgrade totalFees from uint64 to uint256 to eliminate any possibility of truncation:

- uint64 public totalFees = 0;
+ uint256 public totalFees = 0;
​
function selectWinner() external {
// ...
uint256 fee = (totalAmountCollected * 20) / 100;
- totalFees = totalFees + uint64(fee);
+ totalFees = totalFees + fee;
// ...
}

The gas cost difference between uint64 and uint256 on modern EVM is negligible since both occupy a full 32-byte EVM word in storage.

Updates

Lead Judging Commences

ai-first-flight-judge Lead Judge about 5 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!