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

Integer overflow in totalFees permanently results in fee loss

quangviet910

Root + Impact

Description

  • The selectWinner() function in PuppyRaffle.sol accumulates fees using totalFees = totalFees + uint64(fee) where totalFees is declared as uint64. Since Solidity ^0.7.6 lacks built-in overflow protection, when accumulated fees exceed type(uint64).max (18,446,744,073,709,551,615 wei ≈ 18.4 ETH), the value wraps around to a small number.

  • The withdrawFees() function requires address(this).balance == uint256(totalFees), which will never be true after overflow, permanently locking all fees in the contract.

function selectWinner() external {
require(block.timestamp >= raffleStartTime + raffleDuration, "PuppyRaffle: Raffle not over");
require(players.length >= 4, "PuppyRaffle: Need at least 4 players");
uint256 winnerIndex =
uint256(keccak256(abi.encodePacked(msg.sender, block.timestamp, block.difficulty))) % players.length;
address winner = players[winnerIndex];
uint256 totalAmountCollected = players.length * entranceFee;
uint256 prizePool = (totalAmountCollected * 80) / 100;
uint256 fee = (totalAmountCollected * 20) / 100;
totalFees = totalFees + uint64(fee); @> fee is casted from uint256 to uint64, totalFees is also uint64
​
uint256 tokenId = totalSupply();

Risk

Likelihood:

  • This occurs automatically after approximately 23 raffles with 4 players at 1 ETH entrance fee (0.8 ETH fees per raffle).

  • Any moderately successful raffle will inevitably hit this limit within weeks of deployment.

Impact:

  • All protocol fees become permanently locked and unrecoverable once overflow occurs.

  • The fee collector loses 100% of accumulated fees with no way to access them. In a popular raffle accumulating 20+ ETH in fees, this represents complete loss of protocol revenue with funds locked forever in the contract.

Proof of Concept

The test below demonstrates integer overflow after some raffles are done, making the totalFees after the raffle is less than before the raffle.

// SPDX-License-Identifier: MIT
pragma solidity ^0.7.6;
pragma experimental ABIEncoderV2;
​
import {Test, console} from "forge-std/Test.sol";
import {PuppyRaffle} from "../src/PuppyRaffle.sol";
​
contract PuppyRaffleTest is Test {
// existing code...
​
function testTotalFeeOverflow() public playersEntered {
// Demonstrate exact overflow behavior
uint64 maxUint64 = type(uint64).max;
​
// Simulate totalFees growing
uint64 totalFees = 0;
uint256 feePerRaffle = 0.8 ether;
uint256 raffleCount = 0;
​
// Keep adding until overflow
while (uint256(totalFees) + feePerRaffle <= maxUint64 && raffleCount < 100) {
totalFees += uint64(feePerRaffle);
raffleCount++;
}
​
console.log("Fees before overflow:", totalFees);
console.log("Raffles completed:", raffleCount);
​
// Add one more to trigger overflow
uint64 beforeOverflow = totalFees;
totalFees += uint64(feePerRaffle);
uint64 afterOverflow = totalFees;
​
console.log(" Expected totalFees:", uint256(beforeOverflow) + feePerRaffle);
console.log(" Actual totalFees (overflowed):", afterOverflow);
console.log(" Loss due to overflow:", (uint256(beforeOverflow) + feePerRaffle - afterOverflow) / 1 ether, "ETH");
​
assertTrue(afterOverflow < beforeOverflow, "Overflow should cause wraparound");
}
}
​

Recommended Mitigation

Use `uint256` for `totalFees` to match the precision of ETH calculations.

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

Lead Judging Commences

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