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

A single `PuppyRaffle::refund` permanently bricks `selectWinner` and locks every remaining stake

verdoolaegelouis

Root + Impact

Root cause: PuppyRaffle::refund blanks a slot but never shrinks players, while selectWinner computes the payout from players.length, which still counts the refunded tickets.

Impact: a single refund makes the contract try to pay out more ETH than it holds, so selectWinner reverts on every call and the raffle — together with all remaining stakes — is locked forever.

Description

  • selectWinner is meant to distribute exactly what the current round collected: 80% to the winner and 20% to the fee address.

  • refund sends the stake back and writes address(0) into the slot, deliberately leaving a hole (@dev This function will allow there to be blank spots in the array). players.length is unchanged, so totalAmountCollected keeps counting money that has already left the contract. The prize transfer then asks for more than the balance and the require reverts, permanently.

function refund(uint256 playerIndex) public {
...
@> players[playerIndex] = address(0); // hole, but players.length stays the same
}
​
function selectWinner() external {
...
@> uint256 totalAmountCollected = players.length * entranceFee; // counts refunded tickets
uint256 prizePool = (totalAmountCollected * 80) / 100;
uint256 fee = (totalAmountCollected * 20) / 100;
...
@> address winner = players[winnerIndex]; // may be address(0)
_safeMint(winner, tokenId); // _safeMint to address(0) reverts
...
@> (bool success,) = winner.call{value: prizePool}("");
require(success, "PuppyRaffle: Failed to send prize pool to winner");
}

Risk

Likelihood:

  • Occurs whenever any player refunds before the draw, which is a documented, intended feature of the protocol. No attacker and no privilege required — one honest refund is enough.

  • There is no code path that ever removes the hole or decrements the length, so the condition persists until the contract is redeployed.

Impact:

  • selectWinner reverts on every call: the prize pool computed from four stakes cannot be paid out of three. All remaining player funds are stuck in the contract.

  • A second, independent revert path exists: if winnerIndex lands on a blanked slot, winner is address(0) and _safeMint reverts first.

  • withdrawFees cannot rescue anything either, since it requires address(this).balance == totalFees and the balance still holds the players' stakes.

Proof of Concept

Four players enter, one refunds. The contract holds 3 ether but selectWinner tries to send a 3.2 ether prize pool:

function test_oneRefundBricksTheRaffle() public {
address[] memory p = new address[](4);
for (uint256 i = 0; i < 4; i++) p[i] = address(uint160(1000 + i));
puppyRaffle.enterRaffle{value: 4 ether}(p); // balance 4 ether
​
vm.prank(address(uint160(1000)));
puppyRaffle.refund(0); // balance 3 ether, length still 4
​
vm.warp(block.timestamp + 2 days);
​
// totalAmountCollected = 4 * 1 ether, prizePool = 3.2 ether > 3 ether held
vm.expectRevert();
puppyRaffle.selectWinner();
}

Control test, showing the draw settles normally when nobody refunds — the revert comes from the stale length, not from the draw itself:

function test_drawSucceedsWithoutRefund() public {
address[] memory p = new address[](4);
for (uint256 i = 0; i < 4; i++) p[i] = address(uint160(1000 + i));
puppyRaffle.enterRaffle{value: 4 ether}(p);
​
vm.warp(block.timestamp + 2 days);
puppyRaffle.selectWinner(); // no revert
​
assertEq(uint256(puppyRaffle.totalFees()), (4 ether * 20) / 100);
}

Both tests pass on the audited commit with forge test.

Recommended Mitigation

Track the number of live entries instead of trusting the array length, and skip blank slots when picking a winner. Swapping the refunded entry with the last one and popping keeps the array dense:

function refund(uint256 playerIndex) public {
...
- players[playerIndex] = address(0);
+ players[playerIndex] = players[players.length - 1];
+ players.pop();
}
- uint256 totalAmountCollected = players.length * entranceFee;
+ uint256 totalAmountCollected = address(this).balance - totalFees;

If the blank-slot design is kept deliberately, then selectWinner must both derive the pot from a separate counter of active entries and re-draw when the selected slot is address(0).

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!