Check ownership of the NFT

[bluesign]

Currently, it allows me to list NFTs I don't own.

https://github.com/onflow/nft-storefront/blob/1bda702c94e81ec404c8945e60a0d63652f5ca51/contracts/NFTStorefront.cdc#L261

here should add a check if the owner of the NFT is also the owner of the storefront.

[rheaplex]

Heya. Thank you for raising this.

This behaviour is by design, to allow shared marketplaces. The currently provided transactions do not support shared marketplaces out of the box, and in particular they do not set up a /public/ capability to support them, but we didn't want to preclude it. The only way to do it at the present would be with a transaction with multiple authorizers.

I don't think this represents a security hole, but if you can think of an exploit we should patch the code in order to remove it, or if not then we should document the design decision in the code.

[bluesign]

Hey, thanks for the quick answer.

This can lead to a problem: (assuming I can deploy a contract)

• I can implement a Collection with {NonFungibleToken.Provider, NonFungibleToken.CollectionPublic}. Which I can men in the middle borrowNFT method to someone else's collection. (as probably they will have some NonFungibleToken.CollectionPublic public link)

Let's say they have Lebron James moment. I can list that moment with an absurd cheap price. Even market listeners, go and check the NFT with borrowNFT (to get data), it will succeed, there is no way to prevent spam with this. (actually, there is another comment with spam on my other issue)

Think about people's frustration on TopShot when they cannot buy the Moment they think they caught, but some bot beat them to it, and multiply by X. Also it is very good for price manipulation, list items with prices lower than market value with fake listings, wait for next victim to list around that, buy, then remove listings.

• Also, this can lead to the same NFT listed many times on the market. A bit of a bad UX. Also destroys confidence I guess.

• Technically I can list all NFTs in the market on my account with %1 markup. When someone buys, I go buy from the other guy, profit.

I think being able to list something I don't own can be abused much. (I know I would).

Shared marketplaces are a good idea I agree, but that can be a little messy.

[CineTek]

Curious what "shared marketplaces" is supposed to be here? Listing a unique NFT on like 5 websites at once?

We are currently preparing a multisig call from our backend to approve transactions that go into our official marketplace, any other listing event is ignored. - This seems to be the recommended approach to "enforce" only specific NFTs and fees to the platform?

[bluesign]

I think, for example, viv3 can implement a storefront and people can list there.

@CineTek In your case, you have 2 accounts for user right? Or you just want to list your NFTs in your storefront? In second case, you don't need multisig I guess, as you can deploy contract already

[CineTek]

sorry @bluesign I am a bit confused here 😅 Based on the description of the Repos it sounds like there is no need to deploy new storefronts on a per project basis anymore? (Unless you need custom mechanics) So from my understanding this is the new de-facto standard contract that any wallet can be initialized with it which makes it easy and convenient to support any NFT collection? So all transactions would go directly through this contract, no matter the projects(?)

[bluesign]

As I understand, this is a generic storefront, either users can store this on their account, or viv3, versus, chainmonsters etc can store on their account and let people list (with the point you mentioned about forcing royalty/commission)

Though I think this shared part is wrong (the point of this issue)

This allows multi-listing, user can list same NFT on chainmonsters and viv3 at the same time. (with different prices etc). But also opening a path to many issues. (you can have 1 legendary item in the game, but it can have 100 listing, even from people who don't own that)

My point about multisig was, instead of doing multisig, you can make a proxy method in your contract, which can create listing on your storefront. (You just need to give your contract a private capability to your storefront) There you can control the type of NFT listed, commission etc.

[rheaplex]

• I can implement a Collection with {NonFungibleToken.Provider, NonFungibleToken.CollectionPublic}. Which I can men in the middle borrowNFT method to someone else's collection. (as probably they will have some NonFungibleToken.CollectionPublic public link)

The Listing constructor checks that you actually have the token , and fails if you do not -

https://github.com/onflow/nft-storefront/blob/main/contracts/NFTStorefront.cdc#L331

• Also, this can lead to the same NFT listed many times on the market. A bit of a bad UX. Also destroys confidence I guess.

You can list the same token in your markeplace multiple times with different cuts going to different aggregators than can then locate and promote the one that pays to them. This is by design.

• Technically I can list all NFTs in the market on my account with %1 markup. When someone buys, I go buy from the other guy, profit.

If you don't have the NFT in a Collection that you have a Provider capability to then you can't list it. It is true that you can list the NFT then remove it from the Collection that the Listing contains a Provider capability to, but this is true of any non-custodial marketplace.

All of that said, what the multiple listing and token removal scenarios point to is that the manual cleanup that this contract, and non-custodial marketplaces in general, involve may have more undesirable consequences than I had previously considered due to the economics/game theory of removing listings. Enforcing ownership of tokens may help with this.

So the useful changes may be:

• Enforce that NFT owner == Storefront owner
• Bundle Listings for the same token together internally so they can be cleaned up together automatically.

The technically difficult problem is deleting the Listing resource when the purchase is being acted on, hence the manual cleanup in the current version. I'll revisit that but it may be unavoidable. The standard transaction for purchase() may be able to handle it, though, which reduces the problem.

I'll raise this internally.

[maggo]

Thank you for the clarification Rhea!

I've been wondering about the issue of "zombie listings" after a purchase or if the token is being transferred away through other means. Maybe the public cleanup function could have an alternative check if the token is still in the owners collection and if not delete the listing?

This would also allow a marketplace maintainer to periodically go through all active listings and clean them up if needed

[bluesign]

@rheaplex thanks for the answer.

The Listing constructor checks that you actually have the token, and fails if you do not - If you don't have the NFT in a Collection that you have a Provider capability to then you can't list it.

Those are not actually correct, I can borrowNFT from any user and give it to you technically if I can deploy a contract. (as they will have PublicCollection exposed). All I need to do is something like this.

 pub fun borrowNFT(id: UInt64): &NonFungibleToken.NFT {
            return getAccount(0x01).getCapability(/somepath).borrow().borrowNFT(id)
 }

All of that said, what the multiple listing and token removal scenarios point to is that the manual cleanup that this contract, and non-custodial marketplaces in general, involve may have more undesirable consequences than I had previously considered due to the economics/game theory of removing listings. Enforcing ownership of tokens may help with this.

The technically difficult problem is deleting the Listing resource when the purchase is being acted on, hence the manual cleanup in the current version. I'll revisit that but it may be unavoidable.

For this part, I have another issue #11 (cc: @maggo)

[rheaplex]

Thank you for adding #11 .

I do see what you mean about the malicious implementation. The effects are limited to creating false listings, but at volume these could be a problem. Thank you for raising this.

To address this:

• We could check the owner in the initial borrow, but that's not very Cadence-y.
• We could use just a Provider, and move the token in and out, but that would emit events that have no utility.
• We could create another type to manage access to the token that embodies the guarantees we need.

I wish we could walk the type system well enough to check that the Collection the Capability is on is the Capability from the NonFungibleToken contract that the listed NFT is from.

I'll discuss this internally. 😺

[bluesign]

Unfortunately, there is no easy solution for this.

We could check the owner in the initial borrow, but that's not very Cadence-y.

Totally agreed, this is also a little tricky, as both storefront and NFT can have nil owner also ( if you go this path, not allowing nil is also necessary)

We could use just a Provider and move the token in and out, but that would emit events that have no utility.

On this one Provider will not alone be enough, also there needs to be a Receiver. It makes it a bit more complicated. To prevent events maybe the NFT standard can add Provider interface ownedNFTs.

We could create another type to manage access to the token that embodies the guarantees we need.

This wrapper (box) concept I discussed before on Discord, has pros and cons.

I wish we could walk the type system well enough to check that the Collection the Capability is on is the Capability from the NonFungibleToken contract that the listed NFT is from.

I think this one is solved with solution 1, the problem is with solution 1 (ownership check), we are losing support for big StoreFronts ( like viv3, CM or Versus can have a public StoreFront)

[rheaplex]

To prevent events maybe the NFT standard can add Provider interface ownedNFTs.

Do you mean it could add a method to the Provider interface? This could be implemented maliciously, unfortunately.

Maybe we could add a postcondition to borrowNFT() that ensures that the NFT and Collection have the same owner, although with the same issue about nil.

This is all starting to feel a bit like tx.origin vs. msg.sender . 😺

The problem is with solution 1 (ownership check), we are losing support for big StoreFronts

The original intent for Storefronts was that everyone would have their own, and aggregators would watch for events offchain then display Listings that are of interest to them. I'm wary of restricting other use cases without good reason, but we may well be in good reason territory here.

[bluesign]

I meant putting ownedNFTs from https://github.com/onflow/flow-nft/blob/7fc2335bdd1ae9160d83a76b532e29833662a306/contracts/NonFungibleToken.cdc#L116

to https://github.com/onflow/flow-nft/blob/7fc2335bdd1ae9160d83a76b532e29833662a306/contracts/NonFungibleToken.cdc#L85

So asProvider Capabilitywill have access to raw storage dictionary, can get NFT and then later put it back without emitting any events.

Yeah I think original intent is cover, little restriction doesn't hurt.

Also it is ugly but we can check NFT.getType().identifier and Collection.getType.identifier has same prefix ( inspired by your Cadence issue ) But this also blocks later can be developed MultiNFT collections. Not so good for composability.