Skip to main content

MetalSwap NFT Staking Pool

Smart Contract Security Assessment

May 30, 2024

MetalSwap

SUMMARY

ID
Description
STATUS

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

P1

Lack of test suite

P1PROTOCOL-LEVEL-CONSIDERATION

Lack of test suite
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:

Category
Description
CRITICAL
Can be profitably exploited by any knowledgeable third-party attacker to drain a portion of the system’s or users’ funds OR the contract does not function as intended and severe loss of funds may result.
HIGH
Third-party attackers or faulty functionality may block the system or cause the system or users to lose funds. Important system invariants can be violated.
MEDIUM
Examples:
  • User or system funds can be lost when third-party systems misbehave.
  • DoS, under specific conditions.
  • Part of the functionality becomes unusable due to a programming error.
LOW
Examples:
  • Breaking important system invariants but without apparent consequences.
  • Buggy functionality for trusted users where a workaround exists.
  • Security issues which may manifest when the system evolves.

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

H1

NFTStakingPool::setGovernance assigns DEFAULT_ADMIN_ROLE incorrectly

H1HIGH

NFTStakingPool::setGovernance assigns DEFAULT_ADMIN_ROLE incorrectly
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.

H2

Incorrect reward calculation

H2HIGH

Incorrect reward calculation
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.

H3

No minimums are set when interacting with Uniswap V3 in the _addNFT and _removeNFT functions

H3HIGH

No minimums are set when interacting with Uniswap V3 in the _addNFT and _removeNFT functions
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

M1

Storage is read after having been set to 0

M1MEDIUM

Storage is read after having been set to 0
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.

M2

Rewards calculation does not reflect the contribution of the users to the pool.

M2MEDIUM

Rewards calculation does not reflect the contribution of the users to the pool.
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.

M3

Unstaking cooldown can be circumvented

M3MEDIUM

Unstaking cooldown can be circumvented
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 (...) {
...
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);
}
...
}

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.

M4

SafeERC20 wrappers are not ubiquitously used

M4MEDIUM

SafeERC20 wrappers are not ubiquitously used
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.

M5

If NFTStakingPool contract runs out of a particular reward, entire reward mechanism becomes disabled

M5MEDIUM

If NFTStakingPool contract runs out of a particular reward, entire reward mechanism becomes disabled
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.

M6

NFTStakingPool::forcedUnstake* function do not update the state accordingly

M6MEDIUM

NFTStakingPool::forcedUnstake* function do not update the state accordingly
acknowledged

Acknowledged

According to the protocol team, the forcedUnstake* functions will only be used in an emergency situation, and as such it is assumed to be called only if the state of the smart contract is already degraded

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.

M7

A user might be able to extract more rewards than what would correspond to their position for the time it has been staked

M7MEDIUM

A user might be able to extract more rewards than what would correspond to their position for the time it has been staked
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

L1

The activateRewardToken function can be used to deactivate a token

L1LOW

The activateRewardToken function can be used to deactivate a token
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.

L2

Member isActive of the RewardToken struct is not checked before being updated

L2LOW

Member isActive of the RewardToken struct is not checked before being updated
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.

L3

assignPartialRewards and collectPartialFees do not check the state that the NFTStakingPool is in

L3LOW

assignPartialRewards and collectPartialFees do not check the state that the NFTStakingPool is in
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.

L4

Gas Optimization

L4LOW

Gas Optimization
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];
// Dedaub:

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.)

N1

Admin can transfer out Ether and all ERC20 tokens

N1CENTRALIZATION

Admin can transfer out Ether and all ERC20 tokens
partially resolved

Partially resolved

The capability of transferring ERC20 assets that are not owned by the NFTSTakingPool, i.e., approved amounts, has been removed.

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.

N2

Admin can transfer NFT to arbitrary address using forcedUnstake function

N2CENTRALIZATION

Admin can transfer NFT to arbitrary address using forcedUnstake function
dismissed

Dismissed

According to the protocol team the forcedUnstake function is a safety feature for emergency situations and will be controlled by a 2/3 multisig.

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.

N3

Admin can change important parameters of the system

N3CENTRALIZATION

Admin can change important parameters of the system
partially resolved

Partially resolved

After the changes implemented by the protocol team, the admin can still set the feeManagerAddress, the roundDurationInSec and the rewardRuntime.

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.

N4

Users cannot unstake when the NFTStakingPool is paused

N4CENTRALIZATION

Users cannot unstake when the NFTStakingPool is paused
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.

A1

A1ADVISORY

Use of error codes in NFTStakingPool contract is not recommended
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.

A2

Inconsistency between different paused statuses prior to initialisation in the NFTStakingPool contract

A2ADVISORY

Inconsistency between different paused statuses prior to initialisation in the NFTStakingPool contract
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.

A3

Calling setMainPoolParameters and setSecondaryPoolParameters multiple times risks creating unintended consequences for the protocol

A3ADVISORY

Calling setMainPoolParameters and setSecondaryPoolParameters multiple times risks creating unintended consequences for the protocol
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.

A4

Lack of validations in setMainPoolParameters and setSecondaryPoolParameters functions

A4ADVISORY

Lack of validations in setMainPoolParameters and setSecondaryPoolParameters functions
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.

A5

Overlapping functionality between setMainPoolParameters and setPauseStatuses functions

A5ADVISORY

Overlapping functionality between setMainPoolParameters and setPauseStatuses functions
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.

A6

checkUpkeep function of the LoopExecutionCollectionAssign contract can return empty performData

A6ADVISORY

checkUpkeep function of the LoopExecutionCollectionAssign contract can return empty performData
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.

A7

No-op additions

A7ADVISORY

No-op additions
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.

A8

Inconsistent revert

A8ADVISORY

Inconsistent revert
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.

A9

Misleading variable name

A9ADVISORY

Misleading variable name
resolved

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

A10

Misleading event emission

A10ADVISORY

Misleading event emission
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.

A11

Unnecessary storage reads

A11ADVISORY

Unnecessary storage reads
resolved

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

A12

Counterintuitive order of checks

A12ADVISORY

Counterintuitive order of checks
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.

A13

Compiler bugs

A13ADVISORY

Compiler bugs
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.