Eggstravaganza
First Flight #37
Beginner FriendlySolidity
100 EXP
View results
Previous Next
Submission Details
Severity: medium
Valid

Risk of locked assets due to use of `_mint()` instead of `_safeMint()`

twilly

Summary

The EggstravaganzaNFT::mintEgg function calls the _mint() function rather than the more robust _safeMint() function to mint the egg NFT to the destination players. The _safeMint() function includes an essential safety mechanism that verifies a recipient contract’s capability to receive and manage ERC-721 tokens by calling the onERC721Received method. This check ensures the recipient implements the ERC721Receiver interface, reducing the risk of tokens being sent to incompatible contracts.

Vulnerability Details

The mintEgg function uses _mint instead of _safeMint and returns true, even if the minting fails.

function mintEgg(address to, uint256 tokenId) external returns (bool) {
require(msg.sender == gameContract, "Unauthorized minter");
@> _mint(to, tokenId);
totalSupply += 1;
return true;
}

Impact

Using ERC721::_mint() can mint ERC721 tokens to addresses which don't support ERC721 tokens, while ERC721::_safeMint() ensures that ERC721 tokens are only minted to addresses which support them. OpenZeppelin discourages the use of _mint().

Furthermore, the function returns true even when the minting process fails which can lead to incorrect event data being emitted.

Proof of Code

Add the below BadRecipient.sol contract to the src folder. This is an example contract that cannot properly receive or transfer NFTs:

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.23;
​
contract BadRecipient {
// This contract does NOT implement IERC721Receiver
// It has no onERC721Received function
// It also lacks logic to transfer ERC721 tokens
​
// Fallback function to accept ETH (if any), but no token handling
receive() external payable {}
​
// A dummy function to simulate inability to transfer tokens
function transferToken(address, uint256) external pure {
revert("BadRecipient: Cannot transfer tokens");
}
}

Import the contract inside the EggHuntGameTest.t.sol file:

import "../src/BadRecipient.sol";

Add the following test and run forge test --mt testMintEggToBadRecipient

function testMintEggToBadRecipient() public {
// Mint an egg by simulating a call from the game contract.
vm.prank(address(game));
uint tokenId = 99;
​
// mint to a contract that cannot receive NFTs
// The internal call to `_mint` fails, however `mintEgg` is hardcoded to return true
bool success = nft.mintEgg(address(badRecipient), tokenId);
assertTrue(success);
​
// Check that token is owned by the badRecipient contract.
assertEq(nft.ownerOf(tokenId), address(badRecipient));
​
// Verify that the totalSupply counter incremented from 0 to 1.
assertEq(nft.totalSupply(), 1);
​
// Verify that the NFT is locked and cannot be transfered
vm.expectRevert("BadRecipient: Cannot transfer tokens");
vm.prank(address(badRecipient));
badRecipient.transferToken(alice, tokenId);
​
// Confirm that badRecipient is still the NFT owner
assertEq(nft.ownerOf(tokenId), address(badRecipient));
}

Tools Used

Manual review, Foundry for tests

Recommendations

Use the _safeMint function instead of _mint. Additionally, add logic to check if minting succeeded instead of returning the hardcoded value of true.

Updates

Lead Judging Commences

m3dython Lead Judge 8 months ago
Submission Judgement Published
Validated
Assigned finding tags:

Unsafe ERC721 Minting

Protocol doesn't check if recipient contracts can handle ERC721 tokens

Support

FAQs

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

Give us feedback!