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

[M-1] uint64 Overflow on totalFees Permanently Locks Fees in Contract

lmsand

Root + Impact

  • Root: Unsafe uint64 Cast on totalFees in selectWinner()

  • Impact: withdrawFees() Becomes Permanently Uncallable, Locking All Accumulated Fees

Description

  • The contract accumulates fees in a uint64 variable called totalFees

  • Every time selectWinner() is called, the fee is cast from uint256 to uint64 before being added

  • uint64 has a maximum value of 18,446,744,073,709,551,615 — roughly 18.4 ETH worth of fees

  • In Solidity 0.7.x there is no automatic overflow protection, so when totalFees exceeds this max it silently wraps back to zero or a small number

  • withdrawFees() uses an exact equality check == to verify the contract balance matches totalFees

  • After overflow, the real ETH balance and totalFees will never match, making withdrawFees() permanently revert

  • This means all accumulated fees are locked in the contract forever with no recovery mechanism.

// @audit unsafe cast to uint64, will overflow with enough raffles
totalFees = totalFees + uint64(fee);
​
// @audit == check will never pass after overflow
require(address(this).balance == uint256(totalFees),
"PuppyRaffle: There are currently players active!");

Risk

Likelihood: Low

  • Requires significant raffle volume to accumulate 18.4 ETH in fees

Not something an attacker triggers intentionally — happens through normal protocol usage

  • With 4 players at 1 ETH each, overflow occurs after ~23 raffles

Impact: High

  • All accumulated fees are permanently locked with no rescue mechanism

feeAddress loses all fees forever

  • Contract would need to be fully redeployed

  • No way to recover funds without a new deployment

Proof of Concept

The following demonstrates that casting past the uint64 max silently wraps the value, and shows how many raffles are needed to trigger it:

function test_uint64Overflow() public {
// uint64 max = 18,446,744,073,709,551,615
uint64 max = type(uint64).max;
​
// Each raffle with 4 players at 1 ETH generates 0.8 ETH in fees
uint256 feePerRaffle = (entranceFee * 4 * 20) / 100;
​
// How many raffles until overflow?
uint256 rafflesToOverflow = uint256(max) / feePerRaffle;
console.log("Raffles needed to overflow:");
console.log(rafflesToOverflow);
​
// Show what happens when we cast a large fee to uint64
uint256 largeFee = uint256(max) + 1;
uint64 overflowedFee = uint64(largeFee);
​
// overflowedFee silently wraps to 0
assertEq(uint256(overflowedFee), 0);
}

Recommended Mitigation

Replace uint64 with uint256 for totalFees to match the precision of the fee calculation and eliminate overflow risk. Also replace the == check with >= in withdrawFees:

- uint64 public totalFees = 0;
+ uint256 public totalFees = 0;
​
- totalFees = totalFees + uint64(fee);
+ totalFees = totalFees + fee;
​
- require(address(this).balance == uint256(totalFees),
- "PuppyRaffle: There are currently players active!");
+ require(address(this).balance >= totalFees,
+ "PuppyRaffle: There are currently players active!");
Updates

Lead Judging Commences

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