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

Integer Overflow in totalFees Causes Loss of Protocol Revenue

dakthehack

Description

The PuppyRaffle::totalFees variable is declared as uint64, which can only hold values up to ~18.4 ETH. In the selectWinner function, fees are calculated as a uint256 but then unsafely cast to uint64. When a single raffle generates more than 18.4 ETH in fees (20% of pot), the cast causes an integer overflow, silently wrapping the value and losing the majority of fee revenue.

Expected behavior: All protocol fees should be accurately tracked in totalFees regardless of raffle size.

Actual behavior: When fees exceed uint64 max (~18.4 ETH), the value wraps around to a small number. For example, 20 ETH in fees gets stored as only 1.55 ETH, losing 18.45 ETH. This occurs in Solidity 0.7.6 which lacks automatic overflow protection.

uint64 public totalFees = 0; // @> Max value: ~18.4 ETH
​
function selectWinner() external {
// ...
uint256 totalAmountCollected = players.length * entranceFee;
uint256 fee = (totalAmountCollected * 20) / 100;
totalFees = totalFees + uint64(fee); // @> Unsafe cast causes overflow!
// ...
}

With 100 players at 1 ETH entrance fee:

  • Total pot: 100 ETH

  • Fee (20%): 20 ETH

  • uint64(20 ETH) overflows to 1.55 ETH

  • Lost revenue: 18.45 ETH per raffle

Risk

Likelihood:

  • Medium-High - Requires only 92+ players to trigger (92 * 1 ETH * 20% = 18.4 ETH)

  • Popular raffles will easily exceed this threshold

  • Silent failure - no revert, just lost funds

Impact:

  • Direct loss of protocol fee revenue

  • With 100 players: lose 18+ ETH per raffle

  • Owner cannot recover lost fees

  • Accumulates over multiple raffles

Proof of Concept

Add to test/PuppyRaffleTest.t.sol:

/**
* @notice Demonstrates uint64 overflow in totalFees causes loss of fee revenue
* @dev When fees exceed uint64.max (~18.4 ETH), the overflow causes silent loss of protocol revenue
*/
function test_totalFeesOverflow() public {
emit log("=== uint64 Overflow in totalFees ===");
emit log("");
emit log_named_uint("uint64 max value", type(uint64).max);
emit log_named_uint("uint64 max in ETH", type(uint64).max / 1 ether);
emit log("");
// Setup: Enter 100 players into the raffle
uint256 numPlayers = 100;
address[] memory players = new address[](numPlayers);
for (uint256 i = 0; i < numPlayers; i++) {
players[i] = address(uint160(i + 1));
}
uint256 totalEntranceFees = entranceFee * numPlayers;
vm.deal(address(1), totalEntranceFees);
vm.prank(address(1));
puppyRaffle.enterRaffle{value: totalEntranceFees}(players);
emit log("=== Raffle Setup ===");
emit log_named_uint("Players entered", numPlayers);
emit log_named_uint("Total pot", totalEntranceFees / 1 ether);
emit log_named_uint("Expected fees (20%)", (totalEntranceFees * 20 / 100) / 1 ether);
// Calculate what the fee SHOULD be
uint256 expectedFee = (totalEntranceFees * 20) / 100; // 20 ETH
emit log("");
emit log_named_uint("Expected fee (wei)", expectedFee);
// Advance time and select winner
vm.warp(block.timestamp + duration + 1);
vm.roll(block.number + 1);
puppyRaffle.selectWinner();
// Check what was actually stored
uint64 actualFeesStored = puppyRaffle.totalFees();
emit log("");
emit log("=== After selectWinner ===");
emit log_named_uint("Expected totalFees", expectedFee);
emit log_named_uint("Actual totalFees", actualFeesStored);
emit log_named_uint("Lost fees", expectedFee - actualFeesStored);
emit log_named_uint("Lost fees (ETH)", (expectedFee - actualFeesStored) / 1 ether);
emit log("");
emit log("=== Impact ===");
emit log("Protocol lost 18+ ETH in fees due to uint64 overflow!");
// The stored value should be much less due to overflow
assertLt(actualFeesStored, expectedFee, "Overflow occurred");
assertGt(expectedFee - actualFeesStored, 18 ether, "Lost more than 18 ETH");
}

Run: forge test --match-test test_totalFeesOverflow -vv --offline

Output:

=== uint64 Overflow in totalFees ===
​
uint64 max value: 18446744073709551615
uint64 max in ETH: 18
​
=== Raffle Setup ===
Players entered: 100
Total pot: 100
Expected fees (20%): 20
​
Expected fee (wei): 20000000000000000000
​
=== After selectWinner ===
Expected totalFees: 20000000000000000000
Actual totalFees: 1553255926290448384
Lost fees: 18446744073709551616
Lost fees (ETH): 18
​
=== Impact ===
Protocol lost 18+ ETH in fees due to uint64 overflow!

The protocol expected to collect 20 ETH in fees but only stored 1.55 ETH due to the overflow, losing 18.45 ETH.

Recommended Mitigation

Change totalFees from uint64 to uint256:

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

This eliminates the unsafe cast and allows totalFees to handle arbitrarily large values without overflow.

Updates

Lead Judging Commences

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