ProjectOpenSea / operator-filter-registry

MIT License
312 stars 89 forks source link

Marketplaces can be designed to easily bypass this filter #16

Closed ryley-o closed 1 year ago

ryley-o commented 2 years ago

This filter is only able to filter out specific operator addresses and codehashes, both of which can be made dynamic for (future) marketplaces. For example, a marketplace can be designed that deploys an operator contract for each user, or even for each order, with a slightly different codehash via a pseudo-random salt in creation code. It would be infeasible for this registry to denylist every dynamically created operator contract (with intentionally different codehashes), rendering it useless in the long run (likely only a couple of months).

Also, I want to note that I very much appreciate the open and honest conversations on this topic from you all ❤️

miracle2k commented 2 years ago

It's true that this can be done, and OpenSea should know, because Project Wyvern with their per-user proxies was designed this way, right?

However, this isn't necessarily the end of the story. Factors to consider:

Slokh commented 2 years ago

Yeah in the repo README we acknowledge this.

This is not a foolproof approach - but it makes bypassing creator fees less liquid and easy at scale.

Not sure I 100% believe that people will go out of their way to build marketplaces that bypass the basic filter and even if they do they are likely to be more inefficient and would need to acquire adoption.

ryley-o commented 2 years ago

I very much appreciate your perspective and discussion on this topic ❤️🙏

From a purely technical perspective, I think the added capabilities of the proposed filter are:

Added capabilities:

Capability Shortcomings:

Additionally, the following side effects are induced:

Undesirable Side-Effects

Unclear Side-Effects (depends on user/issuer, likely project dependent)

Would love some discussion on the technical list above! My general concern here is that if the filter is rendered ineffective, tokens that implement this filter will end up with only all of the undesirable side effects.

miracle2k commented 2 years ago

Would love some discussion on the technical list above! My general concern here is that if the filter is rendered ineffective, tokens that implement this filter will end up with only all of the undesirable side effects.

@Slokh can we add a mechanism for a collection to permanently revoke their own ability to filter? In the base class, or maybe in the examples.

ryley-o commented 1 year ago

@Slokh can we add a mechanism for a collection to permanently revoke their own ability to filter? In the base class, or maybe in the examples.

agree, because in the current implementation, from a purely technical perspective:

∴ if filter is ineffective, the contracts that implement it are left with only centralization risk + gas inefficiency.

Regardless of non-technical discussions surrounding all of this, implementing a 1-way killswitch on this filter is a win-win.

operatorfilterer commented 1 year ago

Closing, as this is noted in the Readme and documentation - see https://github.com/ProjectOpenSea/operator-filter-registry/blob/main/src/RevokableOperatorFilterer.sol for an example with a permanent revocation method.

nidhhoggr commented 1 year ago

Although this issue is closed, I'm not seeing the point in permanently revoking the OperatorFilterer. Yes, unregister is also available which effectively achieves the same purpose but there could be a scenario where I want to only temporarily disable the filterer without having to re-register and re-subscribe. Also arguably unregister does not achieve the same purpose because the modifier, when toggled, would return early instead of making calls to the registry contract. Switching the modifier from a 1-way killswitch to a 2-way toggle has no negative impact on gas prices and last, if you're going to spend the deployment cost to deploy all the bytecode you might as well keep the code operable. The only thing that suffers from switching the killswitch to a toggle is immutability which is not an applicable advantage in any context. The changes to achieve this are simple: https://github.com/ProjectOpenSea/operator-filter-registry/blob/70c9b21fc5d6287375555c29b7606e0204dd0a40/src/custom/CustomSwitchableOperatorFilterer.sol

ryley-o commented 1 year ago

The only thing that suffers from switching the killswitch to a toggle is immutability which is not an applicable advantage in any context.

I would very strongly argue that retaining the ability to revert to a state where tokens become immutable is incredibly important, especially if this filter does not prove to be very effective in the medium-to-long run. The tradeoffs between a user-owned asset and an admin-managed asset must be weighed by each individual project, but a one-way switch is certainly a design pattern worth considering, and has no drawback.

If a separate toggle switch is desired, that should be considered independently, and can certainly be implemented alongside a one-way killswitch.

nidhhoggr commented 1 year ago

@ryley-o The one-way switch has already been implemented: RevokableOperatorFilterer. But the drawbacks are the one's I've mentioned above: essentially the inability to toggle between "user-owned" and "admin-managed" in your own words. From what I gather you're arguing immutability is an advantage because the token permanently becomes again a "user-owned asset" upon OfferFilterer revokation. I would somewhat agree but I think "admin-managed" is the tradeoff NFT owners must make based on the shift towards zero royalties.

If the problem here is that the OfferFilterer will eventually become ineffective at preventing royalty-fee trading then why shouldn't this library account for Operator Inclusion filtering as opposed to Operator Exclusion filtering? So instead of making an operator address/codehash blacklist we add the ability to instead refer to an operator address/codehash whitelist?

ryley-o commented 1 year ago

From what I gather you're arguing immutability is an advantage because the token permanently becomes again a "user-owned asset" upon OfferFilterer revokation. I would somewhat agree but I think "admin-managed" is the tradeoff NFT owners must make based on the shift towards zero royalties.

++ my point is definitely that many devs, owners, etc. in the NFT space view immutability as one of the fundamental reasons the technology is so important, thus a one-way killswitch to revert this change will be viewed as an advantage by some. (certainly a tradeoff by NFT owners though, as you mention above).

While an allowlist for Operator Inclusion would be a more effective filter (although not foolproof - a marketplace could still take custody of tokens listed for sale, or a simple wrapper could be used to easily bypass that kind of filter), it comes with the drawback of not seamlessly integrating with new protocols, inventions, etc., and requires admin approval for everything. Personally, and this last part is my personal opinion, an opt-in filter would be a non-starter for me as an NFT collector and dev.

nidhhoggr commented 1 year ago

it comes with the drawback of not seamlessly integrating with new protocols, inventions, etc., and requires admin approval for everything.

It would require management of a whitelist, but it seems like the blacklist has the capability of growing larger than the proposed whitelist. It also seems to be the same case with how a collection is "approved" by a marketplace. Other than that, I'm not sure how switching the Registry filter mechanism (inclusion or exclusion) would impact the seamless integration of new protocols, inventions whether it was using one or the other. The contract themselves suffer from these limitations based on the immutability of the blockchain, unless you're using the "Upgradeable" contracts.

In any case if the OperatorFilterer implements that proposed in #42 it then (the registrant contract) has the ability to choose which Registry to decide operator exclusion, and/or inclusion. Based on the interface signature IOperatorFilterRegistry it's possible to deploy an OperatorFiltererRegistry contract that instead uses the inclusion method, as the primary method used in the modifiers is isOperatorAllowed. https://github.com/ProjectOpenSea/operator-filter-registry/blob/0fc9a6baabbb60618b3ea58aee7089dbed6c8bef/src/IOperatorFilterRegistry.sol

Several points here: 1) Any ERC721/1155 Registrant can switch the Registry it refers to at any time. 2) The OperatorFiltererRegistry can be implemented to be blacklist or whitelist based 3) Any ERC721/1155 Registrant can revoke the OperatorFilterer at any time (killswitch or toggle).

So there's a fair amount of scalability built-in thus far that makes these 3 items possible with the exception of merging #42. If it turns out the blacklist is no longer effective the registrant can switch to a whitelist Registry or revoke the operator filterer as a last resort.

ryley-o commented 1 year ago

This issue was intended to highlight the specific technical issues of:

The logic above leads to the conclusion that a 1-way killswitch makes sense if the operator filter is not effective in the medium to long term.

Additionally, as you point out above, there are many different flavors of operator filter that could be implemented, if devs should so choose to implement those.