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

Refunded player tombstones permanently block new entries and settlement

rvoffindings

Root + Impact

Description

refund() treats a refunded entry as inactive by replacing it with address(0), but it does not shrink players. Every other consumer treats players.length and every slot as an active, funded entry. Two refunds therefore create duplicate zero-address tombstones that make every future enterRaffle() revert. The same stale length also makes selectWinner() calculate a prize from already-refunded tickets.

function refund(uint256 playerIndex) public {
address playerAddress = players[playerIndex];
require(playerAddress == msg.sender, "PuppyRaffle: Only the player can refund");
payable(msg.sender).sendValue(entranceFee);
// @> The inactive position remains in the active array.
players[playerIndex] = address(0);
}
​
// @> Two refunded positions compare as address(0) == address(0).
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");
}
}
​
// @> Refunded positions still inflate both the minimum and the prize.
uint256 totalAmountCollected = players.length * entranceFee;
uint256 prizePool = (totalAmountCollected * 80) / 100;

Risk

Likelihood: High

  • Any participant can refund its own entry, and an attacker can control two addresses.

  • Both attacker tickets are returned, so the net attack cost is only gas.

  • Once two tombstones exist, every new entry deterministically compares the duplicate zeros and reverts.

Impact: High

  • New entries are permanently blocked for the current round.

  • Settlement can become insolvent because the prize is computed from refunded positions, locking honest deposits with no cancellation or recovery function.

Proof of Concept

function testPoC_TwoRefundTombstonesBlockEntryAndWinnerSelection() public {
_enter(raffle, 6, 200, ENTRANCE_FEE);
address[] memory entrants = _players(6, 200);
​
vm.prank(entrants[4]);
raffle.refund(4);
vm.prank(entrants[5]);
raffle.refund(5);
​
address[] memory newcomer = new address[](1);
newcomer[0] = address(999);
vm.expectRevert("PuppyRaffle: Duplicate player");
raffle.enterRaffle{value: ENTRANCE_FEE}(newcomer);
​
vm.warp(block.timestamp + DURATION + 1);
vm.expectRevert();
raffle.selectWinner();
}

With six 1 ether entries and two refunds, the raffle holds 4 ether but tries to pay a 4.8 ether prize. The test passed against commit 08e5b1fc6939b8da7792b2d13e43000c519d8897.

Recommended Mitigation

Remove refunded entries from the active array before transferring ETH. A swap-and-pop keeps players.length equal to funded, eligible positions and also follows checks-effects-interactions.

function refund(uint256 playerIndex) public {
address playerAddress = players[playerIndex];
- payable(msg.sender).sendValue(entranceFee);
- players[playerIndex] = address(0);
+ uint256 lastIndex = players.length - 1;
+ if (playerIndex != lastIndex) {
+ players[playerIndex] = players[lastIndex];
+ }
+ players.pop();
+ payable(playerAddress).sendValue(entranceFee);
}

Also reject address(0) on entry and add invariants that every active slot is non-zero and funded.

Updates

Lead Judging Commences

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