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

Prize pool calculation includes refunded players causing insufficient balance revert

alaaeldinsabib

Summary

The selectWinner function calculates the prize pool using players.length * entranceFee, which incorrectly includes refunded players whose slots remain in the array as address(0). This overestimates the actual contract balance, causing the prize transfer to fail when the calculated amount exceeds available funds, permanently bricking the raffle and locking all user deposits.

Description

When a player calls refund(), their ETH is returned but their address slot in the players array is only set to address(0) instead of being removed. The selectWinner function uses players.length to compute totalAmountCollected, assuming every array slot represents an active player.

After any refund, players.length remains unchanged while the actual contract balance decreases. This causes totalAmountCollected, prizePool, and fee to be calculated based on phantom players. When the contract attempts to send the inflated prize pool, the transaction reverts due to insufficient balance, leaving the remaining funds permanently trapped and the protocol in a broken state.

Root Cause

File: src/PuppyRaffle.sol (lines 131-134)

uint256 totalAmountCollected = players.length * entranceFee; // Includes refunded players
uint256 prizePool = (totalAmountCollected * 80) / 100;
uint256 fee = (totalAmountCollected * 20) / 100;
totalFees = totalFees + uint64(fee);

Risk

Severity: High
Likelihood: High
Impact: High

  • ❌ Prize pool calculation exceeds actual contract balance

  • ❌ Transaction reverts when attempting to send non-existent ETH

  • ❌ Remaining user funds permanently locked

  • ❌ Raffle protocol permanently bricked after any refund

  • ✅ Triggers immediately after the first refund

Proof of Concept

Scenario: 4 players enter (4 ETH total). 2 players refund their tickets. The contract still calculates the prize based on 4 players instead of 2, causing the payout to fail.

function test_PrizePoolCalculationIncludesRefundedPlayers() public {
// Step 1: 4 players enter (4 ETH total)
address[] memory players = new address[](4);
players[0] = address(1);
players[1] = address(2);
players[2] = address(3);
players[3] = address(4);
raffle.enterRaffle{value: entranceFee * 4}(players);
console.log("Initial balance:", address(raffle).balance); // 4 ETH
// Step 2: 2 players refund their tickets
vm.prank(address(3));
raffle.refund(2);
vm.prank(address(4));
raffle.refund(3);
console.log("Balance after refunds:", address(raffle).balance); // 2 ETH
vm.warp(block.timestamp + duration + 1);
// Step 3: Contract still calculates prize based on 4 players
uint256 expectedPrizePool = (4 * entranceFee * 80) / 100; // 3.2 ETH
uint256 actualBalance = address(raffle).balance; // 2 ETH
console.log("Calculated prize pool:", expectedPrizePool);
console.log("Actual available balance:", actualBalance);
// Step 4: selectWinner will revert because it tries to send 3.2 ETH but only 2 ETH exists
vm.expectRevert();
raffle.selectWinner();
console.log("VULNERABILITY: Prize calculation includes refunded players, transaction fails!");
}

Test Output:

Initial balance: 4000000000000000000
Balance after refunds: 2000000000000000000
Calculated prize pool: 3200000000000000000
Actual available balance: 2000000000000000000
VULNERABILITY: Prize calculation includes refunded players, transaction fails!

What This Proves:

  1. ✅ players.length doesn't decrease on refund

  2. ✅ Prize pool is calculated on phantom players

  3. ✅ Transaction reverts due to insufficient balance

  4. ✅ Protocol becomes permanently unusable

Recommended Mitigation

Track the number of active players separately, or calculate the prize pool based on actual contract balance:

// Option 1: Track active players count
uint256 public activePlayerCount;
​
function enterRaffle(address[] memory newPlayers) public payable {
require(msg.value == entranceFee * newPlayers.length, "PuppyRaffle: Must send enough to enter raffle");
for (uint256 i = 0; i < newPlayers.length; i++) {
require(players[i] == address(0) || i >= players.length, "Duplicate"); // Simplified duplicate check
players.push(newPlayers[i]);
activePlayerCount++; // ✅ Increment on entry
}
// ... duplicate validation ...
}
​
function refund(uint256 playerIndex) public {
// ... validation ...
players[playerIndex] = address(0);
activePlayerCount--; // ✅ Decrement on refund
payable(msg.sender).sendValue(entranceFee);
emit RaffleRefunded(playerAddress);
}
​
function selectWinner() external {
require(activePlayerCount >= 4, "PuppyRaffle: Need at least 4 active players");
uint256 totalAmountCollected = activePlayerCount * entranceFee; // ✅ Use active count
uint256 prizePool = (totalAmountCollected * 80) / 100;
uint256 fee = (totalAmountCollected * 20) / 100;
// ... rest of logic
}

Alternative (Simpler): Calculate based on actual balance:

uint256 actualBalance = address(this).balance;
uint256 prizePool = (actualBalance * 80) / 100;
uint256 fee = actualBalance - prizePool; // Fee gets the remainder

Why This Fixes It:

  1. ✅ Prize calculation matches actual available funds

  2. ✅ Prevents insufficient balance reverts

  3. ✅ Keeps protocol functional after refunds

  4. ✅ Ensures accurate fee distribution

References

  • CWE-682: Incorrect Calculation

  • SWC-131: Presence of Unused Variable

  • OpenZeppelin Pull Payment pattern

Updates

Lead Judging Commences

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