feat: add DOS testnet ENSv2 deployment - #2
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces the deployment scripts and contracts for the DOS Name Service on testnet, including a standard wrapped-native token WrappedDOS and its corresponding unit tests. The review feedback highlights a critical handoff gap where ownership of the core TLD tokens is retained by the temporary deployer instead of being transferred to the final owner, and recommends adding a zero-address validation check for the final owner during the handoff process to prevent accidental loss of protocol control.
| function _handoff(Deployment memory deployment, address initialOwner, address owner) internal { | ||
| if (owner == initialOwner) { | ||
| return; | ||
| } | ||
|
|
||
| _handoffRoles(deployment.rootRegistry, initialOwner, owner, _rootRegistryRoles()); | ||
| _handoffRoles(deployment.dosRegistry, initialOwner, owner, _dosRegistryRoles()); | ||
| _handoffRoles(deployment.reverseRegistry, initialOwner, owner, _rootRegistryRoles()); | ||
| _handoffRoles(deployment.priceOracle, initialOwner, owner, DEFAULT_ROLE_BITMAP); | ||
| _handoffRoles( | ||
| deployment.permissionedResolverImplementation, | ||
| initialOwner, | ||
| owner, | ||
| PermissionedResolverLib.ROLE_CAN_NAME | ||
| | PermissionedResolverLib.ROLE_CAN_NAME_ADMIN | ||
| ); | ||
| _handoffRoles( | ||
| deployment.userRegistryImplementation, | ||
| initialOwner, | ||
| owner, | ||
| RegistryRolesLib.ROLE_CAN_NAME | RegistryRolesLib.ROLE_CAN_NAME_ADMIN | ||
| ); | ||
|
|
||
| deployment.contractNamer.transferOwnership(owner); | ||
| deployment.gatewayProvider.transferOwnership(owner); | ||
| deployment.dosRegistrar.transferOwnership(owner); | ||
| } |
There was a problem hiding this comment.
Critical Handoff Gap: TLD Token Ownership Retained by Deployer
During the deployment phase, deploy is called with initialOwner (the temporary broadcaster) as the owner parameter. This registers the core TLDs ("dos" and "reverse" on rootRegistry, and reverseLabel on reverseRegistry) under the ownership of initialOwner, minting the corresponding ERC1155 tokens to them.
However, in _handoff, these ERC1155 tokens are never transferred to the final owner. Because these tokens are registered without RegistryRolesLib.ROLE_CAN_TRANSFER_ADMIN (and with 0 roles for reverseLabel), they are permanently non-transferable by default. This leaves the temporary deployer as the permanent owner of the core TLDs, retaining ultimate admin control and preventing the final owner from fully managing them.
Suggested Fix:
- Modify
DeployDOS.s.solto includeRegistryRolesLib.ROLE_CAN_TRANSFER_ADMINin_tldTokenRoles()and when registeringreverseLabelso that they can be transferred. - In
_handoff, transfer the tokens to the finalownerusingsafeTransferFrom:
uint256 dosTokenId = deployment.rootRegistry.getTokenId(uint256(keccak256("dos")));
uint256 reverseTokenId = deployment.rootRegistry.getTokenId(uint256(keccak256("reverse")));
deployment.rootRegistry.safeTransferFrom(initialOwner, owner, dosTokenId, 1, "");
deployment.rootRegistry.safeTransferFrom(initialOwner, owner, reverseTokenId, 1, "");
uint256 reverseLabelTokenId = deployment.reverseRegistry.getTokenId(uint256(keccak256(bytes(deployment.reverseRegistrar.reverseLabel()))));
deployment.reverseRegistry.safeTransferFrom(initialOwner, owner, reverseLabelTokenId, 1, "");| function _handoff(Deployment memory deployment, address initialOwner, address owner) internal { | ||
| if (owner == initialOwner) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
To prevent accidental loss of ownership or administrative control over the entire protocol, it is highly recommended to validate that the final owner address is not address(0) before proceeding with the handoff.
function _handoff(Deployment memory deployment, address initialOwner, address owner) internal {
require(owner != address(0), "DeployDOSTestnet: owner cannot be zero address");
if (owner == initialOwner) {
return;
}
c9a4da1 to
e78a0a7
Compare
Summary
.dosstack with WDOS payment supportVerification
forge test: 903 passed, 0 failedforge fmt --checkgit diff --checkSecurity