First Flight #2: Puppy Raffle
First Flight #2
Beginner FriendlyFoundryNFT
100 EXP
View results
Previous Next
Submission Details
Severity: high
Valid

uint64 public totalFees causes withdrawFees() function to fail if fees go higher than 18,446,744,073,709,551,615 which is the maximum value of uint64 can hold

Dan

Summary

Use of a uint64 data type for the totalFees variable leads to withdrawFees() function failures when working with large values, such as 19.2e18 or higher as 18,446,744,073,709,551,615 is the maximum value of uint64 can hold. Changing the variable 'totalFees' data type to uint256 can handle much larger values without potential overflow or wraparound issues.
Once fees go even higher than 19.2e18 functions below will be affected as well:
selectWinner()

Vulnerability Details

Below withdrawFees() function will fail if fees are higher than 18,446,744,073,709,551,615 which is the maximum value of uint64 totalFees can hold, meaning fees will stay locked in the contract forever.

// We do some storage packing to save gas
address public feeAddress;
@> uint64 public totalFees = 0;
​
/// @notice this function will withdraw the fees to the feeAddress
function withdrawFees() external {
require(address(this).balance == uint256(totalFees), "PuppyRaffle: There are currently players active!");
@> uint256 feesToWithdraw = totalFees;
totalFees = 0;
(bool success,) = feeAddress.call{value: feesToWithdraw}("");
require(success, "PuppyRaffle: Failed to withdraw fees");
}

Once fees go even higher than 19.2e18 functions below functions will be affected as well:
selectWinner()

Impact

withdrawFees() function fails when tested in foundry
Foundry testing

  • using 4 players fees can be withdrawn from the contract when entranceFee is set to 23e18 as calculated fee will be below maximum value that uint64 can take, fee will be (23,000,000,000,000,000,000 * 4) * 20 / 100 = 18,400,000,000,000,000,000

@> entranceFee = 23e18;
​
modifier playersEntered() {
address[] memory players = new address[](4);
players[0] = playerOne;
players[1] = playerTwo;
players[2] = playerThree;
players[3] = playerFour;
puppyRaffle.enterRaffle{value: entranceFee * 4}(players);
_;
}
​
function testWithdrawFees() public playersEntered {
vm.warp(block.timestamp + duration + 1);
vm.roll(block.number + 1);
​
uint256 expectedPrizeAmount = ((entranceFee * 4) * 20) / 100;
​
puppyRaffle.selectWinner();
puppyRaffle.withdrawFees();
assertEq(address(feeAddress).balance, expectedPrizeAmount);
}
Running 1 test for test/PuppyRaffleTest.t.sol:PuppyRaffleTest
[PASS] testWithdrawFees() (gas: 317155)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 2.68ms

  • fees can't be withdrawn from the contract when entranceFee is set to 24e18, meaning withdrawFees() function fails as value of fees is higher than the maximum value of uint64 can hold, fee will be (24,000,000,000,000,000,000 * 4) * 20 / 100 = 19,200,000,000,000,000,000 which is higher than uint64 max take of 18,446,744,073,709,551,615

@> entranceFee = 24e18;
modifier playersEntered() {
address[] memory players = new address[](4);
players[0] = playerOne;
players[1] = playerTwo;
players[2] = playerThree;
players[3] = playerFour;
puppyRaffle.enterRaffle{value: entranceFee * 4}(players);
_;
}
​
function testWithdrawFees() public playersEntered {
vm.warp(block.timestamp + duration + 1);
vm.roll(block.number + 1);
​
uint256 expectedPrizeAmount = ((entranceFee * 4) * 20) / 100;
​
puppyRaffle.selectWinner();
puppyRaffle.withdrawFees();
assertEq(address(feeAddress).balance, expectedPrizeAmount);
}
Running 1 test for test/PuppyRaffleTest.t.sol:PuppyRaffleTest
[FAIL. Reason: Assertion failed.] testWithdrawFees() (gas: 326792)
Test result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 2.98ms

  • different test can be made by checking the puppyContract balance when winner is picked and the
    fees returned from the contract by using 23e18 or `24e18' as entrance fee:

function testWithdrawFees() public playersEntered {
vm.warp(block.timestamp + duration + 1);
vm.roll(block.number + 1);
​
uint256 expectedFeesAmount = ((entranceFee * 4) * 20) / 100;
​
puppyRaffle.selectWinner();
​
uint64 totalCurrentFeesAfterTheWinnerIsPicked = puppyRaffle.getFees();
​
console.log(address(puppyRaffle).balance);
console.log(totalCurrentFeesAfterTheWinnerIsPicked);
​
}

  • now the improper conversion directly effects how fees are calculated in selectWinner() function once the uint64 is overflown, meaning when a larger integer type is casted to a smaller one (e.g., uint256 to uint64), it truncates the higher bits, effectively taking only the least significant bits, this can lead to unexpected results when the source value is larger than the maximum value a uint64 can hold, which is 18,446,744,073,709,551,615
    -- in PuppyRaffle contract exactly that happens, if 4 players enter the raffle with entranceFee set to 24e18 the fees will be truncated and calculated incorrectly, this means when fee is converted to uint64 the fee becomes less than it should, example below:

@> entranceFee = 24e18;
​
function testFees() public playersEntered {
vm.warp(block.timestamp + duration + 1);
vm.roll(block.number + 1);
​
puppyRaffle.selectWinner();
​
uint256 totalAmountCollected = 4 * entranceFee;
uint256 currentFees = (totalAmountCollected * 20) / 100;
uint64 totalCurrentFeesConvertedToUint64 = uint64(currentFees);
uint64 feesReturnedFromTheContract = puppyRaffle.getFees();
​
console.log("Total amount collected: ", totalAmountCollected);
console.log("Current Fee in uint256: ", currentFees);
console.log("Fee converted in uint64 as per contract code: ", totalCurrentFeesConvertedToUint64);
console.log("Fee returned from the contract using getter function: ", feesReturnedFromTheContract);
}
Running 1 test for test/PuppyRaffleTest.t.sol:PuppyRaffleTest
[PASS] testFees() (gas: 289834)
Logs:
Total amount collected: 96000000000000000000
Current Fee in uint256: 19200000000000000000
Fee converted in uint64 as per contract code: 753255926290448384
Fee returned from the contract using getter function: 753255926290448384

  • mathematical calculation:

(24,000,000,000,000,000,000 * 4) * 20 / 100 = 19,200,000,000,000,000,000 -> this should be correct fee and not `753255926290448384`

Tools Used

  • foundry

Recommendations

  • code changes recommendations:

- uint64 public totalFees = 0;
+ uint256 public totalFees = 0;

  • by changing the above also consider to change below lines of code as there will be no need for conversion from uint64 to uint256:

- 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

Hamiltonite Lead Judge almost 2 years ago
Submission Judgement Published
Validated
Assigned finding tags:

unsafe cast of fee to uint64

overflow-uint64

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.