NFTfi - Token V2 (Final Assessment) - NFTfi

Some smart contracts under analysis have owners with privileged rights to perform administrative operations and need to be trusted to not act maliciously.

- contracts/BaseTokenLock.sol [Line: 20](contracts/BaseTokenLock.sol#L20)

	abstract contract BaseTokenLock is Ownable, Pausable {

- contracts/BaseTokenLock.sol [Line: 160](contracts/BaseTokenLock.sol#L160)

	    function setTokenUtilityAccounting(address _newTokenUtilityAccounting) external onlyOwner {

- contracts/BaseTokenLock.sol [Line: 169](contracts/BaseTokenLock.sol#L169)

	    function setCooldown(uint256 _cooldown) external onlyOwner {

- contracts/BaseTokenLock.sol [Line: 181](contracts/BaseTokenLock.sol#L181)

	    function pause() external onlyOwner {

- contracts/BaseTokenLock.sol [Line: 193](contracts/BaseTokenLock.sol#L193)

	    function unpause() external onlyOwner {

- contracts/DistributorRegistry.sol [Line: 7](contracts/DistributorRegistry.sol#L7)

	contract DistributorRegistry is Ownable {

- contracts/DistributorRegistry.sol [Line: 32](contracts/DistributorRegistry.sol#L32)

	    function addDistributor(uint256 seasonNumber, address distributor) external onlyOwner {

- contracts/DistributorRegistry.sol [Line: 39](contracts/DistributorRegistry.sol#L39)

	    function removeDistributor(uint256 _seasonNumber) external onlyOwner {

- contracts/DistributorTokenLock.sol [Line: 114](contracts/DistributorTokenLock.sol#L114)

	    function setProtocolSignerAddress(address _protocolSignerAddress) external onlyOwner {

- contracts/MerkleDistributor.sol [Line: 21](contracts/MerkleDistributor.sol#L21)

	contract MerkleDistributor is Ownable, Pausable {

- contracts/MerkleDistributor.sol [Line: 114](contracts/MerkleDistributor.sol#L114)

	    function drain(uint256 _amount) public onlyOwner {

- contracts/MerkleDistributor.sol [Line: 126](contracts/MerkleDistributor.sol#L126)

	    function pause() external onlyOwner {

- contracts/MerkleDistributor.sol [Line: 138](contracts/MerkleDistributor.sol#L138)

	    function unpause() external onlyOwner {

- contracts/TokenUtilityAccounting.sol [Line: 12](contracts/TokenUtilityAccounting.sol#L12)

	contract TokenUtilityAccounting is Ownable {

- contracts/TokenUtilityAccounting.sol [Line: 107](contracts/TokenUtilityAccounting.sol#L107)

	    function addTokenLocks(address[] memory _tokenLockAddresses) external onlyOwner {

- contracts/TokenUtilityAccounting.sol [Line: 111](contracts/TokenUtilityAccounting.sol#L111)

	    function removeTokenLocks(address[] memory _tokenLockAddresses) external onlyOwner {

It is recommended to mitigate centralization issues by implementing multi-signature mechanisms. This prevents a single entity from performing administrative and protected tasks unilaterally.

Remediation Plan

ACKNOWLEDGED: The NFTfi team acknowledged the issue.

The Check-Effects-Interactions in a best-practice in Solidity programming and suggests that a contract should first check all conditions and make necessary state changes (effects) before interacting with other contracts or external addresses.

In the current contract, set a violation of the CEI pattern that does not lead to reentrancy attack. However, as it's a best-practice, it is always recommended to follow the pattern, always performing external calls after checks/validations and state changes.

- contracts/BaseTokenLock.sol [Line: 150](contracts/BaseTokenLock.sol#L150)

    function _withdraw(uint256 _amount, uint256 _requestTimestamp) internal whenNotPaused {
        require(_amount <= lockedTokens[msg.sender], "withdraw amount > total");
        // cooldown checking feature can be turned off by setting it to 0
        if (cooldown > 0) {
            require(block.timestamp >= _requestTimestamp + cooldown, "cooldown not up");
            _checkAndDeleteRequest(_amount, msg.sender, _requestTimestamp);
        }
        if (cooldown == 0) {
            if (withdrawalRequestAmounts[msg.sender] > 0) {
                // if cooldown is disabled, we have to delete existing
                // cooldowns, otherwise re-using it will cause anomalies
                _checkAndDeleteRequest(_amount, msg.sender, _requestTimestamp);
            } else {
                // if there are no withdrawalRequests anymore, we have to unlock here
                tokenUtilityAccounting.unlock(msg.sender, _amount);
            }
        }

        lockedTokens[msg.sender] -= _amount;

        nftfiToken.safeTransfer(msg.sender, _amount);
        emit Withdrawn(_amount, msg.sender);
    }

It is recommended to perform external contract calls after all state changes are performed and events are emitted.

Remediation Plan

ACKNOWLEDGED: The NFTfi team acknowledged the issue.

Assigning values to address state variables without checking for address(0) is generally not recommended.

- contracts/DistributorTokenLock.sol [Line: 40](contracts/DistributorTokenLock.sol#L40)

	        protocolSignerAddress = _protocolSignerAddress;

- contracts/DistributorTokenLock.sol [Line: 115](contracts/DistributorTokenLock.sol#L115)

	        protocolSignerAddress = _protocolSignerAddress;

It is recommended to perform input validations to avoid setting any address state variable to address(0).

Remediation Plan

ACKNOWLEDGED: The NFTfi team acknowledged the issue.

During the analysis of the contracts under scope, it was specifically in the MerkleDistributor smart contract, it was identified the use of magic numbers. This can reduce code readability, as some literals might not be so obvious or intuitive.

- contracts/MerkleDistributor.sol [Line: 56](contracts/MerkleDistributor.sol#L56)

	        uint256 claimedWordIndex = _index / 256;

- contracts/MerkleDistributor.sol [Line: 57](contracts/MerkleDistributor.sol#L57)

	        uint256 claimedBitIndex = _index % 256;

- contracts/MerkleDistributor.sol [Line: 59](contracts/MerkleDistributor.sol#L59)

	        uint256 mask = (1 << claimedBitIndex);

- contracts/MerkleDistributor.sol [Line: 64](contracts/MerkleDistributor.sol#L64)

	        uint256 claimedWordIndex = _index / 256;

- contracts/MerkleDistributor.sol [Line: 65](contracts/MerkleDistributor.sol#L65)

	        uint256 claimedBitIndex = _index % 256;

It is recommended to declare constants and use it across the code, instead of literals or "magic numbers", for improved code readability and maintenance.

For example:

uint16 someConstant = 256;

Remediation Plan

ACKNOWLEDGED: The NFTfi team acknowledged the issue.

In Solidity smart contract development, replacing hard-coded revert message strings with the Error() syntax is an optimization strategy that can significantly reduce gas costs. Hard-coded strings, stored on the blockchain, increase the size and cost of deploying and executing contracts.

The Error() syntax allows for the definition of reusable, parameterized custom errors, leading to a more efficient use of storage and reduced gas consumption. This approach not only optimizes gas usage during deployment and interaction with the contract but also enhances code maintainability and readability by providing clearer, context-specific error information.

It is recommended to replace hard-coded revert strings in require statements for custom errors, which can be done following the logic below.

1. Standard require statement (to be replaced):

require(condition, "Condition not met");

2. Declare the error definition to state

error ConditionNotMet();

3. As currently is not possible to use custom errors in combination with require statements, the standard syntax is:

if (!condition) revert ConditionNotMet();

More information about this topic in Official Solidity Documentation.

Remediation Plan

ACKNOWLEDGED: The NFTfi team acknowledged the issue.

It was identified an outdated compiler version.

As from the official Solidity documentation, it is recommended to use the latest released version of Solidity.

It was identified that the contracts in the set under analysis are using solc version 0.8.19, hence, outdated, considering the current Solidity compiler (solc) version is 0.8.24.

Update to the most recent version of Solidity (0.8.24), by changing the pragma as follows:

pragma solidity 0.8.24;

Remediation Plan

ACKNOWLEDGED: The NFTfi team acknowledged the issue.