Notes for Auditors

from Alex (Gaus)

There is quite a lot going on within the NFTX protocol. At the root of it all are vaults that store NFTs in a 1:1 ratio to how many vTokens are minted. It is important that the 1:1 invariant always remains true and that any NFTs entering the vault are the expected type of NFTs (described by the vault's assetAddress, allowAllItems, and eligibilityStorage variables).

The inventory staking contract takes vTokens in exchange for minting xNFT positions. Each xNFT has a vTokenShareBalance that tracks its portion of the contract's total vToken holdings. It is important that the absolute amount of vTokens that a position can withdraw (determined by the contract's holdings and the position's vToken share) never goes down—unless as a result of paying an earlyWithdraw penalty.

Unlike V2, NFTX v3 has its own AMM, which is a Uniswap v3 fork with some (fairly minor) modifications. It is important that, outside of the modifications, the NFTX AMM functions how UniV3 functions and that it's not possible for the pools to be drained.

In the past, we have suffered security issues with our V2 marketplace zap and with the V2 CryptoPunks vault. Given this, we like to be extra cautious about it not being possible to drain assets from the wallets of users who interact with the NFTX protocol (as was the case with the 2022 p0n1 bug bounty). Likewise, we like to be mindful that the CryptoPunks contract is a non-standard NFT contract deployed before ERC721 was released, so any contracts interacting with it may require added attention.

NFTXVaultUpgradeable, NFTXVaultFactoryUpgradeableV3, and NFTXInventoryStakingV3Upgradeable all implement PausableUpgradeable which allows certain functions to be paused by team "guardian" addresses. This may be used if there is a reason to think that the protocol is at risk, so it's important for it to function correctly. After the protocol is paused and the issue is addressed, the protocol can then be unpaused by the DAO (following a 7-day voting period).

In NFTX v2, all fees were in vToken, so there was no need for TWAP feeds. With V3, vault fees are paid in ETH, and the amounts charged are determined using TWAPs. So, although no critical asset handling depends on TWAPs, they are still central to the protocol's usecase. Other aspects of the protocol that may require some added attention include merging inventory (xNFT) positions, adding to inventory and liquidity positions, and withdrawing from positions.

Lastly, a few final points... First, relating to TWAPs, low liquidity can make it difficult/impossible for users to buy or sell NFTs—this is a known issue that we may try to solve in the future. Second, vaults are built to work for ERC1155 collections as well, but we spend less time using such vaults, so it is important that they are reviewed too, and (3) NFTX makes extensive use of upgradeable proxies, which will be controlled by the NFTX DAO.