MetaPool

Story Integration

6.1 Incorrect implementation of StakedIP::mint

The mint function is intended to accept shares as its first argument and return the required number of assets. Its purpose is to mint the exact number of shares for the receiver by depositing the corresponding amount of underlying assets. However, the function is incorrectly overridden:

function mint(uint256 _assets, address _receiver) public override returns (uint256) {
   _transferAndUnwrap(msg.sender, _assets);


   uint256 shares = previewMint(_assets);
   _deposit(msg.sender, _receiver, _assets, shares);


   return shares;
}

The implementation contains several critical issues:

1. The function incorrectly accepts assets instead of shares as input, contradicting its intended behavior; 2. It transfers the received _assets(which are the shares) instead of calculating and transferring the required assets for minting the specified shares; 3. It incorrectly computes shares using previewMint, whereas previewMint actually returns the amount of assets needed to mint the exact number of shares;

Recommendation

Update the mint function to correctly accept shares as input and ensure it transfers the required assets from the user:

function mint(uint256 shares, address receiver) public override returns (uint256) {
   uint256 assets = previewMint(shares);

   _transferAndUnwrap(msg.sender, assets);


   _deposit(msg.sender, receiver, assets, shares);


   return assets;
}

Resolution

Fixed in commit d91cdae

6.2 Users cannot deposit with WIP

The StakedIP contract allows users to deposit into the vault using WIP. The deposit function is designed to transfer WIP to the contract and subsequently unwrap it into IP:

function depositIP(address _receiver) public payable returns (uint256) {
   uint256 shares = previewDeposit(msg.value);


   _deposit(address(this), _receiver, msg.value, shares);

   return shares;
}

function _transferAndUnwrap(address _from, uint256 _amount) private {
   address asset = asset();
   IERC20(asset).safeTransferFrom(_from, address(this), _amount);
==>    IWIP(asset).withdraw(_amount);
}

However, the StakedIP contract incorrectly prevents the WIP contract from sending native tokens due to a restrictive condition in the receive fallback function:

receive() external payable {
==> require(msg.sender != asset()
           && msg.sender != withdrawal, InvalidDepositSender(msg.sender));
   depositIP(msg.sender);
}

Since WIP must send native tokens when unwrapping, this restriction causes the withdraw function to fail, leading to a permanent failure of deposits using WIP. As a result, users cannot deposit through WIP

Recommendation

Revise the revert condition in the receive fallback function to allow the WIP contract to send native tokens to StakedIP.

Resolution

Fixed in commit b9fc12d

6.3 Excessive IP tokens in Withdrawal contract cannot be restaked to StakedIP

When withdrawal requests are made, the operator can cover withdrawals by calling coverWithdrawals, which transfers IP tokens to the Withdrawal contract. The Withdrawal contract may hold more IP tokens than required for withdrawals. In such cases, these excess tokens should be restaked back into the StakedIP vault, hence the utility of sendIPToRestake function. There are two key implementation issues that prevent this from happening:

1. The sendIPToRestake function is restricted to being called only by the StakedIP contract:

function sendIPToRestake(uint256 _amount) external onlyStaking {
   if (address(this).balance <= totalPendingWithdrawals) revert NotEnoughIPtoStake(_amount, 0);


   uint256 ipRemaining = address(this).balance - totalPendingWithdrawals;


   if (_amount > ipRemaining) revert NotEnoughIPtoStake(_amount, ipRemaining);


   stIP.sendValue(_amount);
}

This restriction is problematic because the StakedIP contract cannot directly call the Withdrawal contract to retrieve excess tokens. Instead, the Withdrawal contract should be responsible for transferring these tokens back to StakedIP. 2. The receive fallback function in StakedIP incorrectly blocks the Withdrawal contract from sending IP tokens:

receive() external payable {
   require(msg.sender != asset() && msg.sender != withdrawal, InvalidDepositSender(msg.sender));
   depositIP(msg.sender);
}

This restriction prevents the Withdrawal contract from restaking its excess IP tokens by calling sendIPToRestake.

Due to these implementation flaws, any excess IP tokens held by the Withdrawal contract are stuck and cannot be restaked.

Recommendation

Revisit the revert condition of receive fallback in StakedIP. Also, remove the onlyStaking modifier from sendIPToRestake.

Resolution

Fixed in commit 13e6f70

6.4 Remove validator operation is buggy, creates duplicate entry in the array

_removeValidator function in StakedIPVaultOperations does not correctly delete the item from the validators array. As a result, the same item could result in duplicate entries for the same validator.

function _removeValidator(bytes memory _validatorCmpPubkey) private {
   uint256 _validatorsLength = validatorsLength;


   require(_validatorsLength > 1, ValidatorsEmptyList());


   uint256 _index = getValidatorIndex(_validatorCmpPubkey);
   Validator memory validator = _validators[_index];


   require(validator.targetStakePercent == 0, ValidatorHasTargetPercent(validator));


   if (_index != _validatorsLength - 1) {
==>    _validators[_index] = _validators[_validatorsLength - 1];
   } else {
       delete _validators[_index];
   }
  //...
}

In the case where, the element being deleted is not the last element of the array, the last element of the array is copied into the index of the element being deleted. But, as the last element is not popped from the array, this will result in two instances of the same validator.

Example:
validators[
          {index:0,pubkey:"A"},
          {index:1,pubkey:"B"},
          {index:2,pubkey:"C"}
        ];


// Remove pubkey=="B" would result as below

validators[
          {index:0,pubkey:"A"},
          {index:2,pubkey:"C"},
          {index:2,pubkey:"C"}
        ];

This will impact how the validators array is used in the contract. For example, redistribute function will return incorrect results.

Recommendation

Pop the last element of the validators array.

Resolution

Fixed in commit 16ab74e

6.5 RewardsManager cannot accept Native tokens

RewardsManager contract tracks the accrued rewards which are then restaked as part of compounding. A treasury fee is deducted from the rewards which is sent to the treasury address.

function sendRewardsAndFees() external {
   (uint rewards, uint treasuryFee) = getManagerAccrued();


   IStakedIP(stakedIP).injectRewards{ value: rewards }();
   payable(treasury).sendValue(treasuryFee);


   emit SendRewards(msg.sender, rewards);
   emit SendFees(msg.sender, treasuryFee);
}

function getManagerAccrued() public view returns (uint rewards, uint treasuryFee) {
   uint balance = address(this).balance;
   treasuryFee = (balance * rewardsFeeBp) / ONE_HUNDRED;
   rewards = balance - treasuryFee;
}

The issue arises from how the accrued amount is computed in the contract. As the RewardsManager contract does not have a receive or payable functions, there seems no way in how the balance of native tokens in the contract will accrue for distribution.

This issue impacts the working of reward accruals for the protocol.

Recommendation

Add receive function so that rewards can be received into the contract from story protocol.

Resolution

Fixed in commit 0be2869

6.6 User staked tokens could be locked in Withdraw contract

StakedIP contract overrides the _withdraw function of ERC4626 library contract and implements a withdrawal queue that has a waiting period before the request can be completed. On _withdraw call, the accounting for shares and assets are updated and a request is submitted to the Withdrawal contract. As a result, the withdrawal will not be completed in a single transaction.

function _withdraw(
   address _caller,
   address _receiver,
   address _owner,
   uint256 _assets,
   uint256 _shares
) internal override onlyFullyOperational {
   if (_shares == 0) revert InvalidZeroAmount();
   if (_caller != _owner) _spendAllowance(_owner, _caller, _shares);


   _burn(_owner, _shares);
   totalUnderlying -= _assets;

   IWithdrawal(withdrawal).requestWithdraw(_assets, _caller, _receiver);

   emit Withdraw(msg.sender, _receiver, _owner, _shares, _assets);
}

When the user tries to process the pending withdrawal request, the issues arises if the _receiver setup in the request is a smart contract that does not accept native tokens. In that case, the completeWithdraw will revert and the funds will remain locked in the Withdrawal contract.

function completeWithdraw(uint256 _request_id) external {
   WithdrawRequest memory _pendingUserWithdraw = _userPendingWithdrawals[msg.sender][_request_id];
   //...
   payable(_pendingUserWithdraw.receiver).sendValue(_pendingUserWithdraw.amount);
   //...
}

Recommendation

Add an option for the request owner to send WIP instead of IP tokens.

Resolution

Fixed in commit d700889

6.7 Use of transfer when sending native tokens

In order to transfer native tokens, the StakedIP contract is currently using the transfer function. This is not a recommended function to move native tokens due to the fixed gas limit of 2300. As the gas cost is subject to change as part of infra upgrades.

Also, if the recipient is a smart contract like a multi-sign wallet, 2300 gas will not cover the gas required to complete the transfer leading to revert.

function mintIP(uint256 _shares, address _receiver) external payable returns (uint256) {
   //...
   uint256 change = msg.value - assets;
   if (change > 0) {
       payable(msg.sender).transfer(change);
   }
   //...
}

Recommendation

Use .sendValue function to move native tokens like in other places in the contract.

Resolution

Fixed in commit 735a14f

6.8 Emit event only when rewards are disbursed

In RewardsManager contact, sendRewardsAndFees function can be called by anyone to process the accrued rewards. Rewards are injected to the StakedIP contract and the applicable fee is transferred to the treasury address.

function sendRewardsAndFees() external nonReentrant {
   (uint256 rewards, uint256 treasuryFee) = getManagerAccrued();

   if (rewards > 0) IStakedIP(stakedIP).injectRewards{ value: rewards }();
   if (treasuryFee > 0) payable(treasury).sendValue(treasuryFee);

==> emit SendRewardsAndFees(msg.sender, rewards, treasuryFee);
}

The event SendRewardsAndFees should be emitted only when accrued rewards are processed. In the current implementation, the event is emitted even when there are no accrued rewards.

Recommendation

emit SendRewardsAndFees event only when rewards > 0.

Resolution

Fixed in commit 0eed09c

6.9 IStakedIPVaultOperations interface is not used

Functions of IStakedIPVaultOperations interface are migrated from dedicated operations contract to StakedIP contract, but the contract does not derive from IStakedIPVaultOperations.

interface IStakedIPVaultOperations {
 function getValidatorIndex(bytes memory _validatorUncmpPubkey) external view returns (uint256);

 function isValidatorListed(bytes memory _validatorUncmpPubkey) external view returns (bool);
}

Recommendation

StakedIP contract should inherit from IStakedIPVaultOperations interface.

Resolution

Removed the interface

6.10 Unused imports/Error in Withdrawal contract

The Withdrawal contract has imports from other files, but they were never used in the contract.

• StakedIP;
• ERC4626Upgradeable;

Like wise, the below error was also not used in the contract.

• WithdrawAlreadyCompleted;

Recommendation

Remove redundant imports or objects from the contract.

Resolution

Fixed in commit edf0024

6.11 StakedIP is not fully ERC4626-compatible

In the _deposit function, the first parameter is the caller of the function. But, in the case of depositIP and mintIP, the address of the contract is being passed as a caller instead of msg.sender.

function depositIP(address _receiver) public payable returns (uint256) {
   uint256 shares = previewDeposit(msg.value);

   _deposit(address(this), _receiver, msg.value, shares);

   return shares;
}

While the contract can indeed deposit IP in the staking setup via setupStaking, users can still call the function to deposit IP and in such cases, the depositor will be logged as the contract’s address instead of the caller.

In addition, the first parameter of the Withdraw event in _withdraw is emitted with the msg.sender directly, while the function already accepts the caller in its first argument and that argument should be used instead.

Recommendation

Consider:

1. Passing msg.sender as caller for _deposit in the depositIP function. 2. Updating the first argument of the Withdraw event emitted in _withdraw from msg.sender to _caller.

Resolution

Fixed in commit cdd6806

6.12 getValidatorIndex should iterate over validatorsLength

The StakedIP contract tracks the current number of validators using validatorsLength. While the max number of validators is 10, In the case where the current number is smaller, the getValidatorIndex function is still iterating over the full length of the array. Instead, while searching for validator index, the logic can iterate up to validatorsLength instead of validators.length.

function getValidatorIndex(bytes memory _validatorCmpPubkey) public view returns (uint256) {
   Validator[MAX_VALIDATORS] memory validators = _validators;

   bytes32 _validatorPubkeyHash = keccak256(_validatorCmpPubkey);
   for (uint256 i = 0; i < validators.length; ++i) {
       if (keccak256(validators[i].cmpPubkey) == _validatorPubkeyHash) {
           return i;
       }
   }

   revert ValidatorNotFount(_validatorCmpPubkey);
}

Recommendation

Update for loop to iterate on validatorsLength value instead of validators.length.

Resolution

Fixed in commit 55916ab

6.13 injectRewards should be callable only from RewardManager contract

When sendRewardsAndFees function is called on the RewardManager contract, the reward funds are sent from the RewardManager contract to the StakedIP contract.

function sendRewardsAndFees() external nonReentrant {
   (uint256 rewards, uint256 treasuryFee) = getManagerAccrued();

   if (rewards > 0) IStakedIP(stakedIP).injectRewards{ value: rewards }();
   //...
}

But, in the StakedIP contract, there is no access control assigned to the injectRewards function. As a result any one can call the function.

function injectRewards() external payable {
   require(msg.value >= minDepositAmount, LessThanMinDeposit());
   totalUnderlying += msg.value;
}

Recommendation

Assign a modifier that will restrict the access for injectRewards to RewardManager contract only.

Resolution

Fixed in commit 090f129

6.14 Ownership is more secure with two-step transfers

As owner is a critical role in managing admin functions of the protocol, it is recommended to implement a two step transfer process. In the first step, the current owner initiates the process for the new account to take over the ownership. As a second step, the new account will claim the ownership. This ensures that the caller of the claiming function has control on the new account.

contract RewardsManager is IRewardsManager, Ownable, ReentrancyGuard {

Recommendation

Consider using the Ownable2Step library contract of openzeppelin instead of Ownable. Likewise for StakedIP and Withdrawal contracts, consider using the Ownable2StepUpgradeable library contract.

Resolution

Fixed in commit cfb9d4d

6.15 Unlocked and multiple solidity versions

It is recommended to use locked and single solidity versions in the project. The below two interface files are using an unlocked and older version of Solidity.

• IWIP.sol;
• IIPTokenStaking.sol;

pragma solidity ^0.8.23;

Recommendation

It is recommended to use a single and locked version of Solidity.

Resolution

Fixed in commit 41c0b29