Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mVeNFT can't trigger the vote function for one epoch #9

Closed
c4-bot-3 opened this issue Sep 25, 2024 · 4 comments
Closed

mVeNFT can't trigger the vote function for one epoch #9

c4-bot-3 opened this issue Sep 25, 2024 · 4 comments
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working duplicate-21 🤖_09_group AI based duplicate group recommendation satisfactory satisfies C4 submission criteria; eligible for awards sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity")

Comments

@c4-bot-3
Copy link
Contributor

Lines of code

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

Vulnerability details

Description

The protocol has the Attach feature that lets the users delegate their voting power to mVeNFT (Managed Voting NFT) which will manage voting and reward collection without requiring the user’s active participation.
Note the mVeNFT is owned by a managed (strategy) contract e.g,CompoundVeFNXManagedNFTStrategyUpgradeable

Users can attach a veNFT to a mVeNFT using VoterUpgradeableV2.sol#attachToManagedNFT() and to detache calls VoterUpgradeableV2.sol#dettachFromManagedNFT()

In case the last owner veNFT in mVeNFT triggers dettachFromManagedNFT().
It will enter this IF block

File: VoterUpgradeableV2.sol#dettachFromManagedNFT()

589:         if (weight == 0) {
590:             _reset(managedTokenId);
591:             delete lastVotedTimestamps[managedTokenId];
592:         } else {

So, the reset function will delete all the voting weight values of the mVeNFT and its lastVotedTimestamps[]

Now, after time a new user calls attachToManagedNFT()

    function attachToManagedNFT(uint256 tokenId_, uint256 managedTokenId_) external nonReentrant onlyNftApprovedOrOwner(tokenId_) {
        _checkVoteDelay(tokenId_);
        _checkVoteWindow();
        IManagedNFTManager(managedNFTManager).onAttachToManagedNFT(tokenId_, managedTokenId_);
        _poke(managedTokenId_);
    }

which sub-call to _poke(). even if the voting weight of mVeNFT is still empty.
the poke will sub-call to update the value of lastVotedTimestamps[] to the current time.

This will leave the mVeNFT with zero votes in the current epoch (it is not able to call the vote function due to _checkVoteDelay() )

Impact

The mVeNFT can't trigger the vote function for one epoch, the user(s) end up with zero rewards in this epoch

Tools Used

Manual Review

Recommended Mitigation Steps

function attachToManagedNFT(uint256 tokenId_, uint256 managedTokenId_) external nonReentrant onlyNftApprovedOrOwner(tokenId_) {
        _checkVoteDelay(tokenId_);
        _checkVoteWindow();
        IManagedNFTManager(managedNFTManager).onAttachToManagedNFT(tokenId_, managedTokenId_);
+        if (poolVote[managedTokenId_] != 0) {
        _poke(managedTokenId_);
+         }
        emit AttachToManagedNFT(tokenId_, managedTokenId_);
    }

Assessed type

Invalid Validation

@c4-bot-3 c4-bot-3 added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working labels Sep 25, 2024
c4-bot-1 added a commit that referenced this issue Sep 25, 2024
@c4-bot-13 c4-bot-13 added the 🤖_09_group AI based duplicate group recommendation label Sep 25, 2024
@c4-judge
Copy link

alcueca marked the issue as primary issue

@c4-judge c4-judge added the primary issue Highest quality submission among a set of duplicates label Sep 26, 2024
@b-hrytsak
Copy link

b-hrytsak commented Sep 30, 2024

Hi, thanks for your submission.

The _updateLastVotedTimestamp was not supposed to be in the _poke method, so cases like yours became possible

/**
     * @dev Updates the voting preferences for a given tokenId after changes in the system.
     * @param tokenId_ The tokenId for which to update voting preferences.
     */
    function _poke(uint256 tokenId_) internal {
       //** code **//
        _updateLastVotedTimestamp(tokenId_);
    }

Thank you for your participation

@b-hrytsak b-hrytsak added the sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") label Sep 30, 2024
@c4-judge
Copy link

alcueca marked the issue as satisfactory

@c4-judge c4-judge added the satisfactory satisfies C4 submission criteria; eligible for awards label Sep 30, 2024
@c4-judge c4-judge removed the primary issue Highest quality submission among a set of duplicates label Sep 30, 2024
@c4-judge
Copy link

alcueca marked issue #21 as primary and marked this issue as a duplicate of 21

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working duplicate-21 🤖_09_group AI based duplicate group recommendation satisfactory satisfies C4 submission criteria; eligible for awards sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity")
Projects
None yet
Development

No branches or pull requests

4 participants