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

Integer Overflow in totalFees (uint64) Causes Locked Funds and Broken Fee Withdrawal

nuribnuu

Description

The PuppyRaffle::selectWinner function accumulates fees by converting the calculated fee (a uint256) to uint64 before adding to totalFees. The totalFees variable is declared as uint64, but the actual fee amount can exceed 2^64 - 1 (approximately 1.84e19 wei) when many players enter the raffle. Solidity 0.7.6 performs silent truncation when casting a larger uint256 to a smaller type (uint64), causing an integer overflow. This results in totalFees storing an incorrect (much smaller) value, while the contract’s actual balance holds the full fee amount. Consequently, the withdrawFees function’s balance check (address(this).balance == uint256(totalFees)) will always fail, permanently locking the fee funds in the contract.

// Root cause in selectWinner() and withdrawFees()
contract PuppyRaffle {
// @> totalFees declared as uint64, maximum ~1.84e19 wei
uint64 public totalFees = 0;
​
function selectWinner() external {
// ...
uint256 fee = (totalAmountCollected * 20) / 100;
// @> Explicit cast from uint256 to uint64 – overflows silently
totalFees = totalFees + uint64(fee);
// ...
}
​
function withdrawFees() external {
// @> This equality will fail if overflow occurred
require(address(this).balance == uint256(totalFees), "PuppyRaffle: There are currently players active!");
// ...
}
}

Risk

Likelihood: Medium – occurs whenever the accumulated fee exceeds ~1.84e19 wei (≈18.4 ETH). With an entrance fee of 1 ETH and a 20% fee, this happens after ~92 players enter.

Impact: High – all fee funds (20% of total entrance fees) become permanently stuck in the contract. The owner cannot withdraw them, and there is no alternative mechanism to recover the funds. Legitimate fee income is lost forever.

Proof of Concept

The following test demonstrates the overflow and its consequences:

function testTotalFeesOverflow() public {
// 1. Create 100 players (fee will exceed uint64 max)
uint256 numPlayers = 100;
address[] memory players = new address[](numPlayers);
for (uint160 i = 0; i < numPlayers; i++) {
players[i] = address(i + 1);
}
​
// 2. All players enter the raffle
puppyRaffle.enterRaffle{value: entranceFee * numPlayers}(players);
​
// 3. Fast‑forward to end of raffle
vm.warp(block.timestamp + duration + 1);
vm.roll(block.number + 1);
​
// 4. Calculate expected fee (20% of total)
uint256 totalCollected = entranceFee * numPlayers;
uint256 expectedFee = (totalCollected * 20) / 100;
​
// 5. Select winner – overflow occurs here
puppyRaffle.selectWinner();
​
// 6. Get stored totalFees (overflowed value)
uint64 storedFees = puppyRaffle.totalFees();
console.log("Stored fees (uint64 after overflow):", storedFees);
console.log("Expected fee (without overflow) :", expectedFee);
​
// 7. Prove overflow: storedFees is much smaller than expectedFee
assertLt(storedFees, expectedFee);
​
// 8. withdrawFees() reverts because balance != storedFees
vm.expectRevert("PuppyRaffle: There are currently players active!");
puppyRaffle.withdrawFees();
}

Run the test:

forge test --match-test testTotalFeesOverflow -vvvv

Output (relevant section):

[PASS] testTotalFeesOverflow() (gas: 5421165)
Logs:
Stored fees (uint64 after overflow): 1553255926290448384 [1.553e18]
Expected fee (without overflow) : 20000000000000000000 [2e19]
Traces:
[529] PuppyRaffle::withdrawFees()
└─ ← [Revert] PuppyRaffle: There are currently players active!

The test passes, confirming that:

  • storedFees (1.55e18) is far less than expectedFee (2e19) – overflow proven.

  • withdrawFees() reverts, locking the 20 ETH fee in the contract.

Recommended Mitigation

Change totalFees to uint256 to avoid overflow entirely. Remove the unsafe cast and use standard uint256 arithmetic. Also update the withdrawFees function accordingly (no changes needed besides the type).

contract PuppyRaffle {
- uint64 public totalFees = 0;
+ uint256 public totalFees = 0;
​
function selectWinner() external {
// ...
uint256 fee = (totalAmountCollected * 20) / 100;
- totalFees = totalFees + uint64(fee);
+ totalFees = totalFees + fee;
// ...
}
​
function withdrawFees() external {
- require(address(this).balance == uint256(totalFees), "PuppyRaffle: There are currently players active!");
+ require(address(this).balance == totalFees, "PuppyRaffle: There are currently players active!");
// ...
}
}
Updates

Lead Judging Commences

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