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

`totalFees` is stored as `uint64` and silently truncates, permanently bricking `withdrawFees()` and locking all protocol fees

junzai

Summary

Protocol fees are accumulated in a uint64 state variable. In Solidity 0.7.6, the explicit conversion uint64(fee) silently truncates (keeps the low 64 bits) and uint64 addition wraps without any overflow check (checked arithmetic only arrived in 0.8.0). Once cumulative fees pass 2^64 wei (≈ 18.45 ether), the recorded totalFees no longer equals the ether actually held, and withdrawFees() — which requires strict equality between the contract balance and totalFees — reverts forever. All collected protocol fees are locked with no other exit function.

Vulnerability Details

In src/PuppyRaffle.sol:

// line 29
uint64 public totalFees = 0;
​
// selectWinner(), lines 154–155
uint256 fee = (totalAmountCollected * 20) / 100;
totalFees = totalFees + uint64(fee); // silent truncation in 0.7.6
​
// withdrawFees(), line 180
require(address(this).balance == uint256(totalFees),
"PuppyRaffle: There are currently players active!");

Trigger paths:

  1. Single round: one round's fee exceeds ≈ 18.45 ether, i.e. round turnover above ≈ 92.2 ether (e.g. 93 players at a 1 ether entrance fee; enterRaffle places no cap on the number of players).

  2. Cumulative: totalFees is never reset except by a successful withdrawFees(), so many small rounds accumulate across the 2^64 boundary just as well if the fee address does not withdraw regularly.

  3. Relatedly, the strict-equality check has zero tolerance for balance drift: ether force-sent via selfdestruct also makes balance > totalFees and bricks withdrawals, with no recovery function.

Impact

The loser is the protocol / fee address: once triggered, all accumulated fees — the truncated difference plus every future fee — become permanently unwithdrawable, and each further round only widens the gap (no self-healing path). No attacker is required; normal operation (a large round, or long-term fee accumulation) reaches the boundary on its own.

Severity is assessed as Medium rather than High because the single-round path needs ≈ 92 ether of turnover, the cumulative path depends on operating cadence, and the bug locks fee withdrawal rather than destroying user principal.

Proof of Concept

Verified locally with forge test (entranceFee 25 ether × 4 players = 100 ether turnover, single-round fee = 20 ether):

function test_totalFeesTruncationBricksWithdrawFees() public {
uint256 bigFee = 25 ether;
PuppyRaffle raffle = new PuppyRaffle(bigFee, feeAddress, duration);
address[] memory players = new address[](4);
players[0] = playerOne; players[1] = playerTwo;
players[2] = playerThree; players[3] = playerFour;
vm.deal(address(this), 100 ether);
raffle.enterRaffle{value: 100 ether}(players);
​
vm.warp(block.timestamp + duration + 1);
raffle.selectWinner();
​
// True fee is 20 ether, but totalFees stored
// uint64(20 ether) = 20e18 mod 2^64 = 1553255926290448384 wei
assertEq(uint256(raffle.totalFees()), 1553255926290448384);
assertEq(address(raffle).balance, 20 ether);
vm.expectRevert(bytes("PuppyRaffle: There are currently players active!"));
raffle.withdrawFees();
}

Recommended Mitigation

Store fees in uint256 and drop the narrowing cast (totalFees = totalFees + fee;), and make withdrawFees() tolerant of drift — e.g. check balance >= totalFees and transfer exactly totalFees — so a balance mismatch can never permanently lock the fees.

Updates

Lead Judging Commences

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