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

selectWinner miscalculates prize pool and fees when players have refunded

fyuryy

PuppyRaffle::selectWinner miscalculates prize pool and fees when players have refunded

Description

The selectWinner function calculates totalAmountCollected as players.length * entranceFee. However, players.length includes slots that were zeroed out by refunded players who already withdrew their entrance fees. The contract no longer holds that ETH, but the prize pool and fee calculations still assume it does.

This causes the contract to attempt sending 80% of a larger amount than it actually holds, which either drains ETH belonging to protocol fees or causes the transaction to revert if the balance is insufficient.

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);
// ...
(bool success,) = winner.call{value: prizePool}("");
require(success, "PuppyRaffle: Failed to send prize pool to winner");
}

Risk

Likelihood:

  • Any raffle where at least one player refunds before selectWinner is called triggers this miscalculation

  • Refunding is a core feature of the contract, so this scenario is expected during normal usage

Impact:

  • The prize pool is inflated beyond the actual ETH available, causing the winner to receive fees that belong to the protocol -- or causing the entire selectWinner call to revert if the balance is too low

  • The fee variable is also inflated, recording phantom fees in totalFees that the contract cannot back, further compounding the integer overflow issue and breaking withdrawFees

Proof of Concept

  1. Five players enter the raffle, depositing 5 ETH total into the contract.

  2. One player refunds, withdrawing 1 ETH. The contract now holds 4 ETH, but players.length is still 5.

  3. Time elapses and selectWinner is called. It computes totalAmountCollected = 5 * 1 ETH = 5 ETH, prizePool = 4 ETH (80%), and fee = 1 ETH (20%).

  4. The contract only has 4 ETH, so sending 4 ETH as the prize pool leaves 0 ETH for the 1 ETH fee that was recorded in totalFees.

  5. The protocol believes it has 1 ETH in fees, but the contract balance is 0. withdrawFees will always revert.

function testMiscalculatedPrizePool() public {
// Five players enter the raffle
address[] memory players = new address[](5);
players[0] = playerOne;
players[1] = playerTwo;
players[2] = playerThree;
players[3] = playerFour;
players[4] = address(5);
puppyRaffle.enterRaffle{value: entranceFee * 5}(players);
// One player refunds -- contract now holds 4 ETH, but players.length is still 5
vm.prank(playerOne);
puppyRaffle.refund(0);
uint256 contractBalanceAfterRefund = address(puppyRaffle).balance;
assertEq(contractBalanceAfterRefund, entranceFee * 4);
vm.warp(block.timestamp + duration + 1);
vm.roll(block.number + 1);
// selectWinner calculates prizePool as 80% of 5 ETH = 4 ETH
// and fee as 20% of 5 ETH = 1 ETH
// But the contract only has 4 ETH total
puppyRaffle.selectWinner();
// After sending 4 ETH prize, the contract has 0 ETH left
// but totalFees records 1 ETH in phantom fees
assertEq(address(puppyRaffle).balance, 0);
assert(puppyRaffle.totalFees() > 0);
// withdrawFees will revert because there's no ETH to back the recorded fees
vm.expectRevert();
puppyRaffle.withdrawFees();
}

Recommended Mitigation

Track the actual number of active (non-refunded) players, or calculate based on the contract's real balance instead of array length:

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 totalAmountCollected = address(this).balance - uint256(totalFees);
uint256 prizePool = (totalAmountCollected * 80) / 100;
uint256 fee = (totalAmountCollected * 20) / 100;
totalFees = totalFees + uint64(fee);
// ...
}

Alternatively, maintain an activePlayerCount variable that is decremented on refund and use it for the calculation.

Updates

Lead Judging Commences

ai-first-flight-judge Lead Judge 1 day ago
Submission Judgement Published
Validated
Assigned finding tags:

[H-04] `PuppyRaffle::refund` replaces an index with address(0) which can cause the function `PuppyRaffle::selectWinner` to always revert

## Description `PuppyRaffle::refund` is supposed to refund a player and remove him from the current players. But instead, it replaces his index value with address(0) which is considered a valid value by solidity. This can cause a lot issues because the players array length is unchanged and address(0) is now considered a player. ## Vulnerability Details ```javascript players[playerIndex] = address(0); @> uint256 totalAmountCollected = players.length * entranceFee; (bool success,) = winner.call{value: prizePool}(""); require(success, "PuppyRaffle: Failed to send prize pool to winner"); _safeMint(winner, tokenId); ``` If a player refunds his position, the function `PuppyRaffle::selectWinner` will always revert. Because more than likely the following call will not work because the `prizePool` is based on a amount calculated by considering that that no player has refunded his position and exit the lottery. And it will try to send more tokens that what the contract has : ```javascript uint256 totalAmountCollected = players.length * entranceFee; uint256 prizePool = (totalAmountCollected * 80) / 100; (bool success,) = winner.call{value: prizePool}(""); require(success, "PuppyRaffle: Failed to send prize pool to winner"); ``` However, even if this calls passes for some reason (maby there are more native tokens that what the players have sent or because of the 80% ...). The call will thankfully still fail because of the following line is minting to the zero address is not allowed. ```javascript _safeMint(winner, tokenId); ``` ## Impact The lottery is stoped, any call to the function `PuppyRaffle::selectWinner`will revert. There is no actual loss of funds for users as they can always refund and get their tokens back. However, the protocol is shut down and will lose all it's customers. A core functionality is exposed. Impact is high ### Proof of concept To execute this test : forge test --mt testWinnerSelectionRevertsAfterExit -vvvv ```javascript function testWinnerSelectionRevertsAfterExit() public playersEntered { vm.warp(block.timestamp + duration + 1); vm.roll(block.number + 1); // There are four winners. Winner is last slot vm.prank(playerFour); puppyRaffle.refund(3); // reverts because out of Funds vm.expectRevert(); puppyRaffle.selectWinner(); vm.deal(address(puppyRaffle), 10 ether); vm.expectRevert("ERC721: mint to the zero address"); puppyRaffle.selectWinner(); } ``` ## Recommendations Delete the player index that has refunded. ```diff - players[playerIndex] = address(0); + players[playerIndex] = players[players.length - 1]; + players.pop() ```

Support

FAQs

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

Give us feedback!