Potential Integer Overflow in Fee Calculation
Root + Impact
Description
-
* The contract tracks accumulated fees in a `uint64` variable `totalFees`, but calculates fees as `uint256` in the `selectWinner()` function.
* When adding the calculated fee to `totalFees`, it's cast to `uint64`. If `totalFees` approaches the maximum value of `uint64` (18,446,744,073,709,551,615), adding more fees will cause an overflow, wrapping around to a smaller value.
```solidity:30:30:src/PuppyRaffle.sol
uint64 public totalFees = 0;
```
```solidity:131:134:src/PuppyRaffle.sol
uint256 totalAmountCollected = players.length * entranceFee;
uint256 prizePool = (totalAmountCollected * 80) / 100;
uint256 fee = (totalAmountCollected * 20) / 100;
totalFees = totalFees + uint64(fee);
```
Risk
Likelihood:
-
* This occurs when the contract has processed many raffles and accumulated fees approach the `uint64` maximum (~18.4 ETH at 1e18 wei per ETH)
* With high entrance fees or many raffles, this threshold can be reached in production
* The overflow will silently occur without reverting, corrupting the fee accounting
Impact:
-
* Fee accounting becomes incorrect after overflow
* `withdrawFees()` may attempt to withdraw incorrect amounts
* Owner may lose track of actual accumulated fees
* Potential for funds to become permanently locked if accounting is corrupted
Proof of Concept
Recommended Mitigation
Lead Judging Commences
[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.
Rewards breakdown
Live
The contest is live. Earn rewards by submitting a finding.
Judging
Submissions are being reviewed by our AI judge. Results will be available in a few minutes.
View all submissionsRewards Distribution
The contest is complete and the rewards are being distributed.