First Flight #11: Snek-Raffle
First Flight #11
Beginner FriendlyFoundryNFT
100 EXP
View results
Previous Next
Submission Details
Severity: high
Invalid

The `fulfillRandomWord` function uses `ERC721::_mint` function to mints a snek NFT

bube

Summary

The ERC721::mint function is used in the fulfillRandomWord function in order to mints a snek NFT for the winner.

Vulnerability Details

The fulfillRandomWord function finds the winner and mints the reward - snek token.

​
def fulfillRandomWords(request_id: uint256, random_words: uint256[MAX_ARRAY_SIZE]):
index_of_winner: uint256 = random_words[0] % len(self.players)
recent_winner: address = self.players[index_of_winner]
self.recent_winner = recent_winner
self.players = []
self.raffle_state = RaffleState.OPEN
self.last_timestamp = block.timestamp
rarity: uint256 = random_words[0] % 3
self.tokenIdToRarity[ERC721._total_supply()] = rarity
log WinnerPicked(recent_winner)
@> ERC721._mint(recent_winner, ERC721._total_supply())
send(recent_winner, self.balance)
​

But in order the winner to receive the reward, the function uses the ERC721::_mint function. According to the ERC721 comments the use of _mint is discouraged. The safe_mint function should be used:

​
@internal
def _mint(owner: address, token_id: uint256):
"""
@dev Mints `token_id` and transfers it to `owner`.
@notice Note that `token_id` must not exist and
`owner` cannot be the zero address.
​
@> WARNING: Usage of this method is discouraged,
use `_safe_mint` whenever possible.
@param owner The 20-byte owner address.
@param token_id The 32-byte identifier of the token.
"""
assert owner != empty(address), "ERC721: mint to the zero address"
assert not(self._exists(token_id)), "ERC721: token already minted"
​
self._before_token_transfer(empty(address), owner, token_id)
# Checks that the `token_id` was not minted by the
# `_before_token_transfer` hook.
assert not(self._exists(token_id)), "ERC721: token already minted"
​
# Theoretically, the following line could overflow
# if all 2**256 token IDs were minted to the same owner.
# However, since we have bounded the dynamic array
# `_all_tokens` by the maximum value of `uint64`,
# this is no longer even theoretically possible.
self._balances[owner] = unsafe_add(self._balances[owner], 1)
self._owners[token_id] = owner
log Transfer(empty(address), owner, token_id)
​
self._after_token_transfer(empty(address), owner, token_id)
​

Impact

The ERC721::_mint function doesn't check if the receiver can accept ERC721 token. If the receiver can not accept NFT, the function will not revert and the token will be stucked. The safe_mint function checks if the receiver can accept NFT tokens using _check_on_erc721_received. And the transaction will revert if the receiver can not accept the token.
Therefore, ERC721::safe_mint function should be used instead of ERC721::_mint function.

Tools Used

Manual Review

Recommendations

Use ERC721::safe_mint function instead of ERC721::_mint, but be careful, because the _check_on_erc721_received in safe_mint can be risky for reentrancy attack. Call the safe_mint function after all state changes and use reentrancy guard.

Updates

Lead Judging Commences

inallhonesty Lead Judge over 1 year ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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