How can I fix a problem with transferring ERC721 by approved contract?

Question

Hey everyone

I'm new to NFT development and developing an ERC721 token for property ownership and marketplace. I implemented two functions to enable a user to put their token on sale for another user and approve the contract and when the other user has paid for it he receives the token and the first user receives the paid ETH. but I get ERC721: caller is not token owner or approved because the transferFrom() is called by the second user's address, not the contract address.

How can I call the transferFrom() by the contract address? What is the best thing to do in this case?

Here are my functions:

function submitSale(address buyer, uint256 tokenId, uint256 price) public {
        if (!_isApprovedOrOwner(msg.sender, tokenId)) {
            revert notTokenOwnerOrApproved();
        }
        tokenIdToBuyer[tokenId] = buyer;
        tokenIdtoPrice[tokenId] = price;
        approve(address(this), tokenId);
        console.log("Sale submited successfuly");
    }

    function payForToken(uint256 tokenId) public payable {
        if (tokenIdToBuyer[tokenId] != msg.sender) {
            revert tokenNotForSaleForThisAddress();
        }
        if (msg.value < tokenIdtoPrice[tokenId]) {
            revert paidAmountIsNotEnough();
        }

        (bool paid, ) = ownerOf(tokenId).call{value: msg.value}("");
        if (paid) {
            TransferFrom(ownerOf(tokenId), msg.sender, tokenId);
            delete tokenIdToBuyer[tokenId];
            delete tokenIdtoPrice[tokenId];
        } else {
            revert();
        }
    }

Answer 1

There are many different ways you can implement a mechanism to buy and sell NFTs that would fit your needs.

The Problem

Note, this problem assumes the ERC721 and the marketplace contract are two seperate contracts, if they are both contained into one contract with inheritance, then this issue isn't present.

Your implementation appears to correctly use transferFrom(), the issue of "transferFrom() is called by the second user's address, not the contract address." isn't an issue since transferFrom() is actually being called by the contract address not the second user like you thought.

To see this visually, take these example contracts:

contract A {

function bar(address b) public view returns(address, address ) {
    return (msg.sender, B(b).foo());
}

}


contract B {
    function foo() public view returns(address caller) {
        caller = msg.sender;
    }
}

When you call bar() with contract B's address, you will get returned two different addresses. The first address is msg.sender of the function, (the user's address in your case), and the second address is the msg.sender of contract B's foo() function, which is contract A since that is who directly called foo().

This means in your example, when you call transferFrom(), the msg.sender isn't your "second user", but instead your contract is.

You can test this by copy and pasting the code into Remix, if you change the msg.sender in contract B to tx.origin, you will get returned the same address twice when you call bar(). (Note, tx.origin is being depreciated and is not recommended to use in production)

Now back to your issue, since it isn't being caused by a difference in addresses when calling transferFrom(), you may have figured out that your issue is actually occuring when you call approve().

Using the same logic as I've displayed above, when you are calling approve(address(this), tokenId);, you are approving your contract to spend a token on behalf of itself.

Since approve() uses msg.sender, your user's will have to call this function prior to interacting with your contract. When you use a frontend to transfer a token or an NFT, usually without knowing it, you are signing an initial transaction to approve the contract.

This is why you may have to sign multiple transactions in order to make a single transfer.

The Solution

Note, this solution assumes the ERC721 and the marketplace contract are two seperate contracts

The easiest solution to minimize code refactoring would be to simply remove approve() from inside your submitSale() function like so:

function submitSale(address buyer, uint256 tokenId, uint256 price) public {
        if (!_isApprovedOrOwner(msg.sender, tokenId)) {
            revert notTokenOwnerOrApproved();
        }
        tokenIdToBuyer[tokenId] = buyer;
        tokenIdtoPrice[tokenId] = price;
        console.log("Sale submited successfuly");
    }

You would then just need to communicate to your users that they will have to approve() your contract to transfer the NFT on their behalf.

Of course, there are other, more complex, approaches you can take if this doesn't seem desirable to you:

Alt Problem

This problem assumes the NFT and marketplace contract are the same contract using inheritance.

For this example assume the seller is Alice and the buyer is Bob.

When Alice calls submitSale(), she is approving your contract for her NFT:

function submitSale(address buyer, uint256 tokenId, uint256 price) public {
        if (!_isApprovedOrOwner(msg.sender, tokenId)) {
            revert notTokenOwnerOrApproved();
        }
        tokenIdToBuyer[tokenId] = buyer;
        tokenIdtoPrice[tokenId] = price;
        approve(address(this), tokenId);
        console.log("Sale submited successfuly");
    }

And then when Bob goes to buy the NFT, he is trying to transferFrom() the token, but this will always fail since Bob is never actually approved:

function payForToken(uint256 tokenId) public payable {
        if (tokenIdToBuyer[tokenId] != msg.sender) {
            revert tokenNotForSaleForThisAddress();
        }
        if (msg.value < tokenIdtoPrice[tokenId]) {
            revert paidAmountIsNotEnough();
        }

        (bool paid, ) = ownerOf(tokenId).call{value: msg.value}("");
        if (paid) {
            TransferFrom(ownerOf(tokenId), msg.sender, tokenId);
            delete tokenIdToBuyer[tokenId];
            delete tokenIdtoPrice[tokenId];
        } else {
            revert();
        }
    }

By calling getApproved() on your Sepolia deployed contract, you can see the only person approved for tokenId 0, is the contract itself:

Alt Solution 1

Note, this solution assumes the ERC721 and the marketplace contract are the same contract

This solution is not ideal since it requires your users to create an additional transaction

Keeping it as close to your existing code as possible:

function submitSale(
        address buyer,
        uint256 tokenId,
        uint256 price
    ) public onlyTokenOwner(tokenId) {
        if (tokenIdToBuyer[tokenId] != address(0)) {
            revert alreadyOnSale();
        }
        tokenIdToBuyer[tokenId] = buyer;
        tokenIdToPrice[tokenId] = price;
        console.log("Sale submited successfuly");

    }

    function payForToken(uint256 tokenId) public payable {
        if (tokenIdToBuyer[tokenId] != msg.sender) {
            revert tokenNotForSaleForThisAddress();
        }
        if (msg.value < tokenIdToPrice[tokenId]) {
            revert paidAmountIsNotEnough();
        }
        bool paid = msg.value >= tokenIdToPrice[tokenId];
        if (!paid) {
            revert();
        } 
    }

function finalizeSale(uint tokenId) public onlyTokenOwner(tokenId) {
        (bool paid, ) = msg.sender.call{value: tokenIdToPrice[tokenId]}("");
        if(paid) {
        transferFrom(msg.sender, tokenIdToBuyer[tokenId], tokenId);
        delete tokenIdToPrice[tokenId];
        delete tokenIdToBuyer[tokenId];
        }
    }

This solution adds a new function finalizeSale() that is called by the user selling an NFT. This way transferFrom() is being called by someone who is allowed to transfer the token. The buyer sends the money to the contract during payForToken(), which means you'll likely want to add a function to cancel the buy and return the funds to the buyer if they change their mind about purchasing the NFT for any reason.

Alt Solution 2

Note this assumes the ERC721 and the marketplace contract are both the same contract.

This method also tries to keep the code as similar to your existing logic as possible, but without creating an additional function. To do so we can remove the first approve() since it's not being used anyway, and then to transferFrom() the NFT we can use Open Zeppelin's internal _transfer() function to bypass any msg.sender checks:

 function submitSale(
        address buyer,
        uint256 tokenId,
        uint256 price
    ) public onlyTokenOwner(tokenId) {
        if (tokenIdToBuyer[tokenId] != address(0)) {
            revert alreadyOnSale();
        }
        tokenIdToBuyer[tokenId] = buyer;
        tokenIdToPrice[tokenId] = price;
        console.log("Sale submited successfuly");

    }
function payForToken(uint256 tokenId) public payable {
        if (tokenIdToBuyer[tokenId] != msg.sender) {
            revert tokenNotForSaleForThisAddress();
        }
        if (msg.value < tokenIdToPrice[tokenId]) {
            revert paidAmountIsNotEnough();
        }
       ( bool paid,) = tokenIdToBuyer[tokenId].call{value: msg.value}("");
        if (paid) {
            _transfer(ownerOf(tokenId), msg.sender, tokenId);
        } else {
            revert();
        }
    }

Please note that using the internal functions can be potentially dangerous so it is important to test any surrounding conditionals to ensure that the _transfer() call is only made when it makes sense to do so.

Alt Solution 3 - ERC6105

This solution uses the standardized EIP 6105 to list items for sale until an expires timestamp as well as allowing the NFT holder the choice of currency to receive. You can find a reference contract ready to be thrown into Remix at the bottom of the EIP page.

Here are the EIP-6105 versions of the functions you've been using:

function listItem (
    uint256 tokenId,
    uint256 salePrice,
    uint64 expires,
    address supportedToken,
    uint256 historicalPrice
    ) public virtual{
    address tokenOwner = ownerOf(tokenId);
    if(salePrice <= 0) {
        revert SalePriceCannotBeZero();
    }
    if(expires < block.timestamp) {
        revert InvalidExpiresTimestamp();
    }
    if(!_isApprovedOrOwner(msg.sender, tokenId)) {
        revert CallerIsntOwnerNorApproved();
    }
    _listings[tokenId] = Listing(salePrice, expires, supportedToken, historicalPrice);
    emit UpdateListing(tokenId, tokenOwner, salePrice, expires, supportedToken, historicalPrice);
  }

This listItem() function is the equivalent of your submitSale() except this allows anyone who pays the price to purchase it, you can obviously alter the logic to fit your desired application.

Here is the buyItem() function which is the equivalent of your payForToken() function:

  function buyItem(uint256 tokenId, uint256 salePrice, address supportedToken) external nonReentrant payable virtual{
    address tokenOwner = ownerOf(tokenId);
    address buyer = msg.sender;
    uint256 historicalPrice = _listings[tokenId].historicalPrice;

    if(salePrice != _listings[tokenId].salePrice) {
        revert InconsistentSalePrice();
    }
    if(supportedToken != _listings[tokenId].supportedToken) {
        revert InconsistentTokens();
    }
    if(!_isForSale(tokenId)) {
        revert InvalidListing();
    }

    /// @dev Handle royalties
    (address royaltyRecipient, uint256 royalties) = _calculateRoyalties(tokenId, salePrice, historicalPrice);

    uint256 payment = salePrice - royalties;
    if(supportedToken == address(0)){
        if(msg.value != salePrice) {
           revert IncorrectValueSent();
        }
        _processSupportedTokenPayment(royalties, buyer, royaltyRecipient, address(0));
        _processSupportedTokenPayment(payment, buyer, tokenOwner, address(0));
    }
    else{
        uint256 num = IERC20(supportedToken).allowance(buyer, address(this));
        if(num < salePrice) {
            revert InsufficientAllowance();
        }
        _processSupportedTokenPayment(royalties, buyer, royaltyRecipient, supportedToken);
        _processSupportedTokenPayment(payment, buyer, tokenOwner, supportedToken);
    }
    _transfer(tokenOwner, buyer, tokenId);
    emit Purchased(tokenId, tokenOwner, buyer, salePrice, supportedToken, royalties);
  }

Answer 2

Good to see you're delving into NFT development and Solidity. Your problem seems to stem from a misunderstanding in how the transferFrom() function is utilized in the context of ERC721 tokens.

The transferFrom() function is meant to be called by whomever currently has approval. In your case, if you approved the contract itself (which means giving the right to transfer ownership), then the function should not be called via user but rather via another function within the contract that does the required procedure.

Here's how you can do it:

function submitSale(address buyer, uint256 tokenId, uint256 price) public {
        require(_isApprovedOrOwner(msg.sender, tokenId), "ERC721: caller is not token owner or approved");

        tokenIdToBuyer[tokenId] = buyer;
        tokenIdtoPrice[tokenId] = price;

        approve(address(this), tokenId);
    }

    function payForToken(uint256 tokenId) public payable {
        require(tokenIdToBuyer[tokenId] == msg.sender, "This token is not on sale for this address");
        require(msg.value >= tokenIdtoPrice[tokenId], "Payment is not sufficient");

        _transferFrom(ownerOf(tokenId), msg.sender, tokenId);

        (bool paid, ) = ownerOf(tokenId).call{value: msg.value}("");
        require(paid, "Payment failed");

        delete tokenIdToBuyer[tokenId];
        delete tokenIdtoPrice[tokenId];        
    }

Take note in the changes above, I have implemented the following modifications:

1. Replaced standard revert statements with require. It performs the same task but allows you to include a human-readable error message which will help in debugging.
2. Instead of externally calling transferFrom(), let's call it internally with _transferFrom().
3. The internal _transferFrom() is called by payForToken(), which would be executed by the contract, thus getting rid of your issue.

Remember that you need to have a function _transferFrom() inside your contract which directly calls ERC721._transferFrom(). If it's not there, you'll have to add it as per OpenZeppelin's ERC721 contract:

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

        _beforeTokenTransfer(from, to, tokenId);

        // Clear approvals from the previous owner
        _approve(address(0), tokenId);

        _balances[from] -= 1;
        _balances[to] += 1;

        _owners[tokenId] = to;

        emit Transfer(from, to, tokenId);
    }

I hope this helps! Please let me know if you have further questions.