Skip to main content
Dedaub

Nexus Mutual Staking v2

Smart Contract Security Assessment

July 12, 2022

Nexus_Mutual

SUMMARY


ABSTRACT

Dedaub was commissioned to perform a security audit of the Nexus Mutual Staking v2 protocol.

This covered Nexus Mutual’s Staking contracts at commit hash 14a7ed6f5b6df877a2cb7eb9fd390c9cb216ccb2 of the audit/nexus-v2 branch.

The audited contract list is the following:

contracts/modules/staking/StakingTypesLib.sol

contracts/modules/staking/StakingPool.sol

Three auditors worked on the codebase for a week.


SETTING AND CAVEATS

The audit’s main target is security threats, i.e., what the community understanding would likely call "hacking", rather than 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, quantities returned from external protocols) is generally most effectively done through thorough testing rather than human auditing. The scope of the audit includes smart contract code. Interactions with off-chain (front-end or back-end) code are not examined other than to consider entry points for the contracts, i.e., calls into a smart contract that may disrupt the contract’s functioning.

We should emphasize that, independently of any audit, thoroughly testing all contracts is essential for discovering functional bugs that often also lead to security vulnerabilities. We believe that a large number of the issues listed below could have been discovered by a comprehensive test suite.


VULNERABILITIES & FUNCTIONAL ISSUES

This section details issues that affect 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”, by the client, or “resolved”, per the auditors.


CRITICAL SEVERITY

C1

Users can overwrite the pool manager’s deposit records

C1CRITICAL

Users can overwrite the pool manager’s deposit records
open

In depositTo(), request.tokenId can be set to 0 to indicate that a new NFT is requested to be minted (instead of using an already created one).

StakingPool.sol::depositTo():370

// deposit to token id = 0 is not allowed
// we treat it as a flag to create a new token
bool isNewToken = request.tokenId == 0;

When a new token is created, an empty deposit struct is initialized with the user data.

StakingPool.sol::depositTo():389-391

Deposit memory deposit = isNewToken
? Deposit(_accNxmPerRewardsShare, 0, 0, 0)
: deposits[request.tokenId][request.trancheId];

This struct is later stored in the global deposits mapping where all users' records are kept along with the manager’s records. However, the struct is stored at position request.tokenId, even when the latter is 0, hence overwriting the slot reserved for the manager.

StakingPool.sol::depositTo():412

// Dedaub: Users can overwrite manager's deposit records
deposits[request.tokenId][request.trancheId] = deposit;

Note that this issue is related to M1 (below) that also concerns the use of request.tokenId in depositTo.



HIGH SEVERITY

H1

Wrong bitwise operations make several functions unusable

H1HIGH

Wrong bitwise operations make several functions unusable
open

The getters and setters getItemAt(), setItemAt() and newCoverAmount() in StakingTypesLib.sol are intended to be used for extracting or storing a variable inside custom types that can hold multiple variables at once. For example, a custom type of uint256 can hold up to 8x uint32 variables. To implement this functionality, bitwise operations are used to move and place the variables in the right position.

However, the provided implementation doesn’t achieve the desired functionality as it lacks proper type casts and has a number of reversed bitwise shift operations.

A detailed overview of the issues along with our suggestions for fixes is listed below:

StakingTypesLib.sol

function newCoverAmount(
uint48 coverAmount,
uint16 bucketId
) internal pure returns (CoverAmount) {
// Dedaub: uint64() casting before shifting `coverAmount`
// return CoverAmount.wrap( ( coverAmount << 16 ) | bucketId ); // Old
return CoverAmount.wrap( ( uint64(coverAmount) << 16 ) | bucketId );
^^^^^^^ ^
}
----------------------------------------------------------------------------------
function getItemAt(
CoverAmountGroup items,
uint index
) internal pure returns (CoverAmount) {
...

// Dedaub: Bitwise SHIFT RIGHT instead of SHIFT LEFT
// uint64 item = uint64( underlying << (index * 64) ); // Old
uint64 item = uint64( underlying >> (index * 64) );
^^
...
}
----------------------------------------------------------------------------------
function setItemAt(
CoverAmountGroup items,
uint index,
CoverAmount item
) internal pure returns (CoverAmountGroup) {
// Dedaub: uint() casting before shifting `type(uint64).max` &&
No need for uint64() cast for `index * 64`
// uint mask = ~( type(uint64).max << uint64(index * 64) ); // Old
uint mask = ~( uint( type(uint64).max ) << (index * 64) );
^^^^^ ^ ^^^^^^
// Dedaub: Change uint() scope to contain `item` before shifting &&
No need for uint64() cast for `index * 64`
// uint itemUnderlying =
uint( CoverAmount.unwrap(item) << uint64(index * 64) ); // Old
uint itemUnderlying =
uint( CoverAmount.unwrap(item) ) << (index * 64) ;
^ ^^^^^^ ^
...
}
----------------------------------------------------------------------------------
function getItemAt(
BucketTrancheGroup items,
uint index
) internal pure returns (uint32) {
...
// Dedaub: Bitwise SHIFT RIGHT instead of SHIFT LEFT
// return uint32( underlying << (index * 32) ); // Old
return uint32( underlying >> (index * 32) );
^^
}
----------------------------------------------------------------------------------
function setItemAt(
BucketTrancheGroup items,
uint index,
uint32 value
) internal pure returns (BucketTrancheGroup) {
// Dedaub: uint() casting before shifting `type(uint32).max` &&
No need for uint32() cast for `index * 32`
// uint mask = ~( type(uint32).max << uint32(index * 32) ); // Old
uint mask = ~( uint( type(uint32).max ) << (index * 32) );
^^^^^ ^ ^^^^^^

// Dedaub: There is a missing shifting operation for positioning 'value'
at the right place before performing bitwise OR
uint itemUnderlying = uint( value ) << (index * 32);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

// Dedaub: Change `value` with `itemUnderlying
// uint groupUnderlying =
BucketTrancheGroup.unwrap(items) & mask | value; // Old
uint groupUnderlying =
BucketTrancheGroup.unwrap(items) & mask | itemUnderlying;
^^^^^^^^^^^^^^
...
}

—-------------------------------------------------------------------

Furthermore, there are two more issues involving bitwise operations that need attention in StakingPool.sol.

Here amountPerTranche should hold the cover allocation for a specific tranche indexed by i and extracted from packedCoverTrancheAllocation, but instead corrupt data will be retrieved having the index multiplied by 8 and casting the variable with uint32().

StakingPool::deallocateStakeForCover():723

// Dedaub: Shift right needs to be `(i * 32)` instead of `(i * 8)`
uint amountPerTranche = uint32(packedCoverTrancheAllocation >> (i * 8));
^

In the following part, uint() casting is required in order to properly shift trancheAllocation without destroying the data it holds.

StakingPool::updateExpiringCoverAmounts():961

// Dedaub: uint() casting around `trancheAllocation` is required  &&
No need for uint32() cast for `i * 32`
// packedCoverTrancheAllocation |=
trancheAllocation << uint32(i * 32); // Old
packedCoverTrancheAllocation |=
uint(trancheAllocation) << (i * 32);
^^^^^ ^ ^^^^^^

Note that all above suggestions have not been tested and should be verified by the developers.



MEDIUM SEVERITY

M1

depositTo() will always revert when invoked by a new depositor

M1MEDIUM

depositTo() will always revert when invoked by a new depositor
open

A new depositor can call depositTo() and provide a DepositRequest with tokenId == 0. This should cause the contract to mint a new NFT and send it to the provided destination.

When request.tokenId == 0, the if(isNewToken) code block below will run, and this will call _mint(to,request.tokenId). However the NFT with tokenId == 0 is already created in the initialize() function, and sent to the manager. Therefore, _mint() will fail and depositTo() will revert every time a new depositor calls it.

StakingPool::depositTo():372

bool isNewToken = request.tokenId == 0;

if (isNewToken) {
tokenIds[i] = totalSupply++;
address to = request.destination == address(0)
? msg.sender
: request.destination;
_mint(to, request.tokenId);
} else {
tokenIds[i] = request.tokenId;
}

The above issue can be resolved by calling _mint(to,tokenIds[i]) instead.

M2

No update will ever be possible for tranches

M2MEDIUM

No update will ever be possible for tranches
open

updateTranches() provides an important functionality as it updates the accounting of the rewards and shares. It is called in almost every other function that makes a deposit, a withdrawal, stake allocation and deallocation and many other operations.

However, the function will return immediately if the value of firstActiveBucketId is 0. This is done to prevent updates when a pool is new. But since this variable is initialized with 0 and only updated later in the same function, the updating code is never executed, leading the function to always return.

StakingPool::updateTranches():213

  // Dedaub: `firstActiveBucketId` will always be 0
uint _firstActiveBucketId = firstActiveBucketId;
...
// skip if the pool is new
if (_firstActiveBucketId == 0) {
return;
}
...
if (_rewardsSharesSupply == 0) {
firstActiveBucketId = currentBucketId;
...
}
...
firstActiveTrancheId = _firstActiveTrancheId;
firstActiveBucketId = _firstActiveBucketId;

M3

getTotalCapacities() will always revert

M3MEDIUM

getTotalCapacities() will always revert
open

This function calculates and returns the total capacity that a number of tranches can provide per product.

In the function there is a loop that iterates over the range of the tranches that a cover will be applied to. Since <= is used instead of < in the loop, this will exceed the valid trancheCount by 1 and will lead the function to revert.

StakingPool::getTotalCapacities():878

// Dedaub: Loop uses <= instead of <
for (uint i = 0; i <= trancheCount; i++) {
...
}

M4

Deposit extensions when initial tranche has expired are impossible due to unallocated array

M4MEDIUM

Deposit extensions when initial tranche has expired are impossible due to unallocated array
open

The function extendDeposit() tries to create a new DepositRequest when the initial tranche has expired, in order to create a new deposit which is equal to the withdrawn amounts plus the topUpAmount.

However, the depositRequests array never gets allocated before the new DespositRequest is assigned, which will lead the function to revert under these circumstances.

StakingPool::getTotalCapacities():1041

// Dedaub: `depositRequests` doesn't get allocated
DepositRequest[] memory depositRequests;
depositRequests[0] = (
DepositRequest(
withdrawnStake + topUpAmount, // amount
newTrancheId, // trancheId
tokenId, // tokenId
msg.sender // destination
)
);

M5

Incorrect use of constructor in a proxy implementation

M5MEDIUM

Incorrect use of constructor in a proxy implementation
open

A limitation of proxied contracts is that they cannot use constructors, since the constructor would modify the implementation storage instead of the proxy storage.

Immutable variables are not affected by this limitation, since they are not stored in storage but compiled in the code. As a consequence, the initialization of nxm, coverContract and tokenController in StakingPool::constructor is valid, since these variables are immutable.

However, StakingPool::constructor also calls the parent SolmateERC721 constructor:

constructor (
string memory _name,
string memory _symbol,
address _token,
address _coverContract,
ITokenController _tokenController
) SolmateERC721(_name, _symbol) {
nxm = INXMToken(_token);
coverContract = _coverContract;
tokenController = _tokenController;
}

But SolmateERC721 does not use immutable variables, so effectively name and symbol will not be properly stored.

We recommend against using a constructor and initialize all variables in StakingPool::initialize, to ensure that no issues with the proxy pattern arise.



LOW SEVERITY

L1

Lack of reentrancy protection

L1LOW

Lack of reentrancy protection
open

StakingPool functions contain no protection against reentrancy. Although we could not find specific reentrancy vulnerabilities in the code, such issues are hard to spot and easy to be introduced in future changes of the code (including changes to external contracts), so special care should be taken.

As an example, StakingPool::depositTo() performs some updates after transferring the NXM tokens.

function depositTo(DepositRequest[] memory requests) public returns (uint[] memory tokenIds) {
...

// transfer nxm from staker and update pool deposit balance
tokenController.depositStakedNXM(msg.sender, totalAmount, poolId);

// update globals
activeStake = _activeStake;
stakeSharesSupply = _stakeSharesSupply;
rewardsSharesSupply = _rewardsSharesSupply;
}

Now imagine that in a future version, ERC-777 hooks are implemented in NXMToken. Such hooks could allow a malicious user to reenter inside depositTo(), and after the nested calls the variables activeStake, stakeSharesSupply and rewardsSharesSupply would have incorrect values.

To avoid such issues, we recommend using reentrancy guards and/or check-effects- interaction patterns, performing all updates before any interaction. In cases when reentrancy is impossible due to certain assumptions (eg. that a certain function makes no external interaction), it might be useful to document such assumptions to avoid being violated in future versions of the code.



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

Allocating and deallocating stake functions have some redundant calculations

A1ADVISORY

Allocating and deallocating stake functions have some redundant calculations
info

The calculation of trancheCount in allocateStake() and deallocateStakeForCover() can be streamlined for better readability. trancheCount doesn’t depend on timestamps although they are being used in its calculation. More specifically, we refer to the following code segments:

StakingPool.sol::allocateStake():589

// Current code
uint gracePeriodExpiration =
block.timestamp + request.period + request.gracePeriod;
uint firstTrancheIdToUse = gracePeriodExpiration / TRANCHE_DURATION;
uint trancheCount = (block.timestamp / TRANCHE_DURATION + MAX_ACTIVE_TRANCHES) - firstTrancheIdToUse + 1;

// Dedaub: We propose (but developers should verify) that this is equivalent to
uint trancheCount = MAX_ACTIVE_TRANCHES - (request.period + request.gracePeriod) / TRANCHE_DURATION + 1;

StakingPool.sol::deallocateStakeForCover():706

// Current code
uint gracePeriodExpiration =
coverStartTime + request.period + request.gracePeriod;
uint firstTrancheIdToUse = gracePeriodExpiration / TRANCHE_DURATION;
uint trancheCount = (coverStartTime / TRANCHE_DURATION + MAX_ACTIVE_TRANCHES) - firstTrancheIdToUse + 1;
// Dedaub: We propose (but developers should verify) that this is equivalent to
uint trancheCount = MAX_ACTIVE_TRANCHES - (request.period + request.gracePeriod) / TRANCHE_DURATION + 1;

A2

Compiler bugs

A2ADVISORY

Compiler bugs
info

The code is compiled with Solidity 0.8.9 or higher. For deployment, we recommend no floating pragmas, i.e., a specific version, so as to be confident about the baseline guarantees offered by the compiler. Version 0.8.9, in particular, has some known bugs, which we do not believe to 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.

Dedaub