sherlock-audit / 2024-06-velocimeter-judging

11 stars 7 forks source link

0xShoonya - `deposit_For` function in `VotingEscrow.sol` accepts NFTs of all types including locked and managed #680

Closed sherlock-admin3 closed 3 months ago

sherlock-admin3 commented 4 months ago

0xShoonya

Medium

deposit_For function in VotingEscrow.sol accepts NFTs of all types including locked and managed

Summary

The deposit_For function in the VotingEscrow.sol contract incorrectly accepts NFTs of all types (normal, locked, and managed) without proper restrictions. This oversight allows unauthorized modifications to locked and managed NFTs, potentially disrupting the intended voting power mechanics and reward distribution system.

Vulnerability Detail

The deposit_for function is designed to allow users to deposit additional tokens for a given NFT, increasing its locked amount and potentially its voting power. However, the function lacks proper checks to differentiate between normal, locked, and managed NFTs:

   function deposit_for(uint _tokenId, uint _value) external nonreentrant {
        LockedBalance memory _locked = locked[_tokenId];

        require(_value > 0); // dev: need non-zero value
        require(_locked.amount > 0, 'No existing lock found');
        require(_locked.end > block.timestamp, 'Cannot add to expired lock. Withdraw');
        _deposit_for(_tokenId, _value, 0, _locked, DepositType.DEPOSIT_FOR_TYPE);
    }

According to the current design of the protocol, anyone can call deposit_For against a normal NFT, no one should be able to call deposit_For against a locked NFT and finally only RewardsDistributorV2.claim function is allowed to call deposit_For function against a managed NFT for processing rebase rewards.

The vulnerability stems from the following issues:

  1. No check is performed to verify if the NFT is locked or managed.
  2. The function allows anyone to call it for any existing NFT, regardless of its type.
  3. For locked NFTs, this could lead to an increase in voting power, which should not be possible.
  4. For managed NFTs, this bypasses the intended mechanism where only RewardsDistributorV2.claim should modify their balance.

But deposit_For function was found to accept NFT of all types (normal, locked, managed) without any restriction.

Impact

The ability to modify managed NFTs directly breaks the protocol's design, where only the RewardsDistributorV2 should handle these operations. Moreover, users can increase the voting power of locked NFTs, which should not participate in voting.

Code Snippet

https://github.com/sherlock-audit/2024-06-velocimeter/blob/63818925987a5115a80eff4bd12578146a844cfd/v4-contracts/contracts/VotingEscrow.sol#L826-L833

Tool used

Manual Review

Recommendation

Add a type check for NFTs in the deposit_for function:

function deposit_for(uint _tokenId, uint _value) external nonreentrant {
    LockedBalance memory _locked = locked[_tokenId];
    require(_locked.amount > 0, 'No existing lock found');
    require(_locked.end > block.timestamp, 'Cannot add to expired lock. Withdraw');

    // Add type check
    require(nftType[_tokenId] == NFTType.NORMAL, "Can only deposit for normal NFTs");

    require(_value > 0); // dev: need non-zero value
    _deposit_for(_tokenId, _value, 0, _locked, DepositType.DEPOSIT_FOR_TYPE);
}

Duplicate of #558