c4-bot-1 commented 2 months ago

Lines of code

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

Vulnerability details

Impact

Users' veNFT might be temporarily undetachable, preventing users from performing action on their own veNFT.

Proof-of-Concept

When users invoke dettachFromManagedNFT to get their veNFT back from ManagedNFT, _poke is called at the end of the function to update voting power across gauges voted by this ManagedNFT.

function dettachFromManagedNFT(uint256 tokenId_) external nonReentrant onlyNftApprovedOrOwner(tokenId_) {
    _checkVoteDelay(tokenId_);
    _checkVoteWindow();
    IManagedNFTManager managedNFTManagerCache = IManagedNFTManager(managedNFTManager);
    uint256 managedTokenId = managedNFTManagerCache.getAttachedManagedTokenId(tokenId_);
    managedNFTManagerCache.onDettachFromManagedNFT(tokenId_);
    uint256 weight = IVotingEscrowV2(votingEscrow).balanceOfNftIgnoreOwnershipChange(managedTokenId);
    if (weight == 0) {
        _reset(managedTokenId);
        delete lastVotedTimestamps[managedTokenId];
    } else {
        _poke(managedTokenId);
    }
    emit DettachFromManagedNFT(tokenId_);
}

function _poke(uint256 tokenId_) internal {
    address[] memory _poolVote = poolVote[tokenId_];
    uint256[] memory _weights = new uint256[](_poolVote.length);

    for (uint256 i; i < _poolVote.length; ) {
        _weights[i] = votes[tokenId_][_poolVote[i]];
        unchecked {
            i++;
        }
    }
    _vote(tokenId_, _poolVote, _weights);
    _updateLastVotedTimestamp(tokenId_);
}

function _vote(uint256 tokenId_, address[] memory pools_, uint256[] memory weights_) internal {
    _reset(tokenId_);
    uint256 nftVotePower = IVotingEscrowV2(votingEscrow).balanceOfNFT(tokenId_);
    uint256 totalVotesWeight;
    uint256 totalVoterPower;
    for (uint256 i; i < pools_.length; i++) {
        GaugeState memory state = gaugesState[poolToGauge[pools_[i]]];
        if (!state.isAlive) {
            revert GaugeAlreadyKilled();
        }
        totalVotesWeight += weights_[i];
    }
    ...
    ... snipped ...
    ...
}

_poke loads a list of pools and weights voted by ManagedNFT then recast votes again to the same set of pools and weights via calling into _vote.

However, _vote reverts when one of the pool/gauge has already been killed.

Now consider this situation:

Bob attaches his veNFT with ManagedNFT.
ManagedNFT votes for [gaugeA, gaugeB].
gaugeB is killed.
Bob decides to detach his veNFT from ManagedNFT.
Bob's transaction reverts because _poke will attempt to recast the vote on gaugeB.
Bob can't detach his veNFT until ManagedNFT notices and recast the vote excluding gaugeB.

As a result, users' veNFT might be temporarily undetachable when the described scenario happens.

Recommended Mitigations

Users are expected to only include active pools in normal vote flow.
If one of the pool is inactive, we can safely set its weight to zero and skip over it (gracefully ignore it).

function _vote(uint256 tokenId_, address[] memory pools_, uint256[] memory weights_) internal {
    _reset(tokenId_);
    uint256 nftVotePower = IVotingEscrowV2(votingEscrow).balanceOfNFT(tokenId_);
    uint256 totalVotesWeight;
    uint256 totalVoterPower;
    for (uint256 i; i < pools_.length; i++) {
        GaugeState memory state = gaugesState[poolToGauge[pools_[i]]];
        if (!state.isAlive) {
            delete weights_[i];
            delete pools_[i];
            continue;
        }
        totalVotesWeight += weights_[i];
    }

    uint256 time = _epochTimestamp();
    for (uint256 i; i < pools_.length; i++) {
        address pool = pools_[i];
        if(pool == address(0)) continue;
        address gauge = poolToGauge[pools_[i]];

        uint256 votePowerForPool = (weights_[i] * nftVotePower) / totalVotesWeight;
        if (votePowerForPool == 0) {
            revert ZeroPowerForPool();
        }
        if (votes[tokenId_][pool] > 0) {
            revert NoResetBefore();
        }

        poolVote[tokenId_].push(pool);
        votes[tokenId_][pool] = votePowerForPool;
        weightsPerEpoch[time][pool] += votePowerForPool;
        totalVoterPower += votePowerForPool;
        IBribe(gaugesState[gauge].internalBribe).deposit(votePowerForPool, tokenId_);
        IBribe(gaugesState[gauge].externalBribe).deposit(votePowerForPool, tokenId_);
        emit Voted(_msgSender(), tokenId_, votePowerForPool);
    }
    if (totalVoterPower > 0) IVotingEscrowV2(votingEscrow).votingHook(tokenId_, true);
    totalWeightsPerEpoch[time] += totalVoterPower;
}

Assessed type

DoS

b-hrytsak commented 1 month ago

Thank you for your submission.

Although there is a certain safe way to kill a gauge, etc., the described case is possible if the gauge is killed in the middle of an epoch for some reason, and as a result, the veNFT cannot be unhooked from the strategy for some time.

I am not sure that the recommended mitigation is optimal. Redistribution of votes between live pools decision is also not ideal

Thank you for participation

c4-judge commented 1 month ago

alcueca marked the issue as satisfactory