GivingThanks
First Flight #28
Beginner FriendlyFoundry
100 EXP
View results
Previous Next
Submission Details
Severity: medium
Invalid

M-01 Use of `_mint()` May Lock NFTs in Incompatible Contracts

simsecurity

Summary

The GivingThanks::donate function uses _mint() to mint NFTs to donors without ensuring the recipient can handle ERC721 tokens. This can result in NFTs being irreversibly locked if sent to smart contracts that are not designed to receive ERC721 tokens.

Vulnerability Details

In the GivingThanks contract, the donate function mints an NFT to the donor using _mint():

function donate(address charity) public payable {
require(registry.isVerified(charity), "Charity not verified");
(bool sent,) = charity.call{value: msg.value}("");
require(sent, "Failed to send Ether");
​
_mint(msg.sender, tokenCounter);
​
// Create metadata for the tokenURI
string memory uri = _createTokenURI(msg.sender, block.timestamp, msg.value);
_setTokenURI(tokenCounter, uri);
​
tokenCounter += 1;
}

The _mint() function does not check whether the recipient (msg.sender) is a smart contract capable of handling ERC721 tokens. If msg.sender is a contract that does not implement the IERC721Receiver interface, the NFT will be permanently locked in that contract, as it cannot respond to the token transfer appropriately.

Using _safeMint() instead of _mint() ensures that if the recipient is a contract, it must implement onERC721Received(). If it doesn't, the minting operation will revert, preventing the NFT from being locked in an incompatible contract.

Impact

  • Permanent Loss of NFTs: NFTs may become inaccessible if minted to contracts that cannot handle them.

  • User Frustration: Donors using smart contract wallets or interacting through contracts may not receive their NFTs, leading to a poor user experience.

  • Potential Legal and Financial Implications: Loss of valuable NFTs could have legal or financial repercussions for the platform.

Tools Used

  • Manual code review

  • Solidity documentation and ERC721 standard specifications

Recommendations

  • Replace _mint() with _safeMint():

    Modify the donate function as follows:

function donate(address charity) public payable {
require(registry.isVerified(charity), "Charity not verified");
(bool sent,) = charity.call{value: msg.value}("");
require(sent, "Failed to send Ether");
​
- _mint(msg.sender, tokenCounter);
+ _safeMint(msg.sender, tokenCounter);
​
// Create metadata for the tokenURI
string memory uri = _createTokenURI(msg.sender, block.timestamp, msg.value);
_setTokenURI(tokenCounter, uri);
​
tokenCounter += 1;
}

  • Benefit of Using _safeMint():

    • Ensures that if the recipient is a smart contract, it must implement IERC721Receiver, preventing tokens from being locked.

    • Provides an additional safety check without significant overhead.

  • Additional Considerations:

    • Inform users that they should ensure their wallets or contracts are compatible with ERC721 tokens.

    • Consider implementing a fallback mechanism or user notification if the minting fails.

By making this change, you enhance the security and reliability of the NFT minting process, safeguarding user assets and improving overall platform trust.

Updates

Lead Judging Commences

n0kto Lead Judge 10 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

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