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

Narrowing fee accounting to `uint64` can overflow and permanently lock protocol fees

samuraiigintokii

2. Narrowing fee accounting to uint64 can overflow and permanently lock protocol fees

Severity: High

Description

Normally, the protocol collects 20% of the total entrance fees as protocol revenue, which is added to a cumulative totalFees state variable to be withdrawn later by the fee address.

The issue occurs because the calculated fee is a uint256 value, but it is explicitly cast down to a uint64 when added to the totalFees state variable. Because the contract uses Solidity ^0.7.6, this cast will silently truncate if the fee exceeds type(uint64).max. Furthermore, withdrawFees() demands strict equality between address(this).balance and totalFees. Once truncation occurs, these values desync, and fees are locked forever.

function selectWinner() external {
// ...
uint256 totalAmountCollected = players.length * entranceFee;
uint256 prizePool = (totalAmountCollected * 80) / 100;
uint256 fee = (totalAmountCollected * 20) / 100;
// Root cause: explicit cast down to uint64 causes silent overflow in Solidity 0.7.6
//@> totalFees = totalFees + uint64(fee);
// ...
}

Risk

Likelihood: Medium

  • It requires approximately 93 players entering at 1 ether each in a single round for the fee (approx 18.4 ether) to exceed the uint64 limit (~18.44 ether in wei).

  • Popular raffles or malicious griefers could easily reach this threshold.

Impact: High

  • Once totalFees truncates, the withdrawFees() function will permanently revert due to the strict equality check require(address(this).balance == uint256(totalFees)).

  • All collected protocol revenue becomes permanently stuck in the contract.

Proof of Concept

function testH2_Uint64TotalFeesOverflow_BricksWithdraw() public {
// 1. 100 players enter, resulting in 100 ether collected
address[] memory players = new address[](100);
for (uint160 i = 0; i < 100; i++) {
players[i] = address(i + 1);
}
vm.deal(players[0], entranceFee * 100);
vm.prank(players[0]);
puppyRaffle.enterRaffle{value: entranceFee * 100}(players);
​
// 2. Raffle concludes
vm.warp(block.timestamp + duration + 1);
vm.roll(block.number + 1);
// 3. 20 ether fee exceeds uint64 max, silent truncation occurs
puppyRaffle.selectWinner();
​
// 4. totalFees is out of sync with actual balance, withdrawal reverts forever
vm.prank(playerOne); // Assuming playerOne is feeAddress
vm.expectRevert("PuppyRaffle: There are currently players active!");
puppyRaffle.withdrawFees();
}

Recommended Mitigation

Do not cast the fee to a narrower type. Update the totalFees state variable to be a uint256 to match the precision of the calculation and the ETH balance.

- uint64 public totalFees = 0;
+ uint256 public totalFees = 0;
​
function selectWinner() external {
// ...
uint256 fee = (totalAmountCollected * 20) / 100;
- totalFees = totalFees + uint64(fee);
+ totalFees = totalFees + fee;
// ...
}
Updates

Lead Judging Commences

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