code-423n4 / 2022-12-tigris-findings

8 stars 4 forks source link

Use safeMint instead of mint for ERC721 #563

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/BondNFT.sol#L83 https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/BondNFT.sol#L313

Vulnerability details

Impact

Users may lose their NFTs if mint is not implemented properly

Proof of Concept

In BondNFT.createLock, _mint takes in two parameters, _owner and _bond

        _mint(_owner, _bond);

However, if the _owner, which is msg.sender (input parameters passed from Lock.sol), is a contract address that does not support ERC721, the NFT can be frozen in the contract.

As per the documentation of EIP-721:

A wallet/broker/auction application MUST implement the wallet interface if it will accept safe transfers.

https://eips.ethereum.org/EIPS/eip-721

As per the documentation of ERC721.sol by OpenZeppelin:

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC721/ERC721.sol#L274-L285

Tools Used

Manual Review

Recommended Mitigation Steps

use safeMint instead of mint to check received address support for ERC721 implementation

c4-judge commented 1 year ago

GalloDaSballo marked the issue as duplicate of #421

c4-judge commented 1 year ago

GalloDaSballo marked the issue as not a duplicate

GalloDaSballo commented 1 year ago

L

c4-judge commented 1 year ago

GalloDaSballo changed the severity to QA (Quality Assurance)

c4-judge commented 1 year ago

GalloDaSballo marked the issue as grade-c