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

`totalFees` Can Overflow Resulting in Lost of Fees

pacelli

Vulnerability Details

totalFees is declared as an uint64 to take advantage of storage packing, the maximum value for this type is 18_446_744_073_709_551_615 which is aproximately 18.45 ether, considering 20% of the entrance fee per player is collected and added to totalFees, this amount can be reached easily and make the value overflow.

The compiler version used by the developer in the raffle is 0.7.6, versions of the compiler below 0.8.0 does not protect against overflow, meaning that once totalFees surpasses the max value for its type it will reset to zero, resulting in a complete lost of the fees.

Proof of Concept

Assume 92 players have already played the raffle and the totalFees is 18 ether. A new round starts with five players and a new winner is selected, after this round totalFees overflows and the value is aprox. 0.95 ether. Over 17 ether has been lost.

Paste and execute the following code snippet in PuppyRaffleTest.t.sol to see the effect of the bug.

POC
modifier playersEnteredAndSelectWinner() {
address[] memory players = new address[](92);
​
for(uint256 i = 1; i < players.length; i++) {
players[i] = address(i);
}
​
vm.warp(block.timestamp + duration);
vm.roll(block.number + 1);
puppyRaffle.enterRaffle{value: entranceFee * players.length}(players);
puppyRaffle.selectWinner();
assertEq(uint256(puppyRaffle.totalFees()), 18.4 ether);
_;
}
​
function testTotalFeesOverflow() public playersEnteredAndSelectWinner {
​
// new round
address[] memory players = new address[](5);
​
for(uint256 i = 1; i < players.length; i++) {
players[i] = address(i);
}
​
vm.warp(block.timestamp + duration);
vm.roll(block.number + 1);
​
puppyRaffle.enterRaffle{value: entranceFee * players.length}(players);
puppyRaffle.selectWinner();
​
assertApproxEqRel(uint256(puppyRaffle.totalFees()), 0.95 ether, 0.04e18);
}

Impact

Lost of funds.

Tools Used

VS Code and Foundry.

Recommendations

  1. Apply SafeMath library from OpenZeppelin to protect math operations from underflow/overflow.

  2. Increment totalFees type to uint256.

  3. Reduce raffleDuration and raffleStartTime types to uint128 and declared as the top of the contract so they are packed together in the slot zero.

Updates

Lead Judging Commences

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

overflow-uint64

Support

FAQs

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