sherlock-audit / 2024-06-velocimeter-judging

11 stars 7 forks source link

bin2chen - _burn() overrestricts permissions #582

Closed sherlock-admin4 closed 3 months ago

sherlock-admin4 commented 3 months ago

bin2chen

Medium

_burn() overrestricts permissions

Summary

The user is granted permission idToApprovals[_tokenId] = msg.sender but was unable to execute withdraw() because burn() uses approve(address(0), _tokenId); which requires senderIsOwner || senderIsApprovedForAll.

Vulnerability Detail

when execute withdraw() Will use _isApprovedOrOwner() to check for permissions spenderIsOwner || spenderIsApproved || spenderIsApprovedForAll.

But actually idToApprovals[_tokenId] = msg.sender doesn't work because burn() overly restricts permissions to spenderIsOwner | | spenderIsApprovedForAll

    function _burn(uint _tokenId) internal {
...
@>      approve(address(0), _tokenId);

    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

_burn() is overly restrictive, causing users to be unable to execute withdraw() even though they have been granted privileges.

Code Snippet

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

Tool used

Manual Review

    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);
+      _clearApproval(tokenOwner,_tokenId);
        // checkpoint for gov
        _moveTokenDelegates(delegates(tokenOwner), address(0), _tokenId);
        // Remove token
        _removeTokenFrom(tokenOwner, _tokenId);
        emit Transfer(tokenOwner, address(0), _tokenId);
    }

Recommendation

Duplicate of #42