Can't revoke my own approval?

Security
1

Working on a basic sales contract, I have the user set approval for the token ID in order to make the sales contract. If they decide to cancel it I try to revoke my own approval if it still exists like this:

        if(nft.getApproved(tokenId) == address(this)){
            nft.approve(address(0), tokenId);
        }

However it gives an error that the contract is not an owner or operator. Is there no way to do this without having to have them sign a separate approve(0) ?

2

You could try declaring your sales contract as an operator via the setApprovalForAll instead of using approve to approve the sales contract. Then when a user decides to cancel the contract would instead do something like:

        if(nft.isApprovedForAll(nft.ownerOf(tokenId), address(this)){
            nft.setApprovalForAll(address(this), False);
        }

Just an idea.

3

Sorry, nevermind, the contract still wouldn't be able to renounce the operator role that way, bad idea.

4

Thanks, for the reply. Other issue with that is if there were others listings in the collection they would effectively be cancelled as well.

5

That seems a bit of odd behavior.ERC721 specifies that the approve function can be called by the owner or an operator of the token. Seems that the contract (nft in your code) that you are you are trying to interact with, doesn't implement this fully.

Your code seems correct according to the ERC. The OpenZeppelin implementation for reference: approve() and getApproved()

Is the nft contract code available? Looking into it could be useful to determine if anyone besides the owner can call approve

6

Thanks for the reply, I am using the OpenZeppelin implementation. It seems we have a fundamental misunderstanding of what it means to be an operator. Here is the exact code I am using, it is meant to be a test class.

import "hardhat/console.sol";
import "@openzeppelin/contracts/token/ERC721/ERC721.sol";
import "@openzeppelin/contracts/access/Ownable.sol";
import "@openzeppelin/contracts/utils/Counters.sol";

contract BasicNFT is ERC721, Ownable {
    using Counters for Counters.Counter;

    Counters.Counter private _tokenIdCounter;

    constructor() ERC721("BasicNFT", "BNFT") {
        _tokenIdCounter.increment();
        console.log("deployed starting counter is %s", _tokenIdCounter.current());
    }

    function safeMint(address to) public onlyOwner {
        _safeMint(to, _tokenIdCounter.current());
        console.log("minting a new token with ID %s", _tokenIdCounter.current());
        _tokenIdCounter.increment();
    }
}

and in the test itself

    it('release approval when cancelled', async () => {
        await nft.connect(alice).approve(sale.address, 1);
        await sale.init(alice.address, nft.address, 1, 100, 5)
        await sale.cancelSale();
        expect(await sale.state()).to.equal(3);
        // expect(nft.getApproved(1)).to.equal("0x0000000000000000000000000000000000000000");
    });
7

When i copied the links for my prior post, i didn't actually look at the code and made the same assumption as you

from the EIP:

/// @notice Change or reaffirm the approved address for an NFT
/// @dev The zero address indicates there is no approved address.
///  Throws unless `msg.sender` is the current NFT owner, or an authorized
///  operator of the current owner.
/// @param _approved The new approved NFT controller
/// @param _tokenId The NFT to approve
function approve(address _approved, uint256 _tokenId) external payable;

and this is the address check on the approve method

require(
    _msgSender() == owner || isApprovedForAll(owner, _msgSender()),
    "ERC721: approve caller is not owner nor approved for all"
);

My assumption (and probably yours) was that "an authorized operator of the current owner" also meant "the authorized operator for the tokenID"

8

A solution would be overriding the approve function on your NFT contract and allow it to be called by the owner, "an authorized operator of the current owner" and also "the authorized operator for the tokenID".

Not sure if the EIP left "the authorized operator for the tokenID" out of the list for some reason (there might be security implications, can't think of one)

9

Thanks for digging into that. Well bummer, seems it should have the same check I was using as well so you can pass your approval on. So operator means can operator on behalf of entire contract I guess that makes sense. Not really sure the point in having approve for single token. I think I will just continue to use that. At least if my contract reaches this state it can't ever possibly transfer a token anyway but I do wish I could revoke my own permission.

As for your suggestion of overriding the behavior, I am writing contracts to support ERC721 generically so I am just trying to follow the best practice as a market maker.

10

Yeah, not much to do to handle arbitrary NFT contracts.

Approving a single token can be useful, suppose you own 2 nfts and want to sell them on different markets. approving all tokens to both markets creates a risk: if one of the markets has a flaw and gets hacked, both tokens are at risk. Generally leaving tokens approved is not a good idea.

I'm assuming that there was a lapse when the EIP was defined and ended up getting written in stone. At least to me makes sense to be able to revoke or transfer that single token approval. Doesn't seem to have security implications either.

11

Yeah seems like an oversight in the implementation, and what you say is exactly why I opted to just requit approval for the single token to be listed rather than approveForAll.