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

Prize Pool Calculation Doesn't Account for Refunds

dparaste48

Root + Impact

The selectWinner() function calculates the prize pool based on players.length * entranceFee, but this calculation does not account for players who have refunded their entrance fees. This causes the transaction to revert when refunds have occurred, permanently locking the raffle.

Description

Describe the normal behavior in one or more sentences:

  • The winner should receive 80% of the actual funds collected in the raffle

  • The fee address should receive 20% of the actual funds collected

Explain the specific issue or problem in one or more sentences:

  • When a player refunds, the contract balance decreases but players.length remains unchanged (the refunded slot becomes address(0))

  • The prize pool calculation uses players.length * entranceFee which assumes all players are still active

  • When selectWinner() attempts to send the calculated prize pool, the transaction reverts due to insufficient funds in the contract

// @ src/PuppyRaffle.sol:125-154
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; // Wrong calculation
uint256 prizePool = (totalAmountCollected * 80) / 100;
uint256 fee = (totalAmountCollected * 20) / 100;
totalFees = totalFees + uint64(fee);
// ...
(bool success,) = winner.call{value: prizePool}(""); // Reverts - insufficient funds
require(success, "PuppyRaffle: Failed to send prize pool to winner");
_safeMint(winner, tokenId);
}

Example scenario:

  • 4 players enter at 1 ETH each: Contract has 4 ETH, players.length = 4

  • 1 player refunds: Contract has 3 ETH, players.length = 4 (still)

  • Prize pool calculated as: 4 * 1 ETH * 80% = 3.2 ETH

  • Contract only has 3 ETH, so the transaction reverts

Risk

Likelihood:
Reason 1: Occurs whenever any player refunds before winner selection
Reason 2: Very likely in real-world usage as players may change their mind or need funds back

Impact:
Impact 1: Raffle becomes permanently stuck with no way to select a winner
Impact 2: All remaining funds are locked in the contract with no recovery mechanism

Proof of Concept

Place the following test in test/AuditTest.t.sol:

function testPrizePoolRefundBug() public {
address[] memory players = new address[](4);
players[0] = playerOne;
players[1] = playerTwo;
players[2] = playerThree;
players[3] = playerFour;
puppyRaffle.enterRaffle{value: entranceFee * 4}(players);
uint256 balanceBefore = address(puppyRaffle).balance;
console.log("Balance before refund:", balanceBefore);
// Player 4 refunds
uint256 player4Index = puppyRaffle.getActivePlayerIndex(playerFour);
vm.prank(playerFour);
puppyRaffle.refund(player4Index);
uint256 balanceAfter = address(puppyRaffle).balance;
console.log("Balance after refund:", balanceAfter);
vm.warp(block.timestamp + duration + 1);
vm.roll(block.number + 1);
// selectWinner will revert
vm.expectRevert();
puppyRaffle.selectWinner();
console.log("BUG CONFIRMED: selectWinner reverts when refunds occur");
}

Run with: forge test --mt testPrizePoolRefundBug -vv

Expected output:

Balance before refund: 4000000000000000000
Balance after refund: 3000000000000000000
BUG CONFIRMED: selectWinner reverts when refunds occur

Recommended Mitigation

Track the actual amount collected separately from the players array:

+ uint256 public totalAmountCollected;
​
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]);
}
// ... rest of function
}
​
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");
​
players[playerIndex] = address(0);
+ totalAmountCollected -= entranceFee;
payable(msg.sender).sendValue(entranceFee);
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);
+ totalAmountCollected = 0;
// ... rest of function
}
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!