Should OperatorFilterRegistry address be updatable to de-risk potential code errors?

[ph101pp]

Since the OperatorFilterRegistry is a new and un-audited contract, there is no guarantee there won't be any bugs and/or vulnerabilities discovered in future.

Because of that, hardcoding the address to this contract seems risky:

    IOperatorFilterRegistry public constant OPERATOR_FILTER_REGISTRY =
        IOperatorFilterRegistry(0x000000000000AAeB6D7670E522A718067333cd4E);

To de-risk this should the address be updatable via a setOperatorFilterRegistry method?

[nidhhoggr]

Here's how I implemented this: https://github.com/nidhhoggr/operator-filter-registry/blob/1ca53cdaa4bb28937f797fad764c9c97b809dfe1/src/example/CustomRevokableExampleERC721.sol#L24

[dievardump]

OPERATOR_FILTER_REGISTRY being a constant is ideal for gas cost (the operator filter is already costing us enough in gas) but also to be sure that the contract owner will not decide on a whim (or after a hack or anything) to change the OPERATOR_FILTER_REGISTRY for something they control and block some people from transferring just because they decide so.

It to be a constant ensures collectors that no one (except OS... but that's not in their interest) can just decide to start adding random address in the denylist

I'm thinking twice when I see a contract implementing the OperatorFilter, I would think 10 times if I see one implementing an updatable Operator

[ph101pp]

Isn't the operator filter always updatable by changing the subscription? So if I actually wanted to change the list on a whim I can simply interact with the registry to make any change I want?

I think the mutability question is valid and definitely worth considering (ie with a killswitch) but this change has little to no effect on the overall mutability, only on the place and mechanic on how it happens.

An additional benefit @nidhhoggr brought up, is that different registries can be implemented (iE whitelist based) in the future. An updatable registry address allows the contract to benefit from these future developments.

[dievardump]

The current repository doesn't allow you to change subscription. And I don't see why it would. We are on the OS repo, not on OpenZeppelin. They are implementing their own system in a way they try to make the most gas efficient possible,

(ps: Just the idea of an allowlist based registry is even more scary than a denylist one. this thing is horrendous enough, let's not go deeper in this and let's make it the most complicated possible to update for people who don't want to put effort in it and not just create the base of a monster)

[ph101pp]

Correct me if I'm wrong, but the current repo allows you to unsubscribe and resubscribe to any list you wish: https://github.com/ProjectOpenSea/operator-filter-registry/blob/main/src/OperatorFilterRegistry.sol#L343-L357

I've tested this on Goerli and it works.

[dievardump]

The registry yes, but the default operators and the basic OperatorFilter do not provide anything to do it easily, because if this is something the dev want to do, the dev would be better of writing their own implementation of the filter than expecting it out of the box (and if this is something they're not able to code themselves, it might be better for them to not have the possibility to "screw" it)

They deployed the registry but are inviting people to set up their own way of filtering if they want.

As I said before, this OperatorFilter stuff is already adding quite a nice amount of gas to transfers in order to go check on an external registry if someone is allowed to operate or not. So they provide the minimal code required to have the registry working with the less gas overhead, meaning the least functions possible, the more constants possible: subscribe and be done with it.

Just changing from constant to non constant adds several k of gas to the process

[ph101pp]

Imho there is a balance between saving gas and future proofing.

• The downside of this change is the additional gas spent.

• The upside, is de-risking the NFT contract from the OperatorRegistry implementation.

• The functionality / mutability of the resulting NFTs stays exactly the same from an ownership perspective. (Albeit via different mechanism. The complexity of the mechanism is mostly irrelevant in immutable code, since a UI/tooling can make any mechanism that is possible easy to execute.)

Since this is a drop-in and forget implementation for potentially less experienced developers, I would opt for safety over gas. Experienced developers who optimize for gas can make their own choice anyways.

I personally would opt for the safer option over a little gas myself even as a somewhat experienced developer.

That said I will also use the revokable option to de-risk my contract from this episode in NFT history being a temporary stop gap solution.

[nidhhoggr]

@dievardump

The current repository doesn't allow you to change subscription. And I don't see why it would. [...] The registry yes, but the default operators and the basic OperatorFilter do not provide anything to do it easily, because if this is something the dev want to do, the dev would be better of writing their own implementation of the filter than expecting it out of the box

That's incorrect. It's very simple to unregister, re-register and then set your own arbitrary operator filterers with the existing implementation of OPERATOR_FILTER_REGISTRY as a constant. This can be achieved in three simple transactions. It can be done on etherscan, programmatically within the contract and/or it can be made intuitive to non-devs by providing a User interface to accomplish this.

Just the idea of an allowlist based registry is even more scary than a denylist one. this thing is horrendous enough, let's not go deeper in this and let's make it the most complicated possible to update for people who don't want to put effort in it and not just create the base of a monster

The filtering logic is handled on the Registry side, not the Registrant side. So the ERC721/ERC155 (a.k.a Registrants) wouldn't need to update code, regardless of a Registry's filtering implementation. The Registries filtering implementation is all abstracted to isOperatorAllowed. Further, a Registry contract can be deployed implementing a whitelist given the current interface. No further modifications would be necessary to accomplish this. The only thing impacting the registrant would be whether or not they decide to use OpenSeas default OPERATOR_FILTER_REGISTRY address. This is aside from the fact that blacklist implementation possibly won't even be effective in the long-term. See #16

Just changing from constant to non constant adds several k of gas to the process

Im curious, do you have this benchmark readily available? In either case that however is a tradeoff some devs would be willing to take.

[dievardump]

@nidhhoggr

That's incorrect.

The DefaultOperator and the OperatorFilterer do not come with any owner() stuff nor any way to unregister/unsusbcribe/resubscribe out of the box. It's easy to add to it, but the ones that are here do not come with it (and I think it's very good, I stand by the fact we should not bloat the current repo with stuff people should be doing on their own).

This is aside from the fact that blacklist implementation possibly won't even be effective in the long-term

We all know it won't work on the long term. As I said in my previous comment, this thing is horrendous. We all implement it only because as creators, creator fees are the way to be able to continue to build efficiently.

But the idea of whitelist is even more crazy, it would break even more the principle of composability. -> No more staking on non allowlisted contracts, no more pooling on non allowlisted protocols, no more "I create my own BatchTransferer contract to move all the stuff I hold" because it will never work if the damn owner of the collection you hold asset of did not add it to that dumb "whitelist".

The whole idea to need to allow or disallow operators in this way is a complete brain fart, and the only reason we are here is because we(creators) have been bullied into accepting it.

Im curious, do you have this benchmark readily available? In either case that however is a tradeoff some devs would be willing to take.

constants have no write nor read cost, they are replaced in the bytecode by their values.

non constants would cost at least 20k cost more when deploying, and 2k gas at first read (cold read) + several hundred gas for every subsequent read (warm reads). You don't need benchmark to deduct this.

With constant

Running 5 tests for test/OperatorFilterer.t.sol:OperatorFiltererTest
[PASS] testConstructor_copy() (gas: 257763)
[PASS] testConstructor_subscribe() (gas: 177081)
[PASS] testConstructory_noSubscribeOrCopy() (gas: 326674)
[PASS] testFilter() (gas: 41303)
[PASS] testRegistryNotDeployedDoesNotRevert() (gas: 298447)

Without constant

Running 5 tests for test/OperatorFilterer.t.sol:OperatorFiltererTest
[PASS] testConstructor_copy() (gas: 282341)
[PASS] testConstructor_subscribe() (gas: 201659)
[PASS] testConstructory_noSubscribeOrCopy() (gas: 352071)
[PASS] testFilter() (gas: 43957)
[PASS] testRegistryNotDeployedDoesNotRevert() (gas: 323826)

[nidhhoggr]

@dievardump

The DefaultOperator and the OperatorFilterer do not come with any owner() stuff nor any way to unregister/unsusbcribe/resubscribe out of the box.

How is the code below not out of the box? The fact that there's no specific method already implemented in the OperatorFilterer.sol to do this for you is irrelevant. How would they already know which marketplaces you wanted to ban?

    function setOperatorFiltererBannedOperators() public onlyOwner {
        OPERATOR_FILTER_REGISTRY.unregister(address(this));
        OPERATOR_FILTER_REGISTRY.register(address(this));
        OPERATOR_FILTER_REGISTRY.updateOperators(address(this), yourNFTMarketplaceAddresses, true)
    }

I will pass on further debate of whitelist vs. blacklist. That's a topic that can be discussed when the blacklist is no longer effective and OpenSea is back at square one.

You don't need benchmark to deduct this.

Okay so you've provide Foundry Gas Snapshots (a.k.a. a type of "benchmark"). I Appreciate your analysis.

[operatorfilterer]

PR #56 adds an UpdatableOperatorFilterer and the RevokableOperatorFilterer option is now updatable.

Both bypass checks when the registry address is set to address(0). Revoking sets the registry to address(0) and toggles a boolean packed into the same storage slot, which is only read when further attempts to update the registry address are made. The only gas trade-off in the revokable case should be storing the registry address on deploy.