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

Casting from `uint256` to `uint64` of `fee` may result in lost fees

bube

Summary

There is an unsafe cast in PuppyRaffle::selectWinner. The uint256 fee variable is casted to uint64. If the value of fee variable is greater than the max value that can be stored in uint64, the uint64 fee variable will become zero and totalFees will be incorrect calculated.

Vulnerability Details

In selectWinner() function there is a an unsafe cast of fee variable from uint256 to uint64. If the uint256 value is larger than the maximum value a uint64 can hold, the casting operation will result in an overflow error. This could lead to incorrect calculations of the totalFees and lost fees.

​
function selectWinner() external {
require(block.timestamp >= raffleStartTime + raffleDuration, "PuppyRaffle: Raffle not over");
require(players.length >= 4, "PuppyRaffle: Need at least 4 players");
uint256 winnerIndex =
uint256(keccak256(abi.encodePacked(msg.sender, block.timestamp, block.difficulty))) % players.length;
address winner = players[winnerIndex];
uint256 totalAmountCollected = players.length * entranceFee;
uint256 prizePool = (totalAmountCollected * 80) / 100;
@> uint256 fee = (totalAmountCollected * 20) / 100;
@> totalFees = totalFees + uint64(fee);
​
uint256 tokenId = totalSupply();
​
// We use a different RNG calculate from the winnerIndex to determine rarity
uint256 rarity = uint256(keccak256(abi.encodePacked(msg.sender, block.difficulty))) % 100;
if (rarity <= COMMON_RARITY) {
tokenIdToRarity[tokenId] = COMMON_RARITY;
} else if (rarity <= COMMON_RARITY + RARE_RARITY) {
tokenIdToRarity[tokenId] = RARE_RARITY;
} else {
tokenIdToRarity[tokenId] = LEGENDARY_RARITY;
}
​
delete players;
raffleStartTime = block.timestamp;
previousWinner = winner;
(bool success,) = winner.call{value: prizePool}("");
require(success, "PuppyRaffle: Failed to send prize pool to winner");
_safeMint(winner, tokenId);
}

Impact

If the fee exceeds the maximum value a uint64 can hold, the calculation of the totalFees will be incorrect. This could lead to the contract distributing less fees than it should, which could result in financial losses for the contract owner and the players.
Also, if the overflow issue is exploited by a malicious actor, he could potentially manipulate the balances of the contract and the players. For example, a malicious actor could enter the raffle with a large number of players, causing the fee to overflow and the contract to distribute less fees than it should. This could result in financial losses for the contract owner and the players.
The following test demonstrates the unsafe cast from uint256 to uint64 in the selectWinner() function. It shows that a uint256 value that exceeds the maximum value a uint64 can hold can be cast to a uint64 without any errors, but the result is an overflow, where the uint64 value is set to 0. Add the test function testUnsafeCastOverflowInSelectWinner() to the file PuppyRaffleTest.t.sol.

function testUnsafeCastOverflowInSelectWinner() public {
// Calculate a uint256 value that exceeds the maximum value a uint64 can hold
uint256 fee = 2**64;
​
// Try to cast the fee to a uint64
uint64 castFee = uint64(fee);
​
// Assert that the cast operation results in an overflow
assertTrue(castFee == 0);
}

Tools Used

VS Code, Foundry

Recommendations

To mitigate these risks, you can use the OpenZeppelin SafeCast library to safely cast between different types. This library provides functions that check for overflow and underflow conditions before performing the cast, preventing data loss and overflow errors.
Here's an example of how you could use the SafeCast library to safely cast a uint256 to a uint64:

import "@openzeppelin/contracts/utils/math/SafeCast.sol";
​
...
​
function selectWinner() external {
....
uint256 fee = (totalAmountCollected * 20) / 100;
// Use SafeCast to safely cast a uint256 to a uint64
+ totalFees = totalFees + SafeCast.toUint64(fee);
- totalFees = totalFees + uint64(fee);
​
....
}

This function will revert if the uint256 value is larger than the maximum value a uint64 can hold, preventing data loss and overflow errors.

Updates

Lead Judging Commences

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

unsafe cast of fee to uint64

Support

FAQs

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

Give us feedback!