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

[H-5] Incorrect Fee and Prize Calculation Using players.length After Refunds

0xaljzoli

[H-5] Incorrect Fee and Prize Calculation Using players.length After Refunds

Title: Incorrect Fee and Prize Calculation Using players.length After Refunds

Impact: High

Likelihood: Medium

Description

  • The selectWinner function calculates the total collected amount and distributes 80% to the winner and 20% as fees. This calculation should reflect the actual number of active, non-refunded players who have paid entrance fees.

  • The calculation multiplies players.length by entranceFee to determine the total amount collected. However, when a player calls refund, their slot is set to address(0) but remains in the array, meaning players.length does not decrease. This causes the contract to calculate fees and prize amounts based on the original array size rather than the actual number of active participants who still have funds in the contract.

function selectWinner() external {
// ...
- uint256 totalAmountCollected = players.length * entranceFee;
- // @> players.length includes refunded (address(0)) entries
- // @> The actual balance is less because refunded players got their ETH back
​
- uint256 prizePool = (totalAmountCollected * 80) / 100;
- uint256 fee = (totalAmountCollected * 20) / 100;
+ // @> Winner receives more ETH than they should
+ // @> Fee calculation overcounts, but only actual balance is sent
totalFees = totalFees + uint64(fee);

Risk

Likelihood:

  • Any player can call refund to withdraw their entrance fee, creating a gap between the actual contract balance and the calculated totalAmountCollected. Every time a player refunds, the discrepancy grows by one entranceFee.

  • The issue manifests every time selectWinner is called after one or more refunds have occurred. No special attacker action is needed beyond normal user behavior — any participant who requests a refund triggers the miscalculation.

Impact:

  • The prizePool value is calculated based on the inflated totalAmountCollected, meaning the winner may receive ETH that was never actually deposited by active players. This effectively redistributes funds from other legitimate participants or the fee pool to the winner unfairly.

  • The totalFees counter accumulates an inflated value based on players.length, which means withdrawFees may attempt to withdraw more fees than the contract actually holds. The withdrawFees function has a guard (require(address(this).balance == uint256(totalFees))), but because the balance is lower than totalFees, the owner will be permanently unable to withdraw accumulated fees.

Proof of Concept

function testIncorrectFeeCalculation() public {
// 4 players enter with 1 ETH each
address[] memory players = new address[](4);
players[0] = playerOne;
players[1] = playerTwo;
players[2] = playerThree;
players[3] = playerFour;
puppyRaffle.enterRaffle{value: entranceFee * 4}(players);
​
// Player 2 refunds — gets 1 ETH back
uint256 indexOfTwo = puppyRaffle.getActivePlayerIndex(playerTwo);
vm.prank(playerTwo);
puppyRaffle.refund(indexOfTwo);
​
// Contract now has 3 ETH but players.length is still 4
// selectWinner will calculate: totalAmountCollected = 4 * 1 ETH = 4 ETH
// prizePool = 4 ETH * 80% = 3.2 ETH
// But only 3 ETH is actually in the contract!
​
vm.warp(block.timestamp + duration + 1);
vm.roll(block.number + 1);
​
// This will either:
// 1. Revert because winner.call{value: 3.2 ETH} fails (only 3 ETH available)
// 2. Or send 3 ETH to winner and 0 to fees (incorrect distribution)
puppyRaffle.selectWinner();
​
// Owner tries to withdraw fees
// totalFees = totalFees + uint64((4 * 1e18 * 20) / 100) = 0.8 ETH
// But contract balance is 0 ETH (or less than expected)
// withdrawFees will revert: balance != totalFees
vm.expectRevert("PuppyRaffle: There are currently players active!");
puppyRaffle.withdrawFees();
}

This PoC demonstrates the cascading failure caused by the miscalculation. After one player refunds, the contract holds 3 ETH but players.length remains 4. When selectWinner calculates the prize pool as 80% of 4 ETH (3.2 ETH), it exceeds the actual contract balance of 3 ETH. The winner.call{value: prizePool} either reverts (blocking the entire raffle) or succeeds with a lower amount due to address(this).balance being insufficient. Meanwhile, totalFees is set to 0.8 ETH, but since withdrawFees requires address(this).balance == totalFees, the owner can never withdraw because the balances no longer match.

Recommended Mitigation

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;
+ // Use actual contract balance instead of players.length
+ // Since refunds have already been paid out, the remaining balance
+ // accurately reflects the amount to be distributed
+ uint256 totalAmountCollected = address(this).balance;
uint256 prizePool = (totalAmountCollected * 80) / 100;
uint256 fee = (totalAmountCollected * 20) / 100;
totalFees = totalFees + uint64(fee);

Using address(this).balance instead of players.length * entranceFee ensures the fee and prize calculations are based on the actual ETH held by the contract. After refunds are paid out, the remaining balance accurately represents the distributable funds. This also fixes the withdrawFees guard condition, since totalFees will now match the actual contract balance after a successful selectWinner call.

Updates

Lead Judging Commences

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