Voting is possible with a disabled NFT using `VoterUpgradeableV2.poke()`

[c4-bot-9]

Lines of code

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

Vulnerability details

Impact

The owner of a disabled NFT can cast a vote using poke() method. It is possible to deposit FNX token to the disabled NFT, so a malicious NFT owner can manipulate voting power using this vulerability, and this can affect the overall reward calculation.

Proof of Concept

VoterUpgradeableV2.vote() doesn't accept a disabled NFT to cast a vote.

    function vote(
        uint256 tokenId_,
        address[] calldata poolsVotes_,
        uint256[] calldata weights_
    ) external nonReentrant onlyNftApprovedOrOwner(tokenId_) {
        ...
        if (managedNFTManagerCache.isDisabledNFT(tokenId_)) {
            revert DisabledManagedNft();
        }
        ...
    }

But the implementation of VoterUpgradeableV2.poke() doesn't check if the given NFT is disabled or not.

    function poke(uint256 tokenId_) external nonReentrant onlyNftApprovedOrOwner(tokenId_) {
        _checkVoteWindow();
        _poke(tokenId_);
    }

So the owner of a disabled NFT can still cast a vote using poke() method if the NFT is disabled during it is used for voting.

Tools Used

Manual Review

Recommended Mitigation Steps

It is recommended to add a disabled check in the implementation of VoterUpgradeableV2.poke().

    function poke(uint256 tokenId_) external nonReentrant onlyNftApprovedOrOwner(tokenId_) {
        _checkVoteWindow();
+        if (IManagedNFTManager(managedNFTManager).isDisabledNFT(tokenId_)) {
+            revert DisabledManagedNft();
+        }
        _poke(tokenId_);
    }

Assessed type

Other

[b-hrytsak]

Thank you for your submission.

mVeNFTs are centralized and tied to a specific strategy contract, which we trust.

The poke method is not included in the strategy contract, as all locking is permanent and is updated when a new veNFT is added or detached.

The isDisabledNFT is more relevant for managed veNFTs to prevent the attachment of new mVeNFTs and serves as a notification that it is deactivated and no longer in use and not worth using.

[c4-judge]

alcueca marked the issue as satisfactory

[c4-judge]

alcueca marked the issue as selected for report

[Ch-301]

Hello @c4-judge

• The only NFT that could revert here is a mVeNFT, normal veNFT can't get disabled at the first time due to this check.

• Only addresses with MANAGED_NFT_ADMIN role can create mVeNFT.

• Also, only DEFAULT_ADMIN_ROLE address can manage the roles ( by the way they are initialized with the same address ).

Considering the above points and the sponsor https://github.com/code-423n4/2024-09-fenix-finance-findings/issues/18#issuecomment-2382238773 I think this issue is under the centralized risk, from the README file Centralized risk are under the Publicly Known Issues, so we can't assume an admin be malicious. Please let me know your thoughts.

[KupiaSecAdmin]

I agree with the sponsor and @Ch-301. @Ch-301 mentioned the implementation ManagedNFTManagerUpgradeable contract in the above comment, but ManagedNFTManagerUpgradeable is out of scope of this contest. So I think the contract can be used for better understanding of Fenix protocol, but it can't be used as an evidence for a comment. This contest's scope is limited to only 3 contracts, and VoterUpgradeableV2 uses only IManagedNFTManager, and we can't make any assumption about this interface, because ManagedNFTManagerUpgradeable is out of scope of this contest. This issue is valid in the scope of this contest and the sponsor marked this issue as acknowledged instead of dispute.

[nnez]

From my point of view, this issue is valid as the attack path exists. However,

Given the preconditions required:

• mVeNFT is disabled
• mVeNFT is left with votes from unattached VeNFT

and constraints:

• mVeNFT is controlled by trusted role
• vote weights are also controlled by trusted role

I believe that Medium severity would be a better fit for this issue as attacker can't really control the weight of the votes.

[alcueca]

I agree with @Ch-301 in that this is mostly a centralized risk issue, but not completely because the admin can delegate the control of the mVeNFT and it is not clear to me that it wouldn't do it in a regular basis.

With regards to @KupiaSecAdmin, you can use the contracts not in scope as documentation, and that includes understanding how the access control and other configuration will be set up. That will help you determine the likelihood.

Finally, I agree with @nnez. There are a lot of preconditions to this, and quite limited impact. I think that Low is more appropriate than Medium.

[c4-judge]

alcueca changed the severity to QA (Quality Assurance)

[c4-judge]

alcueca marked the issue as grade-a

[liveactionllama]

Per request from the judge, removing the selected for report label, as it was not intended to remain on this issue.