0

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;(

1 Answer 1

0

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:

  1. You don't need UserData.exists. It can be replaced by address(UserData.User) != address(0).
  2. In effect, you can reduce your mapping(address => UserData) to mapping(address => User).
  3. Do you really need a User contract?
  4. Doing an unbounded loop like for (uint256 i = 0; i < userTokenIdsCount is a big no-no. Get a better data structure instead.
  5. All your string operations can probably be sent off-chain.
  6. 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.
3
  • Okay, thanks you. I try to solve this problen for week:) I add itemApproving when trying to solve it. Yes, i know about getRandomNumber, i try chainlink, but it's pretty complex logic for this project. But where do i need to call setApproveForAll to transfer nft? Before i try to make the chain of approving: nft -> user -> auctions -> auction, but it's wrong?
    – Влад
    Feb 14 at 17:11
  • maybe i don't need to create any approve methods in contract? Instead should i call setApproveForAll directly in NFT contract from user's wallet?
    – Влад
    Feb 14 at 18:10
  • ---Instead should i call setApproveForAll directly in NFT contract from user's wallet?--- Yes. Feb 14 at 19:50

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.