First Flight #2: Puppy Raffle
First Flight #2
Beginner FriendlyFoundryNFT
100 EXP
View results
Previous Next
Submission Details
Severity: medium
Invalid

OpenZeppelin's documentation on ERC721.sol state that tokenId should not exist before _safeMint is called (because _safeMint will create it)

happyformerlawyer

Summary

According to OZ's documentation, tokenId must not exist before _safeMint is called, but the selectWinner function sets tokenId to totalSupply() before _safeMint is called.

Vulnerability Details

Here are OpenZeppelin's docs on ERC721.sol, specifcally the _safeMint function:

Safely mints tokenId and transfers it to to.
​
Requirements:
​
tokenId must not exist.
​
If to refers to a smart contract, it must implement IERC721Receiver.onERC721Received, which is called upon a safe transfer.

And here is the selectWinner function which assigns totalSupply() to tokenId before _safeMint is called:

function selectWinner() external {
require(block.timestamp >= raffleStartTime + raffleDuration, "PuppyRaffle: Raffle not over");
require(players.length >= 4, "PuppyRaffle: Need at least 4 players");
uint256 winnerIndex =
uint256(keccak256(abi.encodePacked(msg.sender, block.timestamp, block.difficulty))) % players.length;
address winner = players[winnerIndex];
uint256 totalAmountCollected = players.length * entranceFee;
uint256 prizePool = (totalAmountCollected * 80) / 100;
uint256 fee = (totalAmountCollected * 20) / 100;
totalFees = totalFees + uint64(fee);
​
uint256 tokenId = totalSupply();
​
// We use a different RNG calculate from the winnerIndex to determine rarity
uint256 rarity = uint256(keccak256(abi.encodePacked(msg.sender, block.difficulty))) % 100;
if (rarity <= COMMON_RARITY) {
tokenIdToRarity[tokenId] = COMMON_RARITY;
} else if (rarity <= COMMON_RARITY + RARE_RARITY) {
tokenIdToRarity[tokenId] = RARE_RARITY;
} else {
tokenIdToRarity[tokenId] = LEGENDARY_RARITY;
}
​
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);
}

Impact

_safeMint may fail, bricking the drawing or there could be a collision where _safeMint tries to mint an NFT as the index of totalSupply() but there already appears to be something there.

Tools Used

Manual review

Recommendations

Delete the line assigning totalSupply() to tokenId or set it after _safeMint is called. If you do the latter, you may need to use nonReentrant because you would be updating state after interactions.

Updates

Lead Judging Commences

patrickalphac Lead Judge over 1 year ago
Submission Judgement Published
Invalidated
Reason: Other

Support

FAQs

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