MetalSwap NFT Staking Pool

The protocol consists of two contracts, a monolithic contract called NFTStakingPool of 1786 LOC which provides the main functionality and a supporting contract called LoopExecutionCollectAssign of 98 LOC. The NFTStakingPool contract contains a large number of contract variables, many of which can be changed through admin controlled functionality at runtime. A mix of zero-indexed and one-indexed arrays are used, requiring the developer to frequently switch between these two representations. Non-trivial functionality and calculations are present and the contract variables often need to maintain certain relationships to one another. The audit found several issues of varying severity which would have been caught had a test suite been present. We note that the developers have made an effort to cover various edge cases programmatically, however we strongly recommend that the team develop a strong test suite with good coverage before deployment in order to verify that the protocol behaves as expected.

The setGovernance function of the NFTStakingPool contract revokes the DEFAULT_ADMIN_ROLE from the governance address, but then grants the role to the same governance address rather than the one specified by the newGovernance parameter.

In function NFTStakingPool::_calculateRewardPerRoundByElapsedRounds, if _roundsOfFixedReward==0 and _boostedRounds>=_elapsedRounds, the value _rewardPerRoundsAndToken is calculated as _elapsedRounds * _roundsOfFixedReward * _boostFactor where the _boostFactor is of the form X * 10^6, meaning that the final product should be divided by 10^6. In this case the division by 10^6 is not performed leading to a much larger reward value. Most likely the contract would not hold such a large amount of reward tokens to distribute and the execution would always revert.

The functions _addNFT and _removeNFT of the NFTStakingPool contract provide a number of parameters when making calls to Uniswap V3, amongst which are amount0Min and amount1Min. In both of the above functions, these parameters are set to zero. This is not advisable as it can make these transactions the target of frontrunning attacks which can result in depositing or withdrawing liquidity at a very disadvantageous rate when calling mint or burn. The minimums should be set to an acceptable percentage of the desired amount, making explicit the deviation from the desired amount which is tolerable to the protocol. Alternatively, this amount could be provided by the user according to their personal risk tolerance.

In function NFTStakingPool::_partialUnstake, there is the assignment NFTPosition storage lastNFT = NFTs[_nrOfNFTs] and then the call NTFs.pop() that deletes NFTs[_nrOfNFTs] and thus lastNFT, which points to it, setting all its fields to 0. However, after the call to the pop(), lastNFT.owner is passed as a parameter to the function _updateUserToPositionIds() and is read/used to update the userToPositionIds storage mapping even though it is always 0 (this is not intended), thus the storage updates that are being executed are meaningless. As a result, the results returned by functions getAllStakedNFTPosOfUser and getAllStakedNFTIdsOfUser are inaccurate.

In the function _loopAssignPartialRewards of the NFTStakingPool, the calculation of the rewards which are accumulated by a user does not reflect the contribution of that user to the pool correctly. When both _token0CollectedFeesPartial and _token1CollectedFeesPartial are non-zero, the percentage of the reward granted to the user is calculated using the following formula:

percentualX18 = ((mult18 * nft.token0RoundFees /
                  _token0CollectedFeesPartial) +
                (mult18 * nft.token1RoundFees /
                  _token1CollectedFeesPartial)) / 2;

Which means that the percentage reward is calculated as an average of the percentage of the token0 and token1 fees accumulated by the user. However, since token0 and token1 are different assets, owning A% of token0 fees and B% of token1 fees does not mean that the user owns (A+B/2)% of the total fees’ value in USD. In fact this is only the case when A == B. The computation would be more accurate if the token0 and token1 fees percentages were calculated based on the actual USD value of the fee amounts.

The NFTStakingPool::unstake function checks that at least cooldownInSec seconds have passed since the staking of the NFT, before allowing the user to unstake it. A similar check is performed in the NFTStakingPool::modifyPriceRange function, but it can be circumvented easily with the limitation that a tiny portion of the initial staked position remains staked.

function modifyPriceRange (
    uint256 _tokenId,
    int24 _newTickLower,
    int24 _newTickUpper,
    uint256 _desiredToken0LP,
    uint256 _desiredToken1LP
) external checkSCUnpaused nonReentrant returns (...) {
    // Dedaub: Code omitted for brevity
    bool canUnstake = block.timestamp >=
        NFTs[positionToRemove].timestampStake + cooldownInSec;

    (uint256 recoveredAmount0,
     uint256 recoveredAmount1,
     uint24 poolFeeTier) = _removeNFT(_tokenId, positionToRemove);
    // Dedaub: One can always circumvent this check by setting
    // _desiredToken0LP and _desiredToken1LP to something really small
    if ( (recoveredAmount0 < _desiredToken0LP ||
          recoveredAmount1 < _desiredToken1LP) && !canUnstake ) {
        revert NFTStakingPoolError(13);
    }
    // Dedaub: Code omitted for brevity
}

As can be seen in the code snippet, the canUnstake variable is true only if the current timestamp is at least cooldownInSec seconds greater than the timestamp at the time of staking the NFT. However, even if canUnstake==false, the unstaking will happen if recoveredAmount0<_desiredToken0LP || recoveredAmount1<_desiredToken1LP is false, i.e., the actual (recovered) unstaked amount in token0 and token1 is greater than the desired amount. This aforementioned condition can be satisfied really easily as the _desiredToken0LP and _desiredToken1LP variables are controlled by the user/caller and can be set to a really small value like 1 wei or similar to ensure that the recovered amounts will be greater and the condition will evaluate to false. This way the user is able to unstake recoveredAmount0 - _desiredToken0LP token0 and recoveredAmount1 - _desiredToken1LP token1 tokens prematurely while only leaving staked _desiredToken0LP and _desiredToken1LP tokens respectively.

Not all calls to ERC20 tokens are using the SafeERC20 wrappers and at the same time not all returned values are checked. It is recommended to always either use OpenZeppelin's SafeERC20 library.

The _loopAssignPartialRewards function of the NFTStakingPool contract has the following check:

if (rewardToken.rewardLocked >
        ERC20(rewardToken.addr).balanceOf(address(this))) {
    revert NFTStakingPoolError(16);
}

If anything wrong happens to the rewardLocked calculation for a particular reward token, and this becomes greater than the balance of the reward token owned by the contract, the entire reward functionality will become disabled, as this function will revert and throw away all the work done for other reward tokens and NFTs.

This function should be singled out for extensive testing and simulation, since a problem here would DoS the main functionality of the protocol.

The function could also be made fault tolerant by refactoring it such that a failure in one reward token results in that token being skipped rather than in the entire function reverting.

The functions forcedUnstake and forcedUnstakeAllToOwnersPartial of the NFTStakingPool contract unstake/transfer the NFT(s) but do not update the state structures that track these NFT positions as the unstake function does by calling _partialUnstake. It is understood that the forcedUnstake* functions will only be used for emergency situations, still the omission of state updates would mean that the contract will not be usable anymore, as the state is corrupt/invalid.

The percentage of the rewards distributed to a user is computed based on the fees collected by their position. However, one thing that seems a bit counterintuitive is that when the NFT is staked for the first time, the fees that have already accumulated are not collected to be transferred to the NFT's original owner. Instead, the already accumulated fees are transferred to the feeManagerAddress, i.e., they are forfeited as if they were generated during the staking period. A user that has an active Uniswap V3 position for N weeks could stake its NFT for M weeks (after the first N weeks) without collecting the rewards first and then he would be rewarded for N+M weeks while they have staked for only M weeks. It is not clear if this is an intended behavior or users should only be rewarded for the amount of fees their position has generated while being staked. Assuming that users/stakers have an incentive to forfeit their LP fees for rewards, they could exploit the above to acquire rewards without having staked their NFT long enough.

The function activateRewardToken of the NFTStakingPool contract does not set rewardToken.isActive to true but to _isActive instead, which as a parameter can be given the value false by the caller. This is counterintuitive as in this case activateRewardToken would be used to deactivate a token instead. Indeed, even if the value isActive is set to false, the function still emits a RewardTokenStatusChanged(2, ...) event, where 2 is the code for existing token has been activated according to the comments for this event.

Functions activateRewardToken and deactivateRewardToken of the NFTStakingPool contract do not check the status of rewardToken.isActive before setting it to true and false respectively. Thus a (de)activated reward token could be (de)activated again, unnecessarily emitting a RewardTokenStatusChanged event.

The functions assignPartialRewards and collectPartialFees of the NFTStakingPool contract neither check that the status storage variable is set correctly nor that the paused == true and canStake == false. The status check is performed by the caller of the function, which is the LoopExecutionCollectAssign contract. We believe that adding the aforementioned checks would add extra assurances and prevent execution on invalid state that might be created by an unforeseen action/update to the contract.

The function NFTStakingPool::_loopAssignPartialRewards implements an outer loop over the rewardTokens and an inner loop over the NFTs.

for (uint8 j=0; j<_nrOfRewardTokens; j++) {
    RewardToken storage rewardToken = rewardTokens[j];

    if (rewardToken.isActive) {
        for (i=_lastRewardPos;
             i<_lastRewardPos+_nrOfRewardsToProcess && i<=_nrOfNFTs;
             i++
        ) {
            NFTPosition storage nft = NFTs[i];
            if (_token0CollectedFeesPartial == 0) {
                percentualX18 = (mult18 * nft.token1RoundFees) /
                    _token1CollectedFeesPartial;
            } else if (_token1CollectedFeesPartial == 0) {
                percentualX18 = (mult18 * nft.token0RoundFees) /
                    _token0CollectedFeesPartial;
         	} else {
                percentualX18 = ((mult18 * nft.token0RoundFees /
                    _token0CollectedFeesPartial) +
                    (mult18 * nft.token1RoundFees /
                    _token1CollectedFeesPartial)) / 2;
            }


            rewardToken.rewardsAccumulated[nft.owner] += percentualX18 *
                getRewardForRounds(j, numberOfElapsedRounds) / mult18;
            rewardToken.rewardLocked += percentualX18 *
                getRewardForRounds(j, numberOfElapsedRounds) / mult18;
        }
        if (rewardToken.rewardLocked >
                ERC20(rewardToken.addr).balanceOf(address(this))) {
            revert NFTStakingPoolError(16);
        }
    }
}

As the percentualX18 value that is computed based on the i-th NFT is the same for each reward token of the outer loop, the two loops can be inverted in order to cache these values and save gas. However, there exists a complication, a reward token (of the outer loop) might be inactive and thus it would be inefficient to check this condition for each NFT. One solution could be to create a temporary array of the active reward tokens and use that in the (new) inner loop. Example implementation below:

uint8 activeRewardTokensNr = 0;
uint8[] memory activeRewardTokensIndices = new uint[](n);

for (uint8 j=0; j<_nrOfRewardTokens; j++) {
    RewardToken storage rewardToken = rewardTokens[j];

    if (rewardToken.isActive) {
        emit RoundRewardChanged(address(rewardToken.addr),
            getRewardForRounds(j, numberOfElapsedRounds),
            uint64(block.timestamp));
        activeRewardTokensIndices[activeRewardTokensNr] = j;
        activeRewardTokensNr += 1;
    }
}


for (i=_lastRewardPos;
     i<_lastRewardPos+_nrOfRewardsToProcess && i<=_nrOfNFTs;
     i++
) {
    NFTPosition storage nft = NFTs[i];
    if (_token0CollectedFeesPartial == 0) {
        percentualX18 = (mult18 * nft.token1RoundFees) /
            _token1CollectedFeesPartial;
    } else if (_token1CollectedFeesPartial == 0) {
        percentualX18 = (mult18 * nft.token0RoundFees) /
            _token0CollectedFeesPartial;
    } else {
        percentualX18 = ((mult18 * nft.token0RoundFees /
            _token0CollectedFeesPartial) +
            (mult18 * nft.token1RoundFees /
        	_token1CollectedFeesPartial)) / 2;
    }

    for (uint8 j=0; j
            ERC20(rewardToken.addr).balanceOf(address(this))) {
        revert NFTStakingPoolError(16);
    }
}

The admin of the protocol is able to transfer Ether and all the ERC20 tokens that are held by the NFTStakingPool contract (to be used as reward tokens) via the NFTStakingPool::manageAsset function. The same is true for all ERC20 token approvals to the NFTStakingPool. Uniswap V3 LP NFTs staked to (held by) the NFTStakingPool cannot be transferred out in this way.

The admin of the protocol can use the forcedUnstake function of the protocol to transfer an NFT held by the protocol to an arbitrary address. We recommend only allowing the transfer of the NFT back to the original owner to increase confidence in the protocol.

The admin of the protocol has wide discretion over the system and can change most of the parameters regulating it through the setPrimaryPoolParameters and setSecondaryPoolParameters functions. For instance, the admin is allowed to change the NFTManager, the feeManagerAddress, the governance address, which creates a centralisation risk for various stakeholders of the protocol. In addition the admin can also change a host of internal protocol variables mid-way through the protocol, which are not strictly speaking, configuration parameters. See issue A3 for a discussion of this issue and potential remedies.

The NFTStakingPool::unstake function reverts if the contract is paused. This is intended behavior as to execute a collect-assign loop, no staking or unstaking should take place during the loop. However, as the pausing and unpausing happens not only programmatically (collect-assign loop triggers) but also manually (by the admin of the protocol), there exists the danger of not being able to unstake NFTs due to a malicious/compromised admin indefinitely pausing this functionality.

The NFTStakingPool contract makes use of a single error with the following signature: error NFTStakingPoolError(uint8 errorCode). Following this, it uses error codes ranging from 1-32 whenever this error is used in the code.

The use of error codes is not recommended, due to its error prone nature and the fact that it does not provide useful information to a caller. It is recommended that separate errors with meaningful names be used instead of error codes.

The NFTStakingPool contract defines three contract variables relating to whether certain functionality is paused or not at any given time. The contract initializes the paused variable to true, the canStake variable to true, and the rewardsPaused variable is left uninitialized (so is set to false). For consistency we recommend that the latter two be initialized to false and true respectively, if the protocol will be starting in a paused state pending further configuration.

The setMainPoolParameters and setSecondaryPoolParameters functions of the NFTStakingPool contract can be used to change a number of internal variables which are crucial to the functioning of the protocol. We recommend reviewing the parameter lists of these functions in order to achieve the following goals: (1) variables which affect the internal state and logic of the protocol should not be changed by the admin after initialisation. These should be grouped into an initializer function which can only run once. (2) only bona-fide configuration parameters should be allowed to change - these should exclude those parameters which risk crippling the protocol if changed half way through its operation.

In general the correctness of the protocol should be ensured through a good test suite, as it is not a good idea for the admin to try to manually fix any issues which arise at runtime. Any additional flexibility granted to the admin ‘just in case’ is bound to create unintended consequences unless it has been properly thought through in the first place.

Some potentially dangerous variables to alter half way through the operation of the protocol are listed below:

• feeTier
• _LPToken0 and _LPToken1
• _lastRewardDistributionTimestamp and _lastNFTPos

• _token0CollectedFeesPartial and _token1CollectedFeesPartial
• _lastUnstakePos and _lastRewardPos

The team should consider whether the admin has any legitimate reason for changing these variables half way through the operation of the protocol.

As mentioned in issue A3, the setMainPoolParameters and setSecondaryPoolParameters functions of the NFTStakingPool contract give wide powers to the admin of the protocol to change its functioning. However, there are only a few validations to stop the admin from making a mistake when interacting with these functions. Some of these parameters will have certain values which should be disallowed (for instance a common check for address types is to stop them from being set to address(0)), while there will be acceptable ranges for other parameters, such as _roundDurationInSec, _rewardRuntime and _unstakeCooldownInSec. It is advisable to think about these matters prior to the deployment of the protocol so as to have a better idea of the conditions in which you would like the protocol to operate.

The setMainPoolParameters of the NFTStakingPool allows the admin to change the paused and rewardsPaused variables (although interestingly not the canStake variable). In doing so, this overlaps with the functionality of the setPausedStatuses function. We recommend removing these two parameters from setMainPoolParameters to simplify the signature of this function.

The checkUpkeep function of the LoopExecutionCollectionAssign contract returns the string 0x as its performData, but the empty string “” could be returned instead if there is no data to return.

In function _removeNFT of the NFTStakingPool contract two no-op additions are performed at instructions _userAmount0 += collectAmount0; and _userAmount1 += collectAmount1; as _userAmount0 and _userAmount1 are 0.

Calls to NFTStakingPool::changeRewardMechanism will revert if the _token does not exist in contrast to all other functions, e.g., setBoostedStatusForNrOfRounds. For consistency reasons this function could be refactored to not revert if the _token does not exist.

In the function NFTStakingPool::getRewardTokensDetails the variable name _rewardPerRound is misleading and should be changed to _nextRewardEstimates.

The function NFTStakingPool::forcedUnstake emits a UserUnstaked event even though this operation can only be performed by the admin of the protocol and not a user.

The function NFTStakingPool::assignPartialRewards caches the storage values lastRewDistrTimestamp and roundDurationInSec but ends up not using the cached counterparts everywhere.

The function NFTStakingPool::assignPartialRewards checks the condition _nrOfNFTs == 0 || block.timestamp < lastRewDistrTimestamp + roundDurationInSec, i.e., that there are staked NFTs and that enough time has passed from the last reward distribution, after checking that there are fees to be distributed. This is counterintuitive, as the aforementioned condition is a prerequisite for fees to be available for distribution.

The code is compiled with Solidity 0.8.20. Version 0.8.20, in particular, has some known bugs, which we do not believe affect the correctness of the contracts.