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

Refund holes break raffle accounting and permanently brick payouts at `PuppyRaffle::refund`

kode_n_rolla

Description

  • Normal behavior: The raffle tracks participants in the players array and calculates payouts based on the total number of players. After the raffle duration ends, selectWinner() distributes the prize pool to the winner and accrues protocol fees, which can later be withdrawn by feeAddress via withdrawFees(). Players are allowed to exit early using refund(), receiving back their entrance fee.

  • Problem: The refund() function removes a player by overwriting their slot with address(0) instead of removing the entry or decrementing an active player count. As a result, players.length no longer reflects the number of active participants or the actual ETH balance held by the contract.

    Subsequent accounting in selectWinner() and withdrawFees() relies on players.length to compute the prize pool and fees. After one or more refunds, this mismatch leads to two critical outcomes:

    1. selectWinner() may attempt to transfer more ETH than the contract holds, causing the raffle to revert and become unfinalizable.

    2. Even when the winner payout succeeds, protocol fees become permanently locked because withdrawFees() enforces an invariant based on address(this).balance that can no longer be satisfied.

    Both outcomes result in permanent loss of protocol liveness or funds.

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); // creates a hole, length unchanged
​
emit RaffleRefunded(playerAddress);
}
...
function selectWinner() external {
...
@> uint256 totalAmountCollected = players.length * entranceFee; // handle players.length with address(0) holes
@> 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");
}
...
function withdrawFees() external {
@> require(address(this).balance == uint256(totalFees), "PuppyRaffle: There are currently players active!");
...
}

Risk

Likelihood:

  • Players are explicitly allowed to call refund() at any time before raffle finalization, which deterministically introduces holes into the players array and breaks the invariant between players.length and the contract’s ETH balance.

  • Both selectWinner() and withdrawFees() unconditionally rely on players.length and totalFees for accounting, making the faulty state reachable through normal, intended user behavior without any privileged access or timing assumptions.

Impact:

  • The raffle can become permanently unfinalizable when selectWinner() attempts to transfer a prize pool larger than the available balance, resulting in a persistent denial of service for all participants.

  • Protocol fees can become permanently locked, preventing feeAddress from ever withdrawing accrued fees and causing irreversible loss of funds.

Proof of Concept

The following tests demonstrate two distinct manifestations of the same accounting flaw introduced by refund holes in the players array.

The first test shows that when more than half of the participants refund their tickets, the contract still satisfies the minimum player count check (players.length >= 4), yet the actual ETH balance becomes insufficient to cover the prize pool. As a result, selectWinner() reverts and the raffle cannot be finalized.

The second test shows that even when the winner payout succeeds (because sufficient funds remain to cover the prize pool), protocol fees become permanently locked. After refunds, the contract balance no longer matches totalFees, causing withdrawFees() to revert indefinitely despite the raffle being completed.

function test_withdrawFees_bricked_afterRefundHoles() public {
// 1) Enter with 5 players
address playerFive = address(5);
address[] memory players = new address[](5);
players[0] = playerOne;
players[1] = playerTwo;
players[2] = playerThree;
players[3] = playerFour;
players[4] = playerFive;
​
uint256 depositPerFive = entranceFee * players.length;
​
vm.deal(playerOne, depositPerFive);
vm.prank(playerOne);
puppyRaffle.enterRaffle{value: depositPerFive}(players);
​
// 2) One player refunds -> contract balance decreases but players.length unchanged
uint256 idx = puppyRaffle.getActivePlayerIndex(playerOne);
vm.prank(playerOne);
puppyRaffle.refund(idx);
​
// balance now is 4e
assertEq(address(puppyRaffle).balance, entranceFee * 4);
​
// 3) Time passes and round is finalized
vm.warp(puppyRaffle.raffleStartTime() + duration + 1);
vm.prank(playerTwo);
puppyRaffle.selectWinner();
​
// After selectWinner():
// prizePool = 80% of (5e) = 4e, so contract balance becomes 0
assertEq(address(puppyRaffle).balance, 0);
​
// fee = 20% of (5e) = 1e, so totalFees is now entranceFee
assertEq(uint256(puppyRaffle.totalFees()), entranceFee);
​
// 4) Fees are bricked due to balance-based invariant
vm.expectRevert("PuppyRaffle: There are currently players active!");
vm.prank(feeAddress);
puppyRaffle.withdrawFees();
}
​
function test_selectWinner_reverts_afterRefundHoles_dueToInsufficientBalance() public {
// 1) Enter with 10 players
address[] memory players = new address[](10);
for (uint256 i = 0; i < 10; i++) {
players[i] = address(uint160(i + 1)); // 1..10
}
​
uint256 depositPerTen = entranceFee * players.length;
​
vm.deal(playerOne, depositPerTen);
vm.prank(playerOne);
puppyRaffle.enterRaffle{value: depositPerTen}(players);
​
// 2) Refund 6 players -> balance drops to 4e, but players.length stays 10
for (uint256 i = 0; i < 6; i++) {
uint256 idx = puppyRaffle.getActivePlayerIndex(players[i]);
vm.prank(players[i]);
puppyRaffle.refund(idx);
}
​
assertEq(address(puppyRaffle).balance, entranceFee * 4);
​
// expected prizePool computed from players.length = 10
uint256 expectedPrizePool = (entranceFee * 10) * 80 / 100;
assertEq(expectedPrizePool, entranceFee * 8);
​
// 3) Time passes
vm.warp(puppyRaffle.raffleStartTime() + duration + 1);
​
// 4) selectWinner must revert: tries to pay 8e with only 4e balance
vm.expectRevert("PuppyRaffle: Failed to send prize pool to winner");
vm.prank(players[7]); // any caller
puppyRaffle.selectWinner();
}

Recommended Mitigation

Do not represent refunded players as address(0) while still using players.length for payout and fee accounting. Instead, ensure that accounting is based on the number of active (non-refunded) players and the actual funds available for distribution.

A robust fix is to remove players from the array (swap-and-pop) on refund or maintain an activePlayersCount (and/or activePot) that is updated on enterRaffle() and refund(). Then compute prizePool and fee from these active values rather than players.length.

Additionally, avoid using address(this).balance == totalFees as a proxy for “no active players”, since forced ETH and accounting mismatches can break this invariant. Instead, gate withdrawFees() on an explicit active state (e.g., activePlayersCount == 0) and/or a finalized-round flag.

contract PuppyRaffle is ERC721, Ownable {
using Address for address payable;
​
uint256 public immutable entranceFee;
​
address[] public players;
+ mapping(address => uint256) public playerIndex; // 1-based index; 0 means not active
+ uint256 public activePlayersCount;
​
uint256 public raffleDuration;
uint256 public raffleStartTime;
address public previousWinner;
​
...
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(newPlayers[i] != address(0), "PuppyRaffle: Zero address");
+ require(playerIndex[newPlayers[i]] == 0, "PuppyRaffle: Duplicate player");
players.push(newPlayers[i]);
+ playerIndex[newPlayers[i]] = players.length; // 1-based
+ activePlayersCount += 1;
}
​
- // Check for duplicates
- for (uint256 i = 0; i < players.length - 1; i++) {
- for (uint256 j = i + 1; j < players.length; j++) {
- require(players[i] != players[j], "PuppyRaffle: Duplicate player");
- }
- }
emit RaffleEnter(newPlayers);
}
​
...
- function refund(uint256 playerIndex) public {
+- 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");
+ // Keep the original interface, but validate index belongs to msg.sender
+ require(playerIndex_ < players.length, "PuppyRaffle: Invalid index");
+ address playerAddress = players[playerIndex_];
+ require(playerAddress == msg.sender, "PuppyRaffle: Only the player can refund");
+ require(playerIndex[playerAddress] != 0, "PuppyRaffle: Player already refunded, or is not active");
​
- payable(msg.sender).sendValue(entranceFee);
-
- players[playerIndex] = address(0);
+ // Remove player via swap-and-pop to avoid holes
+ uint256 last = players.length - 1;
+ if (playerIndex_ != last) {
+ address moved = players[last];
+ players[playerIndex_] = moved;
+ playerIndex[moved] = playerIndex_ + 1; // keep 1-based
+ }
+ players.pop();
+ playerIndex[playerAddress] = 0;
+ activePlayersCount -= 1;
+
+ 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");
+ require(activePlayersCount >= 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;
+ // Use active player count for accounting
+ uint256 totalAmountCollected = activePlayersCount * entranceFee;
uint256 prizePool = (totalAmountCollected * 80) / 100;
uint256 fee = (totalAmountCollected * 20) / 100;
totalFees = totalFees + uint64(fee);
​
uint256 tokenId = totalSupply();
...
}
​
...
function withdrawFees() external {
- require(address(this).balance == uint256(totalFees), "PuppyRaffle: There are currently players active!");
+ // Explicitly gate on active state; do not infer it from balance
+ require(activePlayersCount == 0, "PuppyRaffle: There are currently players active!");
uint256 feesToWithdraw = totalFees;
totalFees = 0;
(bool success,) = feeAddress.call{value: feesToWithdraw}("");
require(success, "PuppyRaffle: Failed to withdraw fees");
}

The fix also aligns getActivePlayerIndex with internal accounting by leveraging a constant-time index mapping, removing ambiguity between index 0 and inactive players.

function getActivePlayerIndex(address player) external view returns (uint256) {
- for (uint256 i = 0; i < players.length; i++) {
- if (players[i] == player) {
- return i;
- }
- }
- return 0;
+ uint256 idx = playerIndex[player];
+ if (idx == 0) {
+ return 0; // not active
+ }
+ return idx - 1; // convert from 1-based to 0-based index
}
Updates

Lead Judging Commences

ai-first-flight-judge Lead Judge 15 days 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!