bin2chen - disable_max_lock() user may can't disable max_lock

[sherlock-admin2]

bin2chen

Medium

disable_max_lock() user may can't disable max_lock

Summary

disable_max_lock() uses _isApprovedOrOwner() to check for permissions. However, _isApprovedOrOwner() checks that LockedBalance has not expired, and may revert in some cases, making it impossible to disable max_lock.

Vulnerability Detail

in disable_max_lock() use _isApprovedOrOwner() to check for permissions

    function disable_max_lock(uint _tokenId) external {
@>      assert(_isApprovedOrOwner(msg.sender, _tokenId));
        require(maxLockIdToIndex[_tokenId] != 0,"disabled");

        uint index =  maxLockIdToIndex[_tokenId] - 1;
        maxLockIdToIndex[_tokenId] = 0;

         // Move the last element into the place to delete
        max_locked_nfts[index] = max_locked_nfts[max_locked_nfts.length - 1];

        // update the index 
        maxLockIdToIndex[max_locked_nfts[index]] = index + 1;

        // Remove the last element
        max_locked_nfts.pop();
    }

   function _isApprovedOrOwner(address _spender, uint _tokenId) internal returns (bool) {
@>      max_lock(_tokenId);

...
    }
    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; // Locktime is rounded down to weeks

            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);
            }
        }
    }

In some extreme cases, this can result in max_lock not being able to be turned off, resulting in the failure of all permissions on the tokenId and the inability to perform any operations on the tokenId. Example:

1. admin set max_lock_enabled = true
2. alice set maxLockIdToIndex[NFT_1]=true
3. admin set max_lock_enabled = false
4. NFT_1 Lock expired
5. admin set max_lock_enabled = true
6. after that, isApprovedOrOwner(NFT_1) will revert , the user can't do anything with tokenId.
7. so alice will call disable_max_lock() to close maxLockIdToIndex[NFT_1] = false , to avoid the problem of isApprovedOrOwner () failure

However, step 7 will fail, because disable_max_lock () using isApprovedOrOwner () to check permissions will ` revert'.

Impact

isApprovedOrOwner() always revert, user may can't disable max_lock , and can't perform any operations on the tokenId

Code Snippet

https://github.com/sherlock-audit/2024-06-velocimeter/blob/main/v4-contracts/contracts/VotingEscrow.sol#L894

Tool used

Manual Review

Recommendation

only check idToOwner[_tokenId] == msg.sender

    function disable_max_lock(uint _tokenId) external {
-       assert(_isApprovedOrOwner(msg.sender, _tokenId));
+       assert(idToOwner[_tokenId] == msg.sender);
        require(maxLockIdToIndex[_tokenId] != 0,"disabled");

        uint index =  maxLockIdToIndex[_tokenId] - 1;
        maxLockIdToIndex[_tokenId] = 0;

Duplicate of #622