The intended behavior: totalFees should always equal the protocol fees actually held by the contract, so withdrawFees() can pay them out.
The bug: totalFees is a uint64 and is updated with an unchecked cast under Solidity 0.7.6 (no built-in overflow checks). type(uint64).max is only about 18.446 ETH. As soon as a round's fee (20% of the pot) exceeds that, uint64(fee) discards the high bits and totalFees records a wrong, much smaller value. Because withdrawFees() gates on an exact equality, it can then never be satisfied, locking the fees forever.
Likelihood: Medium. It triggers once cumulative or single-round fees exceed ~18.45 ETH, which is reachable with a large pot or repeated rounds.
Impact: High. Protocol fees are silently lost from the accounting and the ETH backing them is permanently locked, with no recovery path.
Run forge test --match-test test_H3_totalFees_overflow_locks_fees. A 100-ETH pot produces a real fee of 20 ETH. After selectWinner(), the contract holds 20 ETH but totalFees reads 1,553,255,926,290,448,384 wei (~1.553 ETH). The difference, 18,446,744,073,709,551,616 wei (exactly 2^64), is lost, and withdrawFees() reverts with "There are currently players active!".
Store totalFees as uint256 and remove the cast. Do not gate withdrawFees() on address(this).balance == totalFees; track owed fees explicitly and transfer that amount.
## 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.