Vana

VanaPool

6.1 Entity owners can withdraw their minimum registration stake

When creating an entity, the createEntity function requires the owner to deposit a minimum registration stake, which is intended to remain locked indefinitely.

function unstake(uint256 entityId, uint256 shareAmount) external override nonReentrant whenNotPaused {
    // --SNIP
    if (entityInfo.ownerAddress == _msgSender()) {
        uint256 ownerMinShares = (vanaToShare * vanaPoolEntity.minRegistrationStake()) / 1e18;

        if (stakerShares - shareAmount < ownerMinShares) {
            revert CannotRemoveRegistrationStake();
        }
    }
    // --SNIP
}

However, the above check can be easily bypassed. The updateEntity(...) allows the update of the owner and does not move the registration shares to the new owner.

function updateEntity(
    uint256 entityId,
    EntityRegistrationInfo calldata entityRegistrationInfo
) external override whenNotPaused nonReentrant onlyEntityOwner(entityId) {
    // --SNIP

    // Update fields
    entity.ownerAddress = entityRegistrationInfo.ownerAddress;

    emit EntityUpdated(entityId, entityRegistrationInfo.ownerAddress, entityRegistrationInfo.name);
}

The entity owner can leverage this to change the ownerAddress to another andress he controls via updateEntity and then calls unstake to freely withdraw his registration stake.

Recommendation

Consider moving the registration shares to the new entity owner when updating the owner.

6.2 Excessive calls to processRewards(...) cause higher reward rate, regardless of time elapsed

The processRewards(...) is a permissionless function which updates the rewards for an entity. It calculates the rewards to distribute based on the time elapsed and the rate of reward.

function processRewards(uint256 entityId) public override whenNotPaused {
    // ...

    // Calculate theoretical rewards based on maxAPY
    uint256 toDistribute = (entity.activeRewardPool * entity.ratePerSecond * timeElapsed) / 1e18;

    if (toDistribute > entity.lockedRewardPool) {
        toDistribute = entity.lockedRewardPool;
    }

    entity.lockedRewardPool -= toDistribute;
    // @audit activeRewardPool itself is increased by toDistribute
    entity.activeRewardPool += toDistribute;

    // ...
}

However, the function uses activeRewardPool to calculate the rewards and activeRewardPool itself is increased by the rewards to distribute. This creates an accumulating growth in the reward amount over time, which could lead to excessively increasing rewards rather than a more predictable, linear growth based on time elapsed. As a result, the rewards are influenced not only by the time elapsed but also by the number of times processRewards(...) is called.

For example, let's say activeRewardPool is 1000 and ratePerSecond is 2e18. Consider these scenarios:

Scenario A:

1. The first call to processRewards(...) with a time elapsed of 100 results in an activeRewardPool of 201,000;
2. A second call to processRewards(...) with a time elapsed of 200 results in an activeRewardPool of 80,400,000;

Scenario B:

1. A call to processRewards(...) with a time elapsed of 300 results in an activeRewardPool of 600,000;

In both the scenarios, time elapsed is the same ( 300), but the resulting activeRewardPool is significantly different.

Recommendation

Consider using two separate variables: one for the activeRewardPool and another for the totalStake. The totalStake should be used to calculate the rewards to distribute, ensuring that the rewards grow linearly with respect to time elapsed.

Resolution

Fixed in commits 97de94d, 265bcde, and 2177752.

6.3 Same stake can be migrated multiple times

The DLPRootImplementation contract introduces the migrateStakeToVanaPool(...) function, which allows users to migrate their existing DLP stakes to the VANA pool. This function stakes the user's existing amount plus any claimable rewards into the pool for a specific entity.

function migrateStakeToVanaPool(uint256 stakeId, uint256 entityId) external payable nonReentrant whenNotPaused { 
    dlpRootEpoch.createEpochs();

    Stake storage stake = _stakes[stakeId];

    if (stake.endBlock == 0) {
        stake.movedAmount = stake.amount;
        _closeStake(_msgSender(), stakeId);
        stake.movedAmount = 0;
    }

    uint256 toClaim = stake.amount + _calculateStakeRewardUntilEpoch(stakeId, dlpRootEpoch.epochsCount() - 1, true);

    if (toClaim == 0) {
        revert NothingToClaim();
    }

    bool success = dlpRootRewardsTreasury.transferVana(payable(address(this)), toClaim);
    if (!success) {
        revert TransferFailed();
    }

    vanaPoolStaking.stake{value: toClaim}(entityId, stake.stakerAddress);

    emit StakeMigratedToVanaPool(stakeId, stake.amount, entityId);
}

However, the function does not update the stake.amount to 0 or delete its entry from the _stakes mapping, allowing users to call this function repeatedly.

Recommendation

Consider clearing the _stakes entry or updating stake.amount to 0 for the migrated stake.

6.4 Stakers funds are locked on entity removal

Users can create entities via createEntity(...), and stakers can allocate their funds to a specific entity ID through VanaPoolStakingImplementation::stake(...). However, entity owners can remove an entity at any time, updating its status to Removed:

function removeEntity(uint256 entityId) external override whenNotPaused nonReentrant onlyEntityOwner(entityId) {
    Entity storage entity = _entities[entityId];

    if (entity.status != EntityStatus.Active) {
        revert InvalidEntityStatus();
    }

    // Process any pending rewards
    processRewards(entityId);
==>    entity.status = EntityStatus.Removed;   
    _activeEntityIds.remove(entityId);

    emit EntityStatusUpdated(entityId, EntityStatus.Removed);
}

Once an entity is removed, stakers who have already staked funds into it become unable to unstake, as the contract restricts unstaking from non-active entities:

function unstake(uint256 entityId, uint256 shareAmount) external override nonReentrant whenNotPaused {
    // --SNIP
==>    vanaPoolEntity.updateEntityPool(entityId, shareAmount, exactVanaAmount, false);

    bool success = vanaPoolTreasury.transferVana(payable(_msgSender()), exactVanaAmount);
    if (!success) {
        revert TransferFailed();
    }
}

function updateEntityPool(
    uint256 entityId,
    uint256 shares,
    uint256 amount,
    bool isStake
) external override whenNotPaused onlyRole(VANA_POOL_ROLE) {
    if (!hasRole(VANA_POOL_ROLE, msg.sender)) {
        revert NotAuthorized();
    }

    Entity storage entity = _entities[entityId];

    if (entity.status != EntityStatus.Active) {
==>        revert InvalidEntityStatus();
    }
    
    // --SNIP
}

As a result, stakers' funds become permanently locked, as they are unable to unstake from a removed entity.

Recommendation

There are many approaches to fix the issue, it depends on how the team intends to fix the issue. Two possible approaches include:

1. Allow unstaking from removed entities. Since the stake funds are held in the treasury contract rather than the entities themselves, enabling unstaking from removed entities would resolve the issue without security risks;
2. Restrict entity owners from removing an entity while it has active stakes;

Resolution

Fixed by removing the removeEntity(...) function in db7afd9

6.5 Users can withdraw their DLP stakes and migrate to the VANA pool simultaneously

The DLPRootImplementation contract introduces the migrateStakeToVanaPool(...) function, which allows users to migrate their existing VANA stakes to the VANA pool. This function stakes the user's existing amount plus any claimable rewards into the pool for a specific entity.

function migrateStakeToVanaPool(uint256 stakeId, uint256 entityId) external payable nonReentrant whenNotPaused { 
    dlpRootEpoch.createEpochs();

    Stake storage stake = _stakes[stakeId];

    if (stake.endBlock == 0) {
        stake.movedAmount = stake.amount;
        _closeStake(_msgSender(), stakeId);
        stake.movedAmount = 0;
    }

    uint256 toClaim = stake.amount + _calculateStakeRewardUntilEpoch(stakeId, dlpRootEpoch.epochsCount() - 1, true);

    if (toClaim == 0) {
        revert NothingToClaim();
    }

    bool success = dlpRootRewardsTreasury.transferVana(payable(address(this)), toClaim);
    if (!success) {
        revert TransferFailed();
    }

    vanaPoolStaking.stake{value: toClaim}(entityId, stake.stakerAddress);

    emit StakeMigratedToVanaPool(stakeId, stake.amount, entityId);
}

However, the function does not properly update the _stakes mapping to mark the stake as withdrawn or delete its entry, leading to a double withdrawal exploit which can be exploited by users to migrate their stakes to vana pools and withdraw them from the DLPRoot contract as well. Consider the following exploit path:

1. Bob has an active stake in DLPRootImplementation;
2. Bob closes his stake by calling closeStake(...);
3. Bob migrates his stake to VANA pool using migrateStakeToVanaPool(...);
4. Bob withdraws his stake using withdrawStake(...), even though it has already been migrated;

Since the contract does not properly mark the stake as migrated/withdrawn, Bob effectively retrieves his original stake while also having it staked in the VANA pool, leading to contract losses.

Similarly, as there's no check to ensure that already withdrawn stakes cannot be migrated, users can migrate their already withdrawn stakes to the VANA pool.

Recommendation

Consider adding a check to restrict already withdrawn stakes and clearing the _stakes entry or set the withdrawn status to true for the migrated stake.

6.6 processRewards(...) is called too late in unstake(...)

The unstake(...) function allows users to withdraw their staked VANA tokens, receiving an amount proportional to their shares in the entity. However, processRewards(...) is called after the proportion calculation, which means rewards that should be included in the vanaToShare conversion are not accounted for

function unstake(uint256 entityId, uint256 shareAmount) external override nonReentrant whenNotPaused {
    uint256 stakerShares = _stakers[_msgSender()].entities[entityId].shares;
    if (stakerShares == 0 || shareAmount == 0) {
        revert InvalidAmount();
    }

    IVanaPoolEntity.EntityInfo memory entityInfo = vanaPoolEntity.entities(entityId);

    uint256 vanaToShare = vanaPoolEntity.vanaToEntityShare(entityId);

==>    vanaPoolEntity.processRewards(entityId);
    
    // --SNIP
     
    uint256 exactVanaAmount = (1e18 * shareAmount) / vanaToShare;
    // --SNIP

Since vanaToShare is calculated before processing rewards, it does not reflect the most up-to-date active rewards, causing users to receive fewer VANA tokens than they should.

Recommendation

Consider calling vanaPoolEntity::processRewards(...) before calculating the vanaToShare.

6.7 The stake(...) and unstake(...) functions lack slippage protection

Users interact with VanaPoolStakingImplementation via the stake(...) and unstake(...) functions to allocate and deallocate funds from specific entities. However, these functions lack slippage protection, exposing users to potential value losses due to fluctuations in share distribution.

For example, if a user attempts to unstake their shares, they might receive fewer VANA tokens than expected if other stake transactions are processed just before them, increasing the total shares and decreasing their withdrawal amount.

Recommendation

Allow users to specify acceptable minimum values in both stake(...) and unstake(...) functions.

6.8 The unstake(...) function should use entityShareToVana(...) to round down to favor protocol

The unstake(...) function allows users to unstake a specified amount of shares and transfer equivalent amount of VANA tokens to the user. To convert the shares to VANA tokens, it divides the share amount by the result of the vanaToEntityShare(...) function.

function unstake(uint256 entityId, uint256 shareAmount) external override nonReentrant whenNotPaused {
    // ...

    // @audit vanaToEntityShare(...) rounds down the result
    uint256 vanaToShare = vanaPoolEntity.vanaToEntityShare(entityId);

    // ...

    // Store the exact VANA amount corresponding to shares at this point
    // @audit Dividing by a smaller value increases the result
    uint256 exactVanaAmount = (1e18 * shareAmount) / vanaToShare;

    // Update staker's position
    _stakers[_msgSender()].entities[entityId].shares -= shareAmount;

    // Update entity staking data in VanaPoolEntity contract
    vanaPoolEntity.updateEntityPool(entityId, shareAmount, exactVanaAmount, false);

    bool success = vanaPoolTreasury.transferVana(payable(_msgSender()), exactVanaAmount);
    if (!success) {
        revert TransferFailed();
    }

    emit Unstaked(entityId, _msgSender(), exactVanaAmount, shareAmount);
}

However, the vanaToEntityShare(...) rounds down the result. As dividing by a smaller value results in a larger outcome, this leads to the user receiving more tokens than they should.

function vanaToEntityShare(uint256 entityId) external view override returns (uint256) {
    Entity storage entity = _entities[entityId];

    return entity.activeRewardPool > 0 ? (entity.totalShares * 1e18) / entity.activeRewardPool : 1e18;
}

For example, if activeRewardPool is 43197679269104491383 and totalShares is 3499999999999991547, consider the following exploit scenario:

• Bob stakes 640854177891528377162 VANA tokens and receives 51923845460479575293 shares;
• Bob then unstakes all his shares ( 51923845460479575293 shares)and receives 640854177891528385059 VANA tokens;

In this case, Bob receives an extra 7897 VANA tokens due to the rounding error.

Recommendation

Consider using entityShareToVana(...) function instead of vanaToEntityShare(...) for converting shares to VANA tokens. This can be done by multiplying the share amount by the result of entityShareToVana(...) function.

6.9 Unauthorized execution of migrateStakeToVanaPool(...) on behalf of others

The DLPRootImplementation contract introduces the migrateStakeToVanaPool(...) function, which allows users to migrate their existing DLP stakes to the VANA pool. This function stakes the user's existing amount plus any claimable rewards into the pool for a specific entity.

function migrateStakeToVanaPool(uint256 stakeId, uint256 entityId) external payable nonReentrant whenNotPaused { 
    dlpRootEpoch.createEpochs();

    Stake storage stake = _stakes[stakeId];

    if (stake.endBlock == 0) {
        stake.movedAmount = stake.amount;
        _closeStake(_msgSender(), stakeId);
        stake.movedAmount = 0;
    }

    uint256 toClaim = stake.amount + _calculateStakeRewardUntilEpoch(stakeId, dlpRootEpoch.epochsCount() - 1, true);

    if (toClaim == 0) {
        revert NothingToClaim();
    }

    bool success = dlpRootRewardsTreasury.transferVana(payable(address(this)), toClaim);
    if (!success) {
        revert TransferFailed();
    }

    vanaPoolStaking.stake{value: toClaim}(entityId, stake.stakerAddress);

    emit StakeMigratedToVanaPool(stakeId, stake.amount, entityId);
}

However, it can be executed by anyone for any stakeId. Thus, allowing anyone to migrate stake of other users to any entity.

Recommendation

Consider adding a check to ensure that the migrateStakeToVanaPool(...) can only be executed by the owner of the stakeId.

6.10 Excessive locked rewards are not handled upon entity removal

The VanaPoolEntityImplementation contract allows anyone to add rewards to be locked and distributed to stakers who have allocated their funds to a specific entity. The processRewards(...) function is periodically called to distribute these rewards. However, when an entity is removed via the removeEntity(...) function, the contract does not check for excessive locked rewards:

function removeEntity(uint256 entityId) external override whenNotPaused nonReentrant onlyEntityOwner(entityId) {
    Entity storage entity = _entities[entityId];

    if (entity.status != EntityStatus.Active) {
        revert InvalidEntityStatus();
    }

    // Process any pending rewards
    processRewards(entityId);
       entity.status = EntityStatus.Removed;   
    _activeEntityIds.remove(entityId);

    emit EntityStatusUpdated(entityId, EntityStatus.Removed);
}

The entity may have an excess rewards that are not processed yet, and since there is no mechanism to handle any remaining undistributed rewards, these funds become permanently locked within the treasury contract once the entity is removed.

Recommendation

Consider moving any exessive rewards when removing an entity.

Resolution

We are aware of the issue and we didn't decide yet what to do with the excessive rewards.

6.11 removeEntity(...) does not clear entityNameToId

When creating or updating an entity, the contract checks that the entity name isn’t already used by verifying that the mapping value is zero. However, when an entity is removed via removeEntity(...), the mapping entry entityNameToId is not cleared:

function removeEntity(uint256 entityId) external override whenNotPaused nonReentrant onlyEntityOwner(entityId) {
    Entity storage entity = _entities[entityId];

    if (entity.status != EntityStatus.Active) {
        revert InvalidEntityStatus();
    }

    // Process any pending rewards
    processRewards(entityId);
       entity.status = EntityStatus.Removed;   
    _activeEntityIds.remove(entityId);

    emit EntityStatusUpdated(entityId, EntityStatus.Removed);
}

Since the mapping entry for entityNameToId is not cleared, the removed entity’s name remains blocked from future use. This can be exploited by malicious users to monopolize desirable entity names by repeatedly creating and removing entities, thereby preventing legitimate users from registering those names.

Recommendation

Consider clearing the entityNameToId upon entity removal.

Resolution

This is an intended behavior.

6.12 Inconsistent check for minimum stake amount

The stake(...) function ensures that the amount staked is greater than or equal to a minimum threshold ( minStakeAmount).

function stake(uint256 entityId, address recipient) external payable override nonReentrant whenNotPaused {
    // ...

    uint256 stakeAmount = msg.value;

    if (stakeAmount < minStakeAmount) {
        revert InsufficientStakeAmount();
    }

    // ...
}

However, the unstake(...) function does not have a similar check to ensure that after unstaking, the remaining stake is still above the minimum stake amount. This could result in a scenario where users stake an amount above the minimum, but after unstaking a portion, their remaining stake falls below the required minimum threshold.

Additionally, if a user had staked an amount above the minimum threshold and later tries to stake an amount smaller than the minimum, the transaction will revert, even though the user's total stake is still above the minimum.

Recommendation

Consider updating the check for the minimum stake amount in the stake(...) function to ensure it properly enforces the minimum threshold. Additionally, add a similar check in the unstake(...) function to ensure that users cannot have a stake less than the minimum required amount, unless they are unstaking all of their shares. This will help maintain consistency between the two functions and prevent users from inadvertently reducing their stake below the minimum threshold.

Resolution

Fixed by removing restrictions from stakers to stake below minStakeAmount in commit 97de94d

6.13 Rounding issue in the unstake(...) function allows owner to unstake below minRegistrationStake

The unstake(...) function allows users to unstake a specified amount of shares and transfer equivalent amount of VANA tokens to the user. It also ensures that entity owner retains at least the minRegistrationStake remaining after unstaking.

function unstake(uint256 entityId, uint256 shareAmount) external override nonReentrant whenNotPaused {
    // ...

    // Get entity info from VanaPoolEntity
    IVanaPoolEntity.EntityInfo memory entityInfo = vanaPoolEntity.entities(entityId);

    // @audit `vanaToEntityShare(...)` rounds down the result
    uint256 vanaToShare = vanaPoolEntity.vanaToEntityShare(entityId);

    // ...

    // If this is the entity owner, ensure they can't unstake below the registration stake
    if (entityInfo.ownerAddress == _msgSender()) {
        // @audit This results in an underestimated value
        uint256 ownerMinShares = (vanaToShare * vanaPoolEntity.minRegistrationStake()) / 1e18;

        if (stakerShares - shareAmount < ownerMinShares) {
            revert CannotRemoveRegistrationStake();
        }
    }

    // ...
}

The function calculates the minimum shares that the owner should retain after unstaking by converting minRegistrationStake (in VANA tokens) to shares using the result of vanaToEntityShare(...). However, because vanaToEntityShare(...) rounds down the result, the calculated ownerMinShares is less than the actual required amount. This allows the owner to unstake more shares than intended, leaving them with fewer shares than required to meet the minRegistrationStake value.

Recommendation

Consider rounding up the result of vanaToShare when calculating ownerMinShares.

Alternatively, consider converting the remaining shares the owner will have after unstaking to VANA tokens by multiplying them with the result of entityShareToVana(...) and comparing that value with the minRegistrationStake.

6.14 migrateStakeToVanaPool(...) doesn't need to be payable

The migrateStakeToVanaPool(...) function is designed to migrate stakes from the DLP system to the VANA pool. However, the function is marked as payable unnecessarily, even though it does not require native tokens to be sent.

function migrateStakeToVanaPool(uint256 stakeId, uint256 entityId) external payable nonReentrant whenNotPaused {}

Recommendation

Consider removing the payable keyword from the function declaration.