Chinmay - If a max locked nft expires, it will be stuck forever

[sherlock-admin2]

Chinmay

High

If a max locked nft expires, it will be stuck forever

Summary

The votingEscrow contract has a max_lock feature by which an veNFT owner can turn max lock on for their nfts which will re-lock the nft for the max duration on any interaction ( increasing amount, increasing unlock time etc.), and this can also be done by anyone since max_lock is a public function.

The problem is that if a max locked NFT's lock expires, then the NFT will be stuck forever.

Vulnerability Detail

Any interaction with the NFT calls isApprovedOrOwner() for auth check. The max_lock() function is embedded into isApprovedorOwner() to facilitate re-locking on any modifications of the NFT position as well as whenever it uses vote/poke/claim in voter contract.

This is the max lock logic :

    function max_lock(uint _tokenId) public {  
        if(maxLockIdToIndex[_tokenId] !=0 && max_lock_enabled) {
            LockedBalance memory _locked = locked[_tokenId];
            uint unlock_time = (block.timestamp + MAXTIME) / WEEK * WEEK; 

            if(unlock_time > _locked.end) { 

                require(_locked.end > block.timestamp, 'Lock expired');
                require(_locked.amount > 0, 'Nothing is locked');
                require(unlock_time <= block.timestamp + MAXTIME, 'Voting lock can be 52 weeks max'); 

                _deposit_for(_tokenId, 0, unlock_time, _locked, DepositType.INCREASE_UNLOCK_TIME);
            }
        } 
    }

The relevant part here is this : require(_locked.end > block.timestamp, 'Lock expired'); This means that if the max_lock() function is reached after the lock expires, it will always revert.

Now since the isApprovedOrOwner() => max_lock is looped in for all max locked nfts in all functions, this logic will always revert after the lock had expired.

• All position modification functions like increase_unlock_time will revert.
• The position can not be withdrawn now because withdraw also first calls into isApprovedOrOwner => max_lock which will revert, thus all locked lp tokens will be stuck forever
• Likewise all voting/poke/claim functions will revert in voter.sol => preveting the user from claiming any pending rewards
• The position cannot be merged/split => because they too first call isApprovedOrOwner() => max_lock
• If the user tries to solve this problem by disabling the max_lock, Even disable_max_lock will revert because it also calls into isApprovedOrOwner before any other logic.

Now the lock can expire naturally if the user did not interact with it for some time, and after that the lp tokens associated with the lock will be stuck forever in the votingescrow contract.

This can happen for many users and tokenIDs.

Impact

Users locked tokens will be permanently stuck in the contract.

High severity because this can happen under normal operations and easily brick funds of a lot of users.

Code Snippet

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

Tool used

Manual Review

Recommendation

Remove the lock expired check from max_lock logic or refactor the max locking logic by calling it separately at required places and removing it from isApprovedOrOwner() flow.

[nevillehuang]

Low severity, there is more than ample time (52 weeks, 1 year) for a user to ensure maxLock is disabled for the specific token and/or invoke another action to continue max_lock. Additionally, the admin can disable max_lock_enabled to help users avoid this revert.

[chinmay-farkya]

Escalate

I think that the argument : there is more than ample time (52 weeks, 1 year) for a user to ensure maxLock is disabled for the specific token and/or invoke another action to continue max_lock. Additionally, the admin can disable max_lock_enabled to help users avoid this revert

is insufficient.

Sure, there is ample time for the user to interact but this is a very special expectation from the user, which is not documented. The consequences are also not documented, so we cannot assume that the user will know how the code dictates their responsibility. And the consequences are large, as they will completely lose control of their position forever.

This is also not similar to the general defi awareness of claiming your rewards before burning your position. This is more serious and different than that, as it involves losing the principal. The user shall be able to recover their principal amount in any case.

This expectation that every user shall know he needs to recurringly interact with the nft every once in a while is flawed and doesn't fall in the domain of usual "user error" and imo is unintended behavior as the withdrawl etc. should not be blocked, especially not an error for those users who do not know the code logic.

the admin can disable max_lock_enabled to help users avoid this revert : true, but that is not a solution coz the team can't keep enabling and disabling the max_lock for every tokenID that gets stuck over the course of protocol lifecycle. The admin is not a trusted entity to allow/disallow "unblocking of stuck positions", at least that is not documented.

Also note that this could affect multiple users' principal funds.

[sherlock-admin3]

Escalate

I think that the argument : there is more than ample time (52 weeks, 1 year) for a user to ensure maxLock is disabled for the specific token and/or invoke another action to continue max_lock. Additionally, the admin can disable max_lock_enabled to help users avoid this revert

is insufficient.

Sure, there is ample time for the user to interact but this is a very special expectation from the user, which is not documented. The consequences are also not documented, so we cannot assume that the user will know how the code dictates their responsibility. And the consequences are large, as they will completely lose control of their position forever.

This is also not similar to the general defi awareness of claiming your rewards before burning your position. This is more serious and different than that, as it involves losing the principal. The user shall be able to recover their principal amount in any case.

This expectation that every user shall know he needs to recurringly interact with the nft every once in a while is flawed and doesn't fall in the domain of usual "user error" and imo is unintended behavior as the withdrawl etc. should not be blocked, especially not an error for those users who do not know the code logic.

the admin can disable max_lock_enabled to help users avoid this revert : true, but that is not a solution coz the team can't keep enabling and disabling the max_lock for every tokenID that gets stuck over the course of protocol lifecycle. The admin is not a trusted entity to allow/disallow "unblocking of stuck positions", at least that is not documented.

Also note that this could affect multiple users' principal funds.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[nevillehuang]

I still maintain this issue as low severity and hence invalid because

1. The escalation claim seems to be hinging on whether or not the user is aware if his NFT is expiring and how the protocol expiry logic works
2. The user has 52 weeks (1 year, which is a very long time) to act before allowing his max locked nft to expire, i.e. he can take an action any time within this period (extending or disabling max lock)
3. Any action that invokes isApprovedOrOwner() will automatically max lock user NFT for another 52 weeks, e.g. if at the point where 10 weeks has passed, it will extend another 52 weeks for the user to act
4. max locking is a permisionless feature, i.e. if a user enable a max_lock flag, any user or even admin can extend the max_locked NFT at any point of time using max_lock_bulk() functionalities, invoking the extension mentioned in point 3
5. The admin can can disable max_lock_enabled to help users avoid this revert, so it would fall under this:

Funds are temporarily stuck and can be recovered by the administrator or owner.

[cvetanovv]

I agree with the @nevillehuang arguments. This is more about the way the protocol works than a bug. The user has one year to take action. Even more, as Nevi has pointed out.

Also, the admin can use the maxLockToggle() function at any time and solve the problem:

    function maxLockToggle() external {
        require(msg.sender == team);
        max_lock_enabled = !max_lock_enabled;
    }

Then there will be no revert at all in max_lock() because it will not enter the if statement:

if(maxLockIdToIndex[_tokenId] !=0 && max_lock_enabled) {

Planning to reject the escalation and leave the issue as is.

[WangSecurity]

Result: Invalid Has duplicates

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• chinmay-farkya: rejected