dettachFromManagedNFT might revert and temporarily prevent users from detaching in certain situation

mediumCode4rena

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.

solidity📋
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:

1. Bob attaches his veNFT with ManagedNFT.
2. ManagedNFT votes for [gaugeA, gaugeB].
3. gaugeB is killed.
4. Bob decides to detach his veNFT from ManagedNFT.
5. Bob's transaction reverts because _poke will attempt to recast the vote on gaugeB.
6. 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