Puppy Raffle

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

Integer overflow / unsafe cast of `PuppyRaffle::totalFees` (uint64 in Solidity 0.7) corrupts fee accounting and can lock fees

Description

totalFees is a uint64 (max ~18.44 ETH) and the contract is compiled with Solidity
0.7.6, which has no built-in overflow checks. In selectWinner, fees are added as
totalFees = totalFees + uint64(fee), where fee is a uint256. Two problems:

  1. The cast uint64(fee) silently truncates any fee larger than type(uint64).max.

  2. The addition can silently overflow and wrap around.

Either way totalFees ends up far smaller than the fees actually held by the contract.

Root Cause

uint64 public totalFees = 0;
...
uint256 fee = (totalAmountCollected * 20) / 100;
@> totalFees = totalFees + uint64(fee); // unsafe cast + unchecked overflow (Solidity 0.7)

And the broken accounting then bricks withdrawals:

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

Because totalFees no longer matches the real ETH balance, this strict equality fails.

Risk

Likelihood: Medium — triggered whenever the collected fee for a round exceeds the
uint64 range (e.g. ~93+ players at a 1 ETH entrance fee), or accumulates past it over
several rounds. No attacker interaction is required; a popular raffle hits it.

Impact: High — fee accounting is corrupted: the protocol under-counts (loses) fees,
and withdrawFees can no longer satisfy its balance check, locking the fees in the
contract.

Proof of Concept

With 100 players the round fee is 20 ETH, which does not fit in uint64, so totalFees
is recorded as ~1.55 ETH instead of 20 ETH:

function test_totalFeesOverflow() public {
uint256 numPlayers = 100;
address[] memory players = new address[](numPlayers);
for (uint256 i = 0; i < numPlayers; i++) players[i] = address(uint160(i + 1));
puppyRaffle.enterRaffle{value: entranceFee * numPlayers}(players);
​
vm.warp(block.timestamp + duration + 1);
puppyRaffle.selectWinner();
​
uint256 expectedFees = (numPlayers * entranceFee * 20) / 100; // 20 ETH
uint256 actualFees = uint256(puppyRaffle.totalFees()); // ~1.55 ETH
assertLt(actualFees, expectedFees); // fees lost to overflow
}

Recommended Mitigation

Use a uint256 for totalFees, remove the unsafe cast, and (optionally) compile with
Solidity >= 0.8 for built-in overflow checks:

- uint64 public totalFees = 0;
+ uint256 public totalFees = 0;
...
- totalFees = totalFees + uint64(fee);
+ totalFees = totalFees + fee;

Also avoid the strict equality in withdrawFees (use >=) so a forced ETH transfer
cannot brick withdrawals.

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!