Fungify FungifyNFT, NFTMinter & Proxy2Step Security Review Report

The mintNFT function allows a user to pay FUNG tokens to mint a Fungify NFT. The amount of FUNG tokens required to mint one NFT is based on the constant EXCHANGE_RATE of 420000.

However, the FUNG token has 18 decimals and the exchange rate is not scaled by 18 decimals. This means that even 1 FUNG token would be able to instantly mint the supply cap of 2300, as currently the price is a very tiny amount.

uint256 public constant EXCHANGE_RATE = 420000;
 
function mintNFT(uint256 fungAmount) public {
    if (fungAmount % EXCHANGE_RATE != 0 || fungAmount == 0) {
        revert InvalidDepositAmount();
    }
    uint256 mintCount = fungAmount / EXCHANGE_RATE;
    if (nftContract.totalSupply() + mintCount > mintCurrentCap) {
        revert ExceedsCurrentCap(mintCount);
    }
 
    fungToken.transferFrom(
        msg.sender,
        address(this),
        fungAmount
    );
 
    nftContract.mint(msg.sender, mintCount);
 
    emit NFTsMinted(msg.sender, mintCount);
}

We recommend to scale the EXCHANGE_RATE to 18 decimals.

For example:

uint256 public constant EXCHANGE_RATE = 420000 * 10 ** 18;

The Fungify NFT extends ERC4907A, which allows owners of tokens to set a temporary user of the NFT (i.e. allowing for NFT rentals). ERC4907A uses the mapping _packedUserInfo to store this information and the owner can set this using setUser.

However, Fungify NFT does not handle this information upon transferring or burning of the NFT, such as by overriding _beforeTokenTransfer or _afterTokenTransfer as is shown in the EIP example implementation: ERC-4907: Rental NFT, an Extension of EIP-721 .

In the example implementation, the user information is deleted upon transfer. It could also be possible to check if the NFT is currently rented out (i.e. timestamp is not expired) and if so, the transfer could be blocked.

import "@erc721a/extensions/ERC4907A.sol";
 
contract FungifyNFT is Upgradeable2Step, ERC721A, ERC721AQueryable, ERC4907A {
    [..]
}

We recommend to handle the ERC4907 information correctly upon changes of NFT ownership.

When minting NFT the NFTMinter.sol calls nftContract.mint(), which in turn calls _mint() of ERC721A, which is not checking the onERC721Received callback on the receiver.

The msg.sender will mint an NFT when NFTMinter.mintNFT() is called. However, if msg.sender is a contract address that does not support ERC721, it will not be notified with the onERC721Received callback and the NFT could become frozen in the contract, as expected as per the ERC721 docs:

Reference: ERC-721: Non-Fungible Token Standard

function mint(address receiver, uint256 quantity) external auth {
    _mint(receiver, quantity);
}

Use safeMint instead of mint to check whether the received address supports ERC721: https://github.com/chiru-labs/ERC721A/blob/b3517b062cfe00d032ff787983a89f3fa790d1ec/contracts/ERC721A.sol#L945

The NFTMinter.sol uses public visibility for constant parameters. Setting constants to private will save deployment gas. This is because the compiler won't have to create non-payable getter functions for deployment calldata, won't need to store the bytes of the values outside of where it's used, and won't add another entry to the method ID table. If necessary, the values can still be read from the verified contract source code.

address public constant BURN_ADDRESS = address(0xdead);
uint256 public constant MAX_BURN_FEE = 1000;
uint256 public constant FEE_PERCENT_DENOMINATOR = 10000;
uint256 public constant MINT_TOTAL_CAP = 2380;
uint256 public constant EXCHANGE_RATE = 420000;

- address public constant BURN_ADDRESS = address(0xdead);
- uint256 public constant MAX_BURN_FEE = 1000;
- uint256 public constant FEE_PERCENT_DENOMINATOR = 10000;
- uint256 public constant MINT_TOTAL_CAP = 2380;
- uint256 public constant EXCHANGE_RATE = 420000;
+ address private constant BURN_ADDRESS = address(0xdead);
+ uint256 private constant MAX_BURN_FEE = 1000;
+ uint256 private constant FEE_PERCENT_DENOMINATOR = 10000;
+ uint256 private constant MINT_TOTAL_CAP = 2380;
+ uint256 private constant EXCHANGE_RATE = 420000;

Commentary from the client:

Not updating. We would rather keep these public despite the increased deployment cost, for user convenience.”

Based on the guidance provided at Style Guide — Solidity 0.8.25 documentation it is advised to use the underscore prefix for internal or private functions rather than external ones. However, the external functions _setCurrentCap and _setBurnFee do not adhere to this style guide. This deviation could potentially complicate the readability of the codebase.

function _setCurrentCap(uint256 _mintCurrentCap) external onlyOwner {
    if (_mintCurrentCap > MINT_TOTAL_CAP) {
        revert CapTooHigh();
    }
    emit CurrentCapUpdated(mintCurrentCap, _mintCurrentCap);
    mintCurrentCap = _mintCurrentCap;
}
 
function _setBurnFee(uint256 _burnFee) external onlyOwner {
    if (burnFee > MAX_BURN_FEE) {
        revert FeeTooHigh();
    }
    emit BurnFeeUpdated(burnFee, _burnFee);
    burnFee = _burnFee;
}

We have identified the following locations where memory parameters of external functions could be marked as calldata in favour of saving gas:

1. NFTMinter.sol: uint[] memory nftIds on line 59.
2. FungifyNFT.sol: uint[] memory nftIds on line 32 and 39.

function burn(uint[] memory nftIds) external auth {
    [..]
}
 
function burnIfOwnedByUser(uint[] memory nftIds, address user) external auth {
    [..]
}
 
function burnNFT(uint[] memory nftIds) public {
    [..]
}

Change the mentioned parameter to calldata instead of memory.

The FungifyNFT.sol contract exhibits inconsistency in ownership verification within its burn functionality. Specifically, there are two distinct burn functions implemented in the contract:

1. The first burn function allows any authorized address to burn NFTs without verifying if the caller is the rightful owner of the NFTs being burned. This lack of ownership verification could introduce centralization concerns, as it enables authorized addresses to burn NFTs that they do not own.
2. In contrast, the second burn function (burnIfOwnedByUser) verifies whether the NFTs being burned belong to a specific user before allowing the burn operation to proceed. This function provides a level of ownership verification, ensuring that the NFT can only be burned if the correct owner was provided. This inconsistency in ownership verification mechanisms raises concerns regarding the decentralized nature of the contract and could potentially lead to misuse or exploitation.

function burn(uint[] memory nftIds) external auth {
    for(uint i; i < nftIds.length;) {
        _burn(nftIds[i]);
        unchecked { i++; }
    }
}

By removing the first burn function, the contract's behavior becomes more aligned with decentralized principles, ensuring that only rightful owners can burn their NFTs.