uint64 truncation of totalFees silently loses fees and permanently locks withdrawFeestotalFees 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.
src/PuppyRaffle.sol:30:
src/PuppyRaffle.sol:133-134:
src/PuppyRaffle.sol:158:
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).
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.
test/PuppyRaffleAudit.t.sol::test_PoC_TotalFeesOverflowLocksFees
Run:
The output is in poc.txt: real fees 18600000000000000000, totalFees 153255926290448384.
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.
## 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.
The contest is live. Earn rewards by submitting a finding.
Submissions are being reviewed by our AI judge. Results will be available in a few minutes.
View all submissionsThe contest is complete and the rewards are being distributed.