Voting power inflation via Merge

[c4-bot-2]

Lines of code

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

Vulnerability details

Where is the bug

The bug is in VotingEscrowUpgradeableV2.merge():

    function merge(
        uint256 tokenFromId_,
        uint256 tokenToId_
    ) external override nonReentrant onlyNftApprovedOrOwner(tokenFromId_) onlyNftApprovedOrOwner(tokenToId_) {
        if (tokenFromId_ == tokenToId_) {
            revert MergeTokenIdsTheSame();
        }
        TokenState memory stateFrom = nftStates[tokenFromId_];
        stateFrom.mergeCheckFrom();
        TokenState memory stateTo = nftStates[tokenToId_];
        stateTo.mergeCheckTo();
        _withdrawClearNftInfo(tokenFromId_, stateFrom);
        _proccessLockChange(
            tokenToId_,
            LibVotingEscrowUtils.toUint256(stateFrom.locked.amount),
            stateFrom.locked.end >= stateTo.locked.end ? stateFrom.locked.end : stateTo.locked.end,
            stateTo.locked,
            DepositType.MERGE_TYPE,
            false
        );
    }

Impact

Allows for users to artificially inflate, their voting power through the NFT merging process. The issue occurs because the merge function incorrectly calculates the resulting voting power when combining two NFTs.

By exploiting the bug, an attacker can:

1. Create multiple NFTs with small token locks and varying lock times
2. Merge these NFTs to generate voting power that significantly exceeds the sum of the individual NFTs' power
3. Gain disproportionate control over the governance system without locking an equivalent amount of tokens

Key consequences:

• Undermines the fairness of voting system
• Opens malicious actors to dominate governance decisions
• Devalues the voting power of honest participants
• Harmful protocol changes or fund misappropriation

Proof of Concept

How it happens:

1. The merge function combines the locked token amounts from both NFTs
2. It uses the later of the two unlock times for the merged NFT
3. The voting power calculation doesn't properly account for this combination, resulting in inflated power

Here is poc to show bug, put in VoterV2.test.ts and run using
npx hardhat test test/core/VoterV2/VoterV2.test.ts:

  it("Should exploit merge mechanism to create voting power inconsistency", async function() {
        const user = signers.otherUser1;
        await token.connect(user).approve(VotingEscrow.target, ethers.MaxUint256);
    
        // Create locks
        const tx1 = await VotingEscrow.connect(user).create_lock_for(ethers.parseEther("1"), 4 * 7 * 24 * 3600, user.address);
        const receipt1 = await tx1.wait();
        const tokenId1 = (receipt1!.logs[0] as EventLog).args![2];
    
        const tx2 = await VotingEscrow.connect(user).create_lock_for(ethers.parseEther("1"), 8 * 7 * 24 * 3600, user.address);
        const receipt2 = await tx2.wait();
        const tokenId2 = (receipt2!.logs[0] as EventLog).args![2];
    
        const initialPower1 = await VotingEscrow.balanceOfNFT(tokenId1);
        const initialPower2 = await VotingEscrow.balanceOfNFT(tokenId2);
    
        // Attempt to create voting power inconsistency
        await VotingEscrow.connect(user).merge(tokenId1, tokenId2);
    
        const finalPower = await VotingEscrow.balanceOfNFT(tokenId2);
        console.log(`Initial power 1: ${initialPower1}`);
        console.log(`Initial power 2: ${initialPower2}`);
        console.log(`Final power: ${finalPower}`);
    
        expect(finalPower).to.be.gt(initialPower1 + initialPower2);
      });

Test output:

Initial power 1: 150607257324080517
Initial power 2: 0
Final power: 608906695153692516
√ Should exploit merge mechanism to create voting power inconsistency (84ms)

As shown, the final power is more than 4 times the sum of the initial powers. This shows a critical bug in the merge function that allows for artificial inflation of voting power.

Recommended Mitigation Steps

1. Strict check so merged voting power doesn't exceed the sum of individual voting powers:

function merge(
    uint256 tokenFromId_,
    uint256 tokenToId_
) external override nonReentrant onlyNftApprovedOrOwner(tokenFromId_) onlyNftApprovedOrOwner(tokenToId_) {
    if (tokenFromId_ == tokenToId_) {
        revert MergeTokenIdsTheSame();
    }
    TokenState memory stateFrom = nftStates[tokenFromId_];
    stateFrom.mergeCheckFrom();
    TokenState memory stateTo = nftStates[tokenToId_];
    stateTo.mergeCheckTo();

    uint256 powerFrom = _balanceOfNFT(tokenFromId_, block.timestamp);
    uint256 powerTo = _balanceOfNFT(tokenToId_, block.timestamp);
    uint256 totalPowerBefore = powerFrom + powerTo;

    _withdrawClearNftInfo(tokenFromId_, stateFrom);
    _proccessLockChange(
        tokenToId_,
        LibVotingEscrowUtils.toUint256(stateFrom.locked.amount),
        stateFrom.locked.end >= stateTo.locked.end ? stateFrom.locked.end : stateTo.locked.end,
        stateTo.locked,
        DepositType.MERGE_TYPE,
        false
    );

    uint256 powerAfter = _balanceOfNFT(tokenToId_, block.timestamp);
    require(powerAfter <= totalPowerBefore, "Merge resulted in inflated voting power");
}

2. Use a cap on max voting power for a single NFT:

uint256 public constant MAX_VOTING_POWER = 1e24; // adjust as you wish
function _proccessLockChange(
    uint256 tokenId_,
    uint256 amount_,
    uint256 unlockTimestamp_,
    LockedBalance memory oldLocked_,
    DepositType depositType_,
    bool shouldBoosted_
) internal {
    // same

    uint256 newPower = _balanceOfNFT(tokenId_, block.timestamp);
    require(newPower <= MAX_VOTING_POWER, "Voting power exceeds maximum allowed");

    // same
}

3. Use check so total supply doesn't increase after a merge:

function merge(
    uint256 tokenFromId_,
    uint256 tokenToId_
) external override nonReentrant onlyNftApprovedOrOwner(tokenFromId_) onlyNftApprovedOrOwner(tokenToId_) {
    uint256 totalSupplyBefore = supply;
    // same
    require(supply <= totalSupplyBefore, "Total supply increased after merge");
}

Assessed type

Context

[b-hrytsak]

Hi, thanks for your submission. You have used the balanceOfNFT function, which returns 0 when called in the same block when there was a change of ownership of the nft, as this is a fuse for certain manipulations. For a more correct check, you needed to mine 1 block after create_lock call or use balanceOfNftIgnoreOwnershipChange. Then, you can see from your test that everything is fine.

    const tokenId2 = (receipt2!.logs[0] as EventLog).args![2];

+   await mine();

    const initialPower1 = await VotingEscrow.balanceOfNFT(tokenId1);

Thank you for your participation

[c4-judge]

alcueca marked the issue as unsatisfactory:
Invalid