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

Refunded Players Are Still Counted in Prize Calculation Causing `selectWinner()` Denial of Service

nuribnuu

Description

The protocol documentation explicitly states that users are allowed to refund their ticket value through the refund() function and that the raffle should eventually be able to select a winner.

However, the implementation fails to correctly account for refunded players when calculating the prize pool during winner selection.

When a player requests a refund, the contract transfers the entrance fee back to the player and replaces the player's entry with address(0):

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);
​
players[playerIndex] = address(0);
emit RaffleRefunded(playerAddress);
}

Although the contract balance decreases after the refund, the refunded player remains included in the array length.

Later, during winner selection, the total amount collected is calculated using players.length:

uint256 totalAmountCollected = players.length * entranceFee;
uint256 prizePool = (totalAmountCollected * 80) / 100;

Since refunded players are still counted, the contract may attempt to distribute a prize pool larger than its actual balance.

As a result, the prize transfer fails and selectWinner() reverts:

(bool success,) = winner.call{value: prizePool}("");
require(success, "PuppyRaffle: Failed to send prize pool to winner");

This prevents the raffle from completing successfully.

Risk

Likelihood: High

Any legitimate participant can trigger this condition by entering the raffle and later calling refund().

No privileged permissions, special timing, or ownership access are required.

Impact: High

A refunded player reduces the contract balance while still being counted in the prize calculation.

Consequently, selectWinner() may attempt to transfer more ETH than the contract possesses, causing the transaction to revert.

This prevents:

  • Winner selection

  • Prize distribution

  • NFT minting

  • Normal raffle completion

As a result, the raffle can become unfinishable, causing a denial of service against the protocol's primary functionality.

Proof of Concept

The following test demonstrates that a single refund can cause selectWinner() to revert due to insufficient balance.

function testSelectWinnerRevertsAfterRefund() 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);
​
vm.prank(playerOne);
puppyRaffle.refund(0);
​
vm.warp(block.timestamp + duration + 1);
​
vm.expectRevert(
bytes("PuppyRaffle: Failed to send prize pool to winner")
);
​
puppyRaffle.selectWinner();
}

Run the test:

forge test --match-test testSelectWinnerRevertsAfterRefund -vvvv

Output (relevant section):

[PASS] testSelectWinnerRevertsAfterRefund()
​
PuppyRaffle::selectWinner()
├─ [OutOfFunds]
└─ ← [Revert] PuppyRaffle: Failed to send prize pool to winner

The test passes, confirming that a refunded player remains included in the prize calculation while the refunded ETH has already left the contract, causing winner selection to fail.

Recommended Mitigation

Track active participants separately from the raw array length and calculate prize amounts based only on active entries.

One possible approach is to remove refunded players from the participant set entirely rather than replacing them with address(0).

Alternatively, count only active players during winner selection:

- uint256 totalAmountCollected = players.length * entranceFee;
+ uint256 totalAmountCollected = activePlayerCount * entranceFee;

The accounting used for prize distribution should always reflect the actual funds held by the contract rather than the historical size of the players array.

Updates

Lead Judging Commences

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