_processLockChange fails to account for boostedValue when incrementing permanentTotalSupply

[c4-bot-1]

Lines of code

https://github.com/code-423n4/2024-09-fenix-finance/blob/main/contracts/core/VotingEscrowUpgradeableV2.sol#L470

Vulnerability details

Impact

This could lead to an inaccurate balance between the actual locked amount and the permanentTotalSupply. In the worst case, users might not be able to unlock their permanent locks due to over-deduction by previous users with boosted balances.

Proof-of-Concept

The _proccessLockChange function is responsible for updating the locked balance of an NFT. When processing a lock change, the function checks if the lock is eligible for a boost reward from the VeBoost contract.

If the lock is eligible, the function calculates the boost amount and adds it to the total locked amount.

However, when updating permanentTotalSupply (for permanent lock case), the function only adds the original deposit amount, excluding the boosted amount. This can lead to an inaccurate permanent total supply calculation, as shown in the following code snippet:

function _proccessLockChange(
    uint256 tokenId_,
    uint256 amount_,
    uint256 unlockTimestamp_,
    LockedBalance memory oldLocked_,
    DepositType depositType_,
    bool shouldBoosted_
) internal {

    ...
    ... sniped...
    ...

    uint256 boostedValue;
    IVeBoost veBoostCached = IVeBoost(veBoost);
    if (address(veBoostCached) != address(0) && shouldBoosted_) {
        if (depositType_ == DepositType.CREATE_LOCK_TYPE || depositType_ == DepositType.DEPOSIT_FOR_TYPE) {
            if (
                (LibVotingEscrowUtils.roundToWeek(block.timestamp + veBoostCached.getMinLockedTimeForBoost()) <= newLocked.end ||
                    newLocked.isPermanentLocked) && amount_ >= veBoostCached.getMinFNXAmountForBoost()
            ) {
                uint256 calculatedBoostValue = veBoostCached.calculateBoostFNXAmount(amount_);
                uint256 availableFNXBoostAmount = veBoostCached.getAvailableBoostFNXAmount();
                boostedValue = calculatedBoostValue < availableFNXBoostAmount ? calculatedBoostValue : availableFNXBoostAmount;
            }
        }
    }
    newLocked.amount += LibVotingEscrowUtils.toInt128(boostedValue); // <-- @audit boostedValue is added to total locked amount
    uint256 diff = LibVotingEscrowUtils.toUint256(newLocked.amount - oldLocked_.amount);
    uint256 supplyBefore = supply;
    supply += diff;
    if (newLocked.isPermanentLocked) {
        permanentTotalSupply += amount_; // <-- @audit this does not include boostedValue that is also locked
    }

    _updateNftLocked(tokenId_, newLocked);

    ...
    ... snipped ...
    ...

In the code above, permanentTotalSupply is only incremented by the original amount_ value, while boostedValue is added to newLocked.amount. This means that when a user withdraws their locked tokens, the contract will deduct more than was originally added to permanentTotalSupply, leading to an underestimation of the permanent total supply.

Recommended Mitigations

    if (newLocked.isPermanentLocked) {
        permanentTotalSupply += (amount_ + boostedValue);
    }

Assessed type

Context

[b-hrytsak]

Hi, thanks for your submission.

Indeed, after refactoring and changes, this point, although known, was missed in the new code

Thank you for your participation

[c4-judge]

alcueca marked the issue as satisfactory

[c4-judge]

alcueca marked the issue as primary issue

[c4-judge]

alcueca marked issue #11 as primary and marked this issue as a duplicate of 11

[c4-judge]

alcueca changed the severity to 3 (High Risk)

[liveactionllama]

Per request from the judge @alcueca, updating the severity of this issue to Medium to match their intent in this comment: #11 (comment).