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

Unchecked uint64 downcast makes `PuppyRaffle::totalFees` overflow and locks all collected fees

verdoolaegelouis

Root + Impact

Root cause: totalFees is a uint64 accumulated with an unchecked uint64(fee) downcast under Solidity 0.7.6, which has no built-in overflow protection.

Impact: once a round's fee exceeds ~18.45 ETH the recorded fee silently wraps, and because withdrawFees requires address(this).balance == totalFees, the real ETH left in the contract can never be withdrawn.

Description

  • totalFees is meant to track exactly the ETH the contract holds on behalf of feeAddress, so that withdrawFees can pay it out.

  • uint64 holds at most 18,446,744,073,709,551,615 wei, roughly 18.45 ETH. Two separate truncations occur, neither of which reverts on 0.7.6: the uint64(fee) cast drops the high bits of a fee larger than the type, and the + wraps once the running total exceeds it. The accounting and the actual balance then diverge permanently.

@> uint64 public totalFees = 0; // caps at ~18.45 ETH
...
uint256 totalAmountCollected = players.length * entranceFee;
uint256 fee = (totalAmountCollected * 20) / 100; // uint256, correct value
@> totalFees = totalFees + uint64(fee); // unchecked cast + unchecked add
...
function withdrawFees() external {
@> require(address(this).balance == uint256(totalFees), "PuppyRaffle: There are currently players active!");
(bool success,) = feeAddress.call{value: totalFees}("");

Risk

Likelihood:

  • Occurs as soon as a single round collects more than ~92 ETH, or as soon as accumulated fees across rounds pass ~18.45 ETH, since withdrawFees is not called automatically and fees pile up between rounds.

  • Needs no attacker at all. Ordinary use of the protocol with a realistic entranceFee reaches the ceiling.

Impact:

  • The fee accounting silently loses value. In the run below, 80 ETH of real fees are recorded as 6.21 ETH: 73.79 ETH vanish from the books.

  • withdrawFees compares the true balance against the wrapped counter, so the equality can never hold again. All fees are locked in the contract forever, and the feeAddress transfer would in any case send only the truncated amount.

Proof of Concept

entranceFee of 100 ether, four players. Real fee is 20% of 400 ether = 80 ether; the contract records 6.21 ether.

function test_totalFeesOverflows() public {
PuppyRaffle big = new PuppyRaffle(100 ether, feeAddress, 1 days);
​
address[] memory p = new address[](4);
for (uint256 i = 0; i < 4; i++) p[i] = address(uint160(2000 + i));
big.enterRaffle{value: 400 ether}(p);
​
vm.warp(block.timestamp + 2 days);
big.selectWinner();
​
uint256 expected = (400 ether * 20) / 100; // 80 ether
uint256 recorded = uint256(big.totalFees()); // ~6.21 ether
​
assertLt(recorded, expected); // fees truncated
assertEq(address(big).balance, expected); // yet the ETH is really there
}

80 ether is 8e19; 8e19 mod 2^64 is 6_213_023_705_161_793_536 wei, i.e. ~6.213 ether. Because the balance is 80 ether and totalFees is 6.21 ether, withdrawFees reverts permanently.

Control test, showing the accounting is exact below the ceiling — the divergence comes from the type, not the formula:

function test_feesAreExactBelowTheCeiling() public {
address[] memory p = new address[](4);
for (uint256 i = 0; i < 4; i++) p[i] = address(uint160(1000 + i));
puppyRaffle.enterRaffle{value: 4 ether}(p); // entranceFee = 1 ether
​
vm.warp(block.timestamp + 2 days);
puppyRaffle.selectWinner();
​
assertEq(uint256(puppyRaffle.totalFees()), (4 ether * 20) / 100);
}

Both tests pass on the audited commit with forge test.

Recommended Mitigation

Widen the accumulator to uint256 and drop the cast. The storage-packing comment no longer applies once the type changes, and on 0.7.6 a checked-math library should guard the addition:

- // We do some storage packing to save gas
address public feeAddress;
- uint64 public totalFees = 0;
+ uint256 public totalFees = 0;
- totalFees = totalFees + uint64(fee);
+ totalFees = totalFees + fee;

Separately, withdrawFees should not gate on an exact balance equality — see the companion finding on forced-ETH locking.

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!