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

Prize Pool Doesn't Account for Refunded Players

dod4ufn

Root + Impact

The selectWinner function calculates the prize pool as players.length * entranceFee, but this doesn't account for players who refunded. If players refund, their entrance fee is not in the contract, but the calculation treats them as if they are, causing the protocol to pay out more than it has or revert if it doesn't have enough funds.

Description

  • The normal behavior is for the total prize pool to equal 80% of all ETH collected from players who haven't refunded.

  • The issue is that the calculation uses players.length which includes refunded players (now address(0) slots). The actual amount in the contract is less than calculated, leading to potential insufficient funds or incorrect payouts.

// Root cause in the codebase with @> marks to highlight the relevant section
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:

  • Every refund reduces the actual balance without reducing players.length, creating a mismatch

  • Even one refund causes the calculation to be incorrect

Impact:

  • Prize pool sent to winner exceeds actual collected funds

  • Protocol becomes insolvent if multiple refunds occur

  • Contract reverts when trying to send prize, preventing winner from claiming

  • Fees are calculated incorrectly as well

Proof of Concept

// Setup: 10 players enter, paying 1 ETH each = 10 ETH in contract
// 5 players refund = 5 ETH leaves, 5 ETH remains
// selectWinner called:
// totalAmountCollected = 10 * 1 = 10 ETH
// prizePool = (10 * 80) / 100 = 8 ETH
// fee = (10 * 20) / 100 = 2 ETH
// But contract only has 5 ETH!
// Sending 8 ETH to winner fails, raffle breaks

Recommended Mitigation

// Track actual funds collected
uint256 public totalAmountCollected = 0;
​
function enterRaffle(address[] memory newPlayers) public payable {
require(msg.value == entranceFee * newPlayers.length, "PuppyRaffle: Must send enough to enter raffle");
+ totalAmountCollected += msg.value;
for (uint256 i = 0; i < newPlayers.length; i++) {
players.push(newPlayers[i]);
}
// ... duplicate check ...
emit RaffleEnter(newPlayers);
}
​
function refund(uint256 playerIndex) public {
address playerAddress = players[playerIndex];
require(playerAddress == msg.sender, "PuppyRaffle: Only the player can refund");
require(playerAddress != address(0), "PuppyRaffle: Player already refunded, or is not active");
​
payable(msg.sender).sendValue(entranceFee);
+ totalAmountCollected -= entranceFee;
players[playerIndex] = address(0);
emit RaffleRefunded(playerAddress);
}
​
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);
// ... rest of function ...
+ totalAmountCollected = 0; // Reset for next raffle
}
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!