Verio

IPA

6.1 A single malicious reward token can DoS the StakePool operations

On every StakePool operation such as stake(...), unstake(...) and claimRewards(...) the internal _claimRewards(...) is called. This function iterates over every incentive pool and then over every reward pool to claim the rewards and send the reward token to the user. If one of the reward tokens happen to be malicious and for example revert on every transfer, then the entire call to _claimRewards(...) will revert. As a result staking/unstaking for a particular IPAsset will not possible. Since removing a reward pool or an incentive pool is not possible, the denial of service (DoS) is permanent.

Recommendation

Multiple approaches can be taken to fix the issue. One includes allowing the user to choose which reward tokens to claim. Another possibility includes skipping tokens that revert or adding an option for the admin to remove such tokens from the list.

Resolution

Fixed in commit: dae5415

We have added a try/catch block that will handle the DOS case, but unfortunately we can do nothing to prevent malicious reward offerers from upgrading their ERC20 contracts to revert on transfer. Users should be aware of the dangers of upgradeable contracts when selecting stake pools with potentially unsafe reward tokens.

6.2 Repeated stake and unstake operations artificially increase the user's IP weight

When a user stakes tokens on an IPAsset, the stake(...) function of the asset's associated StakePool is invoked. If the user's current stake for the specified stakeToken is 0, it indicates that the user has not previously staked with this token. In such cases, the stakeToken address is added to the user's array of staked tokens. Additionally, if this is the first stake for a particular lockup period within the StakePool contract, the stakeToken address is also added to the contract-wide list of staked tokens. The _stake(...) function that handles the relevant state updates is shown in the code snippet below.

function _stake(
    // ...,
    address _user,
    IStakeToken _stakeToken,
    uint256 _amount,
    ILockup.Type _lockup
) internal {
    IStakePool.StakePoolState storage stakePoolState = _stakePoolState();
    if (
        stakePoolState.totalStakePerStakeTokenByLockup[_lockup][_stakeToken]
            == 0
    ) {
        // @audit Add the stake token address to the global list.
        stakePoolState.stakeTokens.push(_stakeToken);
    }
    // @audit Increase the total stake for that stake token.
    stakePoolState.totalStakePerStakeTokenByLockup[_lockup][_stakeToken] +=
        _amount;

    IStakePool.UserPeriodBasedStake storage userStake = stakePoolState
        .stakeByOperatorUser[_operator][_user].userPeriodBasedStake[_lockup];

    if (userStake.stakeByToken[_stakeToken] == 0) {
        // @audit If user's stake is 0, then the address
        // of the token is added to an array of user's tokens.
        userStake.stakeTokens.push(_stakeToken);
    }
    userStake.stakeByToken[_stakeToken] += _amount;
    // ...
}

The problem present in the StakePool contract is that there is no logic that would remove the stake tokens from the arrays. Both user and contract stake get decreased in the _unstake(...) function. However, when the stake reaches 0, the stake tokens are not removed. If the user would stake again with the same token, since his total stake for that token is 0, the stake token would be added to both arrays for the second time.

The two arrays of stake tokens are used in the reward computation logic. The _getUserStakeWeightedInIPForPeriod(...) and _getTotalStakeWeightedInIPForPeriod(...) functions compute the summed stake weights for individual users and the total for all users, respectively. The computation is done by iterating over the stakeTokens array and adding up the stakes for each token. Since the array might contain duplicated elements, the end result might be inflated. Both functions execute similar logic.

Only the _getUserStakeWeightedInIPForPeriod(...) is shown below for brevity.

function _getUserStakeWeightedInIPForPeriod(
    address _operator,
    address _user,
    ILockup.Type _lockup
) internal view returns (uint256) {
    // ...
    IStakePool.UserPeriodBasedStake storage userStake = stakePoolState
        .stakeByOperatorUser[_operator][_user].userPeriodBasedStake[_lockup];
    uint256 userStakeInIP = 0;

    // @audit-issue The array may contain duplicated elements.
    for (uint256 i = 0; i < userStake.stakeTokens.length; i++) {
        IStakeToken stakeToken = userStake.stakeTokens[i];
        uint256 multiplier =
            _selector().lockup().getLockupMultiplier(_lockup);

        // @audit-issue As a result they will be included
        // in the total stake multiple times.
        userStakeInIP += stakeToken.priceInIP(
            userStake.stakeByToken[stakeToken] * multiplier
        );
    }
    return userStakeInIP;
}

By repeatedly calling stake(...) and unstake(...), the user can artificially increase his stake weight to earn more rewards. If the user is the only staker, the total stake weight will also be increased. If other users join later, their share in the total weight will be smaller, and as a result, they will receive fewer rewards.

Recommendation

One of the possible solutions to the problem includes removing the stake token from the stakeTokens array once the user's or contract's stake reaches the value of 0. This way, tokens won't be accounted for twice during reward computations.

6.3 NATIVE token rewards cannot be claimed and will be stuck in the IncentivePool

The current protocol implementation allows users to create a RewardPool with the token's RewardTokenType being either ERC20 or NATIVE. Users can call the addRewards(...) function from the IPAssetStaking contract to add rewards to such a pool. They can supply either ERC20 tokens or NATIVE tokens as rewards.

The problem present in the contract is that the claimRewards(...) function, responsible for reward claiming, handles only ERC20 token transfers. More specifically, the _sendReward(...) function from the IncentivePool contract is supposed to transfer the reward tokens to the user, but it does not contain the logic to handle native token transfers.

// @audit-issue The _sendReward function distinguishes between ERC20 transfers
// to the user OR the operator, but does not transfer NATIVE token rewards.
function _sendReward(/*...*/) internal returns (uint256) {
    // ...
    if (_userStakeAmount > 0) {
        // ...
        if (rewardAmount > 0) {
            if (_operatorAddress == _user) {
                IERC20(rewardPool.rewardToken).safeTransfer(
                    _user, rewardAmount
                );
                // ...
            } else {
                // ...
                IERC20(rewardPool.rewardToken).safeTransfer(
                    _user, userRewardAmount
                );
                // ...
            }
            // ...
        }
    }
    // ...
}

Since there is no way for the admin or the users to withdraw the rewards from the RewardPool other than via claimRewards(...), any native tokens supplied to the reward pool as rewards will be lost. These funds will be stuck in the smart contract.

Recommendation

Consider adding the logic to handle NATIVE token transfers to the _sendReward(...) function.

6.4 The RewardPoolState can be overridden, which will cause previous rewards to be lost

As per the protocol's design an individual user should be able to register only 1 IncentivePool. For 1 IncentivePool, however, the creator can register multiple RewardPools with various tokens and reward configurations. Pool registration is performed via the registerRewardPool(...) function from the IPAssetStaking contract.

In its current state, this function can be called multiple times by the same user. It allows the IncentivePool's creator to override an existing pool with a new one. The following pool configuration is set whenever a new reward pool is created.

IRewardPool.RewardPoolState memory rewardPool = IRewardPool
    .RewardPoolState(
    _rewardPoolConfig.rewardToken,
    _rewardPoolConfig.rewardTokenType,
    _rewardPoolConfig.distributionType,
    _rewardPoolConfig.rewardsPerEpoch,
    0, // @audit rewards per token
    0, // @audit total rewards
    0, // @audit total distributed rewards
    block.number // @audit last epoch block
);

// @audit-issue Second registration of the same reward token
// will override the previous configuration.
incentivePoolState.rewardPoolByRewardToken[_rewardPoolConfig
    .rewardToken] = rewardPool;

This means that the IncentivePool creator can unknowingly or maliciously override the old reward pool with a new one, impacting the rewards distribution mechanism. This can happen whenever the creator attempts to create a new reward pool but will specify the same rewardToken as an already existent pool.

Overriding the pool will reset the lastEpochBlock used during rewards calculation. If this parameter gets reset, user rewards will also reset since no time elapsed to accrue rewards. Also, this allows the IncentivePool creator to change the type of the reward token from ERC20 to NATIVE and vice-versa, change the distribution mechanism from CONTINUOUS to STATIC and vice-versa, or change the number of rewardsPerEpoch.

Furthermore, all tokens added as rewards to the previous rewards pool will be lost because the totalRewards parameter has also been reset.

Recommendation

Consider adding checks to ensure that the creator can override the pool only if all the rewards from the current pool have been claimed.

6.5 The incentive pool creators can collude to DoS a StakePool

Creators have the ability to register IncentivePools for a particular IPAsset and they can also register RewardPools for each IncentivePool.

During reward claiming (which also happens upon staking/unstaking) there is a nested loop iteration over all IncentivePools and over all RewardPools.

Creators can maliciously or unknowingly register too many pools which will lead to out-of-gas reverts whenever users stake/unstake from a specific StakePool. The likelihood of this attack vector is Low since creators must be whitelisted by the Owner, but the impact is High since it will lead to a permanent DoS as you can't remove pools after they are added.

Recommendation

Consider adding a claiming mechanism that skips the tokens if they can’t be transferred.

Resolution

Fixed in commit: c89984a

We have added a maximum number of incentive pools that can be added to any stake pool. Now if incentive pool creators collude, they will not be able to DOS the stake pool. They could still attempt to collude to create incentive pools with bad configurations for users (ie. low reward distribution). This is mitigated by our administration process, as we can always remove a misbehaving incentive pool creator.

6.6 Operator can steal user's rewards by setting the commission to 100 percent

Operators can specify a commission fee that users have to pay whenever they stake/unstake through the said Operator. The fee is expressed in basis points (up to 10.000). The issue is that an operator can specify a fee of 10.000 bps which means that there is no upper bound on the operator's commission.

Right before claiming the rewards on behalf of the user, an operator can increase their commission to 10.000 bps and steal all the rewards in the form of a commission from the user. The severity is low since the user staked with the operator in the first place. The trust assumption however can be easily minimized by introducing an upper bound for the commission, say 2000 bps.

Recommendation

Consider adding a limit to the maximum amount that an operator can claim as commission from users.

Resolution

Will not fix.

Stake provided by an operator on the user's behalf is still maintained separately from the user's own stake. Operators can claim 100% of the rewards from their own stake, but cannot claim rewards that the designated user is otherwise owed.

It is permissionless for an operator to stake on a user's behalf, so any stake provided therein is an act of good-faith and should not be considered to belong to the original user.

6.7 Operators are unable to change their commission fees and reward addresses

The setOperatorCommissionFee(...) function from an OperatorRegistry contract allows an operator to change their commissionFee. The issue is that the function calls the internal _tryPayOperatorUpdateFee function, which requires the msg.value >= operatorUpdateFee. Because the setOperatorCommissionFee function is not marked as payable, operators calling them will have their transactions revert since the msg.value will always be 0. Since zero is lower than the required operatorUpdateFee, the check will revert. The same issue is present in the setOperatorRewardsAddress(...) function.

Recommendation

Consider adding the payable keyword to the setOperatorCommissionFee and setOperatorRewardsAddress functions.

6.8 Admin can update the lockup configuration affecting the currently locked funds

Whenever users stake, they have to choose one LockUp period provided by the protocol. The issue is that this period can be changed by the admin after users have already staked. For example, the lockup period can be increased to a very long time, or the multipliers provided by the LockUp period can be changed. This behavior violates the terms to which the user agreed during staking. The lockup type can be also removed entirely which will cause user funds to be stuck;

Recommendation

Consider adding a delay period before updating LockUps so that users have time to react and decide if they still want to keep their tokens staked after the change takes place.

Resolution

Fixed in commit: 3d2eaa4.

We have implemented a time-lock mechanism for updates to our Lockup contract. The effective time of an update is determined by the previous lockup time. Users are now notified via events that an update is pending, and can choose to unstake before that change goes into effect.

Update from Nethermind Security

It is still possible for an Admin to change the instant lockup to something much longer. The effective update time is based on the previous lockup time, but since in the case of instant lockup this time is 0, the change can become effective instantly.

6.9 Authorized creators can register incentive pools on behalf of other users, which does not let the legitimate creator change the pool's config

The registerIncentivePool(...) from the IPAssetStaking contract is used to register a new incentive pool for a particular IPAsset. It can only be called by the IPAsset owner or an authorized incentive pool creator whitelisted in the CreatorRegistry contract. The caller provides the IncentivePoolConfig, which will be passed to the registerIncentivePool(...) from the StakePool contract. A new incentive pool will eventually be deployed and initialized with the provided configuration.

The creation of the pool is credited to the creator address provided in the IncentivePoolConfig struct and not to the original caller of the registerIncentivePool(...) from the IPAssetStaking contract. The problem with this approach is that these two addresses can be different and possibly even belong to a user who is not the IPAsset owner or registered as a pool creator.

The most significant caveat includes a possibility where someone calls registerIncentivePool(...) on behalf of another creator who wants to register the incentive pool on their own. The legitimate creator's registration will revert since they are already marked as owners of an incentive pool. As a result, they can't change the pool's configuration, which is immutable after deployment.

Recommendation

Consider validating that the msg.sender in the initial call to registerIncentivePool(...) from the IPAssetStaking contract is equal to the creator address provided in the pool config.

6.10 If epoch duration is ever different than 1, users will be losing rewards on each claim

Whenever reward claiming logic is triggered, the _claimRewards(...) function from the IncentivePool contract is executed. It first calls _distributeRewards(...) to compute and update the global reward per token increment, and then it calls _sendRewards(...) to send the reward tokens to the user.

function _distributeRewards(
    uint256 _totalStakedInPool,
    address _rewardToken
) internal returns (uint256) {
    // ...
    uint256 blocksPassed = block.number - rewardPool.lastEpochBlock;
    if (
        blocksPassed > 0
            && rewardPool.totalDistributedRewards < rewardPool.totalRewards
    ) {
        // ...
        rewardPool.rewardPerToken += rewardPerTokenIncrement;
        // ...
        // @audit If the user claimed at block number 3, epoch duration is 2,
        // and previous lastEpochBlock was 0, then the lastEpochBlock will
        // be set to (3 - 0) / 2 * 2 = 2.
        rewardPool.lastEpochBlock += (
            blocksPassed / incentivePoolState.epochDuration
        ) * incentivePoolState.epochDuration;
    }
    return rewardPool.rewardPerToken;
}

The _sendRewards(...) function calls the _calculateUserRewardAmount(...) function to determine the reward amount to send to the user. The computation logic is similar to the one in _distributeRewards(...).

function _calculateUserRewardAmount(
    uint256 _userStakeInIP,
    uint256 _userRewardPerToken,
    uint256 _totalStakedInPool,
    IRewardPool.RewardPoolState storage rewardPool
) internal view returns (uint256) {
    uint256 rewards = 0;
    // ...
    // @audit _distributeRewards(...) updated lastEpochBlock to 2,
    // as a result blocksPassed is 1.
    uint256 blocksPassed = block.number - rewardPool.lastEpochBlock;
    // @audit Since blocksPassed is 1 and there are undistributed rewards,
    // the execution will enter the if statement.
    if (
        blocksPassed > 0
            && rewardPool.totalDistributedRewards < rewardPool.totalRewards
    ) {
        rewards = rewardPool.distributionType
            == IRewardPool.DistributionType.STATIC
            // @audit-issue The rewards amount will be zero since 1 divided by 2
            // will be rounded down to 0.
            ? (blocksPassed / incentivePoolState.epochDuration)
                * rewardPool.rewardsPerEpoch
            : rewardPool.totalRewards - rewardPool.totalDistributedRewards;

        if (rewards == 0) {
            // @audit-issue The early return is triggered. The reward amount
            // to be sent to user is zero.
            return 0;
        }
        // ...
    }
    // ...
}

In the usual scenario (when epoch duration is one block), the _calculateUserRewardAmount(...) wouldn't recompute the reward distribution since lastEpochBlock is the same as block.number and blocksPassed evaluates to zero. This, however, is not the case when the epoch duration is different than one. Whenever that happens, the reward amount will be rounded to zero and returned to the _sendRewards(...) function.

The rewards won't be sent to the user, but the user's rewardPerTokenByRewardToken will be updated with the new reward per token incremented computed in the _distributeRewards(...). As a result, the user lost the ability to claim the rewards for that period.

The impact of this issue would be high since users wouldn't be getting the rewards. However, as confirmed with the Verio team, this scenario can't happen since the epochDuration shouldn't ever be set to a value different than one. As a result, the finding is marked with informational severity.

Recommendation

If epoch duration must ever be set to a different value than one, consider reverting the changes introduced in commit 62c8d18 and replace the call to _calculateUserRewardAmount(...) with an inline reward calculation.

6.11 The IP tokens sent on top of the VerioIP stake will be lost

The stake(...) function in the IPAssetStaking allows users to specify an amount of ERC20 tokens to deposit and msg.value to send if they attempt to deposit NATIVE tokens. The issue in the internal _isValidStakeTokenAmount function is that if the user attempts to deposit a valid amount of ERC20 tokens and erroneously sends a non-zero msg.value, the function will return true. This results in a loss of funds for the user because the NATIVE tokens are not accounted for when depositing ERC20 tokens.

function _isValidStakeTokenAmount(
    address _stakeTokenAddress,
    uint256 _amount
) internal view returns (bool) {
    // ...
    if (stakeTokenType == IStakeToken.Type.ERC20) {
        // @audit-issue missing `&& msg.value == 0` check.
        return _amount >= minStakeAmount;

    } else if (stakeTokenType == IStakeToken.Type.NATIVE) {
        return msg.value >= minStakeAmount && _amount == 0;

    } else {
        return false;
    }
}

Recommendation

The _isValidStakeTokenAmount function should also check that msg.value == 0 when users attempt to stake ERC20 tokens.

6.12 The RewardPool can be registered with 0 reward tokens per epoch

The registerRewardPool(...) function from the IPAssetStaking contract can be used by IPAsset owners or authorized pool creators to register a new RewardPool. The problem with this function is that it does not validate the provided RewardPoolConfig. As a result, it is possible that, mistakenly or intentionally, a pool with zero rewardsPerEpoch can be created. Such a pool will be iterated over during reward claims, but it will never distribute any rewards and will waste the user's gas.

Recommendation

Consider adding missing input validation to the registerRewardPool(...) function.

6.13 When adding rewards, only one token type can be added at a time

The addRewards(...) function from the IPAssetStaking contract allows users to add reward tokens to the RewardPool. The accepted token types include NATIVE and ERC20.

function _addRewards(
    IStakePool _stakePool,
    address _incentivePoolCreator,
    uint256 _amount,
    address _rewardToken
) internal {
    // @audit-issue msg.value can be non-zero even when sending ERC20 tokens.
    _stakePool.addRewards{value: msg.value}(
        msg.sender, _incentivePoolCreator, _amount, _rewardToken
    );
}

It is not enforced that the msg.value must be 0 when adding ERC20 reward token types. The unaware user might want to combine a NATIVE token transfer with an ERC20 one, which would cause one of the two to be lost.

The _handleAddRewards(...) function from the IncentivePool contract that handles the token transfer from the user to the pool can only account for one token type being added as a reward at a time. The msg.value sent on top of the ERC20 tokens would be lost, and native tokens would be stuck in the IncentivePool contract.

function _handleAddRewards(
    address _user,
    IRewardPool.RewardPoolState storage rewardPool,
    uint256 _amount
) internal returns (uint256) {
    if (rewardPool.rewardTokenType == IRewardPool.RewardTokenType.ERC20) {
        IERC20(rewardPool.rewardToken).safeTransferFrom(
            _user, address(this), _amount
        );
        // @audit-issue If msg.value was non zero, NATIVE tokens will be stuck
        // in the contract and not accounted for in the rewards.
        return _amount;
    } else if (
        rewardPool.rewardTokenType == IRewardPool.RewardTokenType.NATIVE
    ) {
        return msg.value;
    } else {
        revert InvalidRewardTokenType(rewardPool.rewardTokenType);
    }
}

Recommendation

Consider adding additional input validation to ensure that msg.value is 0 when adding ERC20 tokens as rewards.

Resolution

Fixed in commit: 9b619ad

We added a check to verify that the supplied amount of tokens is relative to its reward token type. This means that a user can still only supply one reward token at a time, but they will no longer erroneously lose native token supplied when specifying an ERC20 type reward token.

6.14 IPAssetStaking contains functions open to re-entrancy attack vectors

The main functions of the IPAssetStaking contract, such as stake(...) and unstake(...) and their "on-behalf" variants, are protected with re-entrancy guards. These functions contain callbacks to the msg.sender, which can be used to re-enter other functions from the IPAssetStaking contract that lack the nonReentrant modifier. Examples of unprotected functions include the logic to add and claim rewards and register incentive and rewards pools.

Although no active re-entrancy exploits have been found in the codebase at its current state, the following is a list of possible attack vectors that might become active due to fixing other issues presented in this audit report.

The logic in the claimRewards(...) function performs an externall call to the reward token via safeTransfer(...) function. Any of the reward tokens could re-enter the claimRewards(...) function again before claimed reward details are updated. This is a bigger problem when handling NATIVE tokens. If the missing logic to handle NATIVE token rewards is added to the protocol, the claimRewards(...) function could be repeatedly re-entered to drain the RewardPool of NATIVE tokens. User's rewardDetails are updated after an external call that could be used to re-enter and claim again.

Other possible attack vector includes a call to unstake(...). The amount of tokens to unstake is provided by the user and is not sanitized. Users can specify an arbitrarily large number higher than their original deposit. Before the actual unstake happens, the specified amount of funds will be withdrawn from the StakePool and sent to the user via the withdraw(...) function. Malicious users could use these funds to manipulate the state (e.g., add the funds as rewards). In its current form, the call to unstake(...) with an amount larger than the initial deposit would revert at the very end of the call chain due to arithmetic underflow during the user's stake token accounting updates.

Recommendation

Consider adding nonReentrant modifiers to the remaining functions in the IPAssetStaking contract to decrease the possible attack surface via re-entrancies.

6.15 Unused code

The function _ipAssetRegisteredWithIPAssetRegistry(...) from the IPAssetStaking contract is unused.

Recommendation

Consider removing the unused code from the contract to keep the codebase clean and readable.