diff --git a/.solcover.js b/.solcover.js index 423a675ae..0c8307529 100644 --- a/.solcover.js +++ b/.solcover.js @@ -4,5 +4,3 @@ module.exports = { 'contracts/token/mocks/ZeroTokenMock.sol' ] }; -// TODO: in package.json set check-coverage script from --branches -// to equal 87 when Access Control is added! diff --git a/.solhint.json b/.solhint.json index 3fff01c20..ec9d4dd17 100644 --- a/.solhint.json +++ b/.solhint.json @@ -19,6 +19,8 @@ } ], "constructor-syntax": "warn", - "named-parameters-mapping": "error" + "named-parameters-mapping": "error", + "comprehensive-interface": "error", + "max-line-length": ["error", 120] } } \ No newline at end of file diff --git a/contracts/access/AccessControlled.sol b/contracts/access/AccessControlled.sol new file mode 100644 index 000000000..6ad462141 --- /dev/null +++ b/contracts/access/AccessControlled.sol @@ -0,0 +1,35 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.18; + +import { ZNSRoles } from "./ZNSRoles.sol"; +import { IZNSAccessController } from "./IZNSAccessController.sol"; + + +abstract contract AccessControlled is ZNSRoles { + event AccessControllerSet(address accessController); + + IZNSAccessController internal accessController; + + modifier onlyRole(bytes32 role) { + accessController.checkRole(role, msg.sender); + _; + } + + /** + * @dev This is here to make sure the external function is always implemented in children, + * otherwise we will not be able to reset the module (not ideal since it might + * not get to the final interface of a child). + * TODO AC: how do we make sure this gets to the final interface? + */ + function setAccessController(address _accessController) external virtual; + + function getAccessController() external view returns (address) { + return address(accessController); + } + + function _setAccessController(address _accessController) internal { + require(_accessController != address(0), "AC: _accessController is 0x0 address"); + accessController = IZNSAccessController(_accessController); + emit AccessControllerSet(_accessController); + } +} diff --git a/contracts/access/IZNSAccessController.sol b/contracts/access/IZNSAccessController.sol new file mode 100644 index 000000000..65cf0952c --- /dev/null +++ b/contracts/access/IZNSAccessController.sol @@ -0,0 +1,16 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.18; + +import { IAccessControlUpgradeable } from "@openzeppelin/contracts-upgradeable/access/IAccessControlUpgradeable.sol"; + + +interface IZNSAccessController is IAccessControlUpgradeable { + function initialize( + address[] calldata governorAddresses, + address[] calldata operatorAddresses + ) external; + + function checkRole(bytes32 role, address account) external view; + + function setRoleAdmin(bytes32 role, bytes32 adminRole) external; +} diff --git a/contracts/access/ZNSAccessController.sol b/contracts/access/ZNSAccessController.sol new file mode 100644 index 000000000..840457e3b --- /dev/null +++ b/contracts/access/ZNSAccessController.sol @@ -0,0 +1,45 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.18; + +import { AccessControlUpgradeable } from "@openzeppelin/contracts-upgradeable/access/AccessControlUpgradeable.sol"; +import { IZNSAccessController } from "./IZNSAccessController.sol"; +import { ZNSRoles } from "./ZNSRoles.sol"; + + +contract ZNSAccessController is AccessControlUpgradeable, ZNSRoles, IZNSAccessController { + // solhint-disable-next-line func-name-mixedcase + function initialize( + address[] calldata governorAddresses, + address[] calldata adminAddresses + ) external override initializer { + // give roles to all addresses + _grantRoleToMany(GOVERNOR_ROLE, governorAddresses); + _grantRoleToMany(ADMIN_ROLE, adminAddresses); + + // all of the governors control admins TODO AC: ??? + _setRoleAdmin(ADMIN_ROLE, GOVERNOR_ROLE); + // all of the governors control governors TODO AC: ??? + _setRoleAdmin(GOVERNOR_ROLE, GOVERNOR_ROLE); + // all of the admins control registrar TODO AC: ??? + _setRoleAdmin(REGISTRAR_ROLE, ADMIN_ROLE); + } + + // TODO AC: should we keep this function here so that we can get standardized message? + // test this function for gas usage with a standardized message vs a custom message + // when using the recommended method of `hasRole` + function checkRole(bytes32 role, address account) external view override { + _checkRole(role, account); + } + + // TODO AC: is this function necessary? how often will it be used? + function _grantRoleToMany(bytes32 role, address[] calldata addresses) internal { + for (uint256 i = 0; i < addresses.length; i++) { + _grantRole(role, addresses[i]); + } + } + + // TODO AC: how safe is this? + function setRoleAdmin(bytes32 role, bytes32 adminRole) external override onlyRole(GOVERNOR_ROLE) { + _setRoleAdmin(role, adminRole); + } +} diff --git a/contracts/access/ZNSRoles.sol b/contracts/access/ZNSRoles.sol new file mode 100644 index 000000000..4f473e96b --- /dev/null +++ b/contracts/access/ZNSRoles.sol @@ -0,0 +1,19 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.18; + + +abstract contract ZNSRoles { + // TODO AC: test getting this from AC contract vs inheriting these roles in every other contract + // the highest rank, only assigns Admins + bytes32 public constant GOVERNOR_ROLE = keccak256("GOVERNOR_ROLE"); + // the main maintainer role, that gets access to all system functions + // TODO AC: should we split responsibilities in a better way? + bytes32 public constant ADMIN_ROLE = keccak256("ADMIN_ROLE"); + // operator can be here to future proof, if we need a new role + // so we don't have to upgrade all contracts + // TODO AC: change name of this role + bytes32 public constant OPERATOR_ROLE = keccak256("OPERATOR_ROLE"); + // this role is here specifically for the ZNSEthRegistrar contract + bytes32 public constant REGISTRAR_ROLE = keccak256("REGISTRAR_ROLE"); + // TODO AC: what other roles do we need here? +} diff --git a/contracts/distribution/IZNSEthRegistrar.sol b/contracts/distribution/IZNSEthRegistrar.sol index 087a736a7..dc24bcc05 100644 --- a/contracts/distribution/IZNSEthRegistrar.sol +++ b/contracts/distribution/IZNSEthRegistrar.sol @@ -1,6 +1,7 @@ // SPDX-License-Identifier: MIT pragma solidity ^0.8.18; + interface IZNSEthRegistrar { event DomainRegistered( bytes32 indexed domainHash, @@ -17,6 +18,15 @@ interface IZNSEthRegistrar { address indexed registrant ); + // TODO AC: remove ZNS from names here and in state vars + event ZnsRegistrySet(address znsRegistry); + + event ZnsTreasurySet(address znsTreasury); + + event ZnsDomainTokenSet(address znsDomainToken); + + event ZnsAddressResolverSet(address znsAddressResolver); + function registerDomain( string calldata name, address domainContent @@ -25,4 +35,14 @@ interface IZNSEthRegistrar { function revokeDomain(bytes32 domainHash) external; function reclaimDomain(bytes32 domainHash) external; + + function setZnsRegistry(address znsRegistry_) external; + + function setZnsTreasury(address znsTreasury_) external; + + function setZnsDomainToken(address znsDomainToken_) external; + + function setZnsAddressResolver(address znsAddressResolver_) external; + + function setAccessController(address accessController_) external; } diff --git a/contracts/distribution/IZNSPriceOracle.sol b/contracts/distribution/IZNSPriceOracle.sol index 4869f15c8..4b64b978f 100644 --- a/contracts/distribution/IZNSPriceOracle.sol +++ b/contracts/distribution/IZNSPriceOracle.sol @@ -6,7 +6,6 @@ interface IZNSPriceOracle { event PriceMultiplierSet(uint256 multiplier); event BaseLengthSet(uint256 length, bool isRootDomain); event BaseLengthsSet(uint256 rootDomainLength, uint256 subdomainLength); - event ZNSRegistrarSet(address registrar); event FeePercentageSet(uint256 feePercentage); /** @@ -61,6 +60,12 @@ interface IZNSPriceOracle { uint256 priceMultiplier; } + function initialize( + address accessController_, + PriceParams calldata priceConfig_, + uint256 regFeePercentage_ + ) external; + /** * @notice Get the price of a given domain name length * @param name The name of the domain to check @@ -115,16 +120,4 @@ interface IZNSPriceOracle { * @param subdomainLength The length for subdomains */ function setBaseLengths(uint256 rootLength, uint256 subdomainLength) external; - - /** - * @notice Set the ZNSRegistrar for this contract - * @param registrar The registrar to set - */ - function setZNSRegistrar(address registrar) external; - - /** - * @notice Return true if a user is authorized, otherwise false - * @param user The user to check - */ - function isAuthorized(address user) external view returns (bool); } diff --git a/contracts/distribution/IZNSTreasury.sol b/contracts/distribution/IZNSTreasury.sol index f01d703a5..8e21823d1 100644 --- a/contracts/distribution/IZNSTreasury.sol +++ b/contracts/distribution/IZNSTreasury.sol @@ -34,11 +34,9 @@ interface IZNSTreasury { uint256 indexed amount ); - /** - * @notice Emitted when the admin user is set - * @param user The admin user to set - */ - event AdminSet(address user, bool status); + event ZnsPriceOracleSet(address znsPriceOracle); + + event ZnsStakingTokenSet(address znsStakingToken); event ZeroVaultAddressSet(address zeroVault); @@ -51,9 +49,9 @@ interface IZNSTreasury { function unstakeForDomain(bytes32 domainHash, address owner) external; - function setZNSRegistrar(address znsRegistrar_) external; - function setZeroVaultAddress(address zeroVaultAddress) external; - function setAdmin(address user, bool status) external; + function setPriceOracle(address znsPriceOracle_) external; + + function setStakingToken(address stakingToken_) external; } diff --git a/contracts/distribution/ZNSEthRegistrar.sol b/contracts/distribution/ZNSEthRegistrar.sol index 35bb2ff95..fef82b86f 100644 --- a/contracts/distribution/ZNSEthRegistrar.sol +++ b/contracts/distribution/ZNSEthRegistrar.sol @@ -6,24 +6,28 @@ import { IZNSRegistry } from "../registry/IZNSRegistry.sol"; import { IZNSTreasury } from "./IZNSTreasury.sol"; import { IZNSDomainToken } from "../token/IZNSDomainToken.sol"; import { IZNSAddressResolver } from "../resolver/IZNSAddressResolver.sol"; -import { IZNSPriceOracle } from "./IZNSPriceOracle.sol"; +import { AccessControlled } from "../access/AccessControlled.sol"; -contract ZNSEthRegistrar is IZNSEthRegistrar { +contract ZNSEthRegistrar is AccessControlled, IZNSEthRegistrar { IZNSRegistry public znsRegistry; IZNSTreasury public znsTreasury; IZNSDomainToken public znsDomainToken; IZNSAddressResolver public znsAddressResolver; - IZNSPriceOracle public znsPriceOracle; - mapping(bytes32 parentDomainHash => mapping(address user => bool status)) - public subdomainApprovals; - - modifier onlyOwner(bytes32 domainHash) { + modifier onlyNameOwner(bytes32 domainHash) { require( msg.sender == znsRegistry.getDomainOwner(domainHash), - "ZNSEthRegistrar: Not the Domain Owner" + "ZNSEthRegistrar: Not the Owner of the Name" + ); + _; + } + + modifier onlyTokenOwner(bytes32 domainHash) { + require( + msg.sender == znsDomainToken.ownerOf(uint256(domainHash)), + "ZNSEthRegistrar: Not the owner of the Token" ); _; } @@ -33,17 +37,18 @@ contract ZNSEthRegistrar is IZNSEthRegistrar { * for registering ZNS domains and subdomains */ constructor( - IZNSRegistry znsRegistry_, - IZNSTreasury znsTreasury_, - IZNSDomainToken znsDomainToken_, - IZNSAddressResolver znsAddressResolver_, - IZNSPriceOracle znsPriceOracle_ + address accessController_, + address znsRegistry_, + address znsTreasury_, + address znsDomainToken_, + address znsAddressResolver_ ) { - znsRegistry = znsRegistry_; - znsTreasury = znsTreasury_; - znsDomainToken = znsDomainToken_; - znsAddressResolver = znsAddressResolver_; - znsPriceOracle = znsPriceOracle_; + _setAccessController(accessController_); + // TODO AC: should we call protected functions in the constructor/initialize? + setZnsRegistry(znsRegistry_); + setZnsTreasury(znsTreasury_); + setZnsDomainToken(znsDomainToken_); + setZnsAddressResolver(znsAddressResolver_); } /** @@ -55,7 +60,7 @@ contract ZNSEthRegistrar is IZNSEthRegistrar { function registerDomain( string calldata name, address resolverContent - ) external returns (bytes32) { + ) external override returns (bytes32) { require( bytes(name).length != 0, "ZNSEthRegistrar: Domain Name not provided" @@ -78,14 +83,26 @@ contract ZNSEthRegistrar is IZNSEthRegistrar { _setDomainData(domainHash, msg.sender, resolverContent); - emit DomainRegistered(domainHash, tokenId, name, msg.sender, address(znsAddressResolver)); + emit DomainRegistered( + domainHash, + tokenId, + name, + msg.sender, + address(znsAddressResolver) + ); return domainHash; } - function revokeDomain( - bytes32 domainHash - ) external onlyOwner(domainHash) { + function revokeDomain(bytes32 domainHash) + external + override + // TODO: figure out how to guard this so people can stake tokens + // without the risk of staking contract or wallet to call reclaim+revoke + // from underneath them + onlyNameOwner(domainHash) + onlyTokenOwner(domainHash) + { uint256 tokenId = uint256(domainHash); znsDomainToken.revoke(tokenId); znsTreasury.unstakeForDomain(domainHash, msg.sender); @@ -94,18 +111,64 @@ contract ZNSEthRegistrar is IZNSEthRegistrar { emit DomainRevoked(domainHash, msg.sender); } - // TODO: Access Control - function reclaimDomain(bytes32 domainHash) external { - uint256 tokenId = uint256(domainHash); - require( - znsDomainToken.ownerOf(tokenId) == msg.sender, - "ZNSEthRegistrar: Not owner of Token" - ); + function reclaimDomain(bytes32 domainHash) + external + override + onlyTokenOwner(domainHash) + { znsRegistry.updateDomainOwner(domainHash, msg.sender); emit DomainReclaimed(domainHash, msg.sender); } + function setZnsRegistry(address znsRegistry_) public override onlyRole(ADMIN_ROLE) { + require( + znsRegistry_ != address(0), + "ZNSEthRegistrar: znsRegistry_ is 0x0 address" + ); + znsRegistry = IZNSRegistry(znsRegistry_); + + emit ZnsRegistrySet(znsRegistry_); + } + + function setZnsTreasury(address znsTreasury_) public override onlyRole(ADMIN_ROLE) { + require( + znsTreasury_ != address(0), + "ZNSEthRegistrar: znsTreasury_ is 0x0 address" + ); + znsTreasury = IZNSTreasury(znsTreasury_); + + emit ZnsTreasurySet(znsTreasury_); + } + + function setZnsDomainToken(address znsDomainToken_) public override onlyRole(ADMIN_ROLE) { + require( + znsDomainToken_ != address(0), + "ZNSEthRegistrar: znsDomainToken_ is 0x0 address" + ); + znsDomainToken = IZNSDomainToken(znsDomainToken_); + + emit ZnsDomainTokenSet(znsDomainToken_); + } + + function setZnsAddressResolver(address znsAddressResolver_) public override onlyRole(ADMIN_ROLE) { + require( + znsAddressResolver_ != address(0), + "ZNSEthRegistrar: znsAddressResolver_ is 0x0 address" + ); + znsAddressResolver = IZNSAddressResolver(znsAddressResolver_); + + emit ZnsAddressResolverSet(znsAddressResolver_); + } + + function setAccessController(address accessController_) + external + override(AccessControlled, IZNSEthRegistrar) + onlyRole(ADMIN_ROLE) + { + _setAccessController(accessController_); + } + /** * @notice Set domain data appropriately for a newly registered domain * diff --git a/contracts/distribution/ZNSPriceOracle.sol b/contracts/distribution/ZNSPriceOracle.sol index 564102bef..64b9769ee 100644 --- a/contracts/distribution/ZNSPriceOracle.sol +++ b/contracts/distribution/ZNSPriceOracle.sol @@ -5,8 +5,9 @@ import { IERC20Upgradeable } from "@openzeppelin/contracts-upgradeable/token/ERC import { Initializable } from "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol"; import { IZNSPriceOracle } from "./IZNSPriceOracle.sol"; import { StringUtils } from "../utils/StringUtils.sol"; +import { AccessControlled } from "../access/AccessControlled.sol"; -contract ZNSPriceOracle is IZNSPriceOracle, Initializable { +contract ZNSPriceOracle is AccessControlled, IZNSPriceOracle, Initializable { using StringUtils for string; uint256 public constant PERCENTAGE_BASIS = 10000; @@ -21,40 +22,19 @@ contract ZNSPriceOracle is IZNSPriceOracle, Initializable { /** * @notice Struct for each configurable price variable */ + // TODO: rework and add more setters for every single var PriceParams public priceConfig; - /** - * @notice The address of the ZNS Registrar we are using - */ - address public znsRegistrar; - - /** - * @notice Track authorized users or contracts - * TODO access control for the entire system - */ - mapping(address user => bool isAuthorized) public authorized; - - /** - * @notice Restrict a function to only be callable by authorized users - */ - modifier onlyAuthorized() { - require(authorized[msg.sender], "ZNS: Not authorized"); - _; - } function initialize( + address accessController_, PriceParams calldata priceConfig_, - address znsRegistrar_, // TODO do we need to keep this here if we always set it through the setter? uint256 regFeePercentage_ - ) public initializer { + ) public override initializer { // Set pricing and length parameters priceConfig = priceConfig_; feePercentage = regFeePercentage_; - - // Set the user and registrar we allow to modify prices - znsRegistrar = znsRegistrar_; - authorized[msg.sender] = true; - authorized[znsRegistrar_] = true; + _setAccessController(accessController_); } /** @@ -65,7 +45,7 @@ contract ZNSPriceOracle is IZNSPriceOracle, Initializable { function getPrice( string calldata name, bool isRootDomain - ) external view returns ( + ) external view override returns ( uint256 totalPrice, uint256 domainPrice, uint256 fee @@ -96,7 +76,7 @@ contract ZNSPriceOracle is IZNSPriceOracle, Initializable { totalPrice = domainPrice + fee; } - function getRegistrationFee(uint256 domainPrice) public view returns (uint256) { + function getRegistrationFee(uint256 domainPrice) public view override returns (uint256) { return (domainPrice * feePercentage) / PERCENTAGE_BASIS; } @@ -110,7 +90,7 @@ contract ZNSPriceOracle is IZNSPriceOracle, Initializable { function setMaxPrice( uint256 maxPrice, bool isRootDomain - ) external onlyAuthorized { + ) external override onlyRole(ADMIN_ROLE) { if (isRootDomain) { priceConfig.maxRootDomainPrice = maxPrice; } else { @@ -133,7 +113,7 @@ contract ZNSPriceOracle is IZNSPriceOracle, Initializable { * to make up for this. * @param multiplier The new price multiplier to set */ - function setPriceMultiplier(uint256 multiplier) external onlyAuthorized { + function setPriceMultiplier(uint256 multiplier) external override onlyRole(ADMIN_ROLE) { require( multiplier >= 300 && multiplier <= 400, "ZNS: Multiplier out of range" @@ -143,7 +123,7 @@ contract ZNSPriceOracle is IZNSPriceOracle, Initializable { emit PriceMultiplierSet(multiplier); } - function setRegistrationFeePercentage(uint256 regFeePercentage) external onlyAuthorized { + function setRegistrationFeePercentage(uint256 regFeePercentage) external override onlyRole(ADMIN_ROLE) { feePercentage = regFeePercentage; emit FeePercentageSet(regFeePercentage); } @@ -158,7 +138,7 @@ contract ZNSPriceOracle is IZNSPriceOracle, Initializable { function setBaseLength( uint256 length, bool isRootDomain - ) external onlyAuthorized { + ) external override onlyRole(ADMIN_ROLE) { if (isRootDomain) { priceConfig.baseRootDomainLength = length; } else { @@ -176,34 +156,15 @@ contract ZNSPriceOracle is IZNSPriceOracle, Initializable { function setBaseLengths( uint256 rootLength, uint256 subdomainLength - ) external onlyAuthorized { + ) external override onlyRole(ADMIN_ROLE) { priceConfig.baseRootDomainLength = rootLength; priceConfig.baseSubdomainLength = subdomainLength; emit BaseLengthsSet(rootLength, subdomainLength); } - /** - * @notice Set the ZNSRegistrar for this contract - * @param registrar The registrar to set - */ - function setZNSRegistrar(address registrar) external onlyAuthorized { - require(registrar != address(0), "ZNS: Zero address for Registrar"); - - // Modify the access control for the new registrar - authorized[znsRegistrar] = false; - authorized[registrar] = true; - znsRegistrar = registrar; - - emit ZNSRegistrarSet(registrar); - } - - /** - * @notice Return true if a user is authorized, otherwise false - * @param user The user to check - */ - function isAuthorized(address user) external view returns (bool) { - return authorized[user]; + function setAccessController(address accessController) external override onlyRole(ADMIN_ROLE) { + _setAccessController(accessController); } /** diff --git a/contracts/distribution/ZNSTreasury.sol b/contracts/distribution/ZNSTreasury.sol index 67d822c56..a1b6738c5 100644 --- a/contracts/distribution/ZNSTreasury.sol +++ b/contracts/distribution/ZNSTreasury.sol @@ -2,27 +2,27 @@ pragma solidity ^0.8.18; import { IZNSTreasury } from "./IZNSTreasury.sol"; -import { IZNSEthRegistrar } from "./IZNSEthRegistrar.sol"; import { IZNSPriceOracle } from "./IZNSPriceOracle.sol"; -// TODO: fix when token is sorted out -import { IZeroTokenMock } from "../token/mocks/IZeroTokenMock.sol"; +import { AccessControlled } from "../access/AccessControlled.sol"; +import { IERC20 } from "@openzeppelin/contracts/token/ERC20/IERC20.sol"; -contract ZNSTreasury is IZNSTreasury { - /** - * @notice The address of the registrar we are using - */ - address public znsRegistrar; - +contract ZNSTreasury is AccessControlled, IZNSTreasury { /** * @notice The price oracle */ IZNSPriceOracle public znsPriceOracle; /** - * @notice The ZERO ERC20 token + * @notice The payment/staking token */ - IZeroTokenMock public zeroToken; + // TODO: this should be changed to be more general + // we might not use ZERO, but any other token here + // so change the naming and change the interface for IERC20, + // instead of a specific ZERO token interface! + // Make sure it is general on all contracts where it's present! + // TODO: change all transfer calls to safeTransfer! + IERC20 public stakingToken; /** * @notice Address of the Zero Vault, a wallet or contract which gathers all the fees. @@ -31,35 +31,18 @@ contract ZNSTreasury is IZNSTreasury { mapping(bytes32 domainHash => uint256 amountStaked) public stakedForDomain; - // TODO access control - mapping(address user => bool isAdmin) public admin; - - modifier onlyRegistrar() { - require( - msg.sender == znsRegistrar, - "ZNSTreasury: Only ZNSRegistrar is allowed to call" - ); - _; - } - - modifier onlyAdmin() { - require(admin[msg.sender], "ZNSTreasury: Not an allowed admin"); - _; - } constructor( - IZNSPriceOracle znsPriceOracle_, - IZeroTokenMock zeroToken_, - address znsRegistrar_, - address admin_, // TODO remove when proper access control is added, + address accessController_, + address znsPriceOracle_, + address stakingToken_, address zeroVault_ ) { + _setAccessController(accessController_); _setZeroVaultAddress(zeroVault_); // TODO change from mock - zeroToken = zeroToken_; - znsPriceOracle = znsPriceOracle_; - znsRegistrar = znsRegistrar_; - admin[admin_] = true; + setStakingToken(stakingToken_); + setPriceOracle(znsPriceOracle_); } function stakeForDomain( @@ -67,18 +50,18 @@ contract ZNSTreasury is IZNSTreasury { string calldata domainName, address depositor, bool isTopLevelDomain - ) external onlyRegistrar { + ) external override onlyRole(REGISTRAR_ROLE) { // Get price and fee for the domain (, uint256 stakeAmount, uint256 registrationFee) = znsPriceOracle.getPrice( domainName, isTopLevelDomain ); - // Transfer stake amount and fee - zeroToken.transferFrom(depositor, address(this), stakeAmount); - // TODO make sure we show the approval process to the user here to avoid failed transfer - // TODO can we make it so it needs a single approval only?! - zeroToken.transferFrom(depositor, zeroVault, registrationFee); + // Transfer stake amount and fee + stakingToken.transferFrom(depositor, address(this), stakeAmount); + // TODO make sure we show the approval process to the user here to avoid failed transfer + // TODO can we make it so it needs a single approval only?! + stakingToken.transferFrom(depositor, zeroVault, registrationFee); // Record staked amount for this domain stakedForDomain[domainHash] = stakeAmount; @@ -89,32 +72,40 @@ contract ZNSTreasury is IZNSTreasury { function unstakeForDomain( bytes32 domainHash, address owner - ) external onlyRegistrar { + ) external override onlyRole(REGISTRAR_ROLE) { uint256 stakeAmount = stakedForDomain[domainHash]; require(stakeAmount > 0, "ZNSTreasury: No stake for domain"); delete stakedForDomain[domainHash]; - // TODO: require owner == ownerOrOperator from registry? - // remove this comment when AccessControl is added. - // if proper acccess control exists here and in Registrar.revoke - // it will be sufficient to check the owner at the entry point - zeroToken.transfer(owner, stakeAmount); + stakingToken.transfer(owner, stakeAmount); emit StakeWithdrawn(domainHash, owner, stakeAmount); } - function setZNSRegistrar(address znsRegistrar_) external onlyAdmin { + function setZeroVaultAddress(address zeroVaultAddress) external override onlyRole(ADMIN_ROLE) { + _setZeroVaultAddress(zeroVaultAddress); + } + + // TODO AC: should we call a protected function in the constructor/initialize? + function setPriceOracle(address znsPriceOracle_) public override onlyRole(ADMIN_ROLE) { require( - znsRegistrar_ != address(0), - "ZNSTreasury: Zero address passed as znsRegistrar" + znsPriceOracle_ != address(0), + "ZNSTreasury: znsPriceOracle_ passed as 0x0 address" ); - znsRegistrar = znsRegistrar_; - emit ZNSRegistrarSet(znsRegistrar_); + znsPriceOracle = IZNSPriceOracle(znsPriceOracle_); + emit ZnsPriceOracleSet(znsPriceOracle_); } - function setZeroVaultAddress(address zeroVaultAddress) external onlyAdmin { - _setZeroVaultAddress(zeroVaultAddress); + function setStakingToken(address stakingToken_) public override onlyRole(ADMIN_ROLE) { + require(stakingToken_ != address(0), "ZNSTreasury: stakingToken_ passed as 0x0 address"); + + stakingToken = IERC20(stakingToken_); + emit ZnsStakingTokenSet(stakingToken_); + } + + function setAccessController(address accessController_) external override onlyRole(ADMIN_ROLE) { + _setAccessController(accessController_); } function _setZeroVaultAddress(address zeroVaultAddress) internal { @@ -123,26 +114,4 @@ contract ZNSTreasury is IZNSTreasury { zeroVault = zeroVaultAddress; emit ZeroVaultAddressSet(zeroVaultAddress); } - - function setAdmin(address user, bool status) external onlyAdmin { - require(user != address(0), "ZNSTreasury: No zero address admins"); - - // If a user is given Admin status, they can remove any other admin's status as well - // To protect against this, we require that the user is the sender if setting - // status to `false` - if (status == false) { - require( - msg.sender == user, - "ZNSTreasury: Cannot unset another users admin access" - ); - } - - admin[user] = status; - - emit AdminSet(user, status); - } - - function isAdmin(address user) external view returns (bool) { - return admin[user]; - } } diff --git a/contracts/registry/IZNSRegistry.sol b/contracts/registry/IZNSRegistry.sol index 25d52e2cb..78f85588e 100644 --- a/contracts/registry/IZNSRegistry.sol +++ b/contracts/registry/IZNSRegistry.sol @@ -50,6 +50,8 @@ interface IZNSRegistry { bool allowed ); + function initialize(address owner) external; + /** * @notice Emit when a new ZNSRegistrar address is set * @param znsRegistrar The new address diff --git a/contracts/registry/ZNSRegistry.sol b/contracts/registry/ZNSRegistry.sol index 0db091286..47e13c002 100644 --- a/contracts/registry/ZNSRegistry.sol +++ b/contracts/registry/ZNSRegistry.sol @@ -1,7 +1,8 @@ // SPDX-License-Identifier: MIT pragma solidity ^0.8.18; -import { ERC1967UpgradeUpgradeable } from "@openzeppelin/contracts-upgradeable/proxy/ERC1967/ERC1967UpgradeUpgradeable.sol"; +import { ERC1967UpgradeUpgradeable } +from "@openzeppelin/contracts-upgradeable/proxy/ERC1967/ERC1967UpgradeUpgradeable.sol"; import { Initializable } from "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol"; import { IZNSRegistry } from "./IZNSRegistry.sol"; @@ -14,8 +15,7 @@ contract ZNSRegistry is IZNSRegistry, ERC1967UpgradeUpgradeable { * @notice Mapping `domainHash` to `DomainRecord` struct to hold information * about each domain */ - mapping(bytes32 domainHash => DomainRecord domainRecord) - private records; + mapping(bytes32 domainHash => DomainRecord domainRecord) private records; /** * @notice Mapping of `owner` => `operator` => `bool` to show accounts that @@ -52,7 +52,7 @@ contract ZNSRegistry is IZNSRegistry, ERC1967UpgradeUpgradeable { * Initialize the ZNSRegistry contract, setting the owner of the `0x0` domain * to be the account that deploys this contract */ - function initialize(address znsRegistrar_) public initializer { + function initialize(address znsRegistrar_) public override initializer { require( znsRegistrar_ != address(0), "ZNSRegistry: Registrar can not be 0x0 address" @@ -65,7 +65,7 @@ contract ZNSRegistry is IZNSRegistry, ERC1967UpgradeUpgradeable { * * @param domainHash The hash of a domain's name */ - function exists(bytes32 domainHash) external view returns (bool) { + function exists(bytes32 domainHash) external view override returns (bool) { return _exists(domainHash); } @@ -78,7 +78,7 @@ contract ZNSRegistry is IZNSRegistry, ERC1967UpgradeUpgradeable { function isOwnerOrOperator( bytes32 domainHash, address candidate - ) public view returns (bool) { + ) public view override returns (bool) { address owner = records[domainHash].owner; return candidate == owner || operators[owner][candidate]; } @@ -88,7 +88,7 @@ contract ZNSRegistry is IZNSRegistry, ERC1967UpgradeUpgradeable { * * @param znsRegistrar_ The new ZNSRegistrar */ - function setZNSRegistrar(address znsRegistrar_) external { + function setZNSRegistrar(address znsRegistrar_) external override { // TODO When we have access control, only be callable by admin!! require( znsRegistrar_ != address(0), @@ -107,7 +107,8 @@ contract ZNSRegistry is IZNSRegistry, ERC1967UpgradeUpgradeable { * @param operator The account to allow/disallow * @param allowed The true/false value to set */ - function setOwnerOperator(address operator, bool allowed) external { + // TODO AC: add access control + function setOwnerOperator(address operator, bool allowed) external override { operators[msg.sender][operator] = allowed; emit OperatorPermissionSet(msg.sender, operator, allowed); @@ -120,7 +121,7 @@ contract ZNSRegistry is IZNSRegistry, ERC1967UpgradeUpgradeable { */ function getDomainRecord( bytes32 domainHash - ) external view returns (DomainRecord memory) { + ) external view override returns (DomainRecord memory) { return records[domainHash]; } @@ -131,7 +132,7 @@ contract ZNSRegistry is IZNSRegistry, ERC1967UpgradeUpgradeable { */ function getDomainOwner( bytes32 domainHash - ) external view returns (address) { + ) external view override returns (address) { return records[domainHash].owner; } @@ -142,7 +143,7 @@ contract ZNSRegistry is IZNSRegistry, ERC1967UpgradeUpgradeable { */ function getDomainResolver( bytes32 domainHash - ) external view returns (address) { + ) external view override returns (address) { return records[domainHash].resolver; } @@ -157,7 +158,7 @@ contract ZNSRegistry is IZNSRegistry, ERC1967UpgradeUpgradeable { bytes32 domainHash, address owner, address resolver - ) external onlyRegistrar { + ) external override onlyRegistrar { _setDomainOwner(domainHash, owner); // We allow creation of partial domains with no resolver address @@ -178,7 +179,7 @@ contract ZNSRegistry is IZNSRegistry, ERC1967UpgradeUpgradeable { address owner, address resolver // TODO AC: make this so only owner can change the owner and not the operator! - ) external onlyOwnerOrOperator(domainHash) { + ) external override onlyOwnerOrOperator(domainHash) { // `exists` is checked implicitly through the modifier _setDomainOwner(domainHash, owner); _setDomainResolver(domainHash, resolver); @@ -194,7 +195,7 @@ contract ZNSRegistry is IZNSRegistry, ERC1967UpgradeUpgradeable { bytes32 domainHash, address owner // TODO AC: make this so only owner can change the owner and not the operator! - ) external onlyOwnerOrOperator(domainHash) { + ) external override onlyOwnerOrOperator(domainHash) { // `exists` is checked implicitly through the modifier _setDomainOwner(domainHash, owner); } @@ -208,7 +209,7 @@ contract ZNSRegistry is IZNSRegistry, ERC1967UpgradeUpgradeable { function updateDomainResolver( bytes32 domainHash, address resolver - ) external onlyOwnerOrOperator(domainHash) { + ) external override onlyOwnerOrOperator(domainHash) { // `exists` is checked implicitly through the modifier _setDomainResolver(domainHash, resolver); } @@ -218,7 +219,7 @@ contract ZNSRegistry is IZNSRegistry, ERC1967UpgradeUpgradeable { * * @param domainHash The hash of the domain name */ - function deleteRecord(bytes32 domainHash) external onlyRegistrar { + function deleteRecord(bytes32 domainHash) external override onlyRegistrar { delete records[domainHash]; emit DomainRecordDeleted(domainHash); diff --git a/contracts/resolver/IZNSAddressResolver.sol b/contracts/resolver/IZNSAddressResolver.sol index 183d88b8d..119568b75 100644 --- a/contracts/resolver/IZNSAddressResolver.sol +++ b/contracts/resolver/IZNSAddressResolver.sol @@ -31,4 +31,6 @@ interface IZNSAddressResolver { bytes32 domainHash, address newAddress ) external; + + function getInterfaceId() external pure returns (bytes4); } diff --git a/contracts/resolver/ZNSAddressResolver.sol b/contracts/resolver/ZNSAddressResolver.sol index 00d1d5e65..01d6d82ab 100644 --- a/contracts/resolver/ZNSAddressResolver.sol +++ b/contracts/resolver/ZNSAddressResolver.sol @@ -26,7 +26,7 @@ contract ZNSAddressResolver is ERC165, IZNSAddressResolver { * @dev Revert if `msg.sender` is not the owner or an operator allowed by the owner * @param domainHash The identifying hash of a domain's name */ - // TODO: Remove this when doing access control. + // TODO AC: Remove this when doing access control (or adapt to work the best way here). // A function like that can be created in Registry, but think // deeper if we want this to be for owner in Registry or owner of the Token in DomainToken! modifier onlyOwnerOrOperator(bytes32 domainHash) { @@ -43,7 +43,7 @@ contract ZNSAddressResolver is ERC165, IZNSAddressResolver { */ function getAddress( bytes32 domainHash - ) external view returns (address) { + ) external view override returns (address) { return addressOf[domainHash]; } @@ -55,7 +55,7 @@ contract ZNSAddressResolver is ERC165, IZNSAddressResolver { function setAddress( bytes32 domainHash, address newAddress - ) external onlyOwnerOrOperator(domainHash) { + ) external override onlyOwnerOrOperator(domainHash) { require(newAddress != address(0), "ZNS: Cant set address to 0"); addressOf[domainHash] = newAddress; @@ -79,7 +79,7 @@ contract ZNSAddressResolver is ERC165, IZNSAddressResolver { /** * @dev Exposes IZNSAddressResolver interfaceId */ - function getInterfaceId() public pure returns (bytes4) { - return type(IZNSAddressResolver).interfaceId; + function getInterfaceId() public pure override returns (bytes4) { + return type(IZNSAddressResolver).interfaceId; } } diff --git a/contracts/token/IZNSDomainToken.sol b/contracts/token/IZNSDomainToken.sol index b613651ec..b4d466f66 100644 --- a/contracts/token/IZNSDomainToken.sol +++ b/contracts/token/IZNSDomainToken.sol @@ -9,5 +9,5 @@ interface IZNSDomainToken is IERC721{ function revoke(uint256 tokenId) external; - function authorize( address account) external; + function authorize(address account) external; } diff --git a/contracts/token/ZNSDomainToken.sol b/contracts/token/ZNSDomainToken.sol index 461c61479..f00d30d6d 100644 --- a/contracts/token/ZNSDomainToken.sol +++ b/contracts/token/ZNSDomainToken.sol @@ -8,7 +8,6 @@ import { IZNSDomainToken } from "./IZNSDomainToken.sol"; * @title A contract for tokenizing domains under the ZNS Architecture */ contract ZNSDomainToken is ERC721, IZNSDomainToken { - // TODO: change for proper name ! constructor(string memory tokenName, string memory tokenSymbol) ERC721(tokenName, tokenSymbol) { authorized[msg.sender] = true; } @@ -31,7 +30,7 @@ contract ZNSDomainToken is ERC721, IZNSDomainToken { * @notice Authorize an address for this contract * @param account The registrar to set */ - function authorize(address account) external onlyAuthorized { + function authorize(address account) external override onlyAuthorized { require(account != address(0), "ZNS: Zero address for authorized account"); // Modify the access control for the given address @@ -46,7 +45,7 @@ contract ZNSDomainToken is ERC721, IZNSDomainToken { * @param to The address that will recieve the newly minted domain token * @param tokenId The TokenId that the caller wishes to mint/register */ - function register(address to, uint256 tokenId) external { + function register(address to, uint256 tokenId) external override { _safeMint(to, tokenId); } @@ -55,7 +54,7 @@ contract ZNSDomainToken is ERC721, IZNSDomainToken { * @dev TODO: Add Access Control, replace require to also other specific contracts to revoke * @param tokenId The tokenId that the caller wishes to burn/revoke */ - function revoke(uint256 tokenId) external onlyAuthorized{ + function revoke(uint256 tokenId) external override onlyAuthorized { _burn(tokenId); } } diff --git a/contracts/token/mocks/ZeroTokenMock.sol b/contracts/token/mocks/ZeroTokenMock.sol index 5b704008c..7108e01b5 100644 --- a/contracts/token/mocks/ZeroTokenMock.sol +++ b/contracts/token/mocks/ZeroTokenMock.sol @@ -18,7 +18,7 @@ contract ZeroTokenMock is ERC20, IZeroTokenMock { return super.balanceOf(user); } - function burn(address account, uint256 amount) external { - _burn(account, amount); + function burn(address account, uint256 amount) external override { + _burn(account, amount); } } diff --git a/package.json b/package.json index f1c975ef5..de9e1fc65 100644 --- a/package.json +++ b/package.json @@ -20,7 +20,7 @@ "test": "hardhat test", "semantic-release": "semantic-release --tag-format='v${version}-dev'", "coverage": "hardhat coverage", - "check-coverage": "istanbul check-coverage --statements 90 --branches 78 --functions 89 --lines 90" + "check-coverage": "istanbul check-coverage --statements 90 --branches 87 --functions 89 --lines 90" }, "pre-commit": [ "lint" diff --git a/test/ZNSAccessController.test.ts b/test/ZNSAccessController.test.ts new file mode 100644 index 000000000..33b11cb99 --- /dev/null +++ b/test/ZNSAccessController.test.ts @@ -0,0 +1,170 @@ +import * as hre from "hardhat"; +import { SignerWithAddress } from "@nomiclabs/hardhat-ethers/signers"; +import { ZNSAccessController } from "../typechain"; +import { deployAccessController } from "./helpers"; +import { expect } from "chai"; +import { ADMIN_ROLE, getAccessRevertMsg, GOVERNOR_ROLE, OPERATOR_ROLE, REGISTRAR_ROLE } from "./helpers/access"; + + +describe("ZNSAccessController", () => { + let deployer : SignerWithAddress; + let znsAccessController : ZNSAccessController; + let governorAccs : Array<SignerWithAddress>; + let adminAccs : Array<SignerWithAddress>; + let randomAccs : Array<SignerWithAddress>; + + beforeEach(async () => { + const accounts = await hre.ethers.getSigners(); + deployer = accounts[0]; + governorAccs = accounts.slice(1, 4); + adminAccs = accounts.slice(4, 7); + randomAccs = accounts.slice(7, 10); + + znsAccessController = await deployAccessController({ + deployer, + governorAddresses: governorAccs.map(acc => acc.address), + adminAddresses: adminAccs.map(acc => acc.address), + }); + }); + + describe("Initial Setup", () => { + it("Should assign GOVERNORs correctly", async () => { + await governorAccs.reduce( + async (acc, { address } : SignerWithAddress) => { + const hasRole = await znsAccessController.hasRole(GOVERNOR_ROLE, address); + expect(hasRole).to.be.true; + }, Promise.resolve() + ); + }); + + it("Should assign ADMINs correctly", async () => { + await adminAccs.reduce( + async (acc, { address } : SignerWithAddress) => { + const hasRole = await znsAccessController.hasRole(ADMIN_ROLE, address); + expect(hasRole).to.be.true; + }, Promise.resolve() + ); + }); + }); + + describe("Role Management from the Initial Setup", () => { + it("GOVERNOR_ROLE should be able to grant GOVERNOR_ROLE", async () => { + const [ governor ] = governorAccs; + const [ { address: newGovernor } ] = randomAccs; + await znsAccessController.connect(governor).grantRole(GOVERNOR_ROLE, newGovernor); + + const hasRole = await znsAccessController.hasRole(GOVERNOR_ROLE, newGovernor); + expect(hasRole).to.be.true; + }); + + it("GOVERNOR_ROLE should be able to revoke GOVERNOR_ROLE", async () => { + const [ governor ] = governorAccs; + const [ { address: existingGovernor } ] = governorAccs; + await znsAccessController.connect(governor).revokeRole(GOVERNOR_ROLE, existingGovernor); + + const hasRole = await znsAccessController.hasRole(GOVERNOR_ROLE, existingGovernor); + expect(hasRole).to.be.false; + }); + + it("GOVERNOR_ROLE should be able to grant ADMIN_ROLE", async () => { + const [ governor ] = governorAccs; + const [ { address: newAdmin } ] = randomAccs; + await znsAccessController.connect(governor).grantRole(ADMIN_ROLE, newAdmin); + + const hasRole = await znsAccessController.hasRole(ADMIN_ROLE, newAdmin); + expect(hasRole).to.be.true; + }); + + it("GOVERNOR_ROLE should be able to revoke ADMIN_ROLE", async () => { + const [ governor ] = governorAccs; + const [ { address: existingAdmin } ] = adminAccs; + await znsAccessController.connect(governor).revokeRole(ADMIN_ROLE, existingAdmin); + + const hasRole = await znsAccessController.hasRole(ADMIN_ROLE, existingAdmin); + expect(hasRole).to.be.false; + }); + + it("ADMIN_ROLE should NOT be able to grant ADMIN_ROLE", async () => { + const [ admin ] = adminAccs; + const [ { address: newAdmin } ] = randomAccs; + await expect( + znsAccessController.connect(admin).grantRole(ADMIN_ROLE, newAdmin) + ).to.be.revertedWith( + getAccessRevertMsg(admin.address, GOVERNOR_ROLE) + ); + }); + + it("ADMIN_ROLE should NOT be able to revoke ADMIN_ROLE", async () => { + const [ admin ] = adminAccs; + const [ { address: existingAdmin } ] = adminAccs; + await expect( + znsAccessController.connect(admin).revokeRole(ADMIN_ROLE, existingAdmin) + ).to.be.revertedWith( + getAccessRevertMsg(admin.address, GOVERNOR_ROLE) + ); + }); + + it("ADMIN_ROLE should NOT be able to grant GOVERNOR_ROLE", async () => { + const [ admin ] = adminAccs; + const [ { address: newGovernor } ] = randomAccs; + await expect( + znsAccessController.connect(admin).grantRole(GOVERNOR_ROLE, newGovernor) + ).to.be.revertedWith( + getAccessRevertMsg(admin.address, GOVERNOR_ROLE) + ); + }); + + it("ADMIN_ROLE should NOT be able to revoke GOVERNOR_ROLE", async () => { + const [ admin ] = adminAccs; + const [ { address: existingGovernor } ] = governorAccs; + await expect( + znsAccessController.connect(admin).revokeRole(GOVERNOR_ROLE, existingGovernor) + ).to.be.revertedWith( + getAccessRevertMsg(admin.address, GOVERNOR_ROLE) + ); + }); + + it("ADMIN_ROLE should be able to grant REGISTRAR_ROLE", async () => { + const [ admin ] = adminAccs; + const [ { address: newRegistrar } ] = randomAccs; + await znsAccessController.connect(admin).grantRole(REGISTRAR_ROLE, newRegistrar); + const has = await znsAccessController.hasRole(REGISTRAR_ROLE, newRegistrar); + expect(has).to.be.true; + }); + + it("ADMIN_ROLE should be able to revoke REGISTRAR_ROLE", async () => { + const [ admin ] = adminAccs; + const [ { address: newRegistrar } ] = randomAccs; + await znsAccessController.connect(admin).grantRole(REGISTRAR_ROLE, newRegistrar); + + await znsAccessController.connect(admin).revokeRole(REGISTRAR_ROLE, newRegistrar); + const has = await znsAccessController.hasRole(REGISTRAR_ROLE, newRegistrar); + expect(has).to.be.false; + }); + + it("GOVERNOR_ROLE should be able to assign new OPERATOR_ROLE as admin for REGISTRAR_ROLE", async () => { + const [ governor ] = governorAccs; + await znsAccessController.connect(governor).setRoleAdmin(REGISTRAR_ROLE, OPERATOR_ROLE); + + const registrarAdminRole = await znsAccessController.getRoleAdmin(REGISTRAR_ROLE); + expect(registrarAdminRole).to.be.equal(OPERATOR_ROLE); + }); + + // eslint-disable-next-line max-len + it("GOVERNOR_ROLE should be able to make himself a new OPERATOR_ROLE's admin and assign this role to anyone", async () => { + const [ governor ] = governorAccs; + const [ { address: newOperator } ] = randomAccs; + + await znsAccessController.connect(governor).setRoleAdmin(OPERATOR_ROLE, GOVERNOR_ROLE); + const roleAdminFrom = await znsAccessController.getRoleAdmin(OPERATOR_ROLE); + expect(roleAdminFrom).to.be.equal(GOVERNOR_ROLE); + + await znsAccessController.connect(governor).grantRole(OPERATOR_ROLE, newOperator); + const has = await znsAccessController.hasRole(OPERATOR_ROLE, newOperator); + expect(has).to.be.true; + }); + }); + + // TODO AC: test setRoleAdmin for someone other than governor + // test initializer +}); diff --git a/test/ZNSEthRegistrar.test.ts b/test/ZNSEthRegistrar.test.ts index cfb5c00b9..292706093 100644 --- a/test/ZNSEthRegistrar.test.ts +++ b/test/ZNSEthRegistrar.test.ts @@ -9,31 +9,41 @@ import { checkBalance } from "./helpers/balances"; import { priceConfigDefault } from "./helpers/constants"; import { getPrice, getPriceObject } from "./helpers/pricing"; import { getDomainHashFromEvent, getTokenIdFromEvent } from "./helpers/events"; +import { BigNumber } from "ethers"; +import { ADMIN_ROLE, getAccessRevertMsg } from "./helpers/access"; +import { ZNSEthRegistrar__factory } from "../typechain"; require("@nomicfoundation/hardhat-chai-matchers"); -// const { constants: { AddressZero } } = ethers; describe("ZNSEthRegistrar", () => { let deployer : SignerWithAddress; let user : SignerWithAddress; + let governor : SignerWithAddress; + let admin : SignerWithAddress; + let randomAcc : SignerWithAddress; let zns : ZNSContracts; let zeroVault : SignerWithAddress; let operator : SignerWithAddress; const defaultDomain = "wilder"; - // const defaultSubdomain = "world"; beforeEach(async () => { - [deployer, zeroVault, user, operator] = await hre.ethers.getSigners(); - // Burn address is used to hold the fee charged to the user when registering - zns = await deployZNS(deployer, priceConfigDefault, zeroVault.address); + [deployer, zeroVault, user, operator, governor, admin, randomAcc] = await hre.ethers.getSigners(); + // zeroVault address is used to hold the fee charged to the user when registering + zns = await deployZNS({ + deployer, + governorAddresses: [deployer.address, governor.address], + adminAddresses: [admin.address], + priceConfig: priceConfigDefault, + zeroVaultAddress: zeroVault.address, + }); - // TODO change this when access control implemented + // TODO AC: change this when access control implemented // Give the user permission on behalf of the parent domain owner await zns.registry.connect(deployer).setOwnerOperator(user.address, true); - // TODO change this when access control implemented + // TODO AC: change this when access control implemented // Give the registrar permission on behalf of the user await zns.registry.connect(user).setOwnerOperator(zns.registrar.address, true); @@ -50,6 +60,23 @@ describe("ZNSEthRegistrar", () => { expect(allowance).to.eq(ethers.constants.MaxUint256); }); + // TODO AC: is this a problem it is set up this way? + it("Should initialize correctly from an ADMIN account only", async () => { + const userHasAdmin = await zns.accessController.hasRole(ADMIN_ROLE, user.address); + expect(userHasAdmin).to.be.false; + + const registrarFactory = new ZNSEthRegistrar__factory(deployer); + const tx = registrarFactory.connect(user).deploy( + zns.accessController.address, + randomAcc.address, + randomAcc.address, + randomAcc.address, + randomAcc.address + ); + + await expect(tx).to.be.revertedWith(getAccessRevertMsg(user.address, ADMIN_ROLE)); + }); + describe("Registers a top level domain", () => { it("Can NOT register a TLD with an empty name", async () => { const emptyName = ""; @@ -59,6 +86,15 @@ describe("ZNSEthRegistrar", () => { ).to.be.revertedWith("ZNSEthRegistrar: Domain Name not provided"); }); + it("Successfully registers a domain without a resolver or resolver content", async () => { + const tx = zns.registrar.connect(user).registerDomain( + defaultDomain, + ethers.constants.AddressZero, + ); + + await expect(tx).to.not.be.reverted; + }); + it("Stakes the correct amount, takes the correct fee and sends fee to Zero Vault", async () => { const balanceBeforeUser = await zns.zeroToken.balanceOf(user.address); const balanceBeforeVault = await zns.zeroToken.balanceOf(zeroVault.address); @@ -232,7 +268,7 @@ describe("ZNSEthRegistrar", () => { const tx = zns.registrar.connect(user).reclaimDomain(domainHash); // Verify Domain is not reclaimed - await expect(tx).to.be.revertedWith("ZNSEthRegistrar: Not owner of Token"); + await expect(tx).to.be.revertedWith("ZNSEthRegistrar: Not the owner of the Token"); // Verify domain is not owned in registrar const registryOwner = await zns.registry.connect(user).getDomainOwner(domainHash); @@ -350,10 +386,10 @@ describe("ZNSEthRegistrar", () => { // Verify transaction is reverted const tx = zns.registrar.connect(user).revokeDomain(fakeHash); - await expect(tx).to.be.revertedWith("ZNSEthRegistrar: Not the Domain Owner"); + await expect(tx).to.be.revertedWith("ZNSEthRegistrar: Not the Owner of the Name"); }); - it("Revoked domain unstakes", async () => { + it("Revoking domain unstakes", async () => { // Verify Balance const balance = await zns.zeroToken.balanceOf(user.address); expect(balance).to.eq(ethers.utils.parseEther("15")); @@ -388,7 +424,7 @@ describe("ZNSEthRegistrar", () => { expect(computedBalanceAfterStaking).to.equal(finalBalance); }); - it("Cannot revoke a domain owned by another user", async () => { + it("Cannot revoke if Name is owned by another user", async () => { // Register Top level const topLevelTx = await defaultRegistration(deployer, zns, defaultDomain); const parentDomainHash = await getDomainHashFromEvent(topLevelTx); @@ -397,10 +433,29 @@ describe("ZNSEthRegistrar", () => { // Try to revoke domain const tx = zns.registrar.connect(user).revokeDomain(parentDomainHash); - await expect(tx).to.be.revertedWith("ZNSEthRegistrar: Not the Domain Owner"); + await expect(tx).to.be.revertedWith("ZNSEthRegistrar: Not the Owner of the Name"); }); - it("After domain has been revoked, an old operator cannot access the Registry", async () => { + it("No one can revoke if Token and Name have different owners", async () => { + // Register Top level + const topLevelTx = await defaultRegistration(deployer, zns, defaultDomain); + const parentDomainHash = await getDomainHashFromEvent(topLevelTx); + const owner = await zns.registry.connect(user).getDomainOwner(parentDomainHash); + expect(owner).to.not.equal(user.address); + + const tokenId = BigNumber.from(parentDomainHash); + + await zns.domainToken.transferFrom(deployer.address, user.address, tokenId); + + // Try to revoke domain as a new owner of the token + const tx = zns.registrar.connect(user).revokeDomain(parentDomainHash); + await expect(tx).to.be.revertedWith("ZNSEthRegistrar: Not the Owner of the Name"); + + const tx2 = zns.registrar.connect(deployer).revokeDomain(parentDomainHash); + await expect(tx2).to.be.revertedWith("ZNSEthRegistrar: Not the owner of the Token"); + }); + + it("After domain has been revoked, an old operator can NOT access Registry", async () => { // Register Top level const tx = await defaultRegistration(user, zns, defaultDomain); const domainHash = await getDomainHashFromEvent(tx); @@ -438,4 +493,131 @@ describe("ZNSEthRegistrar", () => { await expect(tx4).to.be.revertedWith("ZNSRegistry: Not authorized"); }); }); -}); \ No newline at end of file + + describe("State Setters", () => { + describe("#setAccessController", () => { + it("Should set AccessController and fire AccessControllerSet event", async () => { + const currentAC = await zns.registrar.getAccessController(); + const tx = await zns.registrar.connect(deployer).setAccessController(randomAcc.address); + const newAC = await zns.registrar.getAccessController(); + + await expect(tx).to.emit(zns.registrar, "AccessControllerSet").withArgs(randomAcc.address); + + expect(newAC).to.equal(randomAcc.address); + expect(currentAC).to.not.equal(newAC); + }); + + it("Should revert if not called by ADMIN", async () => { + const tx = zns.registrar.connect(user).setAccessController(randomAcc.address); + await expect(tx).to.be.revertedWith( + getAccessRevertMsg(user.address, ADMIN_ROLE) + ); + }); + + it("Should revert if new AccessController is address zero", async () => { + const tx = zns.registrar.connect(deployer).setAccessController(ethers.constants.AddressZero); + await expect(tx).to.be.revertedWith("AC: _accessController is 0x0 address"); + }); + }); + + describe("#setZnsRegistry", () => { + it("Should set ZNSRegistry and fire ZnsRegistrySet event", async () => { + const currentRegistry = await zns.registrar.znsRegistry(); + const tx = await zns.registrar.connect(deployer).setZnsRegistry(randomAcc.address); + const newRegistry = await zns.registrar.znsRegistry(); + + await expect(tx).to.emit(zns.registrar, "ZnsRegistrySet").withArgs(randomAcc.address); + + expect(newRegistry).to.equal(randomAcc.address); + expect(currentRegistry).to.not.equal(newRegistry); + }); + + it("Should revert if not called by ADMIN", async () => { + const tx = zns.registrar.connect(user).setZnsRegistry(randomAcc.address); + await expect(tx).to.be.revertedWith( + getAccessRevertMsg(user.address, ADMIN_ROLE) + ); + }); + + it("Should revert if ZNSRegistry is address zero", async () => { + const tx = zns.registrar.connect(deployer).setZnsRegistry(ethers.constants.AddressZero); + await expect(tx).to.be.revertedWith("ZNSEthRegistrar: znsRegistry_ is 0x0 address"); + }); + }); + + describe("#setZnsTreasury", () => { + it("Should set ZNSTreasury and fire ZnsTreasurySet event", async () => { + const currentTreasury = await zns.registrar.znsTreasury(); + const tx = await zns.registrar.connect(deployer).setZnsTreasury(randomAcc.address); + const newTreasury = await zns.registrar.znsTreasury(); + + await expect(tx).to.emit(zns.registrar, "ZnsTreasurySet").withArgs(randomAcc.address); + + expect(newTreasury).to.equal(randomAcc.address); + expect(currentTreasury).to.not.equal(newTreasury); + }); + + it("Should revert if not called by ADMIN", async () => { + const tx = zns.registrar.connect(user).setZnsTreasury(randomAcc.address); + await expect(tx).to.be.revertedWith( + getAccessRevertMsg(user.address, ADMIN_ROLE) + ); + }); + + it("Should revert if ZNSTreasury is address zero", async () => { + const tx = zns.registrar.connect(deployer).setZnsTreasury(ethers.constants.AddressZero); + await expect(tx).to.be.revertedWith("ZNSEthRegistrar: znsTreasury_ is 0x0 address"); + }); + }); + + describe("#setZnsDomainToken", () => { + it("Should set ZNSDomainToken and fire ZnsDomainTokenSet event", async () => { + const currentToken = await zns.registrar.znsDomainToken(); + const tx = await zns.registrar.connect(deployer).setZnsDomainToken(randomAcc.address); + const newToken = await zns.registrar.znsDomainToken(); + + await expect(tx).to.emit(zns.registrar, "ZnsDomainTokenSet").withArgs(randomAcc.address); + + expect(newToken).to.equal(randomAcc.address); + expect(currentToken).to.not.equal(newToken); + }); + + it("Should revert if not called by ADMIN", async () => { + const tx = zns.registrar.connect(user).setZnsDomainToken(randomAcc.address); + await expect(tx).to.be.revertedWith( + getAccessRevertMsg(user.address, ADMIN_ROLE) + ); + }); + + it("Should revert if ZNSDomainToken is address zero", async () => { + const tx = zns.registrar.connect(deployer).setZnsDomainToken(ethers.constants.AddressZero); + await expect(tx).to.be.revertedWith("ZNSEthRegistrar: znsDomainToken_ is 0x0 address"); + }); + }); + + describe("#setZnsAddressResolver", () => { + it("Should set ZNSAddressResolver and fire ZnsAddressResolverSet event", async () => { + const currentResolver = await zns.registrar.znsAddressResolver(); + const tx = await zns.registrar.connect(deployer).setZnsAddressResolver(randomAcc.address); + const newResolver = await zns.registrar.znsAddressResolver(); + + await expect(tx).to.emit(zns.registrar, "ZnsAddressResolverSet").withArgs(randomAcc.address); + + expect(newResolver).to.equal(randomAcc.address); + expect(currentResolver).to.not.equal(newResolver); + }); + + it("Should revert if not called by ADMIN", async () => { + const tx = zns.registrar.connect(user).setZnsAddressResolver(randomAcc.address); + await expect(tx).to.be.revertedWith( + getAccessRevertMsg(user.address, ADMIN_ROLE) + ); + }); + + it("Should revert if ZNSAddressResolver is address zero", async () => { + const tx = zns.registrar.connect(deployer).setZnsAddressResolver(ethers.constants.AddressZero); + await expect(tx).to.be.revertedWith("ZNSEthRegistrar: znsAddressResolver_ is 0x0 address"); + }); + }); + }); +}); diff --git a/test/ZNSPriceOracle.test.ts b/test/ZNSPriceOracle.test.ts index 11fb4787e..53e99edae 100644 --- a/test/ZNSPriceOracle.test.ts +++ b/test/ZNSPriceOracle.test.ts @@ -6,14 +6,15 @@ import { parseEther } from "ethers/lib/utils"; import { ZNSContracts } from "./helpers/types"; import { deployZNS, getPrice } from "./helpers"; import { priceConfigDefault, registrationFeePercDefault } from "./helpers/constants"; +import { ADMIN_ROLE, getAccessRevertMsg } from "./helpers/access"; require("@nomicfoundation/hardhat-chai-matchers"); describe("ZNSPriceOracle", () => { let deployer : SignerWithAddress; let user : SignerWithAddress; - let mockRegistrar : SignerWithAddress; - let updatedMockRegistrar : SignerWithAddress; + let admin : SignerWithAddress; + let randomAcc : SignerWithAddress; let zns : ZNSContracts; @@ -21,43 +22,28 @@ describe("ZNSPriceOracle", () => { [ deployer, user, - mockRegistrar, - updatedMockRegistrar, + admin, + randomAcc, ] = await hre.ethers.getSigners(); - zns = await deployZNS(deployer); - - await zns.priceOracle.connect(deployer).setZNSRegistrar(mockRegistrar.address); + zns = await deployZNS({ + deployer, + governorAddresses: [deployer.address], + adminAddresses: [admin.address], + }); }); // TODO reg: add tests for proper fee calcs! - it("Confirms the mockRegistrar is authorized", async () => { - const authorized = await zns.priceOracle.isAuthorized(mockRegistrar.address); - expect(authorized).to.be.true; - }); - - it("Confirms the deployer is authorized", async () => { - const authorized = await zns.priceOracle.isAuthorized(deployer.address); - expect(authorized).to.be.true; - }); - - it("Confirms a random user is not authorized", async () => { - const authorized = await zns.priceOracle.isAuthorized(user.address); - expect(authorized).to.be.false; - }); it("Confirms values were initially set correctly", async () => { - const valueCalls = [ zns.priceOracle.feePercentage(), zns.priceOracle.priceConfig(), - zns.priceOracle.znsRegistrar(), ]; const [ feePercentageFromSC, priceConfigFromSC, - znsRegistrarFromSC, ] = await Promise.all(valueCalls); const priceConfigArr = Object.values(priceConfigDefault); @@ -69,10 +55,9 @@ describe("ZNSPriceOracle", () => { ); expect(feePercentageFromSC).to.eq(registrationFeePercDefault); - expect(znsRegistrarFromSC).to.eq(mockRegistrar.address); }); - describe("getPrice", async () => { + describe("#getPrice", async () => { it("Returns 0 price for a root name with no length", async () => { const { totalPrice, @@ -261,11 +246,11 @@ describe("ZNSPriceOracle", () => { }); }); - describe("setBasePrice", () => { + describe("#setBasePrice", () => { it("Allows an authorized user to set the base price", async () => { const newMaxPrice = parseEther("0.7"); - await zns.priceOracle.connect(deployer).setMaxPrice(newMaxPrice, true); + await zns.priceOracle.connect(admin).setMaxPrice(newMaxPrice, true); const params = await zns.priceOracle.priceConfig(); expect(params.maxRootDomainPrice).to.eq(newMaxPrice); @@ -275,7 +260,9 @@ describe("ZNSPriceOracle", () => { const newMaxPrice = parseEther("0.7"); const tx = zns.priceOracle.connect(user).setMaxPrice(newMaxPrice, true); - await expect(tx).to.be.revertedWith("ZNS: Not authorized"); + await expect(tx).to.be.revertedWith( + getAccessRevertMsg(user.address, ADMIN_ROLE) + ); }); it("Allows setting the price to zero", async () => { @@ -335,7 +322,7 @@ describe("ZNSPriceOracle", () => { }); }); - describe("setPriceMultiplier", () => { + describe("#setPriceMultiplier", () => { it("Allows an authorized user to set the price multiplier", async () => { const newMultiplier = BigNumber.from("300"); @@ -348,7 +335,9 @@ describe("ZNSPriceOracle", () => { const newMultiplier = BigNumber.from("300"); const tx = zns.priceOracle.connect(user).setPriceMultiplier(newMultiplier); - await expect(tx).to.be.revertedWith("ZNS: Not authorized"); + await expect(tx).to.be.revertedWith( + getAccessRevertMsg(user.address, ADMIN_ROLE) + ); }); it("Fails when setting to a value below the specified range", async () => { @@ -378,7 +367,7 @@ describe("ZNSPriceOracle", () => { }); }); - describe("setBaseLength(s)", () => { + describe("#setBaseLength(s)", () => { it("Allows an authorized user to set the base length", async () => { const newLength = 5; @@ -392,7 +381,9 @@ describe("ZNSPriceOracle", () => { const newLength = 5; const tx = zns.priceOracle.connect(user).setBaseLength(newLength, true); - await expect(tx).to.be.revertedWith("ZNS: Not authorized"); + await expect(tx).to.be.revertedWith( + getAccessRevertMsg(user.address, ADMIN_ROLE) + ); }); it("Allows setting the base length to zero", async () => { @@ -478,7 +469,9 @@ describe("ZNSPriceOracle", () => { const newLength = 5; const tx = zns.priceOracle.connect(user).setBaseLengths(newLength, newLength); - await expect(tx).to.be.revertedWith("ZNS: Not authorized"); + await expect(tx).to.be.revertedWith( + getAccessRevertMsg(user.address, ADMIN_ROLE) + ); }); it("Adjusts prices correctly when setting base lengths to different values", async () => { @@ -501,38 +494,7 @@ describe("ZNSPriceOracle", () => { }); }); - describe("setZNSRegistrar", () => { - it("Allows an authorized user to modify the registrar address", async () => { - await zns.priceOracle.connect(deployer).setZNSRegistrar(updatedMockRegistrar.address); - - const newAddress = await zns.priceOracle.znsRegistrar(); - expect(newAddress).to.eq(updatedMockRegistrar.address); - }); - - it("Disallows an authorized user to modify the registrar address", async () => { - const tx = zns.priceOracle.connect(user).setZNSRegistrar(updatedMockRegistrar.address); - - await expect(tx).to.be.revertedWith("ZNS: Not authorized"); - }); - - it("Can NOT set znsRegistrar if with zero address", async () => { - const tx = zns.priceOracle.connect(deployer).setZNSRegistrar(ethers.constants.AddressZero); - - await expect(tx).to.be.revertedWith("ZNS: Zero address for Registrar"); - }); - - it("Revokes authorized status from the old registrar when updated", async () => { - await zns.priceOracle.connect(deployer).setZNSRegistrar(updatedMockRegistrar.address); - - const isOldAuthorized = await zns.priceOracle.isAuthorized(mockRegistrar.address); - const isNewAuthorized = await zns.priceOracle.isAuthorized(updatedMockRegistrar.address); - - expect(isOldAuthorized).to.be.false; - expect(isNewAuthorized).to.be.true; - }); - }); - - describe("setRegistrationFeePercentage", () => { + describe("#setRegistrationFeePercentage", () => { it("Successfully sets the fee percentage", async () => { const newFeePerc = BigNumber.from(222); await zns.priceOracle.setRegistrationFeePercentage(newFeePerc); @@ -540,9 +502,17 @@ describe("ZNSPriceOracle", () => { expect(feeFromSC).to.eq(newFeePerc); }); + + it("Disallows an unauthorized user to set the fee percentage", async () => { + const newFeePerc = BigNumber.from(222); + const tx = zns.priceOracle.connect(user).setRegistrationFeePercentage(newFeePerc); + await expect(tx).to.be.revertedWith( + getAccessRevertMsg(user.address, ADMIN_ROLE) + ); + }); }); - describe("getRegistrationFee", () => { + describe("#getRegistrationFee", () => { it("Successfully gets the fee for a price", async () => { const stake = ethers.utils.parseEther("0.2"); const fee = await zns.priceOracle.getRegistrationFee(stake); @@ -552,6 +522,34 @@ describe("ZNSPriceOracle", () => { }); }); + describe("#setAccessController", () => { + it("Successfully sets the access controller", async () => { + const currentAccessController = await zns.priceOracle.getAccessController(); + expect(currentAccessController).to.not.eq(randomAcc.address); + + const tx = await zns.priceOracle.setAccessController(randomAcc.address); + + const newAccessController = await zns.priceOracle.getAccessController(); + expect(newAccessController).to.eq(randomAcc.address); + + await expect(tx).to.emit(zns.priceOracle, "AccessControllerSet").withArgs(randomAcc.address); + }); + + it("Disallows an unauthorized user to set the access controller", async () => { + const tx = zns.priceOracle.connect(user).setAccessController(randomAcc.address); + await expect(tx).to.be.revertedWith( + getAccessRevertMsg(user.address, ADMIN_ROLE) + ); + }); + + it("Disallows setting the access controller to the zero address", async () => { + const tx = zns.priceOracle.connect(deployer).setAccessController(ethers.constants.AddressZero); + await expect(tx).to.be.revertedWith( + "AC: _accessController is 0x0 address" + ); + }); + }); + describe("Events", () => { it("Emits BasePriceSet", async () => { const newMaxPrice = parseEther("0.7"); @@ -573,16 +571,12 @@ describe("ZNSPriceOracle", () => { const tx = zns.priceOracle.connect(deployer).setBaseLength(newLength, true); await expect(tx).to.emit(zns.priceOracle, "BaseLengthSet").withArgs(newLength, true); }); + it("Emits BaseLengthsSet", async () => { const newLength = 5; const tx = zns.priceOracle.connect(deployer).setBaseLengths(newLength, newLength); await expect(tx).to.emit(zns.priceOracle, "BaseLengthsSet").withArgs(newLength, newLength); }); - - it("Emits ZNSRegistrarSet", async () => { - const tx = zns.priceOracle.connect(deployer).setZNSRegistrar(updatedMockRegistrar.address); - await expect(tx).to.emit(zns.priceOracle, "ZNSRegistrarSet").withArgs(updatedMockRegistrar.address); - }); }); -}); \ No newline at end of file +}); diff --git a/test/ZNSRegistry.test.ts b/test/ZNSRegistry.test.ts index 042aec859..6e3783beb 100644 --- a/test/ZNSRegistry.test.ts +++ b/test/ZNSRegistry.test.ts @@ -9,14 +9,6 @@ import { hashDomainLabel, hashDomainName } from "./helpers/hashing"; // eslint-disable-next-line @typescript-eslint/no-var-requires require("@nomicfoundation/hardhat-chai-matchers"); -/** - * TODO should we disallow burning the root domain? - * The process of burning would transfer ownership to the 0x0 address - * Nobody would be able to successfully mint new subdomaisn in this scenario - * In the case the `owner` we set in the initializer is an EOA, it's a - * possibility the account is compromised - */ - describe("ZNSRegistry Tests", () => { let deployer : SignerWithAddress; diff --git a/test/ZNSTreasury.test.ts b/test/ZNSTreasury.test.ts index fb85f0528..1e8d9187f 100644 --- a/test/ZNSTreasury.test.ts +++ b/test/ZNSTreasury.test.ts @@ -4,13 +4,15 @@ import { SignerWithAddress } from "@nomiclabs/hardhat-ethers/signers"; import { checkBalance, deployZNS } from "./helpers"; import { ZNSContracts } from "./helpers/types"; import * as ethers from "ethers"; -import { priceConfigDefault } from "./helpers/constants"; import { hashDomainLabel } from "./helpers/hashing"; +import { ADMIN_ROLE, getAccessRevertMsg, REGISTRAR_ROLE } from "./helpers/access"; require("@nomicfoundation/hardhat-chai-matchers"); describe("ZNSTreasury", () => { let deployer : SignerWithAddress; + let governor : SignerWithAddress; + let admin : SignerWithAddress; let user : SignerWithAddress; let zeroVault : SignerWithAddress; let mockRegistrar : SignerWithAddress; @@ -18,11 +20,25 @@ describe("ZNSTreasury", () => { let zns : ZNSContracts; beforeEach(async () => { - [ deployer, zeroVault, user, mockRegistrar, randomAcc ] = await hre.ethers.getSigners(); - zns = await deployZNS(deployer, priceConfigDefault, zeroVault.address); + [ + deployer, + governor, + admin, + zeroVault, + user, + mockRegistrar, + randomAcc, + ] = await hre.ethers.getSigners(); + + zns = await deployZNS({ + deployer, + governorAddresses: [governor.address], + adminAddresses: [admin.address], + zeroVaultAddress: zeroVault.address, + }); - // Set the registrar as a mock so that we can call the functions - await zns.treasury.connect(deployer).setZNSRegistrar(mockRegistrar.address); + // give REGISTRAR_ROLE to a wallet address to be calling guarded functions + await zns.accessController.connect(admin).grantRole(REGISTRAR_ROLE, mockRegistrar.address); // Give funds to user await zns.zeroToken.connect(user).approve(zns.treasury.address, ethers.constants.MaxUint256); @@ -30,18 +46,16 @@ describe("ZNSTreasury", () => { }); it("Confirms deployment", async () => { - const registrar = await zns.treasury.znsRegistrar(); const priceOracle = await zns.treasury.znsPriceOracle(); - const token = await zns.treasury.zeroToken(); - const isAdmin = await zns.treasury.isAdmin(deployer.address); + const token = await zns.treasury.stakingToken(); + const accessController = await zns.treasury.getAccessController(); - expect(registrar).to.eq(mockRegistrar.address); expect(priceOracle).to.eq(zns.priceOracle.address); expect(token).to.eq(zns.zeroToken.address); - expect(isAdmin).to.be.true; + expect(accessController).to.eq(zns.accessController.address); }); - describe("stakeForDomain", () => { + describe("#stakeForDomain", () => { it("Stakes the correct amount", async () => { const domain = "wilder"; const domainHash = hashDomainLabel(domain); @@ -68,22 +82,24 @@ describe("ZNSTreasury", () => { }); }); - it("Should revert if called from any address that is not ZNSRegistrar", async () => { + it("Should revert if called from an address without REGISTRAR_ROLE", async () => { const domain = "wilder"; const domainHash = hashDomainLabel(domain); - const tx = zns.treasury.connect(user).stakeForDomain( + const tx = zns.treasury.connect(randomAcc).stakeForDomain( domainHash, domain, user.address, true ); - await expect(tx).to.be.revertedWith("ZNSTreasury: Only ZNSRegistrar is allowed to call"); + await expect(tx).to.be.revertedWith( + getAccessRevertMsg(randomAcc.address, REGISTRAR_ROLE) + ); }); }); - describe("unstakeForDomain", () => { + describe("#unstakeForDomain", () => { it("Unstakes the correct amount", async () => { const domain = "wilder"; const domainHash = hashDomainLabel(domain); @@ -109,7 +125,7 @@ describe("ZNSTreasury", () => { }); }); - it("Should revert if called from any address that is not ZNSRegistrar", async () => { + it("Should revert if called from an address without REGISTRAR_ROLE", async () => { const domain = "wilder"; const domainHash = hashDomainLabel(domain); @@ -118,11 +134,13 @@ describe("ZNSTreasury", () => { user.address ); - await expect(tx).to.be.revertedWith("ZNSTreasury: Only ZNSRegistrar is allowed to call"); + await expect(tx).to.be.revertedWith( + getAccessRevertMsg(user.address, REGISTRAR_ROLE) + ); }); }); - describe("setZeroVaultAddress() and ZeroVaultAddressSet event", () => { + describe("#setZeroVaultAddress() and ZeroVaultAddressSet event", () => { it("Should set the correct address of Zero Vault", async () => { const currentZeroVault = await zns.treasury.zeroVault(); expect(currentZeroVault).to.not.eq(mockRegistrar.address); @@ -132,12 +150,14 @@ describe("ZNSTreasury", () => { const newZeroVault = await zns.treasury.zeroVault(); expect(newZeroVault).to.eq(mockRegistrar.address); - await expect(tx).to.emit(zns.treasury, "ZeroVaultAddressSet").withArgs(newZeroVault); + await expect(tx).to.emit(zns.treasury, "ZeroVaultAddressSet").withArgs(mockRegistrar.address); }); - it("Should revert when called from any address that is not admin", async () => { + it("Should revert when called from any address without ADMIN_ROLE", async () => { const tx = zns.treasury.connect(user).setZeroVaultAddress(mockRegistrar.address); - await expect(tx).to.be.revertedWith("ZNSTreasury: Not an allowed admin"); + await expect(tx).to.be.revertedWith( + getAccessRevertMsg(user.address, ADMIN_ROLE) + ); }); it("Should revert when zeroVault is address 0", async () => { @@ -146,17 +166,81 @@ describe("ZNSTreasury", () => { }); }); - describe("setZNSRegistrar", () => { - it("Should set znsRegistrar in storage", async () => { - await zns.treasury.setZNSRegistrar(randomAcc.address); + describe("#setPriceOracle() and ZnsPriceOracleSet event", () => { + it("Should set the correct address of ZNS Price Oracle", async () => { + const currentPriceOracle = await zns.treasury.znsPriceOracle(); + expect(currentPriceOracle).to.not.eq(randomAcc.address); + + const tx = await zns.treasury.setPriceOracle(randomAcc.address); + + const newPriceOracle = await zns.treasury.znsPriceOracle(); + expect(newPriceOracle).to.eq(randomAcc.address); + + await expect(tx).to.emit(zns.treasury, "ZnsPriceOracleSet").withArgs(randomAcc.address); + }); + + it("Should revert when called from any address without ADMIN_ROLE", async () => { + const tx = zns.treasury.connect(user).setPriceOracle(randomAcc.address); + await expect(tx).to.be.revertedWith( + getAccessRevertMsg(user.address, ADMIN_ROLE) + ); + }); + + it("Should revert when znsPriceOracle is address 0", async () => { + const tx = zns.treasury.setPriceOracle(ethers.constants.AddressZero); + await expect(tx).to.be.revertedWith("ZNSTreasury: znsPriceOracle_ passed as 0x0 address"); + }); + }); + + describe("#setStakingToken() and ZnsStakingTokenSet event", () => { + it("Should set the correct address of ZNS Staking Token", async () => { + const currentStakingToken = await zns.treasury.stakingToken(); + expect(currentStakingToken).to.not.eq(randomAcc.address); + + const tx = await zns.treasury.setStakingToken(randomAcc.address); - const registrarFromSC = await zns.treasury.znsRegistrar(); - expect(registrarFromSC).to.be.eq(randomAcc.address); + const newStakingToken = await zns.treasury.stakingToken(); + expect(newStakingToken).to.eq(randomAcc.address); + + await expect(tx).to.emit(zns.treasury, "ZnsStakingTokenSet").withArgs(randomAcc.address); + }); + + it("Should revert when called from any address without ADMIN_ROLE", async () => { + const tx = zns.treasury.connect(user).setStakingToken(randomAcc.address); + await expect(tx).to.be.revertedWith( + getAccessRevertMsg(user.address, ADMIN_ROLE) + ); + }); + + it("Should revert when stakingToken is address 0", async () => { + const tx = zns.treasury.setStakingToken(ethers.constants.AddressZero); + await expect(tx).to.be.revertedWith("ZNSTreasury: stakingToken_ passed as 0x0 address"); + }); + }); + + describe("#setAccessController() and AccessControllerSet event", () => { + it("Should set the correct address of Access Controller", async () => { + const currentAccessController = await zns.treasury.getAccessController(); + expect(currentAccessController).to.not.eq(randomAcc.address); + + const tx = await zns.treasury.setAccessController(randomAcc.address); + + const newAccessController = await zns.treasury.getAccessController(); + expect(newAccessController).to.eq(randomAcc.address); + + await expect(tx).to.emit(zns.treasury, "AccessControllerSet").withArgs(randomAcc.address); + }); + + it("Should revert when called from any address without ADMIN_ROLE", async () => { + const tx = zns.treasury.connect(user).setAccessController(randomAcc.address); + await expect(tx).to.be.revertedWith( + getAccessRevertMsg(user.address, ADMIN_ROLE) + ); }); - it("Should revert if Registrar is address 0", async () => { - const tx = zns.treasury.setZNSRegistrar(ethers.constants.AddressZero); - await expect(tx).to.be.revertedWith("ZNSTreasury: Zero address passed as znsRegistrar"); + it("Should revert when accessController is address 0", async () => { + const tx = zns.treasury.setAccessController(ethers.constants.AddressZero); + await expect(tx).to.be.revertedWith("AC: _accessController is 0x0 address"); }); }); }); diff --git a/test/helpers/access.ts b/test/helpers/access.ts new file mode 100644 index 000000000..ed0a50a23 --- /dev/null +++ b/test/helpers/access.ts @@ -0,0 +1,42 @@ +import { SignerWithAddress } from "@nomiclabs/hardhat-ethers/signers"; +import { ZNSAccessController, ZNSAccessController__factory } from "../../typechain"; +import { ethers } from "ethers"; + + +// role names +export const GOVERNOR_ROLE = ethers.utils.solidityKeccak256( + ["string"], + ["GOVERNOR_ROLE"] +); +export const ADMIN_ROLE = ethers.utils.solidityKeccak256( + ["string"], + ["ADMIN_ROLE"] +); +export const REGISTRAR_ROLE = ethers.utils.solidityKeccak256( + ["string"], + ["REGISTRAR_ROLE"] +); + +export const OPERATOR_ROLE = ethers.utils.solidityKeccak256( + ["string"], + ["OPERATOR_ROLE"] +); + +export const deployAccessController = async ({ + deployer, + governorAddresses, + adminAddresses, +} : { + deployer : SignerWithAddress; + governorAddresses : Array<string>; + adminAddresses : Array<string>; +}) : Promise<ZNSAccessController> => { + const accessControllerFactory = new ZNSAccessController__factory(deployer); + const controller = await accessControllerFactory.deploy(); + + await controller.initialize(governorAddresses, adminAddresses); + return controller; +}; + +export const getAccessRevertMsg = (addr : string, role : string) : string => + `AccessControl: account ${addr.toLowerCase()} is missing role ${role}`; diff --git a/test/helpers/constants.ts b/test/helpers/constants.ts index a7c4f58bd..5ab4e5af2 100644 --- a/test/helpers/constants.ts +++ b/test/helpers/constants.ts @@ -3,12 +3,6 @@ import { PriceParams } from "./types"; import { ethers } from "hardhat"; -export const ZERO_ROOT = "0://"; - -// TODO: figure out what to do with this -// namehash lib does not support the ":" symbol -// export const ZERO_ROOT_HASH = hashDomainName(ZERO_ROOT); - export const registrationFeePercDefault = BigNumber.from("222"); export const priceConfigDefault : PriceParams = { diff --git a/test/helpers/deployZNS.ts b/test/helpers/deployZNS.ts index 14d26c4ce..e081d0c7a 100644 --- a/test/helpers/deployZNS.ts +++ b/test/helpers/deployZNS.ts @@ -1,6 +1,6 @@ import { ZeroTokenMock, - ZeroTokenMock__factory, + ZeroTokenMock__factory, ZNSAccessController, ZNSAddressResolver, ZNSAddressResolver__factory, ZNSDomainToken, @@ -18,6 +18,9 @@ import { ethers } from "hardhat"; import { PriceParams, RegistrarConfig, ZNSContracts } from "./types"; import { SignerWithAddress } from "@nomiclabs/hardhat-ethers/signers"; import { priceConfigDefault, registrationFeePercDefault } from "./constants"; +import { deployAccessController, REGISTRAR_ROLE } from "./access"; +import { BigNumber } from "ethers"; + export const deployRegistry = async ( deployer : SignerWithAddress, @@ -42,22 +45,23 @@ export const deployAddressResolver = async ( return addressResolver; }; -export const deployPriceOracle = async ( - deployer : SignerWithAddress, - registrarAddress : string, - priceConfig : PriceParams, - registrationFee = registrationFeePercDefault -) : Promise<ZNSPriceOracle> => { +export const deployPriceOracle = async ({ + deployer, + accessControllerAddress, + priceConfig, + registrationFee, +} : { + deployer : SignerWithAddress; + accessControllerAddress : string; + priceConfig : PriceParams; + registrationFee : BigNumber; +}) : Promise<ZNSPriceOracle> => { const priceOracleFactory = new ZNSPriceOracle__factory(deployer); const priceOracle = await priceOracleFactory.deploy(); - // The Registrar may not be deployed yet because of the cyclic dependency - // between it and the ZNSPriceOracle. Use an empty string if so - const registrar = !registrarAddress ? "" : registrarAddress; - await priceOracle.initialize( + accessControllerAddress, priceConfig, - registrar, registrationFee ); @@ -80,17 +84,16 @@ export const deployZeroTokenMock = async ( export const deployTreasury = async ( deployer : SignerWithAddress, + accessControllerAddress : string, znsPriceOracleAddress : string, zTokenMockMockAddress : string, - znsRegistrarAddress : string, zeroVaultAddress : string ) : Promise<ZNSTreasury> => { const treasuryFactory = new ZNSTreasury__factory(deployer); const treasury = await treasuryFactory.deploy( + accessControllerAddress, znsPriceOracleAddress, zTokenMockMockAddress, - znsRegistrarAddress, - deployer.address, zeroVaultAddress ); return treasury; @@ -98,28 +101,45 @@ export const deployTreasury = async ( export const deployRegistrar = async ( deployer : SignerWithAddress, + accessController : ZNSAccessController, config : RegistrarConfig ) : Promise<ZNSEthRegistrar> => { const registrarFactory = new ZNSEthRegistrar__factory(deployer); const registrar = await registrarFactory.deploy( + accessController.address, config.registryAddress, config.treasury.address, config.domainTokenAddress, - config.addressResolverAddress, - config.priceOracleAddress + config.addressResolverAddress ); - await config.treasury.connect(deployer).setZNSRegistrar(registrar.address); + await accessController.connect(deployer).grantRole(REGISTRAR_ROLE, registrar.address); return registrar; }; -// TODO reg: make args an object here -export const deployZNS = async ( - deployer : SignerWithAddress, +export const deployZNS = async ({ + deployer, + governorAddresses, + adminAddresses, priceConfig = priceConfigDefault, - zeroVaultAddress = deployer.address -) : Promise<ZNSContracts> => { + registrationFeePerc = registrationFeePercDefault, + zeroVaultAddress = deployer.address, +} : { + deployer : SignerWithAddress; + governorAddresses : Array<string>; + adminAddresses : Array<string>; + priceConfig ?: PriceParams; + registrationFeePerc ?: BigNumber; + zeroVaultAddress ?: string; +}) : Promise<ZNSContracts> => { + const accessController = await deployAccessController({ + deployer, + governorAddresses: [deployer.address, ...governorAddresses], + adminAddresses: [deployer.address, ...adminAddresses], + }); + + // TODO AC: fix this! // Can't set to zero, but registrar address must be given. // Due to order of deployment, add deployer as registrar address for now and change after const registry = await deployRegistry(deployer, deployer); @@ -130,17 +150,18 @@ export const deployZNS = async ( const addressResolver = await deployAddressResolver(deployer, registry.address); - const priceOracle = await deployPriceOracle( + const priceOracle = await deployPriceOracle({ deployer, - ethers.constants.AddressZero, // set to ZNSRegistrar later - priceConfig - ); + accessControllerAddress: accessController.address, + priceConfig, + registrationFee: registrationFeePerc, + }); const treasury = await deployTreasury( deployer, + accessController.address, priceOracle.address, zeroTokenMock.address, - ethers.constants.AddressZero, // set to ZNSRegistrar later, zeroVaultAddress ); @@ -149,12 +170,12 @@ export const deployZNS = async ( registryAddress: registry.address, domainTokenAddress: domainToken.address, addressResolverAddress: addressResolver.address, - priceOracleAddress: priceOracle.address, }; - const registrar = await deployRegistrar(deployer, config); + const registrar = await deployRegistrar(deployer, accessController, config); const znsContracts : ZNSContracts = { + accessController, addressResolver, registry, domainToken, @@ -165,11 +186,11 @@ export const deployZNS = async ( }; // Final configuration steps - await priceOracle.connect(deployer).setZNSRegistrar(registrar.address); + // TODO AC: remove all redundant calls here! and delete hashing of the root and the need + // for Registrar to be owner/operator of the root await domainToken.connect(deployer).authorize(registrar.address); - await treasury.connect(deployer).setZNSRegistrar(registrar.address); - await registry.connect(deployer).setZNSRegistrar(registrar.address); await registry.connect(deployer).setOwnerOperator(registrar.address, true); + await registry.connect(deployer).setZNSRegistrar(registrar.address); // Give 15 ZERO to the deployer and allowance to the treasury await zeroTokenMock.connect(deployer).approve(treasury.address, ethers.constants.MaxUint256); diff --git a/test/helpers/index.ts b/test/helpers/index.ts index eb19f4848..8f00f4b6d 100644 --- a/test/helpers/index.ts +++ b/test/helpers/index.ts @@ -4,3 +4,4 @@ export * from "./pricing"; export * from "./hashing"; export * from "./constants"; export * from "./balances"; +export * from "./access"; diff --git a/test/helpers/types.ts b/test/helpers/types.ts index 282d7057a..fb09b3c43 100644 --- a/test/helpers/types.ts +++ b/test/helpers/types.ts @@ -6,7 +6,7 @@ import { ZNSPriceOracle, ZNSRegistry, ZNSTreasury, - ZeroTokenMock, + ZeroTokenMock, ZNSAccessController, } from "../../typechain"; export type Maybe<T> = T | undefined; @@ -28,10 +28,10 @@ export interface RegistrarConfig { registryAddress : string; domainTokenAddress : string; addressResolverAddress : string; - priceOracleAddress : string; } export interface ZNSContracts { + accessController : ZNSAccessController; addressResolver : ZNSAddressResolver; registry : ZNSRegistry; domainToken : ZNSDomainToken;