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

totalFees is uint64 accumulated without overflow checks (0.7.6) — once fees pass ~18.44 ETH the counter wraps, permanently bricking withdrawFees and stranding the fees

hopeh

Description

The contract compiles with Solc ^0.7.6, which performs no automatic overflow checking, and it accumulates protocol fees into a uint64:

uint64 public totalFees = 0;
...
function selectWinner() external {
...
uint256 fee = (totalAmountCollected * 20) / 100;
totalFees = totalFees + uint64(fee); // uint64 accumulation, no SafeMath, 0.7.6
...
}

type(uint64).max is 18_446_744_073_709_551_615 wei ≈ 18.446 ETH. Once the running total of collected fees exceeds that, totalFees + uint64(fee) silently wraps around modulo 2^64 to a small number instead of reverting. The accounting variable no longer reflects the ETH actually held for fees.

This is fatal because withdrawFees gates on an exact-equality check between the contract balance and totalFees:

function withdrawFees() external {
require(address(this).balance == uint256(totalFees), "PuppyRaffle: There are currently players active!");
...
}

After an overflow, totalFees is far smaller than the real fee ETH sitting in the contract, so address(this).balance == uint256(totalFees) can never hold, and withdrawFees reverts forever. The fees are permanently stranded — a direct loss to the protocol/fee recipient.

Given a realistic entranceFee and enough entrants across rounds (fee is 20% of each round's take), crossing 18.44 ETH of cumulative fees is entirely reachable over the life of the raffle.

Risk

Impact: High. Accrued protocol fees are miscounted and become permanently unwithdrawable once the uint64 accumulator overflows, losing real ETH. The fee-withdrawal path is bricked with no recovery in the contract.

Likelihood: High. Deterministic given sufficient cumulative volume; every round adds to totalFees, and the 18.44 ETH ceiling is modest for a raffle of any traction.

Proof of Concept

function test_totalFeesOverflows() public {
// Drive cumulative fees just over type(uint64).max across rounds.
// Each round: fee = players.length * entranceFee * 20 / 100 is added to totalFees (uint64).
// Run enough rounds so the sum of fees exceeds 18_446_744_073_709_551_615 wei.
​
_runRoundsUntilFeesNear(type(uint64).max); // helper: fills totalFees close to the cap
uint256 balBefore = address(puppyRaffle).balance;
uint64 feesBefore = puppyRaffle.totalFees();
​
_runOneMoreRound(); // pushes the uint64 sum past 2^64
​
// totalFees wrapped to a small value, but the ETH is still in the contract
assertLt(puppyRaffle.totalFees(), feesBefore);
assertGt(address(puppyRaffle).balance, uint256(puppyRaffle.totalFees()));
​
// withdrawFees can now never satisfy its equality check
vm.expectRevert("PuppyRaffle: There are currently players active!");
puppyRaffle.withdrawFees();
}

Expected: totalFees always equals the fee ETH held, and withdrawFees succeeds. Actual: the uint64 wraps, the equality check permanently fails, and the fees are locked.

Recommended Mitigation

Store fees in uint256 and use a compiler with checked arithmetic (upgrade to >=0.8.0, or use SafeMath on 0.7.x):

uint256 public totalFees = 0;
...
totalFees = totalFees + fee; // uint256, checked arithmetic — no wrap, no downcast

Widening totalFees to uint256 removes both the accumulator overflow and the need for the uint64(fee) downcast, and pairs with fixing the strict-equality check in withdrawFees (see the related finding).

Updates

Lead Judging Commences

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