ERC721 Token Transfers and Approvals

Question

ORIGINAL POST (EDITED POST BELOW):

I keep getting the following error in my ERC721 Contract:

“ERC721: transfer caller is not owner nor approved”

This happens when I’m trying to buy a Token that has already been minted and transferred by this same contract to another address.

Luckily I’ve discovered that this error message actually comes directly from the require statement embedded in the standard ERC721 Contract’s transferFrom function, as seen here:

function transferFrom(address from, address to, uint256 tokenId) public virtual override {     
   require(_isApprovedOrOwner(_msgSender(), tokenId), "ERC721: transfer caller is not owner nor approved");
   _transfer(from, to, tokenId);
}

So obviously there’s some kind of permissions/approval business that I need to take care of in order to fix this problem.

The ERC721 Contract provides us with both the approve() and setApprovalForAll() functions - both of which facilitate approvals to either specific or ALL addresses (I think), but after reading the docs. I’m actually more confused than ever on how and where exactly I'm supposed to use these functions.

Is it upon the OWNER of a particular Token to call approve() on their own Token? And if so, when exactly should they be calling that? When they’re minting the Token?

Or is it upon the CONTRACT itself to call setApprovalForAll - passing in it’s own address during that call, so that it can then be able to facilitate the transfer of Tokens to the various addresses that interact with it?

Just who is supposed to be doing the approving - and when?

======================================

PART 2 - UPDATE:

I should have made this update earlier because I discovered the true source of the problem: it's actually in the first require statement written in the ERC721 contract's _transfer() function - which goes like this:

function _transfer(address from, address to, uint256 tokenId) internal virtual {
   require(ownerOf(tokenId) == from, "ERC721: transfer of token that is not own!");
   require(to != address(0), "ERC721: transfer to the zero address");

   ...

What's problematic about that first require() statement is that even if you successfully call approve() or setApprovalForAll (and I did so by calling the latter, passing my own contract's address into it, meaning IT was made the operator by and for every address that owned any Tokens created by it,) well any attempts you'd later make to actually transfer() any of these tokens would immediately fail as they'd be scuttled by that require() statement.
And that's because it explicitly requires the transferrer to be the owner of the Token in question. As-in, merely being the operator is apparently not good enough. At least not the way the contract is currently written by Open-Zeppelin.

Which of course is quite puzzling.

I mean what good does it do us to make another address be an operator if at the end of the day this role does not allow it to actually do any transferring of tokens?

I suppose it's a security issue?

Can't quite tell - at least not yet (and please chime in if you have any insights into this.)

I ended up solving this issue by tweaking that require statement so that it suits my specific needs.

Whether or not doing so compromises the security of my contract in any manner remains to be seen, but I saw/see no other way around it.

Ultimately my original question - or at least my reason for asking it, which is an inquiry into the contract's failure to let us name an operator - either via approve() or setApprovalForAll() - and then have that operator actually and successfully be able to transact tokens on our behalf, remains.
The way that pesky require statement is written out of the box scuttles and therefore completely contradicts the very notion of having operators, and that just doesn't make any sense to me.

If anyone can shed some light on this it'd be great!

Answer 1

To summarize :

• approve(address to, uint256 tokenId) : By calling this method, the sender authorizes the address to to transfer one of his tokens with the Id tokenId.
• setApprovalForAll(address to, bool approved) : By calling this method, the sender authorizes the address to to transfer all his tokens. to is then called an operator of the sender.

In these two functions, to can be either a smart contract or an EOA.

Who is supposed to be doing the approving - and when?

Who ? The NFT owner or an operator of the owner. It will revert otherwise.

When ? In the case the owner needs another user/contract to transfer his tokens on his behalf.

Such a scenario can be seen with decentralized market places for exemple : the users deposit their tokens to the smart contract which acts as an escrow and proceeds to the exchange. The deposit function might look like this :

function deposit(address tokenAddress, uint256 tokenId) public {
require(Token(tokenAddress).transferFrom(msg.sender, address(this), tokenId));
//do some stuff    
}

In order to successfully call deposit, the user has first to approve the contract. Indeed, the contract is the caller of transferFrom and therefore needs to be approved by the NFT owner.

Update

There is no security issue at all with the openzeppelin ERC721 smart contract (at least not the one you point out). Note their contracts are well audited and widely used so that's would be surprising to find such a basic bug in their code.

There are some key concepts about the approve/transfer business you seem misunderstand :

meaning IT was made the operator by and for every address that owned any Tokens created by it)

Please elaborate. A contract can't call itself approve. That's the user who have to. For exemple, if you have 10 users, the 10 ones would need to approve your contract.

And that's because it explicitly requires the transferrer to be the owner of the Token in question.

That's not correct. It requires the from parameter to be the token owner, not the sender. An approved operator should be able to transfer tokens on behalf of the user. If we look at the transferFrom function we have these two important statements (among others) :

• require(_isApprovedOrOwner(_msgSender(), tokenId), "ERC721: transfer caller is not owner nor approved"); : here the contract checks the caller is either the token owner or an approved address
• require(ownerOf(tokenId) == from, "ERC721: transfer of token that is not own!"); : here the contract checks the token is currently owned by from.

I don't know what is your use case/business logic but you should definitely be able to define operators and let them transfer your tokens, without modifying the openzeppelin implementation.

Answer 2

Had the same understanding problem as you. I originally thought the _isApprovedOrOwner() is unnecessary(checks if owner or approved), because at last it is checking if it is the owner.

But when looking closer to the code:

require(_isApprovedOrOwner( _msgSender() , tokenId)`

_transfer(from, to, tokenId)

_isApprovedOrOwner: checking if the msgSender() is ok to operate the token _transfer: checking if the from is ok to operate the token

They are not the same person all the time! msgSender and from is not always the same person! So the checking is essential!

Hope that helps!