Puppy Raffle

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

uint64 truncation of totalFees silently loses fees and permanently locks withdrawFees

[H-04] uint64 truncation of totalFees silently loses fees and permanently locks withdrawFees

Summary

totalFees is a uint64. Each round's uint256 fee is narrowed with uint64(fee) and added without an overflow check, because the contract is compiled with Solidity 0.7.6. Once one round's fee, or the running total, passes 2^64 − 1 wei (about 18.45 ETH), the stored value wraps to a small number. The contract still holds the real fees, so withdrawFees' strict balance == totalFees check can never pass again.

Description

src/PuppyRaffle.sol:30:

uint64 public totalFees = 0;

src/PuppyRaffle.sol:133-134:

uint256 fee = (totalAmountCollected * 20) / 100;
totalFees = totalFees + uint64(fee); // explicit truncating cast + unchecked add (solc 0.7.6)

src/PuppyRaffle.sol:158:

require(address(this).balance == uint256(totalFees), "PuppyRaffle: There are currently players active!");

type(uint64).max = 18,446,744,073,709,551,615 wei. With the deployment's entranceFee of 1e18, a single 93-player round gives fee = 18.6e18. Then uint64(18.6e18) = 18.6e18 − 2^64 = 153,255,926,290,448,384 wei (about 0.153 ETH). The same wrap happens cumulatively, for example after 24 four-player rounds with no withdrawal (24 × 0.8 ETH = 19.2 ETH).

Risk

Likelihood: Medium — needs about 18.45 ETH of fees in one round or accumulated without withdrawal; realistic at the 1 ETH entrance fee.

Impact: High

All protocol fees become permanently unwithdrawable. The contract has no other way to send ETH to feeAddress, and the gap between the balance and totalFees can't be closed through normal flows. In the PoC, 18.6 ETH of fees is locked while totalFees reads 0.153 ETH.

Anyone can reach this, for example by entering enough addresses into one round, and it also happens organically as the protocol grows.

Proof of Concept

test/PuppyRaffleAudit.t.sol::test_PoC_TotalFeesOverflowLocksFees

function test_PoC_TotalFeesOverflowLocksFees() public {
uint256 n = 93;
_enter(1000, n);
vm.warp(block.timestamp + duration + 1);
puppyRaffle.selectWinner();
uint256 realFees = (n * entranceFee * 20) / 100; // 18.6 ETH
assertEq(address(puppyRaffle).balance, realFees);
assertEq(uint256(puppyRaffle.totalFees()), realFees - 2 ** 64); // 0.153 ETH
assertLt(uint256(puppyRaffle.totalFees()), realFees);
vm.expectRevert("PuppyRaffle: There are currently players active!");
puppyRaffle.withdrawFees();
}

Run:

forge test --match-test test_PoC_TotalFeesOverflowLocksFees -vvv

The output is in poc.txt: real fees 18600000000000000000, totalFees 153255926290448384.

Recommended Mitigation

Store totalFees as a uint256 and remove the cast. Also use checked arithmetic, either SafeMath on 0.7.x or a move to Solidity ≥0.8.

- uint64 public totalFees = 0;
+ uint256 public totalFees = 0;
...
- totalFees = totalFees + uint64(fee);
+ totalFees = totalFees + fee;
...
- 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 40 minutes 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!