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

Integer Overflow in totalFees Due to Unsafe uint64 Cast

quanghoa27061998

Root + Impact

Description

  • totalFees is declared as uint64 (max value: ~18.44 ETH) at PuppyRaffle.sol:30. In selectWinner() at line 134, the fee is computed as uint256 then unsafely cast to uint64:

uint64 public totalFees = 0; // @audit max ~18.44 ETH
​
// In selectWinner():
uint256 fee = (totalAmountCollected * 20) / 100;
totalFees = totalFees + uint64(fee); // @audit Unsafe downcast — truncates if fee > uint64 max

  • Solidity 0.7.6 has no built-in overflow checks for type casts. With entranceFee = 1 ETH and 93 players, fee = 18.6 ETH which exceeds uint64 max, causing silent truncation. This corrupts totalFees, and since withdrawFees() checks address(this).balance == uint256(totalFees), the corrupted value causes a permanent mismatch — fees are locked forever.

Risk

Likelihood:

  • Triggers naturally with 93+ players per round at 1 ETH entrance fee

  • Lower entrance fees require more players but the overflow is still inevitable at scale

  • uint64 max is only ~18.44 ETH — a very low threshold for any meaningful protocol

Impact:

  • Protocol fees permanently locked in contract, unrecoverable by owner

  • Fee accounting is silently corrupted — no revert, no warning

  • withdrawFees() permanently reverts due to balance mismatch

Proof of Concept

How the attack works:

  1. A raffle round accumulates 93+ players at entranceFee = 1 ETH, collecting 93 ETH total

  2. When selectWinner() executes, it computes fee = (93 ETH * 20) / 100 = 18.6 ETH

  3. The uint64(fee) cast silently truncates 18.6 ETH because it exceeds uint64 max (~18.44 ETH), corrupting totalFees to a garbage value

  4. withdrawFees() checks address(this).balance == uint256(totalFees) — the corrupted totalFees never matches the actual balance, permanently locking all fees in the contract

PoC code:

function testExploit_IntegerOverflow_TotalFees() public {
// 93 players -> fee = 18.6 ETH > uint64 max (18.44 ETH)
uint256 numPlayers = 93;
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 = (numPlayers * entranceFee * 20) / 100;
uint256 actualTotalFees = puppyRaffle.totalFees();
// totalFees overflowed - actual < expected
assertLt(actualTotalFees, expectedFee, "totalFees should have overflowed");
​
// withdrawFees() permanently reverts because balance != corrupted totalFees
vm.expectRevert("PuppyRaffle: There are currently players active!");
puppyRaffle.withdrawFees();
}
​
// forge test --match-test testExploit_IntegerOverflow_TotalFees -vvv
// Result: PASS — totalFees overflows, withdrawFees() permanently locked

Expected outcome: totalFees is silently corrupted by the unsafe uint64 downcast, and withdrawFees() permanently reverts because address(this).balance never equals the corrupted uint256(totalFees).

Recommended Mitigation

The root cause has two layers: (1) totalFees is declared as uint64, which overflows at ~18.44 ETH — a trivially reachable threshold for any meaningful protocol, and (2) Solidity 0.7.6 does not have built-in overflow checks for arithmetic or type casts, so the overflow is silent.

Primary fix — Widen totalFees to uint256 and use SafeMath:

// State variable change:
- uint64 public totalFees = 0;
+ uint256 public totalFees = 0;
​
// In selectWinner() — use SafeMath for all fee arithmetic:
using SafeMath for uint256;
​
function selectWinner() external {
// ... existing winner selection + RNG logic ...
​
uint256 totalAmountCollected = players.length * entranceFee;
uint256 prizePool = (totalAmountCollected * 80) / 100;
uint256 fee = (totalAmountCollected * 20) / 100;
​
- totalFees = totalFees + uint64(fee); // Unsafe: silent truncation
+ totalFees = totalFees.add(fee); // Safe: reverts on overflow
​
// ... prize distribution ...
}

Why this works:

  • uint256 max is ~1.15 * 10^77 wei — effectively impossible to overflow in any real-world scenario.

  • Removing the uint64() cast eliminates the silent truncation. SafeMath's .add() provides an explicit revert on overflow as an additional safeguard for Solidity <0.8.0.

  • The withdrawFees() balance check (address(this).balance == uint256(totalFees)) no longer suffers from the type mismatch between uint64 storage and uint256 comparison.

Upgrade path (recommended): Migrate to Solidity ^0.8.0 which has built-in overflow checks for all arithmetic operations, eliminating the need for SafeMath entirely. This also catches overflow in players.length * entranceFee (potential overflow if entranceFee and player count are both large).

Also fix withdrawFees() — the strict equality check == is separately vulnerable (see M-001). Replace with internal accounting:

function withdrawFees() external {
require(totalFees > 0, "PuppyRaffle: No fees to withdraw");
uint256 feesToWithdraw = totalFees;
totalFees = 0; // CEI: update state before external call
(bool success,) = feeAddress.call{value: feesToWithdraw}("");
require(success, "PuppyRaffle: Failed to withdraw fees");
}
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!