Puppy Raffle
AI First Flight #1
Beginner FriendlyFoundrySolidityNFT
EXP
View results
Previous Next
Submission Details
Impact: high
Likelihood: high
Invalid

[H-3] Integer overflow of `PuppyRaffle::totalFees` loses fees

sum1t_here

[H-3] Integer overflow of PuppyRaffle::totalFees loses fees

Description: In Solidity versions prior to 0.8.0, integers were subject to integer overflows.

uint64 myVar = type(uint64).max;
// myVar will be 18446744073709551615
myVar = myVar + 1;
// myVar will be 0

Risk: In PuppyRaffle::selectWinner, totalFees are accumulated for the feeAddress to collect later in withdrawFees. However, if the totalFees variable overflows, the feeAddress may not collect the correct amount of fees, leaving fees permanently stuck in the contract.

Proof of Concept:

  1. We first conclude a raffle of 4 players to collect some fees.

  2. We then have 89 additional players enter a new raffle, and we conclude that raffle as well.

  3. totalFees will be:

totalFees = totalFees + uint64(fee);
// substituted
totalFees = 800000000000000000 + 17800000000000000000;
// due to overflow, the following is now the case
totalFees = 153255926290448384;

  1. You will now not be able to withdraw, due to this line in PuppyRaffle::withdrawFees:

require(address(this).balance == uint256(totalFees), "PuppyRaffle: There are currently players active!");

Although you could use selfdestruct to send ETH to this contract in order for the values to match and withdraw the fees, this is clearly not what the protocol is intended to do.

Proof Of Code

Place this into the PuppyRaffleTest.t.sol file.

function testTotalFeesOverflow() public playersEntered {
// We finish a raffle of 4 to collect some fees
vm.warp(block.timestamp + duration + 1);
vm.roll(block.number + 1);
puppyRaffle.selectWinner();
uint256 startingTotalFees = puppyRaffle.totalFees();
// startingTotalFees = 800000000000000000
​
// We then have 89 players enter a new raffle
uint256 playersNum = 89;
address[] memory players = new address[](playersNum);
for (uint256 i = 0; i < playersNum; i++) {
players[i] = address(i);
}
puppyRaffle.enterRaffle{value: entranceFee * playersNum}(players);
// We end the raffle
vm.warp(block.timestamp + duration + 1);
vm.roll(block.number + 1);
​
// And here is where the issue occurs
// We will now have fewer fees even though we just finished a second raffle
puppyRaffle.selectWinner();
​
uint256 endingTotalFees = puppyRaffle.totalFees();
console.log("ending total fees", endingTotalFees);
assert(endingTotalFees < startingTotalFees);
​
// We are also unable to withdraw any fees because of the require check
vm.prank(puppyRaffle.feeAddress());
vm.expectRevert("PuppyRaffle: There are currently players active!");
puppyRaffle.withdrawFees();
}

Recommended Mitigation:

There are a few recommended mitigations here.

  1. Use a newer version of Solidity that does not allow integer overflows by default.

- pragma solidity ^0.7.6;
+ pragma solidity ^0.8.18;

Alternatively, if you want to use an older version of Solidity, you can use a library like OpenZeppelin's SafeMath to prevent integer overflows.

  1. Use a uint256 instead of a uint64 for totalFees.

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

  1. Remove the balance check in PuppyRaffle::withdrawFees

- require(address(this).balance == uint256(totalFees), "PuppyRaffle: There are currently players active!");

We additionally want to bring your attention to another attack vector as a result of this line in a future finding.

Updates

Lead Judging Commences

ai-first-flight-judge Lead Judge 5 days ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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

Give us feedback!