NameWrapper updates (including testnet deployment addresses)

🧑‍💻 ENS Development Contracts
2

We have redeployed these contracts to testnet with some minor changes. I’ve updated the addresses above

5 Likes
5

Hey @mdt or anyone who knows:

I was playing around with these new contracts and was wondering about the metadata service. Is that not expected to be functional yet, or is there a separate Goerli deployment for it? Because it doesn’t look like the ens-metadata-service.appspot.com/name/0x{id} endpoint works currently.

Will the same metadata service be used for both the current NFT contract and the new NameWrapper contract? I ask because the route /name/0x{id} returned from the StaticMetadataService doesn’t have a contract address in it, and we know that the NFT token ID for .eth 2LDs will be different in the old vs. new contracts. So just wondering how all that will work, thanks!

6

Namewrapper address and interface is not updated in metadata service yet. I will be updating it very soon.

3 Likes
7

Redeployed the NameWrapper, Controller and Public Resolver to Goerli. Addresses updated in the above post.

Also updated the NameWrapper interfaceId

Edit: Since Ropsten has been deprecated from infura, we have decided not to deploy the edited contracts to ropsten.

1 Like
8

Edited the NameWrapper interfaceId again. We were accidentally using the interface from the NameWrapper instead of the INameWrapper

2 Likes
9

Hey @jefflau.eth @nick.eth etc

I’ve been playing with the wrapper and realized that when a .eth 2LD expires (ERC721) the ERC1155 is still transferrable / tradable / sellable until or unless the .eth 2LD (ERC721) is re-registered.

As getData sets all fuses to 0 (false) if block.timestamp > expiry.

I think it should set CANNOT_TRANSFER to True instead of setting CAN_DO_ANYTHING?

        function getData(uint256 tokenId)
        ...
            if (block.timestamp > expiry) {
                //fuses = 0; // allow transfers on expiration
                fuses = 4; // CANNOT_TRANSFER ?
            } else {
                fuses = uint32(t >> 160);
            }
        ...

My concern here is that people can register .eth 2LD name’s for 1 month or wrap and let any currently owned .eth 2LD expire - then still be able to sell the ERC1155 on market places after the name expires until someone decides to manually re-register the .eth 2LD.

The 1155 could be traded many times before that happens or someone realizes. Then the person who “paid” for the name will get rugged when a new person registers it again and the token is burned and finally transferred away from the current “holder”.

my foundry test : gist:9b3e75ecc77fdf74a4380e89404f015d (github.com)

2 Likes
10

Hey @lcfr.eth ,

Thanks for this report, it’s very useful to us having you test our contract. There’s a couple problems with this solution as-is:

  1. CANNOT_TRANSFER will continue to be burned after the name is renewed as fuses are ORed together.

  2. Stopping transfers for wrapper expired names means that all names that don’t have expiry will be also untransferrable. We would prefer if possible to not force expiry to be extended for names that don’t require fuse burning.

That being said, this is definitely a problem and we’re going to look into a mitigation now that can fix this problem.

4 Likes
12

Thanks @jefflau.eth (I suggested another solution but it was basically the same so deleted). Missed the | originally in _mint but I see now. :slight_smile:

1 Like
13

Could you prevent that by only making that check if expiry > 0? Like in safeBatchTransferFrom/_transfer:

if (!_canTransfer(fuses) || (expiry > 0 && block.timestamp > expiry)) {
    revert OperationProhibited(bytes32(id));
}

A more general question is, should all wrapped tokens be non-transferrable if the expiry has been reached (and wasn’t 0 to begin with)? Or should this only apply to the .eth 2LD tokens?

2 Likes
14

This ended up being a much longer fix than I anticipated, because the expiry issue was also an issue throughout all subdomains.

To mitigate this, we are now changing the way expiry works so it affects ownership across .eth and subdomains. Here is a short summary of how things have changed with the fix and changes:

  1. Expiry affects ownership if PARENT_CANNOT_CONTROL (PCC) is burned. This means if any name is expired, you lose ownership and therefore control over a name. No transfers, or record setting.
  2. Wrapper expiry and .eth expiry are no longer separate. Wrapper expiry for .eth names are always synced with the registrar contract and will expire, because PCC is always set on .eth names on wrapETH2LD().
  3. Otherwise if PCC is not burned, the name functions just as normal wrapped names without fuses and expired names can be transferred around as normal
  4. Expired names can have their fuses set to anything by the parent up to the parent’s expiry as normal.
  5. Fuses are now split between parent controlled and name controlled. uint16 for owner controlled. uint16 for parent controlled. This allows custom parent controlled fuses which create some interesting usecases that I would like to expand upon in the future
  6. There is now a IS_DOT_ETH fuse, that gets autoburnt by wrapETH2LD(), it shouldn’t be burnable by any other name and should revert if you try and burn it.
  7. setChildFuses() is not callable by .eth names anymore. It was special cased before to allow .eth to extend their expiry, but now that it doesn’t exist, it isn’t necessary anymore
  8. setChildFuses() is special cased for TLD owners. This is done so expiry/fuses can be set for DNSSEC names and so their subdomains can also have their fuses set. Of course this doesn’t give the same guarantees as .eth, but may still be useful.

I’m sure there are other things I have missed out, but these summarise most of what I’ve been working on this week. I still have quite a bit of testing to finish off next week, but wanted to give an update before the weekend. I will work on a longer post explaining things in more detail once the PR has been merged and we are moving to testnet

Here is the WIP PR: Wrapper expiry refactor by jefflau · Pull Request #145 · ensdomains/ens-contracts · GitHub

Thanks again @lcfr.eth for the report, it was much appreciated.

4 Likes
15

@lcfr.eth Had you reported this during the audit, it would have been classified as ‘medium’ and awarded about $3500 of the prize pool. I’ve sent you $3500 USDC and some ENS tokens from ENS Labs as thanks for finding this.

11 Likes
16

Thank you @nick.eth and the whole team :slight_smile:

will continue hacking on the new stuff and bring up any more concerns if any.

1 Like
17

Hey it seems like the new ENS goerli deployments broke the old ReverseRegistrar https://goerli.etherscan.io/address/0x6F628b68b30Dc3c17f345c9dbBb1E483c2b7aE5c causing set name failures, although now it seems like they are able to go through.

Would like to know how the mainnet deployments will affect the current ReverseRegistrar as this is a contract dependency of ours.

18

There can only be one reverse registrar at a time. The new Goerli deployment deploys a new one, which should be backwards-compatible.

How were you fetching the address of the reverse registrar, and what transactions are you sending to it?

19

We hard coded as we assumed it would be constant. Our contract is basically a subdomain registrar that deploys a gnosis safe and assigns it a pod.xyz subdomain, so we are calling setName on the reverse registrar in the context of the safe.

21

@jefflau.eth Something else I noticed in the new contracts, there’s now a hard cap of 255 bytes for all labels:

if (bytes(label).length > 255) {
    revert LabelTooLong(label);
}

This means that nobody would be able to:

  • Register any new .eth 2LDs that have a >255 byte label
  • Wrap any existing .eth 2LDs that have a >255 byte label
  • Create any new subnames (under a wrapped name) that have a >255 byte label

Is this new behavior intentional?

It looks like the encoded name is just used to populate the names mapping, and to emit in the NameWrapped event. If on-chain storage is the concern, couldn’t the contract just refrain from populating the names mapping if it’s >255 bytes, rather than reverting the transaction? Or are there other reasons why we should have a hard cap now?

1 Like
22

That’s correct. The limitation is because a single byte is used to represent the length of each label in DNS encoding, which we use to store name preimages, which are sometimes processed onchain.

1 Like
23

Gotcha, thanks! I thought it was contained to the _addLabel function, but I see that readLabel gets called in the regular wrap function too. So actually it’s broader than I thought, nobody will be able to:

  • Register any new .eth 2LDs that have a >255 byte label
  • Wrap any existing name or subname (.eth or DNS) that has a >255 byte label
  • Create any new subnames (under a wrapped name) that have a >255 byte label

Is that DNS-encoded name actually necessary for anything though? As I mentioned, instead of reverting transactions could the names mapping just not be populated for any such labels?

If this was purely contained to the NameWrapper, and the new registrar controller allowed you to still register non-wrapped names directly, then I don’t think it’d be much of an issue. But that’s not the case right? Once these new contracts are put into place, all new .eth 2LD registrations will be forced through the NameWrapper and wrapped by default, right?

I’m not necessarily defending really long names here, I don’t know if they have any practical use or are just novelties. But if deployed, this would be an important new contract-level restriction that the ENS community would need to be aware of. Also, this means that any currently existing >255 byte .eth 2LDs will suddenly become extremely scarce, as it will be impossible to create any more of them.

1 Like
24

True, except via the legacy registrar contract.

As you observe, it’s used in wrap in order to hash the name and check it’s not a .eth 2LD. Outside that it’s used primarily offchain at the moment, but not exclusively, and it’s much easier to build functionality on top of if it’s guaranteed to have the plaintext for every name.

No, the existing registrar will remain enabled and permit registration of unwrapped names.

25

Ahh got it, thanks! I didn’t realize that the current ETHRegistrarController would remain as a controller for the registrar (that’s what you mean right?). I know that the registrar has a removeController method, and so that would not be performed as a part of the on-chain DAO proposal for these new contracts.

I was going off of what Jeff said in the first post, so that’s where I was confused:

Anyway I think that resolves my concerns then, thanks! Basically the NameWrapper will just not support really long labels, and I think that’s fine. Or at least I can’t think of any problem with that.

===

Just thinking out loud though, wouldn’t it be better for future maintainability if the new ETHRegistrarController was made a controller for the registrar as well, and then it could have an alternate method (or flag parameter) to register unwrapped names directly?

Otherwise now we’ll have two registrar controllers to maintain. So if we want to change the price oracle we’ll need to do it on both. And if we want to make more complex price structure changes that requires code changes for the registrar controller, then we’ll need to make changes to both contracts, deploy both, and replace both as controllers on the .eth registrar.