When users lock their tokens permanently, they cannot receive the boosted amount of FNX for the locked amount

[c4-bot-3]

Lines of code

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

Vulnerability details

Impact

Users cannot receive the boosted amount of FNX fairly, which results in a loss of funds.

Proof of Concept

The boosted FNX serves as a reward to encourage users to lock their tokens for a longer duration and in larger amounts. Therefore, permanent lockers should also receive the boosted FNX. However, when users lock their tokens permanently, they cannot receive the boosted amount of FNX for the locked tokens.

Let's consider the following scenario:

1. Alice creates a lock with 101 FNX for a 1-week duration and decides to lock it permanently after some time.
2. Alice locks her tokens permanently but does not receive the boostedValue amount of FNX.
3. Bob creates a lock with 1 FNX for a 1-week duration and also locks his tokens permanently.
4. Bob deposits 100 FNX for his token and receives the boostedValue amount of FNX. If _boostFNXPercentage is 1000 (10%), then boostedValue is 10.

Even though Alice and Bob both lock the same 101 FNX permanently, only Bob receives the 10 FNX as a boosted amount. If _boostFNXPercentage is 1000 (10%), Alice effectively loses 10% of her funds.

Tools Used

Manual Review

Recommended Mitigation Steps

Implement a mechanism to allocate the boosted FNX to users who lock their tokens permanently.

Assessed type

Other

[c4-judge]

alcueca marked the issue as duplicate of #11

[c4-judge]

alcueca changed the severity to 2 (Med Risk)

[c4-judge]

alcueca marked the issue as duplicate of #6

[c4-judge]

alcueca marked the issue as satisfactory

[KupiaSecAdmin]

@alcueca Thanks for your judging. This is not a duplicate of #11.

• In #11, it describes the lack of an increase in the boosted amount of permanentTotalSupply, even though users receive the boosted amount in the _processLockChange function.
• In #12, it explains that users do not receive the boosted amount in the lockPermanent function.
  When users lock previously created locks permanently, they do not receive the boosted amount.

They refers the different cases:

• 11 refers to the case where users create a lock or deposit with a duration longer than veBoostCached.getMinLockedTimeForBoost() or permanently.

• 12 refers to the case where users permanently lock previously created locks that had a duration shorter than veBoostCached.getMinLockedTimeForBoost().

This is not a duplicate of #11 because they have different root causes, impacts, and mitigations.

[nnez]

Hey there @alcueca
I agree with @KupiaSecAdmin that this is not a duplicate of #11 but I disagree with its validity.

I think the design choice here is to only one-time give boosted amount for a newly deposit that satisfies the criteria (i.e. minimum amount and minimum locked time)

Besides, if we were to allow Alice in the example to receive her boosted amount from her existing locked amount, it would introduce another vulnerability where Alice can drain all boosted rewards:

• Alice deposits 100 FNX for 1-week
• Alice locks her lock permanently —> Alice get boosted FNX
• Alice calls unlockPermanent —> Alice locked time is 3 months
• Alice locks her lock permanently again —> Alice get boosted FNX for the second time

[KupiaSecAdmin]

@nnez Thanks for your comment, but I don't agree with you. As described in the PoC of the report, Alice doesn't receive the boosted reward that she should receive.

I think the design choice here is to only one-time give boosted amount for a newly deposit that satisfies the criteria (i.e. minimum amount and minimum locked time)

From the readme, the design choice is:

**veBoost:** The feature encourages locking for a longer period and a larger amount, as this will result in additional rewards or FNX to the user's deposit

This means that the design choice is not limited to newly deposit. The permanent locking also satisfies the criteria (i.e., minimum amount and minimum locked time), and it should receive the boosted amount.

Besides, if we were to allow Alice in the example to receive her boosted amount from her existing locked amount, it would introduce another vulnerability where Alice can drain all boosted rewards:

Your scenario describes the problem following a simple mitigation: just adding allocation code in the lockPermanent function. This mitigation causes the double allocation of boosted rewards, and I do not agree with your mitigation.

Since users who permanently lock previously created locks with a duration shorter than veBoostCached.getMinLockedTimeForBoost() do not receive the boosted amount, this vulnerability is valid.

[nnez]

Hey there @KupiaSecAdmin Agreed that a prospective migitation should not be used as a reason to dismiss the validity of the finding.

My intention was to illustrate the design based on the existing functions within the contract. The current implementation clearly does not support providing boosted amounts for already locked funds. As the increase_unlock_time function also does not account for this scenario.

My point is, if it is intended to give more boosted reward when extending the lock, there should exist a more complex mechanism within the contract to handle this scenario because the contract would need to differentiate between locked amounts that have already received boosted rewards and those that have not.

Therefore, I believe the validity of this issue hinges on whether this path (receiving boosted funds by extending the lock) is intended or not. In my point of view, the language in the readme does not explain how veBoost mechanism is supposed to work but rather its purpose. However, I might be biased here.

Perhaps additional input from sponsors could provide further clarity on this matter. @b-hrytsak

[KupiaSecAdmin]

@nnez I appreciate your perspective on the design and the current implementation. However, I still believe that the intention behind the veBoost mechanism is clear in the context of incentivizing longer and larger locks.

As demonstrated in the report, both Alice and Bob locked the same amount for the same period. However, Alice did not receive the boostedValue incentive reward, while Bob did.

The protocol should offer the same user experience to all users who have contributed equally. This is a basic principle in DeFi projects. If not, it can lead to dissatisfaction, reduced participation, and potential long-term sustainability issues, ultimately undermining trust and the overall success of the protocol.

I agree that mitigating this issue would be somewhat complex. However, I believe that the level of complexity should not be a reason to violate this principle.

[c4-judge]

alcueca changed the severity to 3 (High Risk)

[c4-judge]

alcueca marked the issue as not a duplicate

[alcueca]

I agree with @nnez here, I don't see enough evidence that there is an actual bug in the code, considering the current implementation. @KupiaSecAdmin is right that the design could be better, and that the design might not be fair or right, but that falls more into the scope of an analysis finding.

I'll invalidate this finding, but I'll leave @b-hrytsak to consider discussing with @KupiaSecAdmin about a better design in an alternative forum.

[c4-judge]

alcueca marked the issue as unsatisfactory: Invalid

[KupiaSecAdmin]

@alcueca Thanks for your comment. The boosted rewards are introduced to encourage users to lock their assets for a longer period and in larger amounts. Consequently, many users may decide to lock their assets permanently to receive rewards, even though the locks were originally intended for a short period. However, the users do not receive the rewards they deserve. I think this is a valid issue.

[b-hrytsak]

The veBoost mechanism is designed to incentivize those who create veNFTs. It also involve a limited amount of tokens for incentives, so it works like FIFO. The goal is to incentivize new veNFTs and new deposits into such veNFTs (as this can be considered a new veNFT) that meet the conditions

However, the design considerations mentioned above are also interesting.

Thank for your comments.

[alcueca]

Sorry @KupiaSecAdmin, but under the contest rules, I do not think that design shortcomings are necessarily considered vulnerabilities.