From 2a7516591d7c4b8efcc902879840247ac956988c Mon Sep 17 00:00:00 2001 From: Nick Johnson Date: Fri, 2 Jun 2023 16:11:30 +0100 Subject: [PATCH 1/8] Initial version of mitigation contracts --- contracts/ethregistrar/ETHRegistrarAdmin.sol | 124 +++++++++++++++ contracts/ethregistrar/IBaseRegistrar.sol | 12 ++ contracts/wrapper/NameWrapperAdmin.sol | 158 +++++++++++++++++++ 3 files changed, 294 insertions(+) create mode 100644 contracts/ethregistrar/ETHRegistrarAdmin.sol create mode 100644 contracts/wrapper/NameWrapperAdmin.sol diff --git a/contracts/ethregistrar/ETHRegistrarAdmin.sol b/contracts/ethregistrar/ETHRegistrarAdmin.sol new file mode 100644 index 00000000..941fe83e --- /dev/null +++ b/contracts/ethregistrar/ETHRegistrarAdmin.sol @@ -0,0 +1,124 @@ +//SPDX-License-Identifier: MIT +pragma solidity ^0.8.17; + +import "./IBaseRegistrar.sol"; +import {Ownable} from "@openzeppelin/contracts/access/Ownable.sol"; +import {Create2} from "@openzeppelin/contracts/utils/Create2.sol"; +import {Address} from "@openzeppelin/contracts/utils/Address.sol"; + +/** + * @dev A proxy contract that wraps new registrar controllers to ensure they don't shorten the duration of registrations. + */ +contract ETHRegistrarControllerProxy { + address public immutable controller; + IBaseRegistrar public immutable registrar; + + constructor(address _controller, IBaseRegistrar _registrar) { + controller = _controller; + registrar = _registrar; + } + + function register( + uint256 id, + address owner, + uint256 duration + ) external returns (uint256) { + require(msg.sender == controller); + require(duration < 365000000 days); + return registrar.register(id, owner, duration); + } + + function registerOnly( + uint256 id, + address owner, + uint256 duration + ) external returns (uint256) { + require(msg.sender == controller); + require(duration < 365000000 days); + return registrar.registerOnly(id, owner, duration); + } + + function renew(uint256 id, uint256 duration) external returns (uint256) { + require(msg.sender == controller); + require(duration < 365000000 days); + return registrar.renew(id, duration); + } +} + +/** + * @dev Contract to act as the owner of the NameWrapper, permitting its owner to make certain changes with additional checks. + * This was implemented in response to a vulnerability disclosure that would permit the DAO to appoint a malicious controller + * that shortens the registration period of affected ENS names. This contract exists to prevent that from happening. + */ +contract ETHRegistrarAdmin is Ownable { + using Address for address; + + IBaseRegistrar public immutable registrar; + + constructor(address _registrar) { + registrar = IBaseRegistrar(_registrar); + } + + /** + * @dev Deploys a controller proxy for the given controller, if one does not already exist. + * Anyone can call this function, but the proxy will only function if added by an authorized + * caller using `addController`. + * @param controller The controller contract to create a proxy for. + * @return The address of the controller proxy. + */ + function deployControllerProxy( + address controller + ) external returns (address) { + address proxyAddress = getProxyAddress(controller); + if (!proxyAddress.isContract()) { + new ETHRegistrarControllerProxy{salt: bytes32(0)}( + controller, + registrar + ); + } + return proxyAddress; + } + + /** + * @dev Authorizes a controller proxy to register and renew names on the registrar. + * @param controller The controller contract to authorize. + */ + function addController(address controller) external onlyOwner { + registrar.addController(getProxyAddress(controller)); + } + + /** + * @dev Deauthorizes a controller proxy. + * @param controller The controller contract to deauthorize. + */ + function removeController(address controller) external onlyOwner { + registrar.removeController(getProxyAddress(controller)); + } + + /** + * @dev Gets the address of the proxy contract for a given controller. + * @param controller The controller contract to get the proxy address for. + * @return The address of the proxy contract. + */ + function getProxyAddress(address controller) public view returns (address) { + return + Create2.computeAddress( + bytes32(0), + keccak256( + abi.encodePacked( + type(ETHRegistrarControllerProxy).creationCode, + controller, + registrar + ) + ) + ); + } + + /** + * @dev Sets the resolver for the TLD this registrar manages. + * @param resolver The address of the resolver to set. + */ + function setResolver(address resolver) external onlyOwner { + registrar.setResolver(resolver); + } +} diff --git a/contracts/ethregistrar/IBaseRegistrar.sol b/contracts/ethregistrar/IBaseRegistrar.sol index 5f43e9c1..cc0ccc8e 100644 --- a/contracts/ethregistrar/IBaseRegistrar.sol +++ b/contracts/ethregistrar/IBaseRegistrar.sol @@ -41,6 +41,18 @@ interface IBaseRegistrar is IERC721 { uint256 duration ) external returns (uint256); + /** + * @dev Register a name, without modifying the registry. + * @param id The token ID (keccak256 of the label). + * @param owner The address that should own the registration. + * @param duration Duration in seconds for the registration. + */ + function registerOnly( + uint256 id, + address owner, + uint256 duration + ) external returns (uint256); + function renew(uint256 id, uint256 duration) external returns (uint256); /** diff --git a/contracts/wrapper/NameWrapperAdmin.sol b/contracts/wrapper/NameWrapperAdmin.sol new file mode 100644 index 00000000..283e8dfb --- /dev/null +++ b/contracts/wrapper/NameWrapperAdmin.sol @@ -0,0 +1,158 @@ +//SPDX-License-Identifier: MIT +pragma solidity ^0.8.17; + +import "./INameWrapper.sol"; +import "./Controllable.sol"; +import {Ownable} from "@openzeppelin/contracts/access/Ownable.sol"; +import {Create2} from "@openzeppelin/contracts/utils/Create2.sol"; +import {Address} from "@openzeppelin/contracts/utils/Address.sol"; + +/** + * @dev A proxy contract that wraps new name wrapper controllers to ensure they don't shorten the duration of registrations. + */ +contract NameWrapperControllerProxy { + address public immutable controller; + INameWrapper public immutable wrapper; + + constructor(address _controller, INameWrapper _wrapper) { + controller = _controller; + wrapper = _wrapper; + } + + /** + * @dev Registers a new .eth second-level domain and wraps it. + * Only callable by authorised controllers. + * @param label The label to register (Eg, 'foo' for 'foo.eth'). + * @param wrappedOwner The owner of the wrapped name. + * @param duration The duration, in seconds, to register the name for. + * @param resolver The resolver address to set on the ENS registry (optional). + * @param ownerControlledFuses Initial owner-controlled fuses to set + * @return registrarExpiry The expiry date of the new name on the .eth registrar, in seconds since the Unix epoch. + */ + function registerAndWrapETH2LD( + string calldata label, + address wrappedOwner, + uint256 duration, + address resolver, + uint16 ownerControlledFuses + ) external returns (uint256 registrarExpiry) { + require(msg.sender == controller); + require(duration < 365000000 days); + return + wrapper.registerAndWrapETH2LD( + label, + wrappedOwner, + duration, + resolver, + ownerControlledFuses + ); + } + + /** + * @notice Renews a .eth second-level domain. + * @dev Only callable by authorised controllers. + * @param tokenId The hash of the label to register (eg, `keccak256('foo')`, for 'foo.eth'). + * @param duration The number of seconds to renew the name for. + * @return expires The expiry date of the name on the .eth registrar, in seconds since the Unix epoch. + */ + function renew( + uint256 tokenId, + uint256 duration + ) external returns (uint256 expires) { + require(msg.sender == controller); + require(duration < 365000000 days); + return wrapper.renew(tokenId, duration); + } +} + +/** + * @dev Contract to act as the owner of the NameWrapper, permitting its owner to make certain changes with additional checks. + * This was implemented in response to a vulnerability disclosure that would permit the DAO to appoint a malicious controller + * that shortens the registration period of affected ENS names. This contract exists to prevent that from happening. + */ +contract NameWrapperAdmin is Ownable { + using Address for address; + + address public immutable wrapper; + + constructor(address _wrapper) { + wrapper = _wrapper; + } + + /** + * @dev Deploys a controller proxy for the given controller, if one does not already exist. + * Anyone can call this function, but the proxy will only function if added by an authorized + * caller using `addController`. + * @param controller The controller contract to create a proxy for. + * @return The address of the controller proxy. + */ + function deployControllerProxy( + address controller + ) external returns (address) { + address proxyAddress = getProxyAddress(controller); + if (!proxyAddress.isContract()) { + new NameWrapperControllerProxy{salt: bytes32(0)}( + controller, + INameWrapper(wrapper) + ); + } + return proxyAddress; + } + + /** + * @dev Authorizes a controller proxy to register and renew names on the wrapper. + * @param controller The controller contract to authorize. + */ + function addController(address controller) external onlyOwner { + Controllable(wrapper).setController(getProxyAddress(controller), true); + } + + /** + * @dev Deauthorizes a controller proxy. + * @param controller The controller contract to deauthorize. + */ + function removeController(address controller) external onlyOwner { + Controllable(wrapper).setController(getProxyAddress(controller), false); + } + + /** + * @dev Gets the address of the proxy contract for a given controller. + * @param controller The controller contract to get the proxy address for. + * @return The address of the proxy contract. + */ + function getProxyAddress(address controller) public view returns (address) { + return + Create2.computeAddress( + bytes32(0), + keccak256( + abi.encodePacked( + type(NameWrapperControllerProxy).creationCode, + controller, + wrapper + ) + ) + ); + } + + /** + * @notice Set the metadata service. Only the owner can do this + * @param _metadataService The new metadata service + */ + function setMetadataService( + IMetadataService _metadataService + ) public onlyOwner { + INameWrapper(wrapper).setMetadataService(_metadataService); + } + + /** + * @notice Set the address of the upgradeContract of the contract. only admin can do this + * @dev The default value of upgradeContract is the 0 address. Use the 0 address at any time + * to make the contract not upgradable. + * @param _upgradeAddress address of an upgraded contract + */ + function setUpgradeContract( + INameWrapperUpgrade _upgradeAddress + ) public onlyOwner { + INameWrapper(wrapper).setUpgradeContract(_upgradeAddress); + } +} From 4c4854db85130738519b81bc79d1b2b7fb8916aa Mon Sep 17 00:00:00 2001 From: Nick Johnson Date: Tue, 18 Jul 2023 15:00:17 +0100 Subject: [PATCH 2/8] Tests for ETHRegistrarAdmin --- contracts/ethregistrar/ETHRegistrarAdmin.sol | 9 +-- test/ethregistrar/TestBaseRegistrar.js | 59 +++++++++++++++----- 2 files changed, 49 insertions(+), 19 deletions(-) diff --git a/contracts/ethregistrar/ETHRegistrarAdmin.sol b/contracts/ethregistrar/ETHRegistrarAdmin.sol index 941fe83e..53377138 100644 --- a/contracts/ethregistrar/ETHRegistrarAdmin.sol +++ b/contracts/ethregistrar/ETHRegistrarAdmin.sol @@ -7,7 +7,7 @@ import {Create2} from "@openzeppelin/contracts/utils/Create2.sol"; import {Address} from "@openzeppelin/contracts/utils/Address.sol"; /** - * @dev A proxy contract that wraps new registrar controllers to ensure they don't shorten the duration of registrations. + * @dev A proxy contract that wraps new registrar controllers to ensure they don't shorten the duration of registrations by causing an overflow. */ contract ETHRegistrarControllerProxy { address public immutable controller; @@ -68,7 +68,7 @@ contract ETHRegistrarAdmin is Ownable { */ function deployControllerProxy( address controller - ) external returns (address) { + ) public returns (address) { address proxyAddress = getProxyAddress(controller); if (!proxyAddress.isContract()) { new ETHRegistrarControllerProxy{salt: bytes32(0)}( @@ -84,6 +84,7 @@ contract ETHRegistrarAdmin is Ownable { * @param controller The controller contract to authorize. */ function addController(address controller) external onlyOwner { + deployControllerProxy(controller); registrar.addController(getProxyAddress(controller)); } @@ -107,8 +108,8 @@ contract ETHRegistrarAdmin is Ownable { keccak256( abi.encodePacked( type(ETHRegistrarControllerProxy).creationCode, - controller, - registrar + uint256(uint160(controller)), + uint256(uint160(address(registrar))) ) ) ); diff --git a/test/ethregistrar/TestBaseRegistrar.js b/test/ethregistrar/TestBaseRegistrar.js index a0c7efaf..b8f0898e 100644 --- a/test/ethregistrar/TestBaseRegistrar.js +++ b/test/ethregistrar/TestBaseRegistrar.js @@ -2,6 +2,11 @@ const ENS = artifacts.require('./registry/ENSRegistry') const BaseRegistrar = artifacts.require( './registrar/BaseRegistrarImplementation', ) +const ETHRegistrarAdmin = artifacts.require('./registrar/ETHRegistrarAdmin') +const ETHRegistrarControllerProxy = artifacts.require( + './registrar/ETHRegistrarControllerProxy', +) +const { expect } = require('chai') const namehash = require('eth-ens-namehash') const sha3 = require('web3-utils').sha3 @@ -21,19 +26,33 @@ contract('BaseRegistrar', function (accounts) { let ens let registrar + let admin + let controllerProxy before(async () => { ens = await ENS.new() + // Deploy the registrar registrar = await BaseRegistrar.new(ens.address, namehash.hash('eth'), { from: ownerAccount, }) - await registrar.addController(controllerAccount, { from: ownerAccount }) + + // Deploy the admin contract and transfer ownership to it + admin = await ETHRegistrarAdmin.new(registrar.address) + await registrar.transferOwnership(admin.address) + + // Create a new proxy for the controller + await admin.addController(controllerAccount, { from: ownerAccount }) + controllerProxy = await ETHRegistrarControllerProxy.at( + await admin.getProxyAddress(controllerAccount), + ) + + // Set the registrar as owner of .eth await ens.setSubnodeOwner('0x0', sha3('eth'), registrar.address) }) it('should allow new registrations', async () => { - var tx = await registrar.register( + var tx = await controllerProxy.register( sha3('newname'), registrantAccount, 86400, @@ -52,7 +71,7 @@ contract('BaseRegistrar', function (accounts) { }) it('should allow registrations without updating the registry', async () => { - var tx = await registrar.registerOnly( + var tx = await controllerProxy.registerOnly( sha3('silentname'), registrantAccount, 86400, @@ -69,7 +88,9 @@ contract('BaseRegistrar', function (accounts) { it('should allow renewals', async () => { var oldExpires = await registrar.nameExpires(sha3('newname')) - await registrar.renew(sha3('newname'), 86400, { from: controllerAccount }) + await controllerProxy.renew(sha3('newname'), 86400, { + from: controllerAccount, + }) assert.equal( (await registrar.nameExpires(sha3('newname'))).toNumber(), oldExpires.add(toBN(86400)).toNumber(), @@ -78,7 +99,7 @@ contract('BaseRegistrar', function (accounts) { it('should only allow the controller to register', async () => { await exceptions.expectFailure( - registrar.register(sha3('foo'), otherAccount, 86400, { + controllerProxy.register(sha3('foo'), otherAccount, 86400, { from: otherAccount, }), ) @@ -86,13 +107,13 @@ contract('BaseRegistrar', function (accounts) { it('should only allow the controller to renew', async () => { await exceptions.expectFailure( - registrar.renew(sha3('newname'), 86400, { from: otherAccount }), + controllerProxy.renew(sha3('newname'), 86400, { from: otherAccount }), ) }) it('should not permit registration of already registered names', async () => { await exceptions.expectFailure( - registrar.register(sha3('newname'), otherAccount, 86400, { + controllerProxy.register(sha3('newname'), otherAccount, 86400, { from: controllerAccount, }), ) @@ -101,7 +122,7 @@ contract('BaseRegistrar', function (accounts) { it('should not permit renewing a name that is not registered', async () => { await exceptions.expectFailure( - registrar.renew(sha3('name3'), 86400, { from: controllerAccount }), + controllerProxy.renew(sha3('name3'), 86400, { from: controllerAccount }), ) }) @@ -180,7 +201,9 @@ contract('BaseRegistrar', function (accounts) { }) it('should allow renewal during the grace period', async () => { - await registrar.renew(sha3('newname'), 86400, { from: controllerAccount }) + await controllerProxy.renew(sha3('newname'), 86400, { + from: controllerAccount, + }) }) it('should allow registration of an expired domain', async () => { @@ -188,20 +211,26 @@ contract('BaseRegistrar', function (accounts) { var expires = await registrar.nameExpires(sha3('newname')) var grace = await registrar.GRACE_PERIOD() await evm.advanceTime(expires.toNumber() - ts + grace.toNumber() + 3600) + await evm.mine() - try { - await registrar.ownerOf(sha3('newname')) - assert.fail('should throw an exception') - } catch (error) {} + await expect(registrar.ownerOf(sha3('newname'))).to.be.reverted - await registrar.register(sha3('newname'), otherAccount, 86400, { + await controllerProxy.register(sha3('newname'), otherAccount, 86400, { from: controllerAccount, }) assert.equal(await registrar.ownerOf(sha3('newname')), otherAccount) }) it('should allow the owner to set a resolver address', async () => { - await registrar.setResolver(accounts[1], { from: ownerAccount }) + await admin.setResolver(accounts[1], { from: ownerAccount }) assert.equal(await ens.resolver(namehash.hash('eth')), accounts[1]) }) + + it('should not allow renewals of longer than 365000000 days', async () => { + await expect( + controllerProxy.renew(sha3('newname'), 365000000 * 86400 + 1, { + from: controllerAccount, + }), + ).to.be.reverted + }) }) From f590ccc7c0b537c4893d032ee4e0aa5ecee57b28 Mon Sep 17 00:00:00 2001 From: Nick Johnson Date: Wed, 19 Jul 2023 11:45:51 +0100 Subject: [PATCH 3/8] Update tests for new wrapper admin contract --- contracts/wrapper/NameWrapperAdmin.sol | 7 +- test/wrapper/NameWrapper.js | 179 +++++++++++++------------ 2 files changed, 100 insertions(+), 86 deletions(-) diff --git a/contracts/wrapper/NameWrapperAdmin.sol b/contracts/wrapper/NameWrapperAdmin.sol index 283e8dfb..1b918eab 100644 --- a/contracts/wrapper/NameWrapperAdmin.sol +++ b/contracts/wrapper/NameWrapperAdmin.sol @@ -88,7 +88,7 @@ contract NameWrapperAdmin is Ownable { */ function deployControllerProxy( address controller - ) external returns (address) { + ) public returns (address) { address proxyAddress = getProxyAddress(controller); if (!proxyAddress.isContract()) { new NameWrapperControllerProxy{salt: bytes32(0)}( @@ -104,6 +104,7 @@ contract NameWrapperAdmin is Ownable { * @param controller The controller contract to authorize. */ function addController(address controller) external onlyOwner { + deployControllerProxy(controller); Controllable(wrapper).setController(getProxyAddress(controller), true); } @@ -127,8 +128,8 @@ contract NameWrapperAdmin is Ownable { keccak256( abi.encodePacked( type(NameWrapperControllerProxy).creationCode, - controller, - wrapper + uint256(uint160(controller)), + uint256(uint160(wrapper)) ) ) ); diff --git a/test/wrapper/NameWrapper.js b/test/wrapper/NameWrapper.js index dc319022..11d61055 100644 --- a/test/wrapper/NameWrapper.js +++ b/test/wrapper/NameWrapper.js @@ -9,7 +9,6 @@ const { shouldRespectConstraints } = require('./Constraints.behaviour') const { ZERO_ADDRESS } = require('@openzeppelin/test-helpers/src/constants') const { deploy } = require('../test-utils/contracts') const { EMPTY_BYTES32, EMPTY_ADDRESS } = require('../test-utils/constants') - const abiCoder = new ethers.utils.AbiCoder() use(solidity) @@ -53,6 +52,8 @@ describe('Name Wrapper', () => { let NameWrapper2 let NameWrapperH let NameWrapperUpgraded + let NameWrappperAdmin + let NameWrapperControllerProxy let MetaDataservice let signers let accounts @@ -140,6 +141,12 @@ describe('Name Wrapper', () => { NameWrapperH = NameWrapper.connect(signers[2]) NameWrapper3 = NameWrapperH + NameWrapperAdmin = await deploy('NameWrapperAdmin', NameWrapper.address) + await NameWrapper.transferOwnership(NameWrapperAdmin.address) + NameWrapperControllerProxy = await ethers.getContractFactory( + 'NameWrapperControllerProxy', + ) + NameWrapperUpgraded = await deploy( 'UpgradedNameWrapperMock', EnsRegistry.address, @@ -978,8 +985,11 @@ describe('Name Wrapper', () => { // Register from another address with registerAndWrap() await BaseRegistrar.addController(NameWrapper.address) - await NameWrapper.setController(account, account) - await NameWrapper.registerAndWrapETH2LD( + await NameWrapperAdmin.addController(account) + const NameWrapperProxy = NameWrapperControllerProxy.attach( + await NameWrapperAdmin.getProxyAddress(account), + ) + await NameWrapperProxy.registerAndWrapETH2LD( label, account2, DAY, @@ -1892,7 +1902,7 @@ describe('Name Wrapper', () => { it('Reverts if called by someone that is not the owner', async () => { // Attempt to attack the contract by setting the upgrade contract to themselves await expect( - NameWrapper2.setUpgradeContract(account2), + NameWrapperAdmin.connect(signers[2]).setUpgradeContract(account2), ).to.be.revertedWith('Ownable: caller is not the owner') }) it('Will setApprovalForAll for the upgradeContract addresses in the registrar and registry to true', async () => { @@ -1910,7 +1920,7 @@ describe('Name Wrapper', () => { ).to.equal(false) //set the upgradeContract of the NameWrapper contract - await NameWrapper.setUpgradeContract(NameWrapperUpgraded.address) + await NameWrapperAdmin.setUpgradeContract(NameWrapperUpgraded.address) expect( await BaseRegistrar.isApprovedForAll( @@ -1927,7 +1937,7 @@ describe('Name Wrapper', () => { }) it('Will setApprovalForAll for the old upgradeContract addresses in the registrar and registry to false', async () => { //set the upgradeContract of the NameWrapper contract - await NameWrapper.setUpgradeContract(DUMMY_ADDRESS) + await NameWrapperAdmin.setUpgradeContract(DUMMY_ADDRESS) expect( await BaseRegistrar.isApprovedForAll( @@ -1940,7 +1950,7 @@ describe('Name Wrapper', () => { ).to.equal(true) //set the upgradeContract of the NameWrapper contract - await NameWrapper.setUpgradeContract(NameWrapperUpgraded.address) + await NameWrapperAdmin.setUpgradeContract(NameWrapperUpgraded.address) expect( await BaseRegistrar.isApprovedForAll( @@ -1967,7 +1977,7 @@ describe('Name Wrapper', () => { }) it('Will not setApprovalForAll for the new upgrade address if it is the address(0)', async () => { //set the upgradeContract of the NameWrapper contract - await NameWrapper.setUpgradeContract(NameWrapperUpgraded.address) + await NameWrapperAdmin.setUpgradeContract(NameWrapperUpgraded.address) expect( await BaseRegistrar.isApprovedForAll( @@ -1983,7 +1993,7 @@ describe('Name Wrapper', () => { ).to.equal(true) //set the upgradeContract of the NameWrapper contract - await NameWrapper.setUpgradeContract(ZERO_ADDRESS) + await NameWrapperAdmin.setUpgradeContract(ZERO_ADDRESS) expect( await BaseRegistrar.isApprovedForAll(NameWrapper.address, ZERO_ADDRESS), @@ -2024,7 +2034,7 @@ describe('Name Wrapper', () => { ) //set the upgradeContract of the NameWrapper contract - await NameWrapper.setUpgradeContract(NameWrapperUpgraded.address) + await NameWrapperAdmin.setUpgradeContract(NameWrapperUpgraded.address) await BaseRegistrar.isApprovedForAll( NameWrapper.address, NameWrapperUpgraded.address, @@ -2063,7 +2073,7 @@ describe('Name Wrapper', () => { NameWrapper.address, ) - await NameWrapper.setUpgradeContract(NameWrapperUpgraded.address) + await NameWrapperAdmin.setUpgradeContract(NameWrapperUpgraded.address) const tx = await NameWrapper2.upgrade(encodedName, 0) @@ -2106,13 +2116,13 @@ describe('Name Wrapper', () => { CAN_DO_EVERYTHING, EMPTY_ADDRESS, ) - await NameWrapper.setUpgradeContract(NameWrapperUpgraded.address) + await NameWrapperAdmin.setUpgradeContract(NameWrapperUpgraded.address) expect(await NameWrapper.upgradeContract()).to.equal( NameWrapperUpgraded.address, ) - await NameWrapper.setUpgradeContract(EMPTY_ADDRESS) + await NameWrapperAdmin.setUpgradeContract(EMPTY_ADDRESS) await expect(NameWrapper.upgrade(encodedName, 0)).to.be.revertedWith( `CannotUpgrade()`, ) @@ -2132,7 +2142,7 @@ describe('Name Wrapper', () => { ) //set the upgradeContract of the NameWrapper contract - await NameWrapper.setUpgradeContract(NameWrapperUpgraded.address) + await NameWrapperAdmin.setUpgradeContract(NameWrapperUpgraded.address) const expectedExpiry = (await BaseRegistrar.nameExpires(labelHash)).add( GRACE_PERIOD, @@ -2167,7 +2177,7 @@ describe('Name Wrapper', () => { EMPTY_ADDRESS, ) - await NameWrapper.setUpgradeContract(NameWrapperUpgraded.address) + await NameWrapperAdmin.setUpgradeContract(NameWrapperUpgraded.address) await NameWrapper.upgrade(encodedName, 0) @@ -2192,7 +2202,7 @@ describe('Name Wrapper', () => { EMPTY_ADDRESS, ) - await NameWrapper.setUpgradeContract(NameWrapperUpgraded.address) + await NameWrapperAdmin.setUpgradeContract(NameWrapperUpgraded.address) await NameWrapper.upgrade(encodedName, 0) @@ -2215,7 +2225,7 @@ describe('Name Wrapper', () => { EMPTY_ADDRESS, ) - await NameWrapper.setUpgradeContract(NameWrapperUpgraded.address) + await NameWrapperAdmin.setUpgradeContract(NameWrapperUpgraded.address) const expectedExpiry = (await BaseRegistrar.nameExpires(labelHash)).add( GRACE_PERIOD, @@ -2250,7 +2260,7 @@ describe('Name Wrapper', () => { ) //set the upgradeContract of the NameWrapper contract - await NameWrapper.setUpgradeContract(NameWrapperUpgraded.address) + await NameWrapperAdmin.setUpgradeContract(NameWrapperUpgraded.address) await expect(NameWrapper2.upgrade(encodedName, 0)).to.be.revertedWith( `Unauthorised("${nameHash}", "${account2}")`, @@ -2288,7 +2298,7 @@ describe('Name Wrapper', () => { expect(ownerOfWrapped).to.equal(account) //set the upgradeContract of the NameWrapper contract - await NameWrapper.setUpgradeContract(NameWrapperUpgraded.address) + await NameWrapperAdmin.setUpgradeContract(NameWrapperUpgraded.address) const tx = await NameWrapper.upgrade(encodedName, 0) @@ -2315,7 +2325,7 @@ describe('Name Wrapper', () => { expect(ownerOfWrappedXYZ).to.equal(account) //set the upgradeContract of the NameWrapper contract - await NameWrapper.setUpgradeContract(NameWrapperUpgraded.address) + await NameWrapperAdmin.setUpgradeContract(NameWrapperUpgraded.address) const tx = await NameWrapper2.upgrade(encodeName('to-upgrade.xyz'), 0) @@ -2374,7 +2384,7 @@ describe('Name Wrapper', () => { expect(ownerOfWrapped).to.equal(account) //set the upgradeContract of the NameWrapper contract - await NameWrapper.setUpgradeContract(NameWrapperUpgraded.address) + await NameWrapperAdmin.setUpgradeContract(NameWrapperUpgraded.address) const tx = await NameWrapper.upgrade(encodeName(name), 0) @@ -2415,7 +2425,7 @@ describe('Name Wrapper', () => { expect(ownerOfWrapped).to.equal(account) //set the upgradeContract of the NameWrapper contract - await NameWrapper.setUpgradeContract(NameWrapperUpgraded.address) + await NameWrapperAdmin.setUpgradeContract(NameWrapperUpgraded.address) await NameWrapper.upgrade(encodedName, 0) @@ -2448,7 +2458,7 @@ describe('Name Wrapper', () => { expect(ownerOfWrapped).to.equal(account) //set the upgradeContract of the NameWrapper contract - await NameWrapper.setUpgradeContract(NameWrapperUpgraded.address) + await NameWrapperAdmin.setUpgradeContract(NameWrapperUpgraded.address) await NameWrapper.upgrade(encodedName, 0) @@ -2485,7 +2495,7 @@ describe('Name Wrapper', () => { ).to.equal(account3) // set the upgradeContract of the NameWrapper contract - await NameWrapper.setUpgradeContract(NameWrapperUpgraded.address) + await NameWrapperAdmin.setUpgradeContract(NameWrapperUpgraded.address) const tx = await NameWrapper.upgrade(encodeName('to-upgrade.xyz'), '0x') expect(tx) @@ -2513,7 +2523,7 @@ describe('Name Wrapper', () => { expect(ownerOfWrappedXYZ).to.equal(account) //set the upgradeContract of the NameWrapper contract - await NameWrapper.setUpgradeContract(NameWrapperUpgraded.address) + await NameWrapperAdmin.setUpgradeContract(NameWrapperUpgraded.address) const tx = await NameWrapper.upgrade( encodeName('to-upgrade.xyz'), @@ -2551,7 +2561,7 @@ describe('Name Wrapper', () => { expect(ownerOfWrappedXYZ).to.equal(account) //set the upgradeContract of the NameWrapper contract - await NameWrapper.setUpgradeContract(NameWrapperUpgraded.address) + await NameWrapperAdmin.setUpgradeContract(NameWrapperUpgraded.address) await expect( NameWrapper2.upgrade(encodeName('to-upgrade.xyz'), 0), @@ -6066,14 +6076,18 @@ describe('Name Wrapper', () => { const label = 'register' const labelHash = labelhash(label) const wrappedTokenId = namehash(label + '.eth') + let NameWrapperProxy before(async () => { await BaseRegistrar.addController(NameWrapper.address) - await NameWrapper.setController(account, true) + await NameWrapperAdmin.addController(account) + NameWrapperProxy = NameWrapperControllerProxy.attach( + await NameWrapperAdmin.getProxyAddress(account), + ) }) it('should register and wrap names', async () => { - await NameWrapper.registerAndWrapETH2LD( + await NameWrapperProxy.registerAndWrapETH2LD( label, account, 86400, @@ -6091,7 +6105,7 @@ describe('Name Wrapper', () => { }) it('allows specifying a resolver address', async () => { - await NameWrapper.registerAndWrapETH2LD( + await NameWrapperProxy.registerAndWrapETH2LD( label, account, 86400, @@ -6103,20 +6117,19 @@ describe('Name Wrapper', () => { }) it('does not allow non controllers to register names', async () => { - await NameWrapper.setController(account, false) await expect( - NameWrapper.registerAndWrapETH2LD( + NameWrapperProxy.connect(signers[2]).registerAndWrapETH2LD( label, account, 86400, EMPTY_ADDRESS, CAN_DO_EVERYTHING, ), - ).to.be.revertedWith('Controllable: Caller is not a controller') + ).to.be.reverted }) it('Transfers the wrapped token to the target address.', async () => { - await NameWrapper.registerAndWrapETH2LD( + await NameWrapperProxy.registerAndWrapETH2LD( label, account2, 86400, @@ -6128,7 +6141,7 @@ describe('Name Wrapper', () => { it('Does not allow wrapping with a target address of 0x0', async () => { await expect( - NameWrapper.registerAndWrapETH2LD( + NameWrapperProxy.registerAndWrapETH2LD( label, EMPTY_ADDRESS, 86400, @@ -6140,7 +6153,7 @@ describe('Name Wrapper', () => { it('Does not allow wrapping with a target address of the wrapper contract address.', async () => { await expect( - NameWrapper.registerAndWrapETH2LD( + NameWrapperProxy.registerAndWrapETH2LD( label, NameWrapper.address, 86400, @@ -6154,7 +6167,7 @@ describe('Name Wrapper', () => { it('Does not allows fuse to be burned if CANNOT_UNWRAP has not been burned.', async () => { await expect( - NameWrapper.registerAndWrapETH2LD( + NameWrapperProxy.registerAndWrapETH2LD( label, account, 86400, @@ -6166,7 +6179,7 @@ describe('Name Wrapper', () => { it('Allows fuse to be burned if CANNOT_UNWRAP has been burned and expiry set', async () => { const initialFuses = CANNOT_UNWRAP | CANNOT_SET_RESOLVER - await NameWrapper.registerAndWrapETH2LD( + await NameWrapperProxy.registerAndWrapETH2LD( label, account, 86400, @@ -6178,7 +6191,7 @@ describe('Name Wrapper', () => { }) it('automatically sets PARENT_CANNOT_CONTROL and IS_DOT_ETH', async () => { - await NameWrapper.registerAndWrapETH2LD( + await NameWrapperProxy.registerAndWrapETH2LD( label, account, 86400, @@ -6190,13 +6203,14 @@ describe('Name Wrapper', () => { }) it('Errors when adding a number greater than uint16 for fuses', async () => { - const tx = await NameWrapper.populateTransaction.registerAndWrapETH2LD( - label, - account, - 86400, - EMPTY_ADDRESS, - 273, - ) + const tx = + await NameWrapperProxy.populateTransaction.registerAndWrapETH2LD( + label, + account, + 86400, + EMPTY_ADDRESS, + 273, + ) const rogueFuse = '40000' // 2 ** 18 in hex tx.data = tx.data.replace('00111', rogueFuse) @@ -6227,7 +6241,7 @@ describe('Name Wrapper', () => { it('Will not wrap a name with an empty label', async () => { await expect( - NameWrapper.registerAndWrapETH2LD( + NameWrapperProxy.registerAndWrapETH2LD( '', account, 86400, @@ -6242,7 +6256,7 @@ describe('Name Wrapper', () => { 'yutaioxtcsbzrqhdjmltsdfkgomogohhcchjoslfhqgkuhduhxqsldnurwrrtoicvthwxytonpcidtnkbrhccaozdtoznedgkfkifsvjukxxpkcmgcjprankyzerzqpnuteuegtfhqgzcxqwttyfewbazhyilqhyffufxrookxrnjkmjniqpmntcbrowglgdpkslzechimsaonlcvjkhhvdvkvvuztihobmivifuqtvtwinljslusvhhbwhuhzty' expect(longString.length).to.equal(256) await expect( - NameWrapper.registerAndWrapETH2LD( + NameWrapperProxy.registerAndWrapETH2LD( longString, account, 86400, @@ -6253,7 +6267,7 @@ describe('Name Wrapper', () => { }) it('emits Wrap event', async () => { - const tx = await NameWrapper.registerAndWrapETH2LD( + const tx = await NameWrapperProxy.registerAndWrapETH2LD( label, account, 86400, @@ -6274,7 +6288,7 @@ describe('Name Wrapper', () => { }) it('Emits TransferSingle event', async () => { - const tx = await NameWrapper.registerAndWrapETH2LD( + const tx = await NameWrapperProxy.registerAndWrapETH2LD( label, account, 86400, @@ -6283,7 +6297,13 @@ describe('Name Wrapper', () => { ) await expect(tx) .to.emit(NameWrapper, 'TransferSingle') - .withArgs(account, EMPTY_ADDRESS, account, wrappedTokenId, 1) + .withArgs( + NameWrapperProxy.address, + EMPTY_ADDRESS, + account, + wrappedTokenId, + 1, + ) }) }) @@ -6291,14 +6311,18 @@ describe('Name Wrapper', () => { const label = 'register' const labelHash = labelhash(label) const wrappedTokenId = namehash(label + '.eth') + let NameWrapperProxy before(async () => { await BaseRegistrar.addController(NameWrapper.address) - await NameWrapper.setController(account, true) + await NameWrapperAdmin.addController(account) + NameWrapperProxy = NameWrapperControllerProxy.attach( + await NameWrapperAdmin.getProxyAddress(account), + ) }) it('Renews names', async () => { - await NameWrapper.registerAndWrapETH2LD( + await NameWrapperProxy.registerAndWrapETH2LD( label, account, 86400, @@ -6306,14 +6330,14 @@ describe('Name Wrapper', () => { CAN_DO_EVERYTHING, ) const expires = await BaseRegistrar.nameExpires(labelHash) - await NameWrapper.renew(labelHash, 86400) + await NameWrapperProxy.renew(labelHash, 86400) expect(await BaseRegistrar.nameExpires(labelHash)).to.equal( expires.toNumber() + 86400, ) }) it('Renews names and can extend wrapper expiry', async () => { - await NameWrapper.registerAndWrapETH2LD( + await NameWrapperProxy.registerAndWrapETH2LD( label, account, 86400, @@ -6322,7 +6346,7 @@ describe('Name Wrapper', () => { ) const expires = await BaseRegistrar.nameExpires(labelHash) const expectedExpiry = expires.toNumber() + 86400 - await NameWrapper.renew(labelHash, 86400) + await NameWrapperProxy.renew(labelHash, 86400) expect(await BaseRegistrar.nameExpires(labelHash)).to.equal( expires.toNumber() + 86400, ) @@ -6333,7 +6357,7 @@ describe('Name Wrapper', () => { }) it('Renewing name less than required to unexpire it still has original owner/fuses', async () => { - await NameWrapper.registerAndWrapETH2LD( + await NameWrapperProxy.registerAndWrapETH2LD( label, account, DAY, @@ -6351,7 +6375,7 @@ describe('Name Wrapper', () => { expect(expiryBefore).to.be.at.most(block1.timestamp + GRACE_PERIOD) //renew for less than the grace period - await NameWrapper.renew(labelHash, 1 * DAY) + await NameWrapperProxy.renew(labelHash, 1 * DAY) const [ownerAfter, fusesAfter, expiryAfter] = await NameWrapper.getData( wrappedTokenId, @@ -6369,27 +6393,6 @@ describe('Name Wrapper', () => { }) }) - describe('Controllable', () => { - it('allows the owner to add and remove controllers', async () => { - const tx = await NameWrapper.setController(account, true) - expect(tx) - .to.emit(NameWrapper, 'ControllerChanged') - .withArgs(account, true) - - const tx2 = await NameWrapper.setController(account, false) - expect(tx2) - .to.emit(NameWrapper, 'ControllerChanged') - .withArgs(account, false) - }) - - it('does not allow non-owners to add or remove controllers', async () => { - await NameWrapper.setController(account, true) - - await expect(NameWrapper2.setController(account2, true)).to.be.reverted - await expect(NameWrapper2.setController(account, false)).to.be.reverted - }) - }) - describe('isWrapped(bytes32 node)', () => { const label = 'something' const wrappedTokenId = namehash(label + '.eth') @@ -6577,7 +6580,7 @@ describe('Name Wrapper', () => { }) it('owner can set a new MetadataService', async () => { - await NameWrapper.setMetadataService(account2) + await NameWrapperAdmin.setMetadataService(account2) expect(await NameWrapper.metadataService()).to.equal(account2) }) @@ -7065,12 +7068,17 @@ describe('Name Wrapper', () => { const labelHash2 = labelhash('sub2') const wrappedTokenId2 = namehash('sub2.sub1.eth') + let NameWrapperProxy + before(async () => { await BaseRegistrar.addController(NameWrapper.address) - await NameWrapper.setController(account, true) + await NameWrapperAdmin.addController(account) + NameWrapperProxy = NameWrapperControllerProxy.attach( + await NameWrapperAdmin.getProxyAddress(account), + ) }) it('Trying to burn child fuses when re-registering a name on the old controller reverts', async () => { - await NameWrapper.registerAndWrapETH2LD( + await NameWrapperProxy.registerAndWrapETH2LD( label1, hacker, 1 * DAY, @@ -7123,7 +7131,7 @@ describe('Name Wrapper', () => { ).to.be.revertedWith(`Unauthorised("${wrappedTokenId1}", "${hacker}")`) }) it('Renewing a wrapped, but expired name .eth in the wrapper, but unexpired on the registrar resyncs expiry', async () => { - await NameWrapper.registerAndWrapETH2LD( + await NameWrapperProxy.registerAndWrapETH2LD( label1, account, 1 * DAY, @@ -7150,7 +7158,7 @@ describe('Name Wrapper', () => { NameWrapper.address, ) - await NameWrapper.renew(labelHash1, 1) + await NameWrapperProxy.renew(labelHash1, 1) owner = await NameWrapper.ownerOf(wrappedTokenId1) let [, , expiry] = await NameWrapper.getData(wrappedTokenId1) @@ -7166,6 +7174,11 @@ describe('Name Wrapper', () => { it('Wraps a name which get stuck forever can be recovered by ROOT owner', async () => { expect(await NameWrapper.ownerOf(namehash('xyz'))).to.equal(EMPTY_ADDRESS) + await NameWrapperAdmin.addController(account) + const NameWrapperProxy = NameWrapperControllerProxy.attach( + await NameWrapperAdmin.getProxyAddress(account), + ) + await EnsRegistry.setApprovalForAll(NameWrapper.address, true) await NameWrapper.wrap(encodeName('xyz'), account, EMPTY_ADDRESS) expect(await NameWrapper.ownerOf(namehash('xyz'))).to.equal(account) From b79a4629b8041ee9a405d1026210e8df642da0d7 Mon Sep 17 00:00:00 2001 From: Nick Johnson Date: Fri, 21 Jul 2023 16:10:53 +0100 Subject: [PATCH 4/8] Add test for wrapper max renewal duration --- test/wrapper/NameWrapper.js | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/test/wrapper/NameWrapper.js b/test/wrapper/NameWrapper.js index 11d61055..3c2aabe8 100644 --- a/test/wrapper/NameWrapper.js +++ b/test/wrapper/NameWrapper.js @@ -6391,6 +6391,19 @@ describe('Name Wrapper', () => { // still expired expect(expiryAfter).to.be.at.most(block1.timestamp + GRACE_PERIOD) }) + + it('should not allow renewals of longer than 365000000 days', async () => { + await NameWrapperProxy.registerAndWrapETH2LD( + label, + account, + 86400, + EMPTY_ADDRESS, + CAN_DO_EVERYTHING, + ) + await expect( + NameWrapperProxy.renew(wrappedTokenId, 365000000 * 86400 + 1), + ).to.be.reverted + }) }) describe('isWrapped(bytes32 node)', () => { From 4d9f2ba6260b8b0065371a8660438818962359f7 Mon Sep 17 00:00:00 2001 From: Nick Johnson Date: Fri, 21 Jul 2023 16:34:20 +0100 Subject: [PATCH 5/8] Add fix for namewrapper upgrade issue --- contracts/wrapper/NameWrapperAdmin.sol | 62 +++++++++++++++++++++----- 1 file changed, 52 insertions(+), 10 deletions(-) diff --git a/contracts/wrapper/NameWrapperAdmin.sol b/contracts/wrapper/NameWrapperAdmin.sol index 1b918eab..58edd0ed 100644 --- a/contracts/wrapper/NameWrapperAdmin.sol +++ b/contracts/wrapper/NameWrapperAdmin.sol @@ -3,6 +3,9 @@ pragma solidity ^0.8.17; import "./INameWrapper.sol"; import "./Controllable.sol"; +import {INameWrapperUpgrade} from "./INameWrapperUpgrade.sol"; +import {BytesUtils} from "./BytesUtils.sol"; +import {ENS} from "../registry/ENS.sol"; import {Ownable} from "@openzeppelin/contracts/access/Ownable.sol"; import {Create2} from "@openzeppelin/contracts/utils/Create2.sol"; import {Address} from "@openzeppelin/contracts/utils/Address.sol"; @@ -70,13 +73,17 @@ contract NameWrapperControllerProxy { * This was implemented in response to a vulnerability disclosure that would permit the DAO to appoint a malicious controller * that shortens the registration period of affected ENS names. This contract exists to prevent that from happening. */ -contract NameWrapperAdmin is Ownable { +contract NameWrapperAdmin is Ownable, INameWrapperUpgrade { + using BytesUtils for bytes; using Address for address; - address public immutable wrapper; + INameWrapper public immutable wrapper; + ENS public immutable ens; + INameWrapperUpgrade public upgradeContract; constructor(address _wrapper) { - wrapper = _wrapper; + wrapper = INameWrapper(_wrapper); + ens = wrapper.ens(); } /** @@ -93,7 +100,7 @@ contract NameWrapperAdmin is Ownable { if (!proxyAddress.isContract()) { new NameWrapperControllerProxy{salt: bytes32(0)}( controller, - INameWrapper(wrapper) + wrapper ); } return proxyAddress; @@ -105,7 +112,10 @@ contract NameWrapperAdmin is Ownable { */ function addController(address controller) external onlyOwner { deployControllerProxy(controller); - Controllable(wrapper).setController(getProxyAddress(controller), true); + Controllable(address(wrapper)).setController( + getProxyAddress(controller), + true + ); } /** @@ -113,7 +123,10 @@ contract NameWrapperAdmin is Ownable { * @param controller The controller contract to deauthorize. */ function removeController(address controller) external onlyOwner { - Controllable(wrapper).setController(getProxyAddress(controller), false); + Controllable(address(wrapper)).setController( + getProxyAddress(controller), + false + ); } /** @@ -129,7 +142,7 @@ contract NameWrapperAdmin is Ownable { abi.encodePacked( type(NameWrapperControllerProxy).creationCode, uint256(uint160(controller)), - uint256(uint160(wrapper)) + uint256(uint160(address(wrapper))) ) ) ); @@ -142,7 +155,7 @@ contract NameWrapperAdmin is Ownable { function setMetadataService( IMetadataService _metadataService ) public onlyOwner { - INameWrapper(wrapper).setMetadataService(_metadataService); + wrapper.setMetadataService(_metadataService); } /** @@ -153,7 +166,36 @@ contract NameWrapperAdmin is Ownable { */ function setUpgradeContract( INameWrapperUpgrade _upgradeAddress - ) public onlyOwner { - INameWrapper(wrapper).setUpgradeContract(_upgradeAddress); + ) external onlyOwner { + if (address(upgradeContract) == address(0)) { + wrapper.setUpgradeContract(this); + } + upgradeContract = _upgradeAddress; + if (address(upgradeContract) == address(0)) { + wrapper.setUpgradeContract(INameWrapperUpgrade(address(0))); + } + } + + function wrapFromUpgrade( + bytes calldata name, + address wrappedOwner, + uint32 fuses, + uint64 expiry, + address approved, + bytes calldata extraData + ) external { + require(msg.sender == address(wrapper)); + if (fuses & IS_DOT_ETH == IS_DOT_ETH) { + bytes32 node = name.namehash(0); + ens.setOwner(node, address(upgradeContract)); + } + upgradeContract.wrapFromUpgrade( + name, + wrappedOwner, + fuses, + expiry, + approved, + extraData + ); } } From 68fac112192c69250642e139c5e72d15b95ff555 Mon Sep 17 00:00:00 2001 From: Nick Johnson Date: Mon, 24 Jul 2023 16:02:12 +0100 Subject: [PATCH 6/8] Fix NW upgrade process and tests --- contracts/wrapper/NameWrapperAdmin.sol | 13 ++- .../wrapper/mocks/UpgradedNameWrapperMock.sol | 10 +-- test/wrapper/NameWrapper.js | 89 +++---------------- 3 files changed, 29 insertions(+), 83 deletions(-) diff --git a/contracts/wrapper/NameWrapperAdmin.sol b/contracts/wrapper/NameWrapperAdmin.sol index 58edd0ed..7182bf0e 100644 --- a/contracts/wrapper/NameWrapperAdmin.sol +++ b/contracts/wrapper/NameWrapperAdmin.sol @@ -78,11 +78,13 @@ contract NameWrapperAdmin is Ownable, INameWrapperUpgrade { using Address for address; INameWrapper public immutable wrapper; + IBaseRegistrar public immutable registrar; ENS public immutable ens; INameWrapperUpgrade public upgradeContract; constructor(address _wrapper) { wrapper = INameWrapper(_wrapper); + registrar = wrapper.registrar(); ens = wrapper.ens(); } @@ -185,10 +187,17 @@ contract NameWrapperAdmin is Ownable, INameWrapperUpgrade { bytes calldata extraData ) external { require(msg.sender == address(wrapper)); + (bytes32 labelhash, uint256 offset) = name.readLabel(0); + bytes32 parentNode = name.namehash(offset); + bytes32 node = keccak256(abi.encodePacked(parentNode, labelhash)); if (fuses & IS_DOT_ETH == IS_DOT_ETH) { - bytes32 node = name.namehash(0); - ens.setOwner(node, address(upgradeContract)); + registrar.transferFrom( + address(wrapper), + address(upgradeContract), + uint256(labelhash) + ); } + ens.setOwner(node, address(upgradeContract)); upgradeContract.wrapFromUpgrade( name, wrappedOwner, diff --git a/contracts/wrapper/mocks/UpgradedNameWrapperMock.sol b/contracts/wrapper/mocks/UpgradedNameWrapperMock.sol index 6516eaa8..fe8c736e 100644 --- a/contracts/wrapper/mocks/UpgradedNameWrapperMock.sol +++ b/contracts/wrapper/mocks/UpgradedNameWrapperMock.sol @@ -43,16 +43,14 @@ contract UpgradedNameWrapperMock is INameWrapperUpgrade { if (parentNode == ETH_NODE) { address registrant = registrar.ownerOf(uint256(labelhash)); require( - msg.sender == registrant && - registrar.isApprovedForAll(registrant, address(this)), - "No approval for registrar" + registrant == address(this), + "Upgrade contract does not own name in registrar" ); } else { address owner = ens.owner(node); require( - msg.sender == owner && - ens.isApprovedForAll(owner, address(this)), - "No approval for registry" + owner == address(this), + "Upgrade contract does not own name in registry" ); } emit NameUpgraded( diff --git a/test/wrapper/NameWrapper.js b/test/wrapper/NameWrapper.js index 3c2aabe8..ad82e819 100644 --- a/test/wrapper/NameWrapper.js +++ b/test/wrapper/NameWrapper.js @@ -1905,102 +1905,38 @@ describe('Name Wrapper', () => { NameWrapperAdmin.connect(signers[2]).setUpgradeContract(account2), ).to.be.revertedWith('Ownable: caller is not the owner') }) - it('Will setApprovalForAll for the upgradeContract addresses in the registrar and registry to true', async () => { - expect( - await BaseRegistrar.isApprovedForAll( - NameWrapper.address, - NameWrapperUpgraded.address, - ), - ).to.equal(false) - expect( - await EnsRegistry.isApprovedForAll( - NameWrapper.address, - NameWrapperUpgraded.address, - ), - ).to.equal(false) - - //set the upgradeContract of the NameWrapper contract - await NameWrapperAdmin.setUpgradeContract(NameWrapperUpgraded.address) - - expect( - await BaseRegistrar.isApprovedForAll( - NameWrapper.address, - NameWrapperUpgraded.address, - ), - ).to.equal(true) - expect( - await EnsRegistry.isApprovedForAll( - NameWrapper.address, - NameWrapperUpgraded.address, - ), - ).to.equal(true) - }) - it('Will setApprovalForAll for the old upgradeContract addresses in the registrar and registry to false', async () => { - //set the upgradeContract of the NameWrapper contract - await NameWrapperAdmin.setUpgradeContract(DUMMY_ADDRESS) - - expect( - await BaseRegistrar.isApprovedForAll( - NameWrapper.address, - DUMMY_ADDRESS, - ), - ).to.equal(true) - expect( - await EnsRegistry.isApprovedForAll(NameWrapper.address, DUMMY_ADDRESS), - ).to.equal(true) - + it('Will set the admin as the upgrade contract when one is set', async () => { //set the upgradeContract of the NameWrapper contract await NameWrapperAdmin.setUpgradeContract(NameWrapperUpgraded.address) + expect(await NameWrapper.upgradeContract()).to.equal( + NameWrapperAdmin.address, + ) + expect(await NameWrapperAdmin.upgradeContract()).to.equal( + NameWrapperUpgraded.address, + ) expect( await BaseRegistrar.isApprovedForAll( NameWrapper.address, NameWrapperUpgraded.address, ), - ).to.equal(true) + ).to.equal(false) expect( await EnsRegistry.isApprovedForAll( NameWrapper.address, NameWrapperUpgraded.address, ), - ).to.equal(true) - - expect( - await BaseRegistrar.isApprovedForAll( - NameWrapper.address, - DUMMY_ADDRESS, - ), - ).to.equal(false) - expect( - await EnsRegistry.isApprovedForAll(NameWrapper.address, DUMMY_ADDRESS), ).to.equal(false) }) - it('Will not setApprovalForAll for the new upgrade address if it is the address(0)', async () => { + it('Will unset the admin as the upgrade contract when the upgrade contract is removed', async () => { //set the upgradeContract of the NameWrapper contract await NameWrapperAdmin.setUpgradeContract(NameWrapperUpgraded.address) - expect( - await BaseRegistrar.isApprovedForAll( - NameWrapper.address, - NameWrapperUpgraded.address, - ), - ).to.equal(true) - expect( - await EnsRegistry.isApprovedForAll( - NameWrapper.address, - NameWrapperUpgraded.address, - ), - ).to.equal(true) - //set the upgradeContract of the NameWrapper contract await NameWrapperAdmin.setUpgradeContract(ZERO_ADDRESS) - expect( - await BaseRegistrar.isApprovedForAll(NameWrapper.address, ZERO_ADDRESS), - ).to.equal(false) - expect( - await EnsRegistry.isApprovedForAll(NameWrapper.address, ZERO_ADDRESS), - ).to.equal(false) + expect(await NameWrapper.upgradeContract()).to.equal(ZERO_ADDRESS) + expect(await NameWrapperAdmin.upgradeContract()).to.equal(ZERO_ADDRESS) }) }) @@ -2119,6 +2055,9 @@ describe('Name Wrapper', () => { await NameWrapperAdmin.setUpgradeContract(NameWrapperUpgraded.address) expect(await NameWrapper.upgradeContract()).to.equal( + NameWrapperAdmin.address, + ) + expect(await NameWrapperAdmin.upgradeContract()).to.equal( NameWrapperUpgraded.address, ) From 0570b4fdf029be4cf8fd03294039b2f5a14a7d23 Mon Sep 17 00:00:00 2001 From: Nick Johnson Date: Wed, 26 Jul 2023 10:23:16 +0100 Subject: [PATCH 7/8] Use ETH_NODE instead of IS_DOT_ETH fuse --- contracts/wrapper/NameWrapperAdmin.sol | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/contracts/wrapper/NameWrapperAdmin.sol b/contracts/wrapper/NameWrapperAdmin.sol index 7182bf0e..a508ab56 100644 --- a/contracts/wrapper/NameWrapperAdmin.sol +++ b/contracts/wrapper/NameWrapperAdmin.sol @@ -74,6 +74,9 @@ contract NameWrapperControllerProxy { * that shortens the registration period of affected ENS names. This contract exists to prevent that from happening. */ contract NameWrapperAdmin is Ownable, INameWrapperUpgrade { + bytes32 private constant ETH_NODE = + 0x93cdeb708b7545dc668eb9280176169d1c33cfd8ed6f04690a0bcc88a93fc4ae; + using BytesUtils for bytes; using Address for address; @@ -190,7 +193,7 @@ contract NameWrapperAdmin is Ownable, INameWrapperUpgrade { (bytes32 labelhash, uint256 offset) = name.readLabel(0); bytes32 parentNode = name.namehash(offset); bytes32 node = keccak256(abi.encodePacked(parentNode, labelhash)); - if (fuses & IS_DOT_ETH == IS_DOT_ETH) { + if (parentNode == ETH_NODE) { registrar.transferFrom( address(wrapper), address(upgradeContract), From c0b23a7ad83585f28780914d1982b9b84fd3ae66 Mon Sep 17 00:00:00 2001 From: Nick Johnson Date: Tue, 1 Aug 2023 12:29:02 +0100 Subject: [PATCH 8/8] Fix typo --- contracts/ethregistrar/ETHRegistrarAdmin.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/ethregistrar/ETHRegistrarAdmin.sol b/contracts/ethregistrar/ETHRegistrarAdmin.sol index 53377138..993bea87 100644 --- a/contracts/ethregistrar/ETHRegistrarAdmin.sol +++ b/contracts/ethregistrar/ETHRegistrarAdmin.sol @@ -46,7 +46,7 @@ contract ETHRegistrarControllerProxy { } /** - * @dev Contract to act as the owner of the NameWrapper, permitting its owner to make certain changes with additional checks. + * @dev Contract to act as the owner of the ETHRegistrar, permitting its owner to make certain changes with additional checks. * This was implemented in response to a vulnerability disclosure that would permit the DAO to appoint a malicious controller * that shortens the registration period of affected ENS names. This contract exists to prevent that from happening. */