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

Integer Overflow in totalFees Causes Permanent Fund Locking

dparaste48

Root + Impact

The totalFees variable is declared as uint64, which has a maximum value of approximately 18.44 ETH. Fees accumulate across multiple raffles without bounds checking, causing an overflow after approximately 23 raffles. This makes fees permanently unwithdrawable.

Description

Describe the normal behavior in one or more sentences:

  • Fees should accumulate correctly across multiple raffles

  • The fee address should be able to withdraw accumulated fees when no raffle is active

Explain the specific issue or problem in one or more sentences:

  • The totalFees variable is uint64 which can only hold up to 18,446,744,073,709,551,615 wei (approximately 18.44 ETH)

  • Each raffle with 4 players at 1 ETH entrance fee generates 0.8 ETH in fees

  • After 23 raffles, totalFees overflows and wraps around to a small value

  • The withdrawFees() function checks address(this).balance == totalFees, which fails after overflow, permanently locking the fees

// @ src/PuppyRaffle.sol:30
uint64 public totalFees = 0; // uint64 max = 18.44 ETH
​
// @ src/PuppyRaffle.sol:134
totalFees = totalFees + uint64(fee); // Can overflow
​
// @ src/PuppyRaffle.sol:157-163
function withdrawFees() external {
require(address(this).balance == uint256(totalFees), "PuppyRaffle: There are currently players active!");
// If totalFees overflowed to 0.76 ETH but balance is 19.2 ETH, this fails
uint256 feesToWithdraw = totalFees;
totalFees = 0;
(bool success,) = feeAddress.call{value: feesToWithdraw}(\"\");
require(success, \"PuppyRaffle: Failed to withdraw fees\");
}

Risk

Likelihood:
Reason 1: Guaranteed to occur after approximately 23 raffles with standard parameters (4 players at 1 ETH)
Reason 2: Occurs faster with higher entrance fees or more players per raffle

Impact:
Impact 1: All accumulated fees become permanently locked in the contract
Impact 2: Fee recipient loses all earned revenue with no recovery mechanism

Proof of Concept

Place the following test in test/AuditTest.t.sol:

function testIntegerOverflow() public {
// Run 24 raffles to trigger overflow
for(uint i=0; i<24; i++) {
address[] memory currentPlayers = new address[](4);
currentPlayers[0] = playerOne;
currentPlayers[1] = playerTwo;
currentPlayers[2] = playerThree;
currentPlayers[3] = playerFour;
puppyRaffle.enterRaffle{value: entranceFee * 4}(currentPlayers);
vm.warp(block.timestamp + duration + 1);
vm.roll(block.number + 1);
puppyRaffle.selectWinner();
}
// totalFees has overflowed
// Expected: 24 * 0.8 ETH = 19.2 ETH
// Actual: wrapped to ~0.76 ETH
// withdrawFees will revert
vm.expectRevert("PuppyRaffle: There are currently players active!");
puppyRaffle.withdrawFees();
}

Run with: forge test --mt testIntegerOverflow -vv

The test passes, confirming that withdrawFees() reverts after overflow.

Recommended Mitigation

Use uint256 instead of uint64 for totalFees:

- 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 1 day 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!