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

Refunds make the raffle insolvent / unselectable

joshuapaul1

Root + Impact

Description

  • When selecting a winner in selectWinner(), totalAmountCollected uses player.length in its calculation, expecting each entry is paid and active

  • But the function refund(uint256 playerIndex) function [line 103] leaves a zero-address hole in that same array and returns the ticket payment.

players[playerIndex] = address(0);

Risk

Likelihood:

  • High since a player can refund and a contract balance reduces, but the player.length isn't updated


Impact:

  • High. the raffle becomes insolvent/unselectable also a zero-address hole can also be selected as winner, causing _safeMint(address(0), ...) to revert


Proof of Concept

Example with four 1 ETH entries:

  • One player refunds; contract balance becomes 3 ETH.

  • players.length remains 4.

  • selectWinner computes a 3.2 ETH prize and 0.8 ETH fee.

  • The 3.2 ETH transfer fails due to insufficient balance, permanently preventing selection unless someone donates funds.

// SPDX-License-Identifier: MIT
pragma solidity ^0.7.6;
​
import {Test} from "forge-std/Test.sol";
import {PuppyRaffle} from "../src/PuppyRaffle.sol";
​
contract PuppyRaffleInsolvencyTest is Test {
uint256 private constant ENTRANCE_FEE = 1 ether;
uint256 private constant RAFFLE_DURATION = 1 days;
​
PuppyRaffle private raffle;
​
address private playerOne = address(0x1);
address private playerTwo = address(0x2);
address private playerThree = address(0x3);
address private playerFour = address(0x4);
​
function setUp() public {
raffle = new PuppyRaffle(
ENTRANCE_FEE,
address(0x99),
RAFFLE_DURATION
);
​
address[] memory entrants = new address[](4);
entrants[0] = playerOne;
entrants[1] = playerTwo;
entrants[2] = playerThree;
entrants[3] = playerFour;
​
raffle.enterRaffle{value: 4 ether}(entrants);
}
​
function test_RefundMakesInitialPrizePayoutImpossible() public {
// Four entries have funded the raffle.
assertEq(address(raffle).balance, 4 ether);
​
// One entrant takes their legitimate refund.
vm.prank(playerOne);
raffle.refund(0);
​
// The refunded slot remains in `players`; balance does not.
assertEq(address(raffle).balance, 3 ether);
assertEq(raffle.players(0), address(0));
assertEq(raffle.players(3), playerFour);
​
vm.warp(block.timestamp + RAFFLE_DURATION + 1);
vm.roll(block.number + 1);
​
/*
* `players.length` remains 4, therefore:
*
* totalAmountCollected = 4 ether
* prizePool = 3.2 ether
* fee = 0.8 ether
*
* The contract has only 3 ether. The ETH call fails and selection
* always reverts, independent of which index is selected.
*/
vm.expectRevert("PuppyRaffle: Failed to send prize pool to winner");
raffle.selectWinner();
}
}

Recommended Mitigation


Use an active-player count and remove refunded players from the raffle set. The simplest approach is swap-and-pop, so players.length always equals the number of funded, eligible entries.

function refund(uint256 playerIndex) external nonReentrant {
address player = players[playerIndex];
​
require(player == msg.sender, "PuppyRaffle: Only the player can refund");
require(player != address(0), "PuppyRaffle: Player is not active");
​
// Remove the refunded player before transferring ETH.
+uint256 lastIndex = players.length - 1;
+players[playerIndex] = players[lastIndex];
+players.pop();
​
(bool success,) = payable(player).call{value: entranceFee}("");
require(success, "PuppyRaffle: Refund transfer failed");
​
emit RaffleRefunded(player);
}
Updates

Lead Judging Commences

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