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

selectWinner() accounts refunded slots and makes rounds insolvent

phantombugz

Root + Impact

Description

refund() zeroes a player's address but does not remove the array slot. selectWinner() later treats players.length as the number of funded entries and computes both the prize and protocol fee from that stale length.

After N entries and R refunds, the contract holds (N-R)entranceFee, but settlement accounts for NentranceFee. With four entries and one refund, the balance is 3 ETH while the winner payment is 3.2 ETH, so every settlement attempt reverts. With five entries and one refund, the 4 ETH prize can be paid, but totalFees records 1 ETH while no ETH remains, permanently breaking fee withdrawal.

function refund(uint256 playerIndex) public {
// ...
payable(msg.sender).sendValue(entranceFee);
// @> The refunded slot remains part of players.length.
players[playerIndex] = address(0);
}
​
function selectWinner() external {
// @> Uses historical slots, not funded active entries.
uint256 totalAmountCollected = players.length * entranceFee;
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");
}

Risk

Likelihood: High

  • Refunds are an intended public feature available to every participant.

  • Any refund creates the accounting mismatch; no special privilege or race is required.

Impact: Medium

  • Small rounds become unable to select a winner, locking active participants in the round.

  • Larger rounds may settle with an unbacked totalFees liability, permanently preventing fee withdrawal.

Proof of Concept

function testRefundMakesFourPlayerRaffleImpossibleToSettle() public {
address[] memory entrants = new address[](4);
entrants[0] = address(1);
entrants[1] = address(2);
entrants[2] = address(3);
entrants[3] = address(4);
raffle.enterRaffle{value: 4 ether}(entrants);
​
vm.prank(address(1));
raffle.refund(0);
​
assertEq(address(raffle).balance, 3 ether);
vm.warp(block.timestamp + 1 days + 1);
​
vm.expectRevert();
raffle.selectWinner(); // attempts to pay 3.2 ether from a 3 ether balance
}

The focused Foundry test passes.

Recommended Mitigation

Track active funded deposits directly and derive settlement amounts from that value, or remove refunded entries with swap-and-pop. Never derive custody liabilities from an array length that includes refunded tombstones.

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

Add tests covering one or more refunds before settlement and assert that the prize, fee, and remaining balance reconcile exactly.

Updates

Lead Judging Commences

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