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

Refunded players break prize accounting and brick `selectWinner`

rubabbel

Root + Impact

Description

selectWinner sizes the pool from players.length * entranceFee, but refund
never removes the entry from players — it only overwrites the slot with
address(0). After any refund the contract holds less ETH than the computed pool,
so the 80% payout call fails on insufficient balance and selectWinner reverts
permanently. A refunded slot can also be chosen as winner == address(0), sending
ETH to the zero address and reverting on _safeMint. withdrawFees is similarly
bricked because its address(this).balance == totalFees invariant no longer holds.

function refund(uint256 playerIndex) public {
...
// @> Slot zeroed but array length unchanged -> still counted in the pool
players[playerIndex] = address(0);
}
​
function selectWinner() external {
...
// @> Uses players.length (includes refunded slots) instead of active/funded players
uint256 totalAmountCollected = players.length * entranceFee;
uint256 prizePool = (totalAmountCollected * 80) / 100;
uint256 fee = (totalAmountCollected * 20) / 100;
...
// @> winner may be address(0); payout can exceed actual balance -> revert / burn
(bool success,) = winner.call{value: prizePool}("");
require(success, "PuppyRaffle: Failed to send prize pool to winner");
_safeMint(winner, tokenId); // reverts if winner == address(0)
}

Risk

Likelihood: High — a single refund by any player, even incidental, triggers the overstatement. Refunds are a first-class, advertised feature.

Impact: High — permanent denial of service of the raffle draw and of fee withdrawal. Funds become stuck and winners can never be paid. A zero-address winner additionally burns the prize.

Proof of Concept

// test/Exploit.t.sol
function testRefundBricksRaffle() public {
_enterFour(); // 4 * entranceFee locked
​
// One player refunds: balance drops to 3e18, but players.length stays 4
vm.prank(playerOne);
puppyRaffle.refund(0);
​
vm.warp(block.timestamp + duration + 1);
vm.roll(block.number + 1);
​
// prizePool = 80% of 4e18 = 3.2e18, but balance = 3e18 -> transfer fails
vm.expectRevert("PuppyRaffle: Failed to send prize pool to winner");
puppyRaffle.selectWinner();
}

Result: [PASS] testRefundBricksRaffle() — a single refund makes selectWinner permanently revert.

Recommended Mitigation

Track active/funded players explicitly, compute the pool from them, remove refunded
entries (swap-and-pop), and reject address(0) winners. Also guard the payout
against the real balance and add the reentrancy guard from H-01.

address[] public players;
+ uint256 public activePlayerCount; // funded, non-refunded players
+ mapping(uint256 => uint256) private playerIndexPlusOne; // slot -> index+1 (for O(1) removal)
uint256 public raffleDuration;
@@
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++;
}
@@
function refund(uint256 playerIndex) public nonReentrant {
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);
+ // swap-and-pop so refunded slots are actually removed
+ uint256 last = players.length - 1;
+ players[playerIndex] = players[last];
+ players.pop();
+ activePlayerCount--;
+
+ payable(msg.sender).sendValue(entranceFee); // interaction last
emit RaffleRefunded(playerAddress);
}
@@
function selectWinner() external nonReentrant {
require(block.timestamp >= raffleStartTime + raffleDuration, "PuppyRaffle: Raffle not over");
- require(players.length >= 4, "PuppyRaffle: Need at least 4 players");
+ require(activePlayerCount >= 4, "PuppyRaffle: Need at least 4 active players");
uint256 winnerIndex = ...; // from secure RNG (H-02)
address winner = players[winnerIndex];
- uint256 totalAmountCollected = players.length * entranceFee;
+ require(winner != address(0), "PuppyRaffle: zero winner");
+ uint256 totalAmountCollected = activePlayerCount * entranceFee;
uint256 prizePool = (totalAmountCollected * 80) / 100;
uint256 fee = (totalAmountCollected * 20) / 100;
- totalFees = totalFees + uint64(fee);
+ totalFees = totalFees + fee; // see M-01 for uint256 type
...
- delete players;
+ delete players; // or clear activePlayerCount and compact
+ activePlayerCount = 0;
raffleStartTime = block.timestamp;
previousWinner = winner;
+ require(address(this).balance >= prizePool, "Insufficient balance");
(bool success,) = winner.call{value: prizePool}("");

Note: the swap-and-pop invalidates stored indices; persist playerIndexPlusOne
on entry if O(1) getActivePlayerIndex/refund-by-index is required.

Updates

Lead Judging Commences

ai-first-flight-judge Lead Judge 42 minutes 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!