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

`PuppyRaffle::totalFees` might cause overflow and uses an unsafe cast, blocking the `PuppyRaffle::withdrawFees` function

smilenot

PuppyRaffle::totalFees might cause overflow and uses an unsafe cast, blocking the PuppyRaffle::withdrawFees function

Description: PuppyRaffle::totalFees is declared as a uint64 variable instead of a uint256. This causes two bugs:

  1. Unsafe cast: PuppyRaffle::fee is declared as a uint256 but when calculating PuppyRaffle::totalFees its casted to uint64 without any safety. If: fee > type(uint64).max (~18.44 ETH) the variable wraps modulo $2^{64}$ — the high bits are silently discarded, so totalFees ends up far below the real amount. For that to happen would be needed 93 players (if PuppyRaffle::entranceFee = 1 ETH): 93 players * 1 ETH * 20 / 100 = 18.6 ETH > type(uint64).max.

  2. Overflow: raffles whose individual fee fits in uint64 still may cause an overflow in PuppyRaffle::totalFees as its a uint64 declared variable. The accumulated sum in totalFees wraps modulo $2^{64}$ as well.

@> uint64 public totalFees = 0;
​
...
​
function selectWinner() external {
...
​
// @audit overflow & casting
@> totalFees = totalFees + uint64(fee);

Impact: As a consequence of this bug, uint64 variable and the uint64 forced cast might silently wrap modulo $2^{64}$ without any warning causing the loss of the real value of PuppyRaffle::totalFees. Both bugs leads to a block of PuppyRaffle::withdrawFees function, which uses PuppyRaffle::totalFees to send the fees and its require(address(this).balance == uint256(totalFees)), as this last require could not be passed the function stay locked forever.

Proof of Concept: In the following test we can see how the unsafe cast corrupts the real accounting of PuppyRaffle::fee reflected in PuppyRaffle::totalFees. Since the raffle was initialized from the start, total fees was 0 so it couldn't be the problem. PuppyRaffle::totalFees and its overflow bug is not tested in the PoC but also produces the same issue and its fixes are the same as the unsafe cast.

PoC

In the following test we clearly see how the real output — uint64 variable — overflows without any warning.

In this case we get as outputs:

The expected total fees are : 19000000000000000000
The real total fees are : 553255926290448384

The uint64 value wrapped: 19e18 was stored as 19e18 - $2^{64}$ 0.55e18.

Place the following test into PuppyRaffle.t.sol.

function test_UnsafeCast() public {
vm.warp(puppyRaffle.raffleStartTime() + puppyRaffle.raffleDuration());
​
uint256 numPlayers = 95;
address[] memory players = new address[](numPlayers);
for (uint256 i = 0; i < numPlayers; i++) {
players[i] = address(i + 1_000_000);
}
puppyRaffle.enterRaffle{value: entranceFee * numPlayers}(players);
​
// Manual calculation in uint256
uint256 expectedTotalAmountCollected = players.length * entranceFee;
uint256 expectedFee = (expectedTotalAmountCollected * 20) / 100;
uint256 expectedTotalFees = expectedFee;
​
assertEq(uint256(puppyRaffle.totalFees()), uint256(0), "total fees should be 0 before entering the raffle");
assertGt(expectedFee, type(uint64).max, "expected fee should be greater than the maximum value of uint64");
puppyRaffle.selectWinner();
​
// Real result in uint64
uint64 realTotalFees = puppyRaffle.totalFees();
​
console2.log("The expected total fees are : ", expectedTotalFees);
console2.log("The real total fees are : ", uint256(realTotalFees));
assertLt(uint256(realTotalFees), expectedTotalFees, "real total fees should be less than the expected value");
​
// Withdraw function results in a block
assertTrue(address(puppyRaffle).balance != uint256(realTotalFees), "puppyRaffle balance shouldn't be equal to the real total fees");
vm.expectRevert("PuppyRaffle: There are currently players active!");
puppyRaffle.withdrawFees();
}

Recommended Mitigation: There are a few recommendations:

  1. Change the PuppyRaffle::totalFees type variable from uint64 to uint256 and remove the forced cast. This solves the issue as the max of a uint256 variable is huge — 1.158e77 —, compared against the uint64 max — 1.845e19.

...
​
- uint64 public totalFees = 0;
+ uint256 public totalFees = 0;
​
...
​
function selectWinner() external {
...
​
- totalFees = totalFees + uint64(fee);
+ totalFees = totalFees + fee;

  1. Consider using the library SafeMath and SafeCast from OpenZeppelin. For compiler 0.7.6 use v3.4.0 from the contract library.

...
import {Address} from "@openzeppelin/contracts/utils/Address.sol";
+ import {SafeMath} from "@openzeppelin/contracts/math/SafeMath.sol";
+ import {SafeCast} from "@openzeppelin/contracts/utils/SafeCast.sol";
​
...
​
contract PuppyRaffle is ERC721, Ownable {
using Address for address payable;
+ using SafeMath for uint256;
+ using SafeCast for uint256;
​
...
​
- totalFees = totalFees + uint64(fee);
+ totalFees = uint256(totalFees).add(fee).toUint64();

Consider that ".toUint64()" will fire a revert which removes the unwarned truncation but does not fix the uint64 space issue. Knowing that, mitigation 2 is better only if the uint64 is strictly wanted, if not, use mitigation 1.

Updates

Lead Judging Commences

ai-first-flight-judge Lead Judge about 2 hours 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!