Naming Credits

tempAdmin has the ability to essentially mint unlimited naming credits (further discussed on NMC-2), as well as control over numerous functions which could negatively affect the rest of the protocol: setRNMAddress, shutOffFeeRecipientUpdates, addAssignerCredits, nullAssignerCredits, shutOffAssignerAssignments, transferTempAdmin.

Ensure tempAdmin is a multi-sig.

NFTR Team: - tempAdmin will be a multi-sig.

The MAX_ASSIGNER_CREDITS and MAX_CREDITS_ASSIGNED offer little to no protection for the protocol tokenomics.

The MAX_ASSIGNER_CREDITS can be easily circumvented by simply calling the addAssignerCredits multiple times.

The MAX_CREDITS_ASSIGNED can be easily circumvented by calling the assignNamingCredits function multiple times or calling assignNamingCreditsBulk with a user list that contains the same address multiple times.

Therefore it is relatively easy for the tempAdmin and assigners to manipulate the tokenomics of the project, potentially creating unlimited naming credits or allowing a particular address to accumulate more than the intended amount of naming credits.

Base the MAX_ASSIGNER_CREDITS and MAX_CREDITS_ASSIGNED on the assigner/user balance and possibly introduce a hard cap on the number of namingCredits that can be in circulation to prevent an unexpected amount of namingCredits being created.

NFTR Team: - The suggested changes were implemented.

The constructor, transferTempAdmin, and updateProtocolFeeRecipient functions all assign important address contract variables without ensuring any of them are not the zero address.

Evaluate whether or not each of these addresses can be assigned to the zero/dead address and add prohibiting requires statements accordingly.

NFTR Team: - The suggested changes were implemented.

The currencyQuantity parameter is required to be equal to numberOfCredits * nftrAddress.namingPriceEther() in the BuyWithEth.YES case, and equal to numberOfCredits * nftrAddress.namingPriceRNM() in the BuyWithEth.NO case.

Therefore all subsequent computations of numberOfCredits * nftrAddress.namingPriceEther() or numberOfCredits * nftrAddress.namingPriceRNM() can be replaced with the currencyQuantity.

Implement the above simplifications.

NFTR Team: - The suggested changes were implemented.

In buyNamingCredits, the require statements inside of the first if statement can be moved into the if statement below. This way the buyWithEth type can be checked just once.

Implement the above simplifications.

NFTR Team: - The suggested changes were implemented.

There are several require statements that appear directly before a transferFrom function call or -= operator that would otherwise revert without the presence of the require statement.

Remove the unnecessary require statements and optionally replace each -= with a .sub alternative if the revert messages are necessary.

NFTR Team: - Acknowledged, but left as is for simplicity.

The nftrAddress and rnmAddress variables are stored as INFTRegistry and IRNM types in the contract, however they are often redundantly cast to INFTRegistry and IRNM types in the buyNamingCredits function.

Remove the redundant casts.

NFTR Team: - The suggested changes were implemented.

On line 120, “buy” is misspelled as “by”.

Replace “by” with “buy” in the comment.

NFTR Team: - The suggested changes were implemented.

On line 195 the uint variable i is initialized to the default value of 0.

Remove the unnecessary assignment.

NFTR Team: - The suggested changes were implemented.

Caching the array length outside a for loop saves reading it on each iteration.

Declare a len variable and use it as the upper bound in the for loop.

NFTR Team: - The suggested changes were implemented.

When dealing with unsigned integer types, comparisons with != 0 are cheaper than with > 0.

Replace the assigners[msg.sender] > 0 check with assigners[msg.sender] != 0.

NFTR Team: - The suggested changes were implemented.

In assignNamingCreditsBulk the address[] memory user and address[] memory numberOfCredits parameters are never altered and therefore can be declared calldata.

Declare the variables calldata.

NFTR Team: - The suggested changes were implemented.

Throughout the contract revert strings that are longer than 32 bytes are used.

Shorten revert strings to less than 32 bytes to save on gas.

NFTR Team: - Acknowledged, but left as is for simplicity.

Throughout the contract there are several require statements that assert the msg.sender is the tempAdmin. These require statements can be deduplicated into a single onlyTempAdmin modifier that can be used on each of these functions.

Create an onlyTempAdmin modifier and apply it to each of these functions.

NFTR Team: - The suggested changes were implemented.

In assignNamingCreditsBulk the numberOfCredits[i] value is read up to five times upon each iteration. Declare a uint creditNum outside of the for loop and cache the numberOfCredits value in it upon each iteration.

Implement the above suggestion.

NFTR Team: - The suggested changes were implemented.

Because the user array’s length is bound by MAX_BULK_ASSIGNMENT, there is no risk of overflow. To reduce bytecode, use an unchecked block in the loop to increment.

Implement the above suggestion.

NFTR Team: - The suggested changes were implemented.

Since Solidity v0.8.4, the more gas-efficient custom-errors have been introduced. They allow for passing dynamic data in the error and remove costly and repeated string error messages.

Consider replacing require statements with custom errors.

https://blog.soliditylang.org/2021/04/21/custom-errors/

NFTR Team: - Acknowledged, but left as is for simplicity.

The functions setRNMAddress and updateProtocolFeeRecipient are declared as public but are never called from within the contract.

Modify the visibility from public to external.

NFTR Team: - The suggested changes were implemented.