ABSTRACT
Dedaub was commissioned to perform a security audit of the MetalSwap protocol. A number of issues were found, of which 3 were high severity, 7 were of medium severity and 4 were of low severity; 4 centralisation concerns were also identified by the audit. The auditors also strongly recommended the development of a comprehensive test suite to ensure the correct working of the protocol, as one is currently absent.
BACKGROUND
Users who deposit liquidity in a Uniswap V3 pool obtain an NFT corresponding to their position. The MetalSwap protocol allows users to stake this NFT in order to earn rewards provided by MetalSwap. Rewards are distributed according to the fees earned by the position as a proportion of all fees earned by the NFTs staked on MetalSwap. The MetalSwap protocol has the ability to boost rewards, and is also able to modify the price range of a staked NFT position at the user’s request.
SETTING & CAVEATS
This audit report mainly covers the contracts of the (at the time) private repository nft-staking-pool of the MetalSwap protocol, at commit number:
3e0810414fdb690b9b9b8fee6615d1956cdec512
Two auditors worked on the codebase for 5 days on the following contracts:
contracts/
├── LoopExecutionCollectAssign.sol
└── NFTStakingPool.sol
The issues found in the smart contracts were subsequently resolved by the team in commit number 1c5d8bd5e7e8440bda4bfa06a09f5f00ac1871a8. A test suite was added in commit number c6e30196c84528ba1ad4cfacf431218db216ac5a and was also reviewed.
The audit’s main target is security threats, i.e., what the community understanding would likely call “hacking”, rather than the regular use of the protocol. Functional correctness (i.e. issues in “regular use”) is a secondary consideration. Typically it can only be covered if we are provided with unambiguous (i.e. full-detail) specifications of what is the expected, correct behavior. In terms of functional correctness, we often trusted the code’s calculations and interactions, in the absence of any other specification. Functional correctness relative to low-level calculations (including units, scaling and quantities returned from external protocols) is generally most effectively done through thorough testing rather than human auditing.
PROTOCOL-LEVEL CONSIDERATIONS
Lack of test suite
Protocol Level Consideration | Status: INFO
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.
VULNERABILITIES & FUNCTIONAL ISSUES
This section details issues affecting the functionality of the contract. Dedaub generally categorizes issues according to the following severities, but may also take other considerations into account such as impact or difficulty in exploitation:
Issue resolution includes “dismissed” or “acknowledged” but no action taken, by the client, or “resolved”, per the auditors.
CRITICAL SEVERITY
[No critical severity issues]
HIGH SEVERITY
NFTStakingPool::setGovernance assigns DEFAULT_ADMIN_ROLE incorrectly
High | Status: RESOLVED
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.
Incorrect reward calculation
High | Status: RESOLVED
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.
No minimums are set when interacting with Uniswap V3 in the _addNFT and _removeNFT functions
High | Status: RESOLVED
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.
MEDIUM SEVERITY
Storage is read after having been set to 0
Medium | Status: RESOLVED
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.
Rewards calculation does not reflect the contribution of the users to the pool.
Medium | Status: RESOLVED
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:
NFTStakingPool::_loopAssignPartialRewards:935-936
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.
Unstaking cooldown can be circumvented
Medium | Status: RESOLVED
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.
NFTStakingPool::modifyPriceRange:1218-1281
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.
SafeERC20 wrappers are not ubiquitously used
Medium | Status: RESOLVED
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.
If NFTStakingPool contract runs out of a particular reward, entire reward mechanism becomes disabled
Medium | Status: RESOLVED
The _loopAssignPartialRewards function of the NFTStakingPool contract has the following check:
NFTStakingPool::_loopAssignPartialRewards:943-945
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.
NFTStakingPool::forcedUnstake* function do not update the state accordingly
Medium | Status: ACKNOWLEDGED
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.
A user might be able to extract more rewards than what would correspond to their position for the time it has been staked
Medium | Status: RESOLVED
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.
LOW SEVERITY
The activateRewardToken function can be used to deactivate a token
Low | Status: RESOLVED
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.
Member isActive of the RewardToken struct is not checked before being updated
Low | Status: RESOLVED
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.
assignPartialRewards and collectPartialFees do not check the state that the NFTStakingPool is in
Low | Status: RESOLVED
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.
Gas Optimization
Low | Status: RESOLVED
The function NFTStakingPool::_loopAssignPartialRewards implements an outer loop over the rewardTokens and an inner loop over the NFTs.
NFTStakingPool::_loopAssignPartialRewards:920-947
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);
}
}
CENTRALIZATION ISSUES
It is often desirable for DeFi protocols to assume no trust in a central authority, including the protocol’s owner. Even if the owner is reputable, users are more likely to engage with a protocol that guarantees no catastrophic failure even in the case the owner gets hacked/compromised. We list issues of this kind below. (These issues should be considered in the context of usage/deployment, as they are not uncommon. Several high-profile, high-value protocols have significant centralization threats.)
Admin can transfer out Ether and all ERC20 tokens
Centralization | Status: PARTIALLY RESOLVED
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.
Admin can transfer NFT to arbitrary address using forcedUnstake function
Centralization | Status: DISMISSED
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.
Admin can change important parameters of the system
Centralization | Status: PARTIALLY RESOLVED
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.
Users cannot unstake when the NFTStakingPool is paused
Centralization | Status: ACKNOWLEDGED
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.
OTHER / ADVISORY ISSUES
This section details issues that are not thought to directly affect the functionality of the project, but we recommend considering them.
Use of error codes in NFTStakingPool contract is not recommended
Advisory | Status: DISMISSED
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.
Inconsistency between different paused statuses prior to initialisation in the NFTStakingPool contract
Advisory | Status: RESOLVED
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.
Calling setMainPoolParameters and setSecondaryPoolParameters multiple times risks creating unintended consequences for the protocol
Advisory | Status: RESOLVED
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:
setMainPoolParameters
- feeTier
- _LPToken0 and _LPToken1
- _lastRewardDistributionTimestamp and _lastNFTPos
setSecondaryPoolParameters
- _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.
Lack of validations in setMainPoolParameters and setSecondaryPoolParameters functions
Advisory | Status: RESOLVED
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.
Overlapping functionality between setMainPoolParameters and setPauseStatuses functions
Advisory | Status: RESOLVED
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.
checkUpkeep function of the LoopExecutionCollectionAssign contract can return empty performData
Advisory | Status: RESOLVED
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.
No-op additions
Advisory | Status: RESOLVED
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.
Inconsistent revert
Advisory | Status: RESOLVED
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.
Misleading variable name
Advisory | Status: RESOLVED
In the function NFTStakingPool::getRewardTokensDetails the variable name _rewardPerRound is misleading and should be changed to _nextRewardEstimates.
Misleading event emission
Advisory | Status: RESOLVED
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.
Unnecessary storage reads
Advisory | Status: RESOLVED
The function NFTStakingPool::assignPartialRewards caches the storage values lastRewDistrTimestamp and roundDurationInSec but ends up not using the cached counterparts everywhere.
Counterintuitive order of checks
Advisory | Status: RESOLVED
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.
Compiler bugs
Advisory | Status: INFO
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.
DISCLAIMER
The audited contracts have been analyzed using automated techniques and extensive human inspection in accordance with state-of-the-art practices as of the date of this report. The audit makes no statements or warranties on the security of the code. On its own, it cannot be considered a sufficient assessment of the correctness of the contract. While we have conducted an analysis to the best of our ability, it is our recommendation for high-value contracts to commission several independent audits, a public bug bounty program, as well as continuous security auditing and monitoring through Dedaub Security Suite.
ABOUT DEDAUB
Dedaub offers significant security expertise combined with cutting-edge program analysis technology to secure some of the most prominent protocols in DeFi. The founders, as well as many of Dedaub’s auditors, have a strong academic research background together with a real-world hacker mentality to secure code. Protocol blockchain developers hire us for our foundational analysis tools and deep expertise in program analysis, reverse engineering, DeFi exploits, cryptography and financial mathematics.