c4-bot-8 commented 1 month ago

Lines of code

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

Vulnerability details

Impact

Withdrawing can be DoSed.

Proof of Concept

In the VotingEscrowUpgradeableV2.onDetachFromManagedNFT function, the locked amount is updated as newBalance_ at L309.

File: contracts\core\VotingEscrowUpgradeableV2.sol
305:         int128 amount = LibVotingEscrowUtils.toInt128(newBalance_);
309:         _updateNftLocked(tokenId_, LockedBalance(amount, LibVotingEscrowUtils.maxUnlockTimestamp(), false));

newBalance_ is greater than the actual deposited amount because it includes lockedRewards received from managedNFTManager at L218.

File: contracts\nest\ManagedNFTManagerUpgradeable.sol
210:         uint256 lockedRewards = IManagedNFTStrategy(IVotingEscrow(votingEscrow).ownerOf(tokenInfo.attachedManagedTokenId)).onDettach(
211:             tokenId_,
212:             tokenInfo.amount
213:         );
214: 
215:         IVotingEscrowV1_2(votingEscrow).onDettachFromManagedNFT(
216:             tokenId_,
217:             tokenInfo.attachedManagedTokenId,
218:@>           tokenInfo.amount + lockedRewards
219:         );

When users withdraw their tokens, the locked amount including lockedRewards is subtracted from supply at L510, but supply only cumulated actual deposited amount, not including lockedRewards. When users withdraw their tokens, the locked amount, including lockedRewards, is subtracted from supply at L510. However, supply only accumulates the actual deposited amount and does not account for lockedRewards.

File: contracts\core\VotingEscrowUpgradeableV2.sol
508:         uint256 amount = LibVotingEscrowUtils.toUint256(state_.locked.amount);
509:         uint256 supplyBefore = supply;
510:@>       supply -= amount;

As a result, this can cause withdrawing DoSed.

Let's consider the following scenario:

Alice and Bob each create locks with 100 FNX individually, and Bob attaches his lock to the managed token: supply = 200.
Bob withdraws his tokens: supply = 200 - 100 = 100.
After some time, Alice detaches from the managedToken and receives 1 FNX as lockedRewards from managedNFTManager. Her locked amount is then updated to 101 FNX, and the locking end time is updated to maxUnlockTimestamp.
After maxUnlockTimestamp, Alice tries to withdraw her tokens, but the operation is reverted due to an underflow: supply = 100 < 101.

Tools Used

Manual Review

Recommended Mitigation Steps

It is recommended to change the code as following:

    function onDettachFromManagedNFT(
        uint256 tokenId_,
        uint256 managedTokenId_,
        uint256 newBalance_
    ) external override nonReentrant onlyManagedNFTManager {
        [...]
        int128 amount = LibVotingEscrowUtils.toInt128(newBalance_);
+       supply = supply + newBalance_ - tokensInfo[tokenId_];
        permanentTotalSupply -= (newBalance_ < permanentTotalSupply ? newBalance_ : permanentTotalSupply);
        nftStates[tokenId_].isAttached = false;
        _updateNftLocked(tokenId_, LockedBalance(amount, LibVotingEscrowUtils.maxUnlockTimestamp(), false));
        LockedBalance memory newManagedLocked = nftStates[managedTokenId_].locked;
        newManagedLocked.amount -= amount < newManagedLocked.amount ? amount : newManagedLocked.amount;
        _updateNftLocked(managedTokenId_, newManagedLocked);
    }

Assessed type

Other

c4-judge commented 1 month ago

alcueca marked the issue as primary issue

b-hrytsak commented 1 month ago

Hi, thanks for your submission.

When dettach , the new balance may actually be higher than the previous one because it includes a reward, as a result, the supply will not be updated by the amount of this reward

It seems that this is an Overseverity for this issue, the only consequences are the withdraw calls that will not be possible for the latest veNFTs due to a mathematics error, with a simple mitigation for help this veNFTs

_withdrawClearNftInfo():

 510: supply -= amount;

Thank you for your participation

nnez commented 1 month ago

I'd like to add some more information here for the judge and sponsors to consider as I also consider submitting this issue but later decided not to due to following reasons:

lockedRewards comes from harvesting the distributed rewards in ManagedNFT's strategy

The strategy distributes the reward via compound function https://github.com/code-423n4/2024-09-fenix-finance/blob/main/contracts/nest/CompoundVeFNXManagedNFTStrategyUpgradeable.sol#L129-L139

function compound() external {
    IERC20Upgradeable fenixCache = IERC20Upgradeable(fenix);
    uint256 currentBalance = fenixCache.balanceOf(address(this));
    if (currentBalance > 0) {
        address votingEscrowCache = votingEscrow;
        fenixCache.safeApprove(votingEscrowCache, currentBalance);
        IVotingEscrowV1_2(votingEscrowCache).deposit_for_without_boost(managedTokenId, currentBalance);
        ISingelTokenVirtualRewarder(virtualRewarder).notifyRewardAmount(currentBalance);
        emit Compound(msg.sender, currentBalance);
    }
}

and we can see here that the rewards are deposited and locked into the corresponding mVeNFT which increases the supply accordingly.

Therefore, VeNFT.supply will already have been accounted for lockedRewards

I assumed that the protocol doesn't deduct the supply as they already accounted for this in how the strategy contract distributes the rewards.

So, deduction again in veNFT.onDettachFromManagedNFT would result in a wrong accounting of total supply.
It's either the strategy contract or veNFT contract that has the incorrect implementation. It can't be both.

b-hrytsak commented 1 month ago

@Nnez thank you. Since the reward is already contained in the mVeNFT, the methods onAttachToManagedNFT and onDetachFromManagedNFT only redistribute the existing balance between veNFT and mVeNFT, which does not change the total supply. The supply should only change when new tokens are added to the contract or withdrawn, which does not occur in the attachment or detachment methods. Since the reward is added through the compound method, it is already accounted for in the total supply.