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

H-4: Refunded player slots allow address(0) to win, permanently DoS-ing selectWinner()

0xjjjjjj

Description

Severity: High

When players call refund(), their slot in the players array is set to address(0) at PuppyRaffle.sol:103:

players[playerIndex] = address(0);

The selectWinner() function at PuppyRaffle.sol:130-153 does not check whether the selected winner is address(0):

address winner = players[winnerIndex]; // can be address(0) if player refunded
(bool success,) = winner.call{value: prizePool}("");
require(success, "PuppyRaffle: Failed to send prize pool to winner");
_safeMint(winner, tokenId); // @audit reverts for address(0) — line 153

This has two impacts: (1) if winner.call{value: prizePool}("") succeeds, the prize ETH is sent to address(0) and permanently lost. (2) OpenZeppelin's _safeMint at line 153 includes require(to != address(0)), so the transaction reverts, causing a permanent DoS on selectWinner() and locking all remaining player funds.

Proof of Concept

function testZeroAddressWinnerReverts() public {
address[] memory entrants = new address[](4);
entrants[0] = playerOne;
entrants[1] = playerTwo;
entrants[2] = playerThree;
entrants[3] = playerFour;
puppyRaffle.enterRaffle{value: entranceFee * 4}(entrants);
​
// 3 of 4 players refund, leaving zero-address slots
vm.prank(playerOne);
puppyRaffle.refund(0);
vm.prank(playerTwo);
puppyRaffle.refund(1);
vm.prank(playerThree);
puppyRaffle.refund(2);
​
vm.warp(block.timestamp + duration + 1);
​
// selectWinner() reverts when it picks a zero-address slot
vm.expectRevert();
puppyRaffle.selectWinner();
}

With 3/4 slots zeroed, there is a 75% chance selectWinner() picks address(0) and reverts. Since selectWinner() is the only function that resets the players array, the raffle is permanently stuck.

Risk

  • Impact: High — selectWinner() becomes permanently uncallable (DoS), locking all remaining player funds with no recovery mechanism.

  • Likelihood: Medium — requires a portion of players to have refunded. With 50%+ refunds, the DoS becomes likely.

Recommended Mitigation

Skip address(0) slots when selecting a winner at PuppyRaffle.sol:130:

address winner = players[winnerIndex];
uint256 attempts = 0;
while (winner == address(0) && attempts < players.length) {
winnerIndex = (winnerIndex + 1) % players.length;
winner = players[winnerIndex];
attempts++;
}
require(winner != address(0), "PuppyRaffle: No active players");

A better long-term fix is to maintain a separate array of only active players, removing players on refund rather than leaving gaps.

Updates

Lead Judging Commences

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