Verio

IP

6.1 In certain cases, removing a validator will stop Delegator stake and unstake operations from finalizing until a new validator is added

Whenever enough funds are gathered in the StakePool contract, the next stake operation will trigger the handleStake(...) function on the Delegator contract. Based on the current distribution, this function will deposit the tokens into an appropriate delegation maturity. The validator that will be chosen for this delegation is selected in the _nextStakeValidator(...) function. Once the validator is picked, the ID is updated for the validator that will be picked on the next stake operation.

function _nextStakeValidator() internal returns (bytes memory) {
    // ...
    // @audit Pick validator for the current delegation, based on the previously
    // set stakeIdx.
    bytes memory validatorPubkey =
        validatorRegistry.validators[validatorRegistry.stakeIdx];

    // @audit Update stakeIdx to prepare for the next time this function is
    // called. IDs iterate over the validators list in a round-robin fashion.
    validatorRegistry.stakeIdx = (validatorRegistry.stakeIdx + 1)
        % validatorRegistry.validators.length;
    
    return validatorPubkey;
}

The problem in the _nextStakeValidator(...) function is that, in certain cases, it might revert due to out-of-bound array access to the validators array.

Consider the following scenario: The validatorRegistry.validators array contains the following validators.

Validator: [A, B, C, D, E]
Array IDX:  0, 1, 2, 3, 4

During the last call to _nextStakeValidator(...), the validatorRegistry.stakeIdx has been set to 4. This means that validator E will be chosen for the next delegation. An issue arises if one of the validators is removed from the list before the next delegation happens. For this example, any validator can be removed, so let us assume that B is removed. This is the updated validatorRegistry.validators array.

Validator: [A, E, C, D]
Array IDX:  0, 1, 2, 3

As a result of calling removeValidator(...), the validator E has been moved in the place of the removed B. The problem is that the stakeIdx still points to index 4, which is no longer in the array. Once the _nextStakeValidator(...) function is called again to pick the validator, it will access an out-of-bound array index, and the execution will revert. The same problem is present in the _nextUnstakeValidator(...) function. Due to this, the handleStake(...) and handleUnstake(...) operations won't work until a new validator is added to the list.

Recommendation

Consider revisiting the stakeIdx logic so that removing the validator does not result in out-of-bound access.

Resolution

Fixed in commit: aedd7cb. Now if removing a validator would result in an out-of-bounds index, the index is reset to 0. As a side effect of the removal process, the order of validators in the selection sequence may still be altered. A new check has also been added to ensure that there will always be at least 1 validator to stake with.

6.2 Potential for stuck funds when removing a bondedMaturity

The removeBondedMaturity will remove a maturity from the delegatorState.bondedMaturities array without checking if that particular maturity still has funds deposited into it.

Users and validators that deposited their funds inside that maturity will have their funds stuck and they are unable to withdraw.

Inside _handleBondedUnstakes(...) function from the Delegator contract, we loop through the active bonded maturities array (uint256 i = 0; i < delegatorState.bondedMaturities.length; i++) before attempting to withdraw. If a certain bonded maturity is removed, then the protocol will not process withdrawals from it anymore.

Although the admin could re-add the same bondedMaturity with the same configuration in order to reenable withdrawals, this can lead to reputational damage if validators & users are temporarily unable to withdraw from the protocol.

Recommendation

Consider adding a check inside the removeBondedMaturity(...) to ensure that the maturity distribution is empty before removing it.

Resolution

Fixed in commit: ae68927. See [Info] Adding, removing, and setting new distribution configs for bonded maturities will break the protocol's invariants issue for details.

6.3 The IP token withdrawals are rounded in the wrong direction

Whenever the users wish to exchange their vIP tokens back to IP tokens, they can call the unstake(...) function from the StakePool contract. Based on the provided vIP token amount, the calculateIPWithdrawal(...) function will compute the amount of IP tokens the user can withdraw from the WithdrawalPool after the unbounding period. The problem with the calculateIPWithdrawal(...) function is that it rounds up in favor of the user.

function calculateIPWithdrawal(uint256 _vIPAmount)
    public
    view
    override
    returns (uint256)
{
    uint256 totalStakedIP = _getTotalStake();
    uint256 totalSupply = _getTotalSupply();

    // ... 

    // @audit The total IP after unstake is rounded down.
    uint256 totalIPAfterUnstake = (totalStakedIP + address(this).balance)
        * (totalSupply - _vIPAmount) / totalSupply;

    // @audit-issue As a result, the returned IP amount to withdraw
    // is slightly higher than it should be. 
    return totalStakedIP + address(this).balance - totalIPAfterUnstake;
}

The rounding error can be considered a dust amount. In the most extreme scenario, if many deposits are assumed, after all users withdraw (with no inflow of rewards), the last user to withdraw will be able to get back slightly less than his initial deposit. The invariant in which 1 vIP token is worth at least 1 IP token will be broken.

Recommendation

To prevent any form of insolvency issues, it is considered a best practice to always round in favor of the protocol. Consider rounding up the totalIPAfterUnstake calculation so that the end result is rounded down.

Resolution

Fixed in commit: 92a7b44. A new formula to calculate the totalIPAfterUnstake has been applied. In this version, the amount is always rounded up.

6.4 The claimRewards(...) function subtracts the fee twice

The claimRewards() function is supposed to transfer a fee to the treasury and check if the insurancePool has debt. If it has, the funds in the RewardPool are meant to cover as much as possible from the debt. If the balance of the RewardPool exceeds the debt, then the difference will be transferred as rewards to the StakePool contract.

The issue is that the fee is subtracted from the address(this).balance twice, so the contract artificially reduces its perceived balance. In some cases, this may prevent RewardPool from covering all the debt of the InsurancePool (although the contract does have all the necessary funds) and transfer excess rewards to the StakePool.

function claimRewards() external override nonReentrant onlyStakePool {
    // ...
    uint256 fee = address(this).balance * rewardPoolState.feeBps / 10000;
    // @audit Fee is first sent to the treasury.
    (bool success,) = payable(treasury).call{value: fee}("");
    require(success, "RewardPool: failed to transfer fee to treasury");

    uint256 insurancePoolDebt = insurancePool.getDebt();
    if (insurancePoolDebt > 0) {
        // @audit-issue Since fee was already sent; address.balance is
        // already lower. The fee is subtracted for the second time.
        uint256 balanceAfterFee = address(this).balance - fee;
        uint256 fundAmount = insurancePoolDebt > balanceAfterFee
            ? balanceAfterFee
            : insurancePoolDebt;

        insurancePool.fund{value: fundAmount}();
    }
    // ...
}

Recommendation

To have a correct representation of the actual balance of the contract balanceAfterFee should be equal to address(this).balance without subtracting the fee again.

6.5 Adding, removing, and setting new distribution configs for bonded maturities will break the protocol's invariants

When deploying the protocol, the _trySetDistribution(...) function from the Delegator contract validates that the maturity durations are stored in ascending order and the BPS sum of all maturities is 10000.

function _trySetDistribution(MaturityConfig[] calldata _maturityConfigs)
    internal
{
    // ...
    uint256 lastMaturityDuration = 0;
    uint256 bpsSum = 0;

    for (uint256 i = 0; i < _maturityConfigs.length; i++) {
        if (
            _maturityConfigs[i].maturity
                == IIPTokenStaking.StakingPeriod.FLEXIBLE
        ) {
            // ...                
            // @audit BPS for every maturity is summed.
            bpsSum += _maturityConfigs[i].bps;
        } else {
            // @audit Maturities are enforced to be in ascending order.
            require(
                _maturityConfigs[i].duration > lastMaturityDuration,
                "Delegator: Maturity durations must be in ascending order"
            );
            // ... 
            lastMaturityDuration = _maturityConfigs[i].duration;
            // @audit BPS for every maturity is summed.
            bpsSum += _maturityConfigs[i].bps;
        }
    }
    // @audit Total distribution must equal 100%
    require(bpsSum == 10000, "Delegator: Maturity bps must sum to 10000");
    // ...
}

The problem is that the other functions that modify the maturity configuration, such as addBondedMaturity and removeBondedMaturity functions, do not consider the bps being added or removed. For example, after the protocol has been deployed, it has four maturities [FLEXIBLE, SHORT, MEDIUM, LONG], and their bps sum == 10000. If the admin adds a new maturity, the addBondedMaturity doesn't redistribute the bps to accommodate the newly added maturity and enforces that their sum is still 10000 bps. The sum of the maturities will exceed 10.000 bps. The same thing happens when maturity is removed via removeBondedMaturity function.

A similar check is missing inside the setDistributionConfigByMaturity(...) function, where the admin can change the configs for a particular distribution. After updating the configs, the sum of all distributions should be 10000 bps.

In addition, the removeBondedMaturity(...) function will reorder the elements of the bondedMaturities array when the element that gets deleted is not the last one in the array.

Building on the previous example, if the admin removes SHORT bonded maturities, the array will look like this [FLEXIBLE, LONG, MEDIUM]. This breaks one of the assumptions about the protocol, where maturities are expected to be in ascending order. If this happens, the _nextStakeDelegation function will attempt to fill the "longest" maturity first, starting from the last index of the bondedMaturities array. As can be seen, the LONG duration is no longer the last one. It is in the middle index, so the maturity that gets filled first will be MEDIUM.

Recommendation

Consider adding logic to redistribute the bps of bonded maturities whenever new ones are added, old ones are removed, or when their params are updated via setDistributionConfigByMaturity and enforce that the sum is always 10.000.

Resolution

Fixed in commit: ae68927. The setters for adding and removing individual bonded maturities, and their configs, have been removed. Now the entirety of the distribution config must be updated at once. This means that the basis point sum, and ascending order of maturities, will be checked each time.

Update from Nethermind Security

The newly added loop in the _trySetDistribution(...) function ensures that old maturity with stake must be present in the new config so that it is not lost. The problem is that the continue keyword from the inner loop does not "continue" in the outer loop. Even if old maturity is found in the new config, the inner loop will finish, and the revert will be hit every time. The iteration should be continued in the outer loop, but it is stopped by a revert. A boolean flag found could be set once a match is found. The outer loop would then revert only when found is false.

6.6 Calls to initialize(...) functions can be front-run

All major protocol smart contracts are proxy contracts initialized via the initialize(...) functions. These functions are unrestricted and transfer the contract ownership to the msg.sender. The attacker may attempt to front-run a call to initialize(...), hoping it will go unnoticed before the protocol starts its operations. If that would be the case, a malicious actor would have access to the admin functionalities.

Recommendation

Consider adding extra validation to the deployment scripts to ensure that the ownership of the contracts was granted to an appropriate address during the project's deployment.

Resolution

Verio components are all AdminControlled and as part of our standard deploy process we try to set the admin for each component to be the deployer initially. If the initializer is front-run, this call will fail because the setAdmin function is only callable by each component's owner.

6.7 Execution cost of stake(...) is not constant for all users

The main entry point to the system for the users is the stake(...) function in the StakePool contract. Individual users' deposits wait in the StakePool contract until their sum reaches a threshold above the minimum stake amount accepted by the Story's IPTokenStaking contract. Once this threshold is reached, the next call to stake(...) will call the handleStake(...) function on the Delegator contract and send all the individual deposits in a batch for staking. The caveat of this design is that the users whose stake happens to pass the threshold will pay more gas for the stake(...) transaction execution since they have to cover the cost of calling the handleStake(...) function.

Recommendation

While this design is correct and efficient, consider informing the users about such a possibility or adding a flag parameter that makes a call to handleStake(...) optional. If the latter is chosen, an additional function would be required for a protocol keeper to move the funds from the StakePool to the Delegator at regular intervals if no user decides to call handleStake(...).

Resolution

Fixed in commit: 12ccd18. Users can now elect whether or not to evaluate the stakePool when calling stake. A new function evaluateStakePool has been added, and will be called by the Verio periodically to ensure continued protocol operation as expected.

6.8 If unstakeInterval is set too low, the limit of 14 concurrend unbounding requests can be exceeded

The unstakeInterval variable in the Delegator contract dictates the frequency with which unstake(...) can be called on the Story's IPTokenStaking contract. As per Story's documentation, a single validator cannot have more than 14 concurrent unbounding requests at the same time. Any subsequent request will be ignored and will silently fail.

In a situation where the number of validator in the Verio protocol is low and the admin sets the unstakeInterval to a low value, the limit of 14 concurrent requests could be exceeded. This would result in a state mismatch between contract's state and the reality since delegations would be deleted from the unstake queue, but in reality they wouldn't be unstaked from the staking contract.

Recommendation

Consider setting a reasonable lower bound for the unstakeInterval in both the setUnstakeInterval(...) and _trySetDelegatorState(...) functions.

6.9 The Minted event is not emitted when minting VerioIP tokens to treasury

Whenever users stake funds with the stake(...) function, the VerioIP tokens are minted to their addresses. The Minted(_staker, vIPToMint) event is emitted with user's address and the amount of tokens minted. This amount however, does not include the mintFee amount of VerioIP tokens that were minted to the treasury address.

function _stake(address _staker, uint256 _stakeAmount) internal {
    // ...
    if (mintFee > 0) {
        // @audit-issue Treasury mint does not emit the Minted event.
        vIP.mint(treasury, mintFee);
    }
    vIP.mint(_staker, vIPToMint);
    emit Minted(_staker, vIPToMint);
}

Recommendation

Consider emitting the Minted event when minting VerioIP tokens to the treasury. The event could be added to the VerioIP token contract so that events are emitted every time, regardless of the token recipient.

Resolution

Fixed in commit: a3efa4. The Minted event has been moved to the VerioIP token contract to ensure it is properly handled for each mint call.

6.10 The _handleAdminStake(...) function does not emit the DelegationStaked event

The adminStake(...) function from the StakePool contract can be used by an admin to inject liquidity at specific maturity for a specific validator. The stake will eventually be processed in the _handleAdminStake(...) function in the Delegator contract. However, unlike its regular _handleStake(...) variant, this function does not emit the DelegationStaked event when the delegation is processed.

Recommendation

Consider adding the missing event emission to the _handleAdminStake(...) function.

6.11 The _handleStake(...) treats potential user refunds as funds to pay fees

The _handleStake() function calls the IPTokenStaking::stake function. Inside this function the code calculates two values, a stakeAmount and a remainder by calling the roundedStakeAmount function.

function roundedStakeAmount(uint256 rawAmount) public pure returns (uint256 amount, uint256 remainder) {
    remainder = rawAmount % STAKE_ROUNDING;
    amount = rawAmount - remainder;
}

This function takes the amount and divides it by STAKE_ROUNDING which is equal to 1 gwei. This means that if the actual amount is not perfectly divisible by 1 gwei, the difference will be treated as a remainder.

The IPTokenStaking::_stake function refunds the remainder to msg.sender which in the context of IPTokenStaking will be the Delegator contract.

function _refundRemainder(uint256 remainder) private {
    (bool success, ) = msg.sender.call{ value: remainder }("");
    require(success, "IPTokenStaking: Failed to refund remainder");
}

The Delegator::receive() function allows the contract to receive the refund, however this means that users who deposit amounts that are not perfectly divisible by 1 gwei will always incurr a small loss due to this rounding. The actual amount is negligible though.

Recommendation

In order to prevent this, a new check can be added in the StakePool::stake function that checks if msg.value % 1 gwei == 0.

Resolution

Fixed in commit: a97f9ac. stake now reverts if the supplied amount is not evenly divisible by 1 gwei.

6.12 The redelegate(...) will revert when called with more attempts than necessary

When calling redelegate(...), the admin has to specify the number of attempts to specify the number of redelegations to make. On every attempt, the function will dequeue the delegation from one validator and enqueue it for the other. At the end of each loop iteration, the Story's IPTokenStaking contract's redelegate(...) function will be called to move the delegation ID between validators.

function _redelegate(
    bytes calldata _fromValidator,
    bytes calldata _toValidator,
    uint256 _attempts
) internal returns (uint256 remainder) {
    // ...
    remainder = validatorRegistry.delegations[_fromValidator].total;
    for (uint256 i = 0; i < _attempts; i++) {
    // ...
        uint256 delegationId; // @audit these two vars are reset on each iteration
        uint256 amount;

        // @audit If there are no more delegations available, the following
        // if/else won't update delegationId and amount.
        if (fromValidatorDelegations.distribution.instant > 0) {
           // ... 
           // process instant delegations
        } else {
           // ... 
           // process bonded delegations
        }
    // ...
        // @audit-issue A call to redelegate will revert since 
        // delegationId and amount are 0.
        ipTokenStaking.redelegate{value: fee}(
            _fromValidator, _toValidator, delegationId, amount
        );
        remainder = remainder > amount ? remainder - amount : 0;
    }
    return remainder;
}

An issue arises if the specified number of attempts exceeds the number of delegations available for a given validator. After all delegations are processed, the loop in the _redelegate(...) might attempt an extra iteration. Since there are no delegations to dequeue and enqueue, the if/else logic won't be executed, but the IPTokenStaking contract will still be called. The problem is that the delegationId and amount used as parameters to the redelegate(...) call will be uninitialized. Since the staking contract does not accept these values, the execution will revert, and the gas used up to this point will be wasted.

Recommendation

Consider terminating the loop early so that the execution does not revert when there are no more delegations to redelegate. Since the remaining amount to redelegate is stored in the remainder variable, one of the possible ways to fix the problem is to stop the loop execution once the remainder reaches 0.

Resolution

Fixed in commit: 7d1f7b3. There is now an exit check that will return early if there are no more delegations to redelegate.

6.13 The bonded maturity unstake threshold check when processing the unstake queue is inaccurate

Whenever the handleUnstake(...) function is called on the Delegator contract, the existing unstake queue will be processed. The _handleUnstakeQueue(...) first checks if the unstake interval has passed, and if so, it attempts to unstake the delegations from the Validators present in the unstake queue. A delegation can be unstaked if its amount exceeds the minimum stake threshold. Since instant unstakes are stored internally as one element, the threshold check in the if branch can check the total of instant unstakes. A similar check is performed in the else if branch for bonded unstakes. The relevant code snippet is shown below.

function _handleUnstakeQueue() internal {
    // ...
    for (uint256 i = 0; i < unstakeQueue.unstakeValidators.length; i++) {
        // ...
    // @audit Process instant unstake
        if (_stakeThresholdMet(instantUnstake.amount)) {
            // ...
            //     @audit-issue The condition below checks if total in queue 
            //     is above the threshold. It should check if the delegation 
            //     we are about to unstake is above the threshold instead.
        } else if (_stakeThresholdMet(bondedUnstakes.total)) {
            // @audit Proces bonded unstakes.
            Delegation memory bondedUnstake = bondedUnstakes.delegations[bondedUnstakes
                .ids[bondedUnstakes.ptr]];
            // ...
            ipTokenStaking.unstake{value: fee}(
                bondedUnstake.validator,
                bondedUnstake.id,
        // @audit A single delegation is unstaked,
        // not the total from the queue.    
                bondedUnstake.amount,
                "" // Empty data parameter
            );
            // ...
        }
    }
    // ...
}

The else if condition is not accurate in the context of bonded maturities since they are unstaked one by one, unlike instant delegations, which are unstaked all in one go. To be accurate, the else if condition should check if the next delegation to unstake satisfies the threshold.

While in the current version of the code, this inaccurate check does not make a difference since all stakes/unstakes will have at least the threshold amount of stake, it might change once rebase(...) logic is implemented. If the validator's stake were to be decreased, then that particular delegation wouldn't satisfy the unstake condition on its own. Still, since the total in the queue could satisfy the check, the execution would continue.

Recommendation

Consider revisiting the condition to unstake bonded maturities in the context of possible future rebases(...).

Resolution

Fixed in commit: b185546. The threshold check here is not to see if the bonded delegations are of sufficient size, but rather to check that there are bonded delegations to unstake from the unstakeQueue before proceeding. The logic has been updated to reflect this intent.

6.14 VerioIP mint and burn functions don't use the whenNotPaused modifier

Because it inherits PausableUpgradeable, the VerioIP contract could be paused, but the mint(...) and burn(...) functions do not use the whenNotPaused modifier. The admin might want to pause token operations in case of emergency, but this won't be possible.

Recommendation

While minting can be controlled via StakePool which can be paused, users are still allowed to burn in the paused state. If it is desired, consider pausing burning as well.

Resolution

Will not fix. This is intentional behavior, as it still allows a user who owns VerioIP to burn it at will. Minting can be disabled by pausing the StakePool because it is the only valid caller of the mint function.

6.15 Missing input validation

Certain important variables in the contracts lack proper validation, which could lead to unexpected behaviors. Below is a non-exhaustive list of missing checks split per file.

StakePool.sol:

• _trySetStakePoolState(...):;

• minStake can be 0
• unbondingPeriod can be 0

• setMinStake(...): minStake can be 0;
• setMintFeeBps(...): missing upper bound check;
• setBurnFeeBps(...): missing upper bound check;

ComponentSelector.sol:

• all setter functions are missing 0 value checks;
• _trySetComponentSelectorState(...): missing 0 value checks on the provided config;

InsurancePool.sol:

• _trySetInsurancePoolState(...): missing 0 value checks on the provided config;
• setClaimant(...): claimant can be 0;

Recommendation

Consider adding extra input validation for the cases outlined above.

Resolution

Fixed in commit: f4e5c4b. The only exception to this fix is the unbondingPeriod, which may be intentionally set to 0 to allow users to instantly withdraw.

6.16 The withdraw(...) function does not follow the Checks-Effects-Interactions pattern

Users can use the withdraw(...) function from the WithdrawalPool contract to withdraw their IP tokens. The function sends the tokens to the user via a low-level call and decreases the totalPendingWithdrawals afterward. Since this value could be modified before the external call, it is considered a best practice to respect the Checks-Effects-Interactions pattern and perform state updates before external calls.

Recommendation

To reduce the potential attack surface, consider moving the state update before the external call.

Resolution

Fixed in commit: 74ab5aa. Although this function is already nonReentrant, the state altering code has been moved before the external call.

6.17 Lack of enforcement of msg.value inside fund(...) functions

The fund() functions in all the above contracts have no enforcement for msg.value. This allows any user to call these functions with msg.value = 0 in order to spam and emit worthless events. There are legitimate situations in which the protocol will call these functions with potentially msg.value = 0. In order to differentiate between these cases, certain constrains can be added to the fund(...) functions. If the caller is one of the contracts of the system, allow msg.value to be zero, else revert. This will allow the system to work as expected, while preventing users from spamming the fund() function with 0 value.

Recommendation

Consider adding checks for msg.value > 0 if msg.sender is not one of the protocol's smart contracts.

6.18 Lack of zero address checks in ComponentSelector setter functions

None of the setter functions in the ComponentSelector contract check if the new address param is address(0) or not.

Recommendation

Consider adding this extra checks to the setter functions.

6.19 No checks for mintFeeBps and burnFeeBps in the StakePool setter functions

When the StakePool contract is deployed and initialized, the _trySetStakePoolState function has two checks for the mintFeeBps and burnFeeBps that enforce these fees to be at most 5%.

if (_stakePoolConfig.mintFeeEnabled) {
    require(_stakePoolConfig.mintFeeBps <= 500, "StakePool: max mint fee bps is 5%");
}

if (_stakePoolConfig.burnFeeEnabled) {
    require(_stakePoolConfig.burnFeeBps <= 500, "StakePool: max burn fee bps is 5%");
}

However, the setter functions for these fees, lack these checks. This allows the admin to potentially set higher fees than the expected maximum 5% threshold.

function setMintFeeBps(uint256 _mintFeeBps) external override onlyAdmin {
    //@audit no check for mintFeeBps <= 500
    StakePoolState storage stakePoolState = _stakePoolState();
    stakePoolState.mintFeeBps = _mintFeeBps;
    emit StakePoolStateUpdated(stakePoolState);
}

Recommendation

Consider adding these extra checks in the setter functions to maintain consistency.

Resolution

Fixed in commit: c24f50c. New functions have been added to vet the basis point configurations for both fees.