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

Refunded entries remain in players, corrupting winner selection and fee accounting

sonicthealpha

Root + Impact

Description

  • he protocol allows users to refund their ticket, which sets their slot in players to address(0) rather than removing it from the array.

  • Later, selectWinner() still uses players.length to choose a winner and to calculate totalAmountCollected.

  • As a result, refunded slots are treated as if they were still active paid entries even though their ETH has already left the contract.

  • This causes several downstream failures:

    • winner can be address(0), causing _safeMint(address(0), tokenId) to revert.

    • prizePool and fee can be computed from refunded entries, overstating the funds that should exist.

    • totalFees can accumulate fees for value that is no longer in the contract.

    • withdrawFees() can become permanently blocked because address(this).balance no longer equals totalFees.

function refund(uint256 playerIndex) public {
...
@> players[playerIndex] = address(0);
...
}
​
function selectWinner() external {
...
@> uint256 winnerIndex =
@> uint256(keccak256(abi.encodePacked(msg.sender, block.timestamp, block.difficulty))) % players.length;
@> address winner = players[winnerIndex];
@> uint256 totalAmountCollected = players.length * entranceFee;
uint256 prizePool = (totalAmountCollected * 80) / 100;
uint256 fee = (totalAmountCollected * 20) / 100;
totalFees = totalFees + uint64(fee);
...
}
​

Risk

Likelihood:

  • Any user can trigger this state simply by entering and then calling refund().

  • No privileged access or complex timing is required.

Impact:

  • Winner selection can fail unexpectedly.

  • Fee accounting can diverge from actual contract balance and lock funds.

  • The raffle can become stuck or economically inconsistent after normal user behavior.

Proof of Concept

// 1. Four players enter the raffle
// 2. One player calls refund(), leaving address(0) in players[]
// 3. After the raffle duration, selectWinner() still uses players.length == 4
// 4. If the random index points to the refunded slot, winner == address(0)
// 5. Even when another winner is chosen, totalAmountCollected assumes 4 paid players
// 6. Fees are accounted for on refunded capital, and withdrawFees() may later revert
​

Recommended Mitigation

More generally, maintain explicit active-player accounting and base payout math only on active paid entries.

-// leave holes in the array
-players[playerIndex] = address(0);
+// remove the refunded player from active accounting
+players[playerIndex] = players[players.length - 1];
+players.pop();
​
Updates

Lead Judging Commences

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