Nexus Mutual Staking v2

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.

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.

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.

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;

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++) {
  ...
}

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

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.

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.

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;

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.