search

code-423n4 / 2024-09-fenix-finance-findings

0 stars 0 forks source link

`boostedValue` should be added to `permanentTotalSupply` for permanently locked tokens #11

Open c4-bot-7 opened 2 months ago

c4-bot-7 commented 2 months ago

Lines of code

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

Vulnerability details

Impact

Unlocking tokens from the permanent locking can be DoSed.

Proof of Concept

In the VotingEscrowUpgradeableV2._processLockChange function, boostedValue is added to the token's locked amount at L465. However, boostedValue is not added to permanentTotalSupply for permanently locked tokens at L470.

File: contracts\core\VotingEscrowUpgradeableV2.sol
465:@>       newLocked.amount += LibVotingEscrowUtils.toInt128(boostedValue);
466:         uint256 diff = LibVotingEscrowUtils.toUint256(newLocked.amount - oldLocked_.amount);
467:         uint256 supplyBefore = supply;
468:         supply += diff;
469:         if (newLocked.isPermanentLocked) {
470:@>           permanentTotalSupply += amount_;
471:         }

In the unlockPermanent function, the token's locked amount is subtracted from permanentTotalSupply at L219.

File: contracts\core\VotingEscrowUpgradeableV2.sol
219:         permanentTotalSupply -= LibVotingEscrowUtils.toUint256(state.locked.amount);

As a result, calling this function may be reverted by the underflow.

Let's consider the following scenario:

  1. Alice and Bob each create locks with 100 FNX for a 1-week duration (< veBoostCached.getMinLockedTimeForBoost).
  2. Alice and Bob lock their tokens permanently: permanentTotalSupply = 100 + 100 = 200.
  3. Alice deposits 1000 FNX (>= veBoostCached.getMinFNXAmountForBoost), and _boostFNXPercentage is 1000 (10%):
    • boostedValue: 100
    • total locked amount: 100 + 1000 + 100 = 1200
    • permanentTotalSupply: 200 + 1000 = 1200
  4. Alice unlocks her token from the permanent lock: permanentTotalSupply = 1200 - 1200 = 0.
  5. Bob tries to unlock his token from the permanent lock, but it is reverted because permanentTotalSupply is 0.

Tools Used

Manual Review

Recommended Mitigation Steps

It is recommended to change the code in the VotingEscrowUpgradeableV2._processLockChange function as following:

Let me know if you need further assistance!

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

Assessed type

DoS

c4-judge commented 2 months ago

alcueca marked the issue as primary issue

b-hrytsak commented 1 month ago

Hi, thanks for your submission.

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

duplicate #6 , or #6 is a duplicate of #11

It is difficult to understand the severity of this issue, it seems to be Overseverity. If we go with the worst-case scenario, the last user/users will not be able to unlock the permanent lock on their veNFTs, which will lead them to some additional temporary lock until the problem is resolved, as they would have to wait 182 days for full unlocking anyway

Thank you for your participation

c4-judge commented 1 month ago

alcueca changed the severity to 2 (Med Risk)

c4-judge commented 1 month ago

alcueca marked the issue as duplicate of #6

c4-judge commented 1 month ago

alcueca marked the issue as satisfactory

c4-judge commented 1 month ago

alcueca marked the issue as selected for report

KupiaSecAdmin commented 1 month ago

@alcueca Thanks for your judging. I think this is high severity. This vulnerability leads not only to a DoS but also to an incorrect calculation of voting power in the _balanceOfNFT function due to the incorrect accumulation of permanentTotalSupply.

File: contracts\core\VotingEscrowUpgradeableV2.sol
532:     function _checkpoint(uint256 tokenId_, LockedBalance memory oldLocked_, LockedBalance memory newLocked_) internal {
             [...]
616:@>       last_point.permanent = LibVotingEscrowUtils.toInt128(permanentTotalSupply);
File: contracts\core\VotingEscrowUpgradeableV2.sol
647:     function _balanceOfNFT(uint256 tokenId_, uint256 timestamp_) internal view returns (uint256 balance) {
649:         if (pointEpoch > 0) {
650:             Point memory lastPoint = nftPointHistory[tokenId_][pointEpoch];
651:             if (lastPoint.permanent > 0) {
652:@>               return LibVotingEscrowUtils.toUint256(lastPoint.permanent);
alcueca commented 1 month ago

This vulnerability leads ... also to an incorrect calculation of voting power

This was not pointed out in the original submission, but it is right.

c4-judge commented 1 month ago

alcueca changed the severity to 3 (High Risk)

b-hrytsak commented 1 month ago

@KupiaSecAdmin @alcueca

In the balanceOfNFT calculation, data regarding the permanentTotalSupply is not used, and the impact of not accounting boostedValue in permanentTotalSupply is limited only to the calculation of the total voting power. This does not affect on votes proccesing, but only the outcome (votingPowerTotalSupply()). The voting power for a user's veNFT will still be calculated correctly.

This statement most likely arose because similar structures and pieces of code are used for the general voting power calculation and for the user. However, last_point in _checkpoint is from supplyPointsHistory, whereas in balanceOfNFT, nftPointHistory is used.

Ch-301 commented 1 month ago

@alcueca Thanks for your judging. I think this is high severity. This vulnerability leads not only to a DoS but also to an incorrect calculation of voting power in the _balanceOfNFT function due to the incorrect accumulation of permanentTotalSupply.

File: contracts\core\VotingEscrowUpgradeableV2.sol
532:     function _checkpoint(uint256 tokenId_, LockedBalance memory oldLocked_, LockedBalance memory newLocked_) internal {
             [...]
616:@>       last_point.permanent = LibVotingEscrowUtils.toInt128(permanentTotalSupply);
File: contracts\core\VotingEscrowUpgradeableV2.sol
647:     function _balanceOfNFT(uint256 tokenId_, uint256 timestamp_) internal view returns (uint256 balance) {
649:         if (pointEpoch > 0) {
650:             Point memory lastPoint = nftPointHistory[tokenId_][pointEpoch];
651:             if (lastPoint.permanent > 0) {
652:@>               return LibVotingEscrowUtils.toUint256(lastPoint.permanent);

hey @c4-judge

I believe there is some wrong assumption in this issue

tl;dr the last_point.permanent and lastPoint.permanent are not the same thing in this logic.

last_point.permanent is related to supplyPointsHistory[] this mapping which is tracking the total supply changes.

However, lastPoint.permanent that used in _balanceOfNFT() is from nftPointHistory[][] this mapping which is recording the changes over time for every veNFT.

and the value of lastPoint.permanent is updated here

            u_new.permanent = permanent;
            nftPointHistory[tokenId_][nftStates[tokenId_].pointEpoch] = u_new;
        }

Which is acutely only this amount here

The impact is more like this QA (nb: not dupl) last user can't call unlockPermanent() successfully

KupiaSecAdmin commented 1 month ago

There is a confunsion for two variables and I agree there is no incorrect calculation of voting power by the permanentTotalSupply. But there still exists DoS vulnerability.

Ch-301 commented 1 month ago

There is a confunsion for two variables and I agree there is no incorrect calculation of voting power by the permanentTotalSupply. But there still exists DoS vulnerability.

I'm not sure if we can call this denial-of-service, because only the last permanent-lock is affected by losing his locked FNX tokens!

alcueca commented 1 month ago

I think the last permanent lock being affected reasonably often merits a medium severity. Thanks @KupiaSecAdmin for retracting your previous statement about permanentTotalSupply.

nnez commented 1 month ago

Sorry if my input might be too late for this but maybe this could be helpful regarding the acutal impact of this.

I actually described the impact @Ch-301 mentioned in my original finding here: https://github.com/code-423n4/2024-09-fenix-finance-findings/issues/6

Although, it's not always the last withdrawal that's gonna have a problem.
For instance, let's say there are 2 existing permanent locks:

Someone creates a new permanent lock, let's say the amount is 200 and they receive 20 boosted FNX:

Now, let's say that someone suddenly changes their mind and call unlockPermanent:

And after this, both lock A and lock B can't be unlocked from permanent lock because it would fail from underflow of permanentTotalSupply

Ch-301 commented 1 month ago

@nnez Yeah, we can create scenarios with 3,4.. locks can't be unlocked from permanent lock because of this issue (the more preconditions required will lead to lower the probability).

So, as @c4-judge mentions here the medium severity is fair.

I think the last permanent lock being affected reasonably often merits a medium severity. ...

Please, @C4-Staff make sure to update the Label here if the judge confirms the medium severity.

liveactionllama commented 1 month ago

Per request from the judge @alcueca, updating the severity of this issue to Medium to match their intent in this comment: https://github.com/code-423n4/2024-09-fenix-finance-findings/issues/11#issuecomment-2392118543.