Puppy Raffle

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

[H-03] `totalFees` uint64 overflow silently destroys protocol fee revenue above ~18.44 ETH

Description

totalFees is declared as uint64 (max ~18.44 ETH). When selectWinner() calculates the 20% fee and casts it to uint64 at line 134, any fee exceeding 18.44 ETH is silently truncated. Solidity 0.7.6 has no built-in overflow protection, so the cast wraps without reverting. The protocol permanently loses the truncated portion.

Vulnerability Details

// src/PuppyRaffle.sol, line 30
uint64 public totalFees = 0;
// src/PuppyRaffle.sol, line 134
totalFees = totalFees + uint64(fee);

Two overflow paths:

  1. Single-raffle truncation: 100 players at 1 ETH each produces fee = 20 ETH. uint64(20e18) = 20e18 mod 2^64 = 1,553,255,926,290,448,384 wei (~1.55 ETH). The protocol loses 18.45 ETH of fees in one raffle.

  2. Accumulation wrap: Even if individual fees stay below 18.44 ETH, repeated additions can wrap totalFees past type(uint64).max. After wrapping, totalFees is a small number, and withdrawFees() sends less than what was actually collected.

Both paths are silent. No revert, no event, no indication that fees were lost.

Risk

Likelihood:

  • A popular raffle with 100+ players at 1 ETH entrance fee triggers this on the very first selectWinner() call. Even with a lower entrance fee (0.1 ETH), 1000 players produces the same 20 ETH fee. The more successful the protocol, the more fees it loses.

Impact:

  • Permanent, irrecoverable loss of protocol revenue. The fee percentage is correctly applied to the prize pool (winner gets the right 80%), but the protocol's 20% share is silently destroyed by the truncating cast. The feeAddress can never recover the lost fees because totalFees understates the actual amount collected.

PoC

100 players enter at 1 ETH each. After selectWinner(), the expected 20 ETH fee is truncated to ~1.55 ETH.

function testExploit_Uint64Overflow() public {
uint256 numPlayers = 100;
address[] memory players = new address[](numPlayers);
for (uint256 i = 0; i < numPlayers; i++) {
players[i] = address(uint160(i + 10));
}
puppyRaffle.enterRaffle{value: entranceFee * numPlayers}(players);
vm.warp(block.timestamp + duration + 1);
vm.roll(block.number + 1);
puppyRaffle.selectWinner();
uint256 expectedFee = 20e18;
uint64 actualFees = puppyRaffle.totalFees();
uint64 truncatedFee = uint64(expectedFee);
// totalFees = ~1.55 ETH instead of 20 ETH
assertTrue(uint256(actualFees) < expectedFee, "Fees truncated by uint64 cast");
assertEq(uint256(actualFees), uint256(truncatedFee), "Matches truncated value");
}

Run: forge test --match-test testExploit_Uint64Overflow -vvvPASSES

Recommendations

Change totalFees from uint64 to uint256. The storage packing with feeAddress saves one slot but costs the protocol its entire fee revenue at scale.

- uint64 public totalFees = 0;
+ uint256 public totalFees = 0;
// In selectWinner():
- totalFees = totalFees + uint64(fee);
+ totalFees = totalFees + fee;

If storage packing is desired, use uint96 (max ~79 billion ETH) which still fits in one slot with address.

Updates

Lead Judging Commences

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