c4-bot-5 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

The users' voting power is calculated incorrectly.

Proof of Concept

In the VotingEscrowUpgradeableV2.onDetachFromManagedNFT function, newBalance_ is subtracted from both permanentTotalSupply and newManagedLocked.amount.

File: contracts\core\VotingEscrowUpgradeableV2.sol
307:         permanentTotalSupply -= (newBalance_ < permanentTotalSupply ? newBalance_ : permanentTotalSupply);
312:         newManagedLocked.amount -= amount < newManagedLocked.amount ? amount : newManagedLocked.amount;

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

File: contracts\nest\ManagedNFTManagerUpgradeable.sol
215:         IVotingEscrowV1_2(votingEscrow).onDettachFromManagedNFT(
216:             tokenId_,
217:             tokenInfo.attachedManagedTokenId,
218:@>           tokenInfo.amount + lockedRewards
219:         );

As a result, this causes incorrect balance of NFT.

Let's consider the following scenario:

Alice locks 100 FNX permanently: permanentTotalSupply = 100.
Bob creates a lock with 100 FNX and attaches it to the managedToken: permanentTotalSupply = 200.
After some time, Alice detaches from the managedToken and receives 10 FNX as lockedRewards from managedNFTManager: permanentTotalSupply = 200 - 100 - 10 = 90.
When Bob tries to vote, his NFT balance is reported as 90, even though he locked 100 FNX permanently.

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_);
+       uint256 oldBalance = newBalance_ - IManagedNFTManager(managedNFTManager).tokensInfo[tokenId_];
-       permanentTotalSupply -= (newBalance_ < permanentTotalSupply ? newBalance_ : permanentTotalSupply);
+       permanentTotalSupply -= oldBalance;
        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;
+       newManagedLocked.amount -= oldBalance;
        _updateNftLocked(managedTokenId_, newManagedLocked);
    }

Assessed type

Other

c4-judge commented 1 month ago

alcueca marked the issue as duplicate of #13