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

Unsafe cast of PuppyRaffle::fee loses fees

maxwellwire17

Root + Impact

Description

  • The PuppyRaffle::selectWinner function should correctly handle the total number of participants when selecting a winner, ensuring that all valid entries in the players The array can participate in the winner selection process.

  • There is an unsafe downcast from uint256 to uint64 When assigning the totalPlayers value. If the number of players exceeds type(uint64).max (18,446,744,073,709,551,615), The value will be silently truncated, causing the contract to only consider a modulo of the actual player count for winner selection.

// Root cause in the codebase with @> marks to highlight the relevant section
function selectWinner() external {
require(block.timestamp >= raffleStartTime + raffleDuration, "PuppyRaffle: Raffle not over");
require(players.length > 0, "PuppyRaffle: No players in raffle");
​
uint256 winnerIndex = uint256(keccak256(abi.encodePacked(msg.sender, block.timestamp, block.difficulty))) % players.length;
address winner = players[winnerIndex];
uint256 fee = totalFees / 10;
uint256 winnings = address(this).balance - fee;
@> totalFees = totalFees + uint64(fee);
players = new address[](0);
emit RaffleWinner(winner, winnings);
}

Risk

Likelihood:

  • This vulnerability can only be triggered when the number of players exceeds type(uint64).max (approximately 18.4 quintillion players)

  • Achieving such a large number of players would require massive gas expenditure and block space, making it impractical on most blockchain networks under normal conditions

Impact:

  • If triggered, the truncation would cause the contract to only recognize the lowest 64 bits of the total player count, potentially allowing a single address to register multiple times and manipulate the winner selection logic

  • The actual winner index calculation would be based on an incorrect player count, potentially excluding legitimate players from winning and creating unfair distribution of the prize pool

Proof of Concept

  1. A raffle proceeds with a little more than 18 ETH worth of fees collected

  2. The line that casts the fee as a uint64 hits

  3. totalFees is incorrectly updated with a lower amount

You can replicate this in foundry's chisel by running the following:

uint256 max = type(uint64).max
uint256 fee = max + 1
uint64(fee)
// prints 0

Recommended Mitigation

Set PuppyRaffle::totalFees to a uint256 instead of a uint64, and remove the casting. Their is a comment which says:

// We do some storage packing to save gas

But the potential gas saved isn't worth it if we have to recast and this bug exists

- uint64 public totalFees = 0;
+ uint256 public totalFees = 0;
.
.
.
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);
+ totalFees = totalFees + fee;
Updates

Lead Judging Commences

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