ERC721 NFT – Why Owner is Not Approved in ERC721?

erc-721nftsolidity

i writte smart contract – auction of nft. You can see code here: https://pastebin.com/rTQHcLHh

So, i have Users contract. There i have connectUser method, it add new User by msg.sender in mapping. Also here is nftInstance and auctionInstance – User contract has access to this.

There are collectItem in User contract – create nft FOR USERADDRES (not user contract address) in nftInstance and save it in mapping, sellItem – create auction in auctionInstance, and placeBid

There is problem. There is finalizeAuction method. It transfers nft to highestBidder address. But i get error – ERC721: transfer caller is not owner nor approved. So, in remix i call ownerOf this nft – it's userAddres (it is saved in User contract on init, see code). But if i call method isApproveForAll, i understand that owner is not the user, but a user contract. Try it yourself!

How to fix that?? I can't understand it;(

Best Answer

The code smell on your code is very strong. I looked around and it is a lot of back and forth trying to understand. You are overcomplicating everything. I think you come from a Java world (me too).

For instance, this function does not improve the code:

function itemApproving(address approved) public {
    setApprovalForAll(approved, true);
}

Just use setApprovalForAll(approved, true); everywhere instead of itemApproving(approved);.

Talking about this setApprovalForAll. It has to be called by the owner of the NFT to do what you expect it to do. It cannot be called by random addresses. However, inside Auction.finalizeAuction you call it, so in effect it is the Auction instance that is asking to setApprovalForAll. Ok, it is not forbidden to do.

But! Later the same Auction instance does nftInstance.itemTransfer(owner,.... This is where it does not work, the auction instance is not allowed to effect the transfer.

You got the meaning of setApprovalForAll in reverse. It is the owner who has to call setApprovalForAll(auction.address, true) in a transaction of its own, sometime before the auction resolves.

For the same reason in User.sellItem, when you do:

nftInstance.itemApproving(
    address(auctionInstance.getAuctionById(auctionId))
)

It is not the real owner that did setApproval, but the User instance.

Further simplify your contracts. A sample:

You don't need UserData.exists. It can be replaced by address(UserData.User) != address(0).
In effect, you can reduce your mapping(address => UserData) to mapping(address => User).
Do you really need a User contract?
Doing an unbounded loop like for (uint256 i = 0; i < userTokenIdsCount is a big no-no. Get a better data structure instead.
All your string operations can probably be sent off-chain.
getRandomNumber() is not really random. It just opens new issues or attack vectors when a user calls it twice in a block, or when a sophisticated attacker knows what value it will have.

Related Solutions

[Ethereum] How to import files into remix (ERC721 contract)

import "github.com/OpenZeppelin/openzeppelin-solidity/contracts/token/ERC721/IERC721.sol";
import "github.com/OpenZeppelin/openzeppelin-solidity/contracts/token/ERC721/IERC721Receiver.sol";
import "github.com/OpenZeppelin/openzeppelin-solidity/contracts/math/SafeMath.sol";
import "github.com/OpenZeppelin/openzeppelin-solidity/contracts/utils/Address.sol";
import "github.com/OpenZeppelin/openzeppelin-solidity/contracts/drafts/Counters.sol";
import "github.com/OpenZeppelin/openzeppelin-solidity/contracts/introspection/ERC165.sol";

This compiles. I am not sure about the v2.0.0 on counter. I replaced that with master.

[Ethereum] approve + safeTransferFrom ERC721 from existing smart contract

It looks like one of your function calls needs to be changed -let's look at the approveAndTransfer function from your contract:

function approveAndTransfer(IERC721 nft, uint256 _tokenId, address _from, address _to) public {
    require(nft.ownerOf(_tokenId) == msg.sender, "NOT OWNER");
    nft.isApprovedForAll(_from, address(this));
    nft.safeTransferFrom(msg.sender, _to,_tokenId);
}

It looks like the goal of the function is to transfer NFTs from the _from address to the _to address. In order to do so, the flow needs to be:

approve the contract to send _from's NFT
contract sends to _to

After the require, though, the contract calls isApprovedForAll on the NFT contract. Looking at OpenZeppelin's ERC721 docs, we can see that this only tells us if an address is approved for another address's NFTs - it doesn't actually do the approving.

It looks like you probably wanted setApprovalForAll, but if we've gotten this far in the conversation, it's worth pointing out why this is probably not the right choice for you either.

Firstly, setApprovalForAll would approve the contract on all of _from's NFTs on this contract, even though the presence of a single _tokenId in the args indicates that the function will only transfer one NFT at a time. It isn't desirable to over-approve like that. Instead, you'd probably have wanted to use the approve function, which takes a tokenId.

Secondly, and maybe this should have even been first, is that you can't approve yourself on someone else's assets. Or, in this case, the contract can't approve itself over msg.sender's NFT. The reasoning is simple - if I could approve myself over someone else's NFTs, then I could just do that and take them whenever I wanted for no reason (also known as stealing). This is why getting a user to approve happens outside of smart contracts in any scenario that I'm aware of - you can use JavaScript to prompt a user to approve, but you can't have a smart contract approve for them.

Hope this helps!

Best Answer

Related Solutions

[Ethereum] How to import files into remix (ERC721 contract)

[Ethereum] approve + safeTransferFrom ERC721 from existing smart contract

Related Topic