@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
Thank you @nick.eth and the whole team 
will continue hacking on the new stuff and bring up any more concerns if any.
1 Like
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.
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?
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.
@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
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
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
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.
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.
Will my name be able to be wrapped?
Nope, that label is 400 bytes I believe
2 Likes
Yes this is true for the new .eth registrar controller contract, and also for the new app. Legacy names could be continued to be registered on the old registrar controller contract until the DAO decides to remove it as a controller. We expect to have this contract on there for several months to ensure the stability of the NameWrapper
3 Likes
If the old registrar controller was ever removed, then people would have no way to register .eth names directly anymore, everyone would be forced through the NameWrapper.
Could the new registrar controller have a boolean flag (or alternative function) to register without wrapping?
As it stands we know one limitation already with the NameWrapper: no label greater than 255 bytes will be allowed. So if the old controller was ever removed, that would now effectively be a new contract-level restriction on .eth registrations.
2 Likes
I can’t see us disabling the old controller any time soon; as long as the pricing is the same and there are no known vulnerabilities, I don’t think there’s a compelling reason to disable it.
2 Likes
Hi all,
Myself and @nick.eth have been hard at working fixing the issue that @lcfr.eth initially reported, and also another issue that @Premm.eth reported. For transparency, Premm was also rewarded a bounty similar to the one lcfr.eth received. Thanks Premm!
In the mean time we found things we could optimise and improve on in the NameWrapper. After much discussion, we have decided to proceed with another audit. This will be the 3rd official audit on the NameWrapper in over 2 years. Whilst this wouldn’t be our ideal situation, each time we have found bugs in the NameWrapper we’ve actually found ways to make it more useful and elegant with more features. However with more features comes increased complexity, and the risk is always increased so we’ve decided to go for a private audit with Code4rena starting 22 November.
Despite the setback, I hope the increased transparency shows we’re dedicated to getting this out and we’re definitely not intentionally dragging our feet.
12 Likes
The new NameWrapper docs are live here: ens-contracts/README.md at master · ensdomains/ens-contracts · GitHub
If anyone has any feedback/questions on the docs, please feel free to contact me. I will work on a less technical in the future about how parent-controlled fuses have implications on NameWrapper usecases as well once things have settled a bit.
6 Likes
I’ve got some notes on the current NameWrapper readme:
Unregistered
Names start out in the unregistered state before they are registered, and return to it after they expire.
To check if a name is Unregistered, verify that
NameWrapper.ownerOfreturnsaddress(0)and so doesRegistry.owner.
When an Emancipated/Locked name expires, NameWrapper.ownerOf will return address(0). However the Registry.owner will not return address(0), it will still return the address of the NameWrapper contract. That doc says that names will “return to the Unregistered state after they expire”, but I think that is not necessarily true, at least not how it is defined here.
Maybe it should say like: “To check if a name is Unregistered, verify that NameWrapper.ownerOf returns address(0) and Registry.owner returns either address(0) or the address of the NameWrapper.”
And then for Unwrapped: “To check if a name is Unwrapped, verify that NameWrapper.ownerOf returns address(0) and Registry.owner returns any address except for address(0) or the address of the NameWrapper.”
Wrapped
Wrapped names do not expire…
…
Expiry
Expiry is only applicable to names in the Emancipated and Locked states.
…
Expiry can still be extended even if a name is in the Wrapped state, but does not have any practical effect on the name.
Unless I’m mistaken, a name can be merely Wrapped, and the parent can burn parent-controlled fuses on the name, along with some expiry. So the expiry may still be applicable to Wrapped names.
If the name is Emancipated or Locked, the following changes happen:
- The NameWrapper returns
address(0)as the name owner fromownerOf()orgetData()if the name is expired.- The NameWrapper treats all fuses as unset and returns uint32(0) from
getData()for fuses if the name is expired.
The first bulletpoint there is true. However the second bulletpoint applies to merely Wrapped names too. If parent-controlled fuses were burned, then the expiry will cause those to reset.
2 Likes
Good point, thank you. Would you mind sending a PR to correct this?
It can, but the expiry has no effect unless CANNOT_UNWRAP is burned.
Good point.
1 Like
Will do!
How so?
The expiry can be set on a Wrapped (but not Emancipated) child name along with any custom parent-controlled fuses (“perks”). This allows the parent to burn a “perk” fuse for a limited amount of time. When the expiry is reached, the fuses reset, but the name itself is otherwise unaffected (unlike for Emancipated/Locked names).
I don’t see any place where CANNOT_UNWRAP precludes the expiry from affecting a merely Wrapped (but not Emancipated) name, unless I missed something.
Also the expiry affects merely Emancipated names too, and those do not have CANNOT_UNWRAP burned either.
1 Like