SHN-03C: Potentially Improper NFT Handling

[JayWelsh]

SHN-03C: Potentially Improper NFT Handling

Type	Severity	Location
Standard Conformity	Informational	SeenHausNFT.sol:L116

Description:

The mint function creates an NFT directly to the marketController which represents a contract and thus has a safe-call hook invoked. However, this hook is inexistent in the codebase.

Example:

function mint(uint256 _supply, address payable _creator, string memory _tokenURI, uint16 _royaltyPercentage, bool _isPhysical)
internal
returns(Consignment memory consignment)
{
    // Get the MarketController
    IMarketController marketController = getMarketController();

    // Make sure royalty percentage is acceptable
    require(_royaltyPercentage <= marketController.getMaxRoyaltyPercentage(), "Royalty percentage exceeds marketplace maximum");

    // Get the next token id
    uint256 tokenId = nextToken++;

    // Store the token info
    Token storage token = tokens[tokenId];
    token.id = tokenId;
    token.uri = _tokenURI;
    token.supply = _supply;
    token.creator = _creator;
    token.isPhysical = _isPhysical;
    token.royaltyPercentage = _royaltyPercentage;

    // Mint the token, sending it to the MarketController
    _mint(address(marketController), tokenId, _supply, new bytes(0x0));

    // Consign the token for the primary market
    consignment = marketController.registerConsignment(Market.Primary, msg.sender, _creator, address(this), tokenId, _supply);
}

Recommendation:

We advise this trait of the system to be evaluated and potentially an unsafe mint operation to be performed instead if no logic is meant to be executed during an NFT's creation.

[JayWelsh]

This may require further reflection, it's unclear to me if this change should be made, and what the benefit of this change would be.

[JayWelsh]

Reluctant to fork the OpenZeppelin token contract, tentatively appealing unless we expect a significant gas saving from this change (as it seems that reducing gas consumption would be the only impact of this change, is this correct?).

[JayWelsh]

The MarketClerkFacet.sol makes use of @openzeppelin/contracts-upgradeable/token/ERC1155/utils/ERC1155HolderUpgradeable.sol & @openzeppelin/contracts-upgradeable/token/ERC721/utils/ERC721HolderUpgradeable.sol, therefore these hooks are called