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

Using address(0) as the in-place refund sentinel makes two refunds collide in the duplicate check, permanently disabling enterRaffle and selectWinner

mrcanard

Using address(0) as the in-place refund sentinel lets two refunds permanently disable the raffle

Description

  • In a normal round, players call enterRaffle() to join and may later call refund() to leave; the duplicate check in enterRaffle() guarantees the same address is never entered twice.

  • refund() marks a departed player by overwriting their slot with address(0) instead of removing it. Since every refund writes that same value, two refunds leave two address(0) slots in players, and the duplicate check compares them as equal to each other — so it reverts as if a real duplicate existed.

// refund(): the departed player is marked with the sentinel address(0) instead of being removed
@> players[playerIndex] = address(0); // src/PuppyRaffle.sol:103
​
// enterRaffle(): every pair of slots is compared; two address(0) slots are equal to each other
for (uint256 i = 0; i < players.length - 1; i++) {
for (uint256 j = i + 1; j < players.length; j++) {
@> require(players[i] != players[j], "PuppyRaffle: Duplicate player"); // lines 86-88
}
}

Risk

Likelihood:

  • Refunding is permissionless, so players reaches two address(0) slots whenever any two different players refund in the same round.

  • A single attacker reaches the same state deliberately and for free by entering two of their own addresses and refunding both — the refund returns the full entrance fee, leaving only gas as a cost.

Impact:

  • Once two slots hold address(0), every later enterRaffle reverts on the duplicate check, so no new player can join for the rest of the round.

  • players is cleared only by delete players in selectWinner, which requires players.length >= 4. When the two refunds leave fewer than four valid players, selectWinner also reverts permanently and the array is never reset — the raffle can never again accept entrants or select a winner, and only a redeploy recovers it.

Proof of Concept

function testBrokenDuplicate() public {
uint256 playerIndex;
​
address[] memory players = new address[](2);
players[0] = playerOne;
players[1] = playerTwo;
puppyRaffle.enterRaffle{value: 2 * entranceFee}(players);
​
playerIndex = puppyRaffle.getActivePlayerIndex(playerOne);
vm.prank(playerOne);
puppyRaffle.refund(playerIndex);
​
playerIndex = puppyRaffle.getActivePlayerIndex(playerTwo);
vm.prank(playerTwo);
puppyRaffle.refund(playerIndex);
​
players = new address[](1);
players[0] = playerThree;
vm.expectRevert("PuppyRaffle: Duplicate player");
puppyRaffle.enterRaffle{value: entranceFee}(players);
}

After the two refunds players = [address(0), address(0)], and the third entry reverts on the duplicate check.

Recommended Mitigation

Skip refunded (zero) slots in the duplicate check so two empty slots no longer collide:

for (uint256 i = 0; i < players.length - 1; i++) {
for (uint256 j = i + 1; j < players.length; j++) {
- require(players[i] != players[j], "PuppyRaffle: Duplicate player");
+ if (players[i] != address(0)) {
+ require(players[i] != players[j], "PuppyRaffle: Duplicate player");
+ }
}
}

A sturdier fix removes the refunded player from the array (e.g. swap-and-pop) instead of leaving a zeroed hole, which also prevents related issues from blank slots (such as selectWinner selecting address(0) as the winner).

Updates

Lead Judging Commences

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