Stuck in setApprovalForAll

Hi,
I'm trying to implement a very simple NFT Marketplace contract.
But on buyNFT operation I cannot get the token to be transferred to the buyer since it's not approved.
I understand that I need to use setApprovalForAll but I've tried many configurations with no luck.
What am I missing here? Any help will be appreciated.
Thanks in advance

:1234: Code to reproduce

Contract:

//SPDX-License-Identifier: Unlicense

pragma solidity ^0.8.9;

import "../openzeppelin-contracts/contracts/token/ERC721/ERC721.sol";
import "../openzeppelin-contracts/contracts/token/ERC721/IERC721Receiver.sol";
import "../openzeppelin-contracts/contracts/token/ERC721/extensions/ERC721Enumerable.sol";
import "../openzeppelin-contracts/contracts/token/ERC721/extensions/ERC721URIStorage.sol";

contract ArttsNFT2 is ERC721URIStorage, ERC721Enumerable, IERC721Receiver {
    address private minter;

    mapping(uint256 => uint256) private _prices;

    constructor(string memory _name, string memory _symbol)
        ERC721(_name, _symbol)
    {
        minter = msg.sender;
    }

    function createNFT(string memory tokenURI, uint256 price)
        public
        returns (uint256)
    {
        require(msg.sender == minter, "Only contract creator can mint");
        uint256 newItemId = totalSupply() + 1;
        _safeMint(minter, newItemId);
        _setTokenURI(newItemId, tokenURI);
        setApprovalForAll(address(this),true);
        _prices[newItemId] = price;
        return newItemId;
    }

    function buyNFT(uint256 tokenId) public payable returns (uint256) {
        require(
            msg.value >= _prices[tokenId],
            "Transaction value is less than item's price"
        );
        payable(minter).transfer(msg.value);
        safeTransferFrom(ownerOf(tokenId), msg.sender, tokenId);
        return tokenId;
    }

    // function safeTransferFrom(address owner, address buyer, uint256 tokenId) override public payable {
    //     require(
    //         msg.value >= _prices[tokenId],
    //         "Transaction value is less than item's price"
    //     );
    //     safeTransferFrom(minter, msg.sender, tokenId);
    //     payable(minter).transfer(msg.value);
    // }


    function priceOf(uint256 tokenId) public view returns (uint256) {
        return _prices[tokenId];
    }

    // function isApprovedForAll(address owner, address operator)
    //     public
    //     view
    //     override
    //     returns (bool)
    // {
    //     if (owner == minter) return true;
    //     else return false;
    // }

    function onERC721Received(
        address _operator,
        address _from,
        uint256 _tokenId,
        bytes calldata _data
    ) external pure override returns (bytes4) {
        return IERC721Receiver.onERC721Received.selector;
    }

    function _beforeTokenTransfer(
        address from,
        address to,
        uint256 tokenId
    ) internal override(ERC721, ERC721Enumerable) {
        super._beforeTokenTransfer(from, to, tokenId);
    }

    function supportsInterface(bytes4 interfaceId)
        public
        view
        override(ERC721, ERC721Enumerable)
        returns (bool)
    {
        return super.supportsInterface(interfaceId);
    }

    function _burn(uint256 tokenId)
        internal
        override(ERC721, ERC721URIStorage)
    {
        super._burn(tokenId);
    }

    function tokenURI(uint256 tokenId)
        public
        view
        override(ERC721, ERC721URIStorage)
        returns (string memory)
    {
        return super.tokenURI(tokenId);
    }
}

Test unit

const nft = artifacts.require("./ArttsNFT2.sol");

var contractInstance
var _name = "NAME"
var _symbol = "SYMBOL"
var _price = ".001"
var _tokenUri = "http://google.com"

contract("ArttsNFT2", accounts => {

  const artts_address = accounts[0];
  const buyer = accounts[1];

  it("...should get an instance to a contract.", async () => {

    contractInstance = await nft.new(_name, _symbol)
    console.log("Contract Address", contractInstance.address)

  });

  it("...should create an NFT and minter should be the owner.", async () => {

    const result = await contractInstance.createNFT(_tokenUri, web3.utils.toBN(web3.utils.toWei(_price, 'ether')).toString());
    assert.equal(await contractInstance.totalSupply(), 1, "There is not 1 token minted in totalSupply");
    const owner = await contractInstance.ownerOf(1);
    assert.equal(owner,artts_address, "minter account does not own the token");

  });


  it("...should buy the NFT and minter should receive payment.", async () => {

    const balance_before = await web3.eth.getBalance(artts_address)
    const response = await contractInstance.buyNFT(1,{
      from: buyer,
      gasPrice: web3.utils.toWei('5', 'gwei'),
      gasLimit: web3.utils.toWei('0.39', 'mwei'),
      value: web3.utils.toWei(_price)
    })
    const owner = await contractInstance.ownerOf(1);
    assert.equal(owner, buyer, "Buyer account does not own the token");
    const balance_after = await web3.eth.getBalance(artts_address)
    assert(balance_after > balance_before, "Minter did not receive any payment");

  });

  
});

Error

   Contract: ArttsNFT2
Contract Address 0x1567A26506Ee76F33b1F60EB4d01C3f73b6EF8cE
    ✓ ...should get an instance to a contract. (351ms)
    ✓ ...should create an NFT and minter should be the owner. (279ms)
    ✓ ...should create another NFT and totalSupply() should be 2. (277ms)
    ✓ ...should create another NFT and totalSupply() should be 3. (280ms)
    1) ...should buy the NFT and minter should receive payment.
    > No events were emitted


  4 passing (3s)
  1 failing

  1) Contract: ArttsNFT2
       ...should buy the NFT and minter should receive payment.:
     Error: Returned error: VM Exception while processing transaction: revert ERC721: transfer caller is not owner nor approved -- Reason given: ERC721: transfer caller is not owner nor approved.
      at Context.<anonymous> (test/ArttsNFT2.js:48:45)
      at runMicrotasks (<anonymous>)
      at processTicksAndRejections (internal/process/task_queues.js:97:5)

:computer: Environment

truffle(develop)> version
Truffle v5.4.15 (core: 5.4.15)
Solidity - ^0.8.9 (solc-js)
Node v12.22.7
Web3.js v1.5.3

Had a hard time figuring this out myself too!

What's happening is that in buyNFT you're using safeTransferFrom. This function will check that the current msg.sender has the ability to transfer the given token. Note that the current msg.sender at that point is the buyer, who definitely does not have the permissions to transfer the token yet. It is not the NFT contract itself, which is what your use of setApprovalForAll would enable.

Using setApprovalForAll is not necessary for the NFT contract itself, because it has direct access and can simply use the internal function _safeTransferFrom and bypass all checks on the sender.

So to summarize the recommendation is: remove setApprovalForAll, use internal _safeTransferFrom.

1 Like

Hi @frangio You have suggested in this answer to not use the setApprovalForAll function, but used the _safeTransferFrom internal function instead. How can this be achieve in a marketplace contract such as described above (and on which I am myself working...)? From my understanding, the _safeTransferFrom function is being used so that the seller does not have to be "present" during the sale, initiated by a purchaser who has agreed to buy an NFT on the marketplace after the owner had put the said NFT for sale at an earlier time. If we are to use the _safeTransferFrom function, this would need to be initiated by the owner of the NFT himself (implying that he would need to go on the marketplace and trigger the sale - and pay for the gas), no? How do markets such as OpenSea implement this (since they apparently use the setApprovalForAll so that users of their platform do not have to pay for gas every time that they wish to put an NFT on sale - the purchaser being the one paying for the gas during a purchase)?

I have also noticed that if and once that the owner has called the setApprovalForAll function, anyone can actually called the _safeTransferFrom function. This is an issue if the selling price is set within the front-end Dapp (i.e. Web Site) and not specified on the blockchain itself (to avoid paying for gas every time that the owner wishes to change the price - here again changing the selling price is feasible in markets such as OpenSea without paying for gas), enabling any purchaser to trigger the sale of the NFT at whichever price he desires, without going through the front-end marketplace (where the price specified by the owner would be used). How can the setApprovalForAll function be used properly? I do not understand in which case this function would be useful (but certainly I am missing something).

Thank you. J

No, the function with an underscore is an internal function that doesn't require the token owner to initiate the transaction. Obviously if you use this function you need to verify in some other way (e.g. a signature) that the token owner has agreed to have their token transferred away under some conditions that have been met.

I can't give a lot more detail right now, I encourage you to try it out.

Thank you @frangio But I am still not understanding who would initiate and sign the transaction if not the owner. If I use the setApprovalForAll and grant my Smart Contract authority to sell, the sale can be initiated by anyone at any price without going through my Web Portal as explained above. I am most likely not understanding something properly, but there appears to be a hole here, or at least this function is not useful in a marketplace type of Smart Contract.

Thanks again for your time.

Maybe your question is not related to this thread. In this thread, the suggestion was to use the internal _safeTransfer because the buy function is embedded into the NFT contract. If your architecture has a separate marketplace contract that is not an option.

If you can please create a new post so we can talk about your architecture's specifics.

However, regarding your previous point:

This doesn't sound good. People will always be able to contact your smart contract directly, so you can't rely on the front end for any critical piece of logic, including prices. The seller can agree to a price by generating an off-chain signature without paying any gas.

Hi Guys,
Sorry for the late response.
Unfortunately _safeTransferFrom requires the sender to be owner or approved so we'd be stuck again since we cannot know in advance who our buyers will be.
The truth is that you cannot execute setApprovalForAll on your own contract, it has no effect (it does not throw any error, though).
So to use it you need to change to a Proxy pattern where you have two contracts .

  1. NFT Contract (E721)
  2. Marketplace contract (whit buyNFT function that calls NFT.safeTransferFrom)

After contracts deployment you need to execute setApprovalForAll(Market.address) on NFT contract to let the market transfer any token upon a purchase.
Hope it helps.
Regards