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

Refunded players are never removed from `players`, corrupting prize/fee math and bricking `selectWinner()`

mohdhuzaifah0786

Root + Impact

Description

  • Normal behavior: After a player refunds, they should no longer count toward the raffle — the prize pool, fee, and winner selection should be based only on players who still have funds in the contract.

  • Specific issue: refund() sets the slot to address(0) but does not shrink players, so players.length still counts refunded entrants. selectWinner() then computes totalAmountCollected = players.length * entranceFee on the inflated count and can also pick a zeroed slot as the winner. The prize/fee are calculated on ETH that already left the contract, so the winner payout exceeds the real balance (reverting), or the winner resolves to address(0) (reverting on _safeMint) — leaving the raffle unable to conclude.

solidity
function refund(uint256 playerIndex) public {
...
@> players[playerIndex] = address(0); // slot zeroed but array length unchanged
}
...
function selectWinner() external {
...
@> uint256 winnerIndex = uint256(keccak256(...)) % players.length; // may index a refunded slot
@> uint256 totalAmountCollected = players.length * entranceFee; // counts refunded players
uint256 prizePool = (totalAmountCollected * 80) / 100;
...
@> (bool success,) = winner.call{value: prizePool}(""); // prizePool > real balance
require(success, "PuppyRaffle: Failed to send prize pool to winner");
}

Risk

Likelihood:

  • Reason 1: Occurs any time at least one player refunds before the draw — a normal, expected user action.

  • Reason 2: Occurs on the next selectWinner() call after such a refund, because the inflated players.length is used directly in the payout math.

Impact:

  • Impact 1: selectWinner() reverts (insufficient balance for the inflated prize, or a zero-address winner), so the raffle becomes un-settleable and funds are stuck.

  • Impact 2: When leftover fees from prior rounds do cover the overpayment, the inflated prize is drained from those fees and totalFees accounting is corrupted.

Proof of Concept

This PoC has 4 players enter (4 ETH), then playerFour refunds — dropping the balance to 3 ETH while players.length stays 4. When selectWinner() runs, it still computes a 3.2 ETH prize on the 4-player count, which exceeds the 3 ETH balance (and may target the zeroed slot), so the call reverts. The vm.expectRevert() confirms the draw is bricked by a single refund.
​
Add to test/PuppyRaffleTest.t.sol and run forge test --mt test_refund_bricks_draw -vvv.
​
solidity
function test_refund_bricks_draw() 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 idx = puppyRaffle.getActivePlayerIndex(playerFour);
vm.prank(playerFour);
puppyRaffle.refund(idx); // balance -> 3 ETH, length stays 4
assertEq(address(puppyRaffle).balance, entranceFee * 3);
​
vm.warp(block.timestamp + duration + 1);
vm.roll(block.number + 1);
​
vm.expectRevert(); // prize (3.2 ETH) > balance, or winner == address(0)
puppyRaffle.selectWinner();
}
​
The selectWinner() call reverts, proving the raffle can no longer settle once anyone has refunded.

Recommended Mitigation

Base all payout math on players who actually still hold funds, not on the raw array length. The cleanest fix is to remove refunded players from the array (swap-and-pop) so players.length always reflects active entrants; both the prize/fee calculation and winner selection then operate on real balances only. This prevents the overpayment, the zero-address winner, and the resulting DoS.
​
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");
​
+ // remove the player from the array so counts stay accurate
+ players[playerIndex] = players[players.length - 1];
+ players.pop();
emit RaffleRefunded(playerAddress);
payable(msg.sender).sendValue(entranceFee);
- players[playerIndex] = address(0);
- emit RaffleRefunded(playerAddress);
}
Updates

Lead Judging Commences

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