Approved addresses cannot merge or withdraw from veLP NFTs because of conflicting checks

Summary

The protocol intends to allow approved addresses to do many things on behalf of a veLP NFT owner. All other functions work properly, but merge and withdraw will always revert if called by an approved address.

Vulnerability Detail

Lets take the example of merge. Here, the isApprovedOrOwner() checks make sure that the 'from' tokenID and 'to' tokenID are both either approved to/ or owned by the caller.

In this merge flow, it later calls _burn(). This is the burn function :

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

        address tokenOwner = ownerOf(_tokenId);

        approve(address(0), _tokenId);  <- @audit

        _moveTokenDelegates(delegates(tokenOwner), address(0), _tokenId);
        _removeTokenFrom(tokenOwner, _tokenId);
        emit Transfer(tokenOwner, address(0), _tokenId);
    }

The relevant part here is the approve() function.

    function approve(address _approved, uint _tokenId) public {
        address tokenOwner = idToOwner[_tokenId];
        require(tokenOwner != address(0));
        require(_approved != tokenOwner);

        bool senderIsOwner = (idToOwner[_tokenId] == msg.sender);
        bool senderIsApprovedForAll = (ownerToOperators[tokenOwner])[msg.sender];
        require(senderIsOwner || senderIsApprovedForAll);

        idToApprovals[_tokenId] = _approved;
        emit Approval(tokenOwner, _approved, _tokenId);
    }

This logic never checks if the msg.sender is an approved address for the tokenID, using idToApprovals[tokenID]. This will revert if called by the approved address, but that is incorrect because the intention is clearly to allow an approved address to use these functions, as seen with the logic of isApprovedorOwner() which is used in the merge and withdraw functions' access control checks.

Impact

Approved addresses will not be able to access merge() and withdraw() functions.

Code Snippet

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

Tool used

Manual Review

Recommendation

In the _burn() function use _clearApproval() in place of approve().

Duplicate of #42

sherlock-audit / 2024-06-velocimeter-judging

Chinmay - Approved addresses cannot merge or withdraw from veLP NFTs because of conflicting checks #599