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

Refunded players remain in the player array, causing winner selection to overpay and revert

cryptobloc

Refunded players remain in the player array, causing winner selection to overpay and revert

Affected Contract

src/PuppyRaffle.sol

Affected Code

refund leaves a blank player slot, while selectWinner calculates the prize and fee from players.length.

players[playerIndex] = address(0);
uint256 totalAmountCollected = players.length * entranceFee;
uint256 prizePool = (totalAmountCollected * 80) / 100;
uint256 fee = (totalAmountCollected * 20) / 100;

Description

Refunds reduce the contract's ETH balance but do not reduce players.length. After any player refunds, selectWinner still assumes that every historical array slot contributed entranceFee.

With four players and one refund, the contract holds only 3 ETH, but selectWinner calculates the prize pool from four players and attempts to pay 3.2 ETH. The payment fails and the raffle cannot complete.

This also leaves address(0) entries eligible as winner slots, which can create additional winner-selection failures.

Risk

The raffle can get stuck after ordinary user refunds because the contract no longer has enough ETH to satisfy the prize amount it calculates. Honest remaining players lose the expected winner-selection flow, the NFT is not minted to a valid winner, and protocol fees are not distributed through the normal round completion path.

Impact

Medium. A normal, intended refund can block the raffle from selecting a winner, minting the puppy NFT, and distributing fees. Remaining users may have to individually refund instead of the round concluding as designed.

Likelihood

High. Refunds are an advertised feature and no malicious behavior is required. Any refunded slot makes the accounting inconsistent.

Proof of Concept

Added test: test/AuditFindings.t.sol

Run:

forge test --match-test testRefundedPlayerBreaksWinnerSelectionAccounting -vvv

The test:

  1. Four players enter with 1 ETH each.

  2. One player refunds, reducing the contract balance to 3 ETH.

  3. The raffle duration passes.

  4. selectWinner computes the prize from players.length == 4 and attempts to send 3.2 ETH.

  5. The call reverts with PuppyRaffle: Failed to send prize pool to winner.

Recommended Mitigation

Track active player count and total active deposits separately from the historical array length. Use those values for winner eligibility, prize calculation, and fee calculation.

Also ensure selectWinner excludes address(0) slots when selecting a winner.

Example mitigation pattern:

uint256 public activePlayerCount;
uint256 public activeDeposits;
​
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++) {
players.push(newPlayers[i]);
}
​
activePlayerCount += newPlayers.length;
activeDeposits += msg.value;
}
​
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);
activePlayerCount -= 1;
activeDeposits -= entranceFee;
​
payable(msg.sender).sendValue(entranceFee);
emit RaffleRefunded(playerAddress);
}
​
function selectWinner() external {
require(activePlayerCount >= 4, "PuppyRaffle: Need at least 4 players");
​
uint256 totalAmountCollected = activeDeposits;
uint256 prizePool = (totalAmountCollected * 80) / 100;
uint256 fee = (totalAmountCollected * 20) / 100;
​
// Pick only from non-zero active player slots.
}

Add regression tests for selecting a winner after one or more players refund.

Updates

Lead Judging Commences

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