Varun_19 - _burn function will always revert even if the caller is the approved spender

[sherlock-admin2]

Varun_19

Medium

_burn function will always revert even if the caller is the approved spender

Summary

_burn function will always revert even if the caller is the approved spender

Vulnerability Detail

Following is _burn function

function _burn(uint _tokenId) internal {
        require(_isApprovedOrOwner(msg.sender, _tokenId), "caller is not owner nor approved");

        address tokenOwner = ownerOf(_tokenId);

        // Clear approval
        approve(address(0), _tokenId);
        // checkpoint for gov
        _moveTokenDelegates(delegates(tokenOwner), address(0), _tokenId);
        // Remove token
        _removeTokenFrom(tokenOwner, _tokenId);
        emit Transfer(tokenOwner, address(0), _tokenId);
    }

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

        address tokenOwner = idToOwner[_tokenId];
        bool spenderIsOwner = tokenOwner == _spender;
        bool spenderIsApproved = _spender == idToApprovals[_tokenId];
        bool spenderIsApprovedForAll = (ownerToOperators[tokenOwner])[_spender];
        return spenderIsOwner || spenderIsApproved || spenderIsApprovedForAll;
    }

As can be seen from above that if msg.sender is approved i.e _spender == idToApprovals[_tokenId] then it is allowed to call _burn function. But when approve(address(0), _tokenId); is called it reverts because it only allows if spenderIsOwner or spenderIsApprovedForAll not if spenderIsApproved as can be seen from the following function

 function approve(address _approved, uint _tokenId) public {
        address tokenOwner = idToOwner[_tokenId];
        // Throws if `_tokenId` is not a valid NFT
        require(tokenOwner != address(0));
        // Throws if `_approved` is the current owner
        require(_approved != tokenOwner);
        // Check requirements
        bool senderIsOwner = (idToOwner[_tokenId] == msg.sender);
        bool senderIsApprovedForAll = (ownerToOperators[tokenOwner])[msg.sender];
        require(senderIsOwner || senderIsApprovedForAll);
        // Set the approval
        idToApprovals[_tokenId] = _approved;
        emit Approval(tokenOwner, _approved, _tokenId);
    }

Impact

Approved spender can not call _burn function thus causes this functionality to be redundant as _burn is called in withdraw function as well as merge function.

Code Snippet

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

Tool used

Manual Review

Recommendation

Instead call _clearApproval function.

Duplicate of #42