Protocol Review

The owner address has the ability to repeatedly change namingPriceRNM by calling updateNamingPriceRNM, but the function lacks any lower and upper bounds on the input. As a result, the owner can modify the RNM price to be as large as possible and frontrun a buyer’s transaction. This would lead to the user experiencing a larger decrease of assets than intended.

Additionally, the owner address holds potentially exploitative abilities to: withdraw, withdrawRNM, curateCollection, updateNamingCreditsProtocolFeeRecipient, shutOffAssignments, assignNamingCredits, setSpecialNames, updateNamingPriceEther, updateProtocolFeeRecipient, updateRnmNamingStartBlock.

Consider defining lower and upper bounds on namingPriceRNM.

Furthermore, consider making owner a multi-sig, optionally with a timelock for improved community oversight.

NFTR Team: - Owner privileges are controlled by a multi-sig.

• RNM and WETH user risks have been mitigated as a new parameters have been introduced in the changeName function (currencyQuantity) that ensures that the contract can only pull what the user intended.
• withdraw: has been eliminated as it wasn’t needed — ETH doesn’t have a way to get stuck in the contract.
• withdrawRNM: Exists as a failsafe mechanism so RNM doesn’t get stuck in the contract if a user decides to send RNM to it.
• curateCollection: a max of 10 collections can be curated.
• updateNamingCreditsProtocolFeeRecipient: updates can now be shutoff if the DAO decides to make that immutable.
• shutOffAssignments: This is meant to be able to shut off naming credit assignments if it’s the will of the DAO.
• assignNamingCredits: has been set to a max of 1,000 assignments.
• setSpecialNames: These names are set right after deploy and functionality is that no more than 1,000 special names can be set, which is how the protocol was designed.
• updateNamingPriceEther: can now be shut off at the DAO’s will
• updateProtocolFeeRecipient: updates can now be shutoff if the DAO decides to make that immutable.
• updateRnmNamingStartBlock: This is meant so that the contract owner can activate tokenomics functionality once RNM goes live.

According to the docs, if an NFT is curated and is in the hold farming period, naming can be free. However, the changeName function still expects payment in the specified currency even if the NFT collection is curated and in the hold farming period.

Implement logic such that a NFT in the holding period can be named for free.

NFTR Team: - The suggested changes were implemented.

Line 336: Because checkOwnership must get the owner and compare it against the msg.sender, the function can simply utilize getOwner instead of duplicating the logic for retrieving the NFT owner.

Line 363: isTokenStructEmpty can simply be return token_in.collectionAddress == address(0) && token_in.tokenId == 0; The statement itself returns a boolean so an if-else is not needed.

Implement the above simplifications.

NFTR Team: - The suggested changes were implemented.

transferFrom is used to transfer RNM from the NFTRegistry contract to the owner, but a transfer could be used instead so approvals can be avoided.

Consider using the transfer function if the RNM token allows it.

NFTR Team: - The suggested changes were implemented.

transferName can potentially lead to an NFT’s name being overwritten without permission from the holder. There must be proper validation done by the marketplace to make sure people can’t arbitrarily send names to another person’s NFT.

Ensure the marketplace contract has the necessary validation checks in place to only transfer a name if the to address acknowledges the transaction whether it is through a name purchase or some other means.

NFTR Team: - Acknowledged. The marketplace contract will take care of this.

The constructor can benefit from zero address checks to help prevent errors during deployment.

Focus on creating seamless deploy scripts and consider adding zero address checks

NFTR Team: - The suggested changes were implemented.

The contract frequently performs variable == true or variable == false which is unnecessary and gas inefficient.

Replace require(variable == true) with require(variable).

Replace require(variable == false) with require(!variable).

NFTR Team: - The suggested changes were implemented.

There is no need to cast holdFarmingAddress to the IHoldFarming interface in functions such as curateCollection as it was already declared with the IHoldFarming type. The variable was not cast in initiateRetroactiveHoldFarming. The same can be said for the namingCreditsAddress variable.

Consider removing the explicit casts to save gas.

NFTR Team: - The suggested changes were implemented.

“Naiming” is misspelled in the comment.

Correct the spelling for cleaner docs.

NFTR Team: - The suggested changes were implemented.

The initiateRetroactiveHoldFarming function calls the initiateHoldFarmingForNFT function in the Hold Farming contract 10,000 times. This may exceed the block gas limit and prevent any collection from getting curated.

Ensure that the block gas limit limit is not exceed or consider executing calls to initiateHoldFarmingForNFT in batches.

NFTR Team: - This function has been eliminated.

• Retroactive hold farming might be taken care of in a different way, if at all.

Consider the scenario where a user registers a special name for their ERC721-compliant burnable NFT. They then proceed to burn their NFT, and ownership is relinquished. As a result, the name of the NFT cannot be changed nor transferred. The special name, a coveted asset to the NFTR protocol, is now lost.

Consider whether or not this is expected behavior. If unexpected, add a function so that if an owner does not exist for a particular NFT, then that NFT’s registered name can be dereserved.

NFTR Team: - This is expected behavior.