The Name Wrapper’s upgrade mechanism contains a vulnerability whereby a malicious DAO could use the upgrade mechanism to seize control of wrapped names without the owner’s consent, documented here.
Since the v2 migration plan makes the upgrade mechanism obsolete, we now know the mechanism will never be required. This EP proposes to remove the DAO’s ability to upgrade the name wrapper.
Specification
Admin control over the name wrapper gives the DAO two functions: it can set the upgrade contract, and it can specify the address of the metadata contract. Since we want to remove the former ability while preserving the latter, we propose the following sequence of actions:
Deploy a new metadata contract, identical to the current one but using a proxy. The proxy instance should be owned by the DAO to provide for future metadata upgrades.
Update the name wrapper to reference the new metadata contract instead of the old one.
This EP is a formalization of this temp check, with a specific action plan. If sentiment is positive I’ll go ahead and perform step 1 and prepare transaction data for steps 2 and 3 as an executable proposal.
I think (but am not confident) that maybe the same fix can be applied, just to the registry as well as the name wrapper. But maybe it requires a bit more complexity?
Doing this requires either removing the ability to set new registrar controllers, or forcing all of them to operate via a proxy that prevents the vulnerability from being exploited. The former is impractical as a new registrar controller will be required for the migration process. We have code for the latter, but it’s unaudited and I’m concerned that the technical risk far exceeds the governance risk here.
As you know, I’m fine with ossifying the registrars, but I know you and many others on the ENS dev team are not so I won’t press that angle hard.
For the proxy solution, what about it makes it particularly risky? Is the code available to look at somewhere?
My concern is that both of these bugs are functionally the same IIUC, where if something/someone compromises the DAO then names can be stolen. Fixing one without fixing the other doesn’t seem like it will change anything from the current situation.
As we’ve discussed before, this is impractical because of our reliance on an external price oracle; we also need to be able to update the registrars as part of the v2 migration plan.
My concern is that it’s a lot more complex than the remediation for the name wrapper upgrade issue, and like that issue it involves making something immutable - which means that the consequences for a mistake can be dire. I worry about the technical risk of a mistake exceeding the governance risk for the next year that we need to continue to manage without a malicious DAO - particularly with the security council in place.
Is it necessary to allow for multiple controllers? If not, I believe the code could be simplified by having the proxy be permanent, and adding the following two functions to it, rather than having a swappable proxy controller.
function transferControl(address controller) external onlyOwner {
this.controller = controller;
}
function relinquishControl() external onlyOwner {
this.controller = address(0);
}
If you wait until v2 to fix this as you propose, would the bug remain for anyone that desires to stay on v1 after v2 is launched? If not, why does waiting until that point in time change the risk profile of the change? Is it because you are planning on “shutting down” (simplification) the v1 registrar with the launch of v2 so the bug would go away by nature of the whole thing being essentially “turned off”?
Multiple controllers are necessary to provide a migration pathway when a controller is upgraded - otherwise, all applications that register names need to swap over at the same time, or stop working.
No, because once v2 is launched we can revoke control over the registrar, because we can be confident we won’t need to add or remove any further controllers at this point.
At the moment, everyone I know and everyone I speak to about ENS believes that the security model is that once you register your name, you don’t have to trust anyone, including the ENS developers or security council, until your name expires.
Would you all (ENS Developers) be willing to make a public announcement in all of the usual places (Twitter, Blog, Discord, notes in the docs, etc.) that makes it very clear to users that for the last year and until such time as ENSv2 is deployed and training wheels are disabled, ENS names will not have the security model intended and the ENS DAO + Security Council can rug any name from any user?
Such a public announcement would at least make me feel like users are not being tricked into thinking ENS names are as uncensorable as the original documentation and messaging implied. I still would much rather see these two bugs fixed, as I would like to continue using ENS as the posterchild for the “right” way to build on Ethereum, but making it plainly clear to all users what deal they are getting at least shows a good faith effort to be honest about the threat model given the unfixed bug.
I don’t think that’s an accurate description of the situation. There exists a vulnerability that could be abused to do this - but it would require writing and deploying a malicious registrar controller, and getting both the DAO and the security council to vote for it. Then, the controller would need to be used to shorten a user’s registration in 90-day increments.
Loudly announcing this seems a bit like asking a plane’s captain to go on the PA and tell everyone “just so you know, I could crash this plane any time I felt like it, and you would all die”. It’s technically accurate, but everyone already knows it and it would be counterproductive to express it in that way.
This is exactly the problem, no one knows it. I haven’t met a single person who was aware that the DAO + Security council could rug names. Every single person I have brought this up to, including highly technical Ethereum folk, believed that ENS names were not claimable by anyone, including DAO/council, until they expire.
The ENS messaging historically has all been very clear about this, and the original design and intended implementation all support this. Almost no one knows that the pilot has the ability to crash the plane, and it feels inappropriate to continue to let people board the plane without making them aware of the situation.
We disclosed the vulnerability publicly when it was discovered, and engaged in public discussions here on the DAO forum over the pros and cons of patching it immediately; we even developed a mitigation that could be deployed if the DAO agreed it was worthwhile. Ultimately the consensus seemed to be that the risks outweighed the rewards. I don’t think it’s reasonable to suggest that this is hidden or unknown.
Building complex software using immutable code is hard. In my opinion, building complex software without bugs is basically impossible. As a rule, the more code, the more bugs.
The more interesting question is not whether the ENS codebase is currently perfect, because it’s not, but instead, how do we deal with the inherent problem that we want all code to be immutable, but there is no perfect way to avoid bugs?
In the NameWrapper, the intended design of the upgrade mechanism was for it to require two parties to agree to upgrade a name: the owner and the ENS DAO. Unfortunately, a bug in the upgrade mechanism itself resulted in it requiring only the ENS DAO.
The current proposal seeks to revoke the upgrade mechanism entirely, but there is another option: fix the upgrade mechanism so that it works as intended. The fix that was previously developed would accomplish this.
Moving forward, there are ways to try to avoid bugs, with the most tried-and-true method being to use fewer lines of code in immutable software and reduce complexity. With ENSv2, one of the goals is to vastly simplify the core immutable code. I agree with this approach. Complexity should be minimized in the core ENS contracts and isolated as much as possible. For higher-level user functionality with more complex code, there must be mechanisms and strategies in place to handle bugs.
At this stage, I do not support the proposal to entirely revoke the upgrade functionality. The ENSv2 registry is not yet available, the NameWrapper remains a very complex codebase, and the upgrade mechanism still serves its original purpose: providing an escape hatch for names with burned fuses in the NameWrapper if it ever becomes necessary.
I would support fixing the upgrade functionality to ensure that both the permission of name owners and the ENS DAO are required to upgrade a name.
There is a difference between posting a GitHub security advisory + a conversation in a forum with a handful of readers and making your users aware of the issue in a way that they can understand. It is very common in industry for companies to publish a CVE and not tell their users about the bug directly, but I think this is a very bad practice. I would like to see ENS do better and if you are going to actively decide to leave a bug that affects the system’s security model unfixed, then make every attempt to proactively inform end-users about the situation in a way that is understandable to them, not only people who subscribe to your technical security advisories on GitHub.
I have brought this issue up with many people, and not a single one knew about it, including people in ENS Discord server, Twitter, voice chats, etc. other than you and Premm.eth. Even you indicated in another thread that you had forgotten about this bug, so maybe the number of people previously aware of it is actually just 1.
The docs also do not mention it, like FAQ | ENS Docs which implies that no one can take a name away from its owner (which is not true at the moment).
The .eth registrar is built to ensure once issued, a name cannot be revoked or taken away from its owner. Potential loss can occur if the owner loses access to their private key, or if the owner forgets to renew their name.
There is also the mention in the docs that the registrar is trustless, which it is not (one must trust the DAO + Security Council):
The ETH Registrar is a special registrar. It allows for trustless on-chain name registration and is in charge of the “.eth” TLD.
I’m not suggesting that you all actively took measures to hide this bug from users, but I don’t get the impression that you have proactively attempted to inform all of your users of the issue, which I think is the right thing to do if you are making the decision on their behalf to change the security model. I suspect that most of the ENS delegates aren’t even aware of this issue or that the trust model isn’t what is advertised.
It is certainly possible that ENS users will agree with you that this bug isn’t worth the risk to fix, but I think they should be properly informed about the situation rather than it being hidden away in a forum thread that a dozen people read.
