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

Integer Overflow in `totalFees` (uint64)

leoyeesir

Root + Impact

Description

  • The totalFees state variable is declared as uint64 (line 39), but the contract uses Solidity 0.7.6 which does not include automatic overflow/underflow protection. When accumulated fees exceed type(uint64).max (18,446,744,073,709,551,615 wei, approximately 18.4 ETH), the value wraps around to zero due to modulo 2^64 arithmetic. This occurs in the selectWinner function where fees are calculated and added:// Root cause in the codebase with @> marks to highlight the relevant section

totalFees = totalFees + uint64(fee); // Line 144

Risk

Likelihood:

  • Medium. While 18.44 ETH may seem large, it represents a relatively modest ceiling for a successful protocol:

    • The entranceFee is configurable by the admin and could be set to significant values for high-value NFT raffles

    • For any moderately successful protocol with regular raffle activity, accumulating 18.44 ETH in protocol fees is a realistic scenario over the protocol's lifetime

    • Once the threshold is crossed, the overflow occurs silently without reverting the transaction, making it difficult to detect until accounting discrepancies appear

Impact:

High. Once overflow occurs (as demonstrated in test output where 18446744073709551606 + 20 = 10):

  • Total Accounting Corruption: The totalFees counter wraps to a near-zero value, permanently losing track of all accumulated fees (~18.44 ETH effectively vanishes from accounting)

  • Withdrawal Dysfunction: The withdrawFees function relies on totalFees for amount calculations; the wrapped value causes withdrawal of incorrect amounts (potentially dust amounts instead of actual accrued fees)

  • Protocol Insolvency Risk: The contract may become unable to correctly track or distribute protocol revenue, potentially requiring emergency migration

Proof of Concept

Description of PoC: The test demonstrates the exact overflow behavior: when totalFees approaches type(uint64).max (18,446,744,073,709,551,615 wei), adding just 20 wei causes catastrophic wraparound to 10 wei. This proves that uint64 provides insufficient headroom for cumulative fee tracking in any non-trivial protocol.


function testTotalFeesOverflow() public {
uint64 maxUint64 = type(uint64).max;
uint64 currentFees = maxUint64 - 9; // Near overflow boundary
uint64 additionalFees = 20; // Triggers overflow
uint64 manualResult = currentFees + additionalFees; // Wraps to 10
uint256 properResult = uint256(currentFees) + uint256(additionalFees);
console.log("maxUint64:", uint256(maxUint64));
console.log("uint64 addition (overflow):", uint256(manualResult));
console.log("proper uint256 addition:", properResult);
// Verification: 18.44 ETH worth of fees becomes 10 wei
assertEq(uint256(manualResult), 10);
assertLt(uint256(manualResult), uint256(additionalFees)); // Overflow confirmed
}
maxUint64: 18446744073709551615 (~18.44 ETH max capacity)
currentFees: 18446744073709551606 (near limit)
additionalFees: 20
uint64 addition (overflow): 10 <-- Critical: 18.44 ETH becomes 10 wei
proper uint256 addition: 18446744073709551626
Overflow would cause lost protocol fees

Recommended Mitigation

Use uint256 (Recommended)
Change totalFees to uint256 to eliminate overflow concerns for any realistic fee accumulation:

uint256 public totalFees; // Line 39
// In selectWinner:
totalFees = totalFees + fee; // Line 144
Updates

Lead Judging Commences

ai-first-flight-judge Lead Judge 10 days 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!