Summary
The uint64(fee) cast results in an overflow for significantly high PuppyRaffle::entranceFee
values or a large number of PuppyRaffle::newPlayers
. As a consequence, this results in an incorrect amount of PuppyRaffle::totalFee
to withdrawal.
Vulnerability Details
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();
...
}
Impact
The maximum value an uint64 can hold in Solidity is 2 **64 - 1 = 18446744073709551615
.
The fee is calculated as 20% of the total amount collected, which is the number of players times the entrance fee (in our test: 1 ETH = 1000000000000000000 wei).
To calculate the maximum number of players that can enter the raffle before an overflow occurs, we need to solve the following equation:
totalAmountCollected * 20 / 100 <= 18446744073709551615
Substituting totalAmountCollected with numberOfPlayers * entranceFee gives us:
numberOfPlayers * entranceFee * 20 / 100 <= 18446744073709551615
Solving for numberOfPlayers gives us:
numberOfPlayers <= (18446744073709551615 * 100) / (entranceFee * 20)
Substituting entranceFee with 1000000000000000000 (1 ETH in wei) gives us:
numberOfPlayers <= (18446744073709551615 * 100) / (1000000000000000000 * 20)
Solving this equation gives us the maximum number of players that can enter the raffle before an overflow occurs:
numberOfPlayers <= 92,2
.
uint256 entranceFee = 1e18;
function testEnter93PlayersAndCheckForOverflow() public {
address[] memory players = new address[](93);
for (uint256 i = 0; i < 93; i++) {
players[i] = address(i + 1);
}
puppyRaffle.enterRaffle{value: entranceFee * 93}(players);
for (uint256 i = 0; i < 93; i++) {
assertEq(puppyRaffle.players(i), players[i]);
}
uint256 totalFees = entranceFee * 93 * 20 / 100;
console.log("The total fees are:", totalFees);
uint64 totalFees64 = uint64(totalFees);
console.log("The total fees are:", totalFees64);
bool overflowOccurred = totalFees > 2 ** 64 - 1;
assertTrue(overflowOccurred, "Overflow should occur");
}
Tools Used
Manual Review
Recommendations
Defining the totalFee
variable as uint256
.
- 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;
uint256 tokenId = totalSupply();
..
}