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

Unchecked uint64 narrowing cast in totalFees silently wraps around, permanently bricking withdrawFees()

comphygxp

Root + Impact

Description

  • totalFees is declared as uint64, while fee is computed from totalAmountCollected as a uint256 (20% of collected entrance fees). The code adds it via an unchecked narrowing cast uint64(fee), which truncates silently (no revert) and never checks range. On solc ^0.7.6, plain arithmetic also has no built-in overflow protection.

  • type(uint64).max is only about 18.4467 ETH. Once the cumulative fee collected across rounds crosses that ceiling, totalFees silently wraps around modulo 2^64 - it no longer equals the ETH actually sitting in the contract. Because withdrawFees() requires address(this).balance == totalFees exactly, after the wrap that equality can never hold again through any normal contract operation, and the real fee ETH becomes permanently unreachable - there is no other function that can move it out.

  • This is not a single extreme-parameter edge case: ordinary, moderately-sized rounds run back-to-back (no interim withdrawFees() call, which nothing in the contract or docs requires) are enough to cross the threshold over time.

function selectWinner() external {
...
uint256 fee = (totalAmountCollected * 20) / 100;
@> totalFees = totalFees + uint64(fee);
...
}
​
function withdrawFees() external {
@> require(address(this).balance == uint256(totalFees), "PuppyRaffle: There are currently players active!");
...
}

Risk

Likelihood:

  • Reason 1 // No attacker or special parameters are needed - purely ordinary, repeated protocol usage (many normal-sized rounds without an interim fee withdrawal) triggers the wraparound over time.

  • Reason 2 // Nothing in the contract enforces a cap on cumulative fees or forces periodic withdrawal, so any moderately active deployment will eventually cross the uint64 ceiling.

Impact:

  • Impact 1 // Real ETH already collected as protocol fees becomes permanently, irrecoverably stuck in the contract - withdrawFees() reverts forever and there is no alternate rescue path.

  • Impact 2 // The on-chain accounting variable silently desyncs from the contract's real balance, corrupting the protocol's own bookkeeping with no error or event to signal it happened.

Proof of Concept

Ran with forge test --match-path "test/PoC_2.t.sol" -vv: [PASS] testCumulativeFeeTruncationBricksWithdrawFeesAcrossNormalRounds(). Logs: "Rounds run before wraparound: 5", true cumulative fee owed = 20 ETH, totalFees actually stored on-chain (wrapped) = 1.553255926290448384 ETH, real contract balance permanently stuck = 20 ETH. The test uses an ordinary 1 ETH entrance fee and 20 players/round (nothing extreme), runs 5 back-to-back rounds with no interim withdrawal, then shows withdrawFees() reverting even though the contract demonstrably holds the full 20 ETH it owes.

// SPDX-License-Identifier: MIT
pragma solidity ^0.7.6;
pragma experimental ABIEncoderV2;
​
import {Test, console} from "forge-std/Test.sol";
import {PuppyRaffle} from "../src/PuppyRaffle.sol";
​
contract PoC_2_FeeTruncation is Test {
PuppyRaffle raffle;
uint256 constant DURATION = 1 days;
uint256 constant ENTRANCE_FEE = 1 ether;
uint256 constant PLAYERS_PER_ROUND = 20;
address constant FEE_ADDRESS = address(0xFEE);
​
function setUp() public {
raffle = new PuppyRaffle(ENTRANCE_FEE, FEE_ADDRESS, DURATION);
}
​
function testCumulativeFeeTruncationBricksWithdrawFeesAcrossNormalRounds() public {
address[] memory roundPlayers = new address[](PLAYERS_PER_ROUND);
for (uint256 i = 0; i < PLAYERS_PER_ROUND; i++) {
address p = address(uint160(0xB0B0000 + i));
roundPlayers[i] = p;
vm.deal(p, 1000 ether);
}
​
uint256 feePerRound = (PLAYERS_PER_ROUND * ENTRANCE_FEE * 20) / 100;
assertLt(feePerRound, uint256(type(uint64).max), "sanity: single round fee is unremarkable");
​
uint256 trueCumulativeFee = 0;
uint256 roundsRun = 0;
​
while (trueCumulativeFee <= uint256(type(uint64).max)) {
for (uint256 i = 0; i < PLAYERS_PER_ROUND; i++) {
address[] memory single = new address[](1);
single[0] = roundPlayers[i];
vm.prank(roundPlayers[i]);
raffle.enterRaffle{value: ENTRANCE_FEE}(single);
}
​
vm.warp(block.timestamp + DURATION + 1);
raffle.selectWinner();
​
trueCumulativeFee += feePerRound;
roundsRun += 1;
}
​
uint256 actualBalance = address(raffle).balance;
assertEq(actualBalance, trueCumulativeFee, "contract balance must equal true cumulative fee");
assertGt(actualBalance, uint256(type(uint64).max), "sanity: real fee balance exceeds uint64 max");
​
uint256 storedTotalFees = uint256(raffle.totalFees());
assertEq(
storedTotalFees,
trueCumulativeFee % (uint256(type(uint64).max) + 1),
"totalFees must equal true cumulative fee truncated mod 2^64"
);
assertTrue(storedTotalFees != actualBalance, "totalFees no longer matches real balance");
​
vm.prank(FEE_ADDRESS);
vm.expectRevert("PuppyRaffle: There are currently players active!");
raffle.withdrawFees();
​
emit log_named_uint("Rounds run before wraparound", roundsRun);
emit log_named_uint("True cumulative fee owed to protocol (wei)", trueCumulativeFee);
emit log_named_uint("totalFees actually stored on-chain (wei, wrapped)", storedTotalFees);
emit log_named_uint("Real contract balance, permanently stuck (wei)", actualBalance);
}
}

Recommended Mitigation

- uint64 public totalFees = 0;
+ uint256 public totalFees = 0;
​
function selectWinner() external {
...
- totalFees = totalFees + uint64(fee);
+ totalFees = totalFees + fee;
...
}

Storing totalFees as uint256 removes the narrowing cast entirely (the storage-packing gas saving is not worth a permanent fund-lockup risk). If a smaller type must be kept for packing, explicitly require the running total stays within range before casting, and revert instead of silently truncating.

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!