fix: [audit] ZNS-1, ZNS-2, vulnerability due to lack of string validation

contracts/price/ZNSCurvePricer.sol

@@ -61,6 +61,9 @@ contract ZNSCurvePricer is AAccessControlled, ARegistryWired, UUPSUpgradeable, I
         bytes32 parentHash,
         string calldata label
     ) public view override returns (uint256) {
+        // Confirms string values are only [a-z0-9]
+        label.validate();
+
         uint256 length = label.strlen();
         // No pricing is set for 0 length domains
         if (length == 0) return 0;
@@ -291,9 +294,10 @@ contract ZNSCurvePricer is AAccessControlled, ARegistryWired, UUPSUpgradeable, I
         if (length <= config.baseLength) return config.maxPrice;
         if (length > config.maxLength) return config.minPrice;
 
-        return
-        (config.baseLength * config.maxPrice / length)
-        / config.precisionMultiplier * config.precisionMultiplier;
+        uint256 price = (config.baseLength * config.maxPrice / length) 
+            / config.precisionMultiplier * config.precisionMultiplier;
+
+        return price;
     }
 
     /**

contracts/registrar/ZNSRootRegistrar.sol

@@ -10,6 +10,7 @@ import { UUPSUpgradeable } from "@openzeppelin/contracts-upgradeable/proxy/utils
 import { IZNSSubRegistrar } from "../registrar/IZNSSubRegistrar.sol";
 import { ARegistryWired } from "../registry/ARegistryWired.sol";
 import { IZNSPricer } from "../types/IZNSPricer.sol";
+import { StringUtils } from "../utils/StringUtils.sol";
 
 
 /**
@@ -37,6 +38,8 @@ contract ZNSRootRegistrar is
     IZNSAddressResolver public addressResolver;
     IZNSSubRegistrar public subRegistrar;
 
+    using StringUtils for string;
+
     /**
      * @notice Create an instance of the ZNSRootRegistrar.sol
      * for registering, reclaiming and revoking ZNS domains
@@ -87,13 +90,18 @@ contract ZNSRootRegistrar is
         string calldata tokenURI,
         DistributionConfig calldata distributionConfig
     ) external override returns (bytes32) {
+        // Confirms string values are only [a-z0-9]
+        name.validate();
+
+        bytes memory nameBytes = bytes(name);
+
         require(
-            bytes(name).length != 0,
+            nameBytes.length != 0,
             "ZNSRootRegistrar: Domain Name not provided"
         );
 
         // Create hash for given domain name
-        bytes32 domainHash = keccak256(bytes(name));
+        bytes32 domainHash = keccak256(nameBytes);
 
         require(
             !registry.exists(domainHash),

contracts/registrar/ZNSSubRegistrar.sol

@@ -6,6 +6,7 @@ import { IZNSRootRegistrar, CoreRegisterArgs } from "./IZNSRootRegistrar.sol";
 import { IZNSSubRegistrar } from "./IZNSSubRegistrar.sol";
 import { AAccessControlled } from "../access/AAccessControlled.sol";
 import { ARegistryWired } from "../registry/ARegistryWired.sol";
+import { StringUtils } from "../utils/StringUtils.sol";
 import { UUPSUpgradeable } from "@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol";
 
 
@@ -16,6 +17,7 @@ import { UUPSUpgradeable } from "@openzeppelin/contracts-upgradeable/proxy/utils
  * of any level is in the `ZNSRootRegistrar.coreRegister()`.
 */
 contract ZNSSubRegistrar is AAccessControlled, ARegistryWired, UUPSUpgradeable, IZNSSubRegistrar {
+    using StringUtils for string;
 
     /**
      * @notice State var for the ZNSRootRegistrar contract that finalizes registration of subdomains.
@@ -74,6 +76,9 @@ contract ZNSSubRegistrar is AAccessControlled, ARegistryWired, UUPSUpgradeable,
         string calldata tokenURI,
         DistributionConfig calldata distrConfig
     ) external override returns (bytes32) {
+        // Confirms string values are only [a-z0-9]
+        label.validate();
+
         bytes32 domainHash = hashWithParent(parentHash, label);
         require(
             !registry.exists(domainHash),

contracts/utils/StringUtils.sol

@@ -33,4 +33,28 @@ library StringUtils {
         }
         return len;
     }
+
+    /**
+     * @dev Confirm that a given string has only alphanumeric characters [a-z0-9]
+     * @param s The string to validate
+     */
+    function validate(string memory s) internal pure {
+        bytes memory nameBytes = bytes(s);
+        uint256 length = nameBytes.length;
+
+        uint256 MAX_INT = 0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff;
+        require(length < MAX_INT, "ZNSRootRegistrar: Domain name too long");
+
+        for (uint256 i; i < length;) {
+            bytes1 b = nameBytes[i];
+            // Valid strings are lower case a-z, 0-9, or a hyphen
+            require(
+                (b > 0x60 && b < 0x7B) || (b > 0x2F && b < 0x3A) || b == 0x2D,
+                "StringUtils: Invalid domain name"
+            );
+            unchecked {
+                ++i;
+            }
+        }
+    }
 }

hardhat.config.ts

@@ -71,7 +71,7 @@ const config : HardhatUserConfig = {
     timeout: 5000000,
   },
   gasReporter: {
-    enabled: true
+    enabled: false
   },
   networks: {
     mainnet: {

test/ZNSCurvePricer.test.ts

@@ -16,6 +16,7 @@ import {
 } from "./helpers";
 import { decimalsDefault, priceConfigDefault, registrationFeePercDefault } from "./helpers/constants";
 import {
+  INVALID_NAME_ERR,
   getAccessRevertMsg,
 } from "./helpers/errors";
 import { ADMIN_ROLE, GOVERNOR_ROLE } from "./helpers/access";
@@ -173,7 +174,7 @@ describe("ZNSCurvePricer", () => {
       // domains that are greater than base length + 1
       const short = "wild";
       const medium = "wilderworld";
-      const long = "wilderworld.beasts.pets.nfts.cats.calico.steve";
+      const long = "wilderworldbeastspetsnftscatscalicosteve";
 
       const expectedShortPrice = await calcCurvePrice(short, priceConfigDefault);
       const shortPrice = await zns.curvePricer.getPrice(domainHash, short);
@@ -188,18 +189,6 @@ describe("ZNSCurvePricer", () => {
       expect(expectedLongPrice).to.eq(longPrice);
     });
 
-    it("Prices Special Characters Accurately", async () => {
-      const domainSpecialCharacterSet1 = "±ƒc¢Ãv";
-      const domainSpecialCharacterSet2 = "œ柸þ€§ﾪ";
-      const domainWithoutSpecials = "abcdef";
-      const expectedPrice = calcCurvePrice(domainWithoutSpecials, priceConfigDefault);
-      let domainPrice = await zns.curvePricer.getPrice(domainHash, domainSpecialCharacterSet1);
-      expect(domainPrice).to.eq(expectedPrice);
-
-      (domainPrice = await zns.curvePricer.getPrice(domainHash, domainSpecialCharacterSet2));
-      expect(domainPrice).to.eq(expectedPrice);
-    });
-
     it("Can Price Names Longer Than 255 Characters", async () => {
       // 261 length
       const domain = "abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyz" +
@@ -257,6 +246,19 @@ describe("ZNSCurvePricer", () => {
   });
 
   describe("#setPriceConfig", () => {
+    it("Can't price a name that has invalid characters", async () => {
+      // Valid names must match the pattern [a-z0-9]
+      const labelA = "WILDER";
+      const labelB = "!?w1Id3r!?";
+      const labelC = "!%$#^*?!#👍3^29";
+      const labelD = "wo.rld";
+
+      await expect(zns.curvePricer.getPrice(domainHash, labelA)).to.be.revertedWith(INVALID_NAME_ERR);
+      await expect(zns.curvePricer.getPrice(domainHash, labelB)).to.be.revertedWith(INVALID_NAME_ERR);
+      await expect(zns.curvePricer.getPrice(domainHash, labelC)).to.be.revertedWith(INVALID_NAME_ERR);
+      await expect(zns.curvePricer.getPrice(domainHash, labelD)).to.be.revertedWith(INVALID_NAME_ERR);
+    });
+
     it("Should set the config for any existing domain hash, including 0x0", async () => {
       const newConfig = {
         baseLength: BigNumber.from("6"),

test/ZNSRootRegistrar.test.ts

@@ -21,9 +21,9 @@ import { BigNumber } from "ethers";
 import { defaultRootRegistration } from "./helpers/register-setup";
 import { checkBalance } from "./helpers/balances";
 import { precisionMultiDefault, priceConfigDefault, registrationFeePercDefault } from "./helpers/constants";
-import { calcCurvePrice, getPriceObject } from "./helpers/pricing";
+import { getPriceObject } from "./helpers/pricing";
 import { getDomainHashFromReceipt, getTokenIdFromReceipt } from "./helpers/events";
-import { getAccessRevertMsg } from "./helpers/errors";
+import { getAccessRevertMsg, INVALID_NAME_ERR } from "./helpers/errors";
 import { ADMIN_ROLE, GOVERNOR_ROLE } from "./helpers/access";
 import { ZNSRootRegistrar__factory, ZNSRootRegistrarUpgradeMock__factory } from "../typechain";
 import { PaymentConfigStruct } from "../typechain/contracts/treasury/IZNSTreasury";
@@ -330,6 +330,105 @@ describe("ZNSRootRegistrar", () => {
       ).to.be.revertedWith("ZNSRootRegistrar: Domain Name not provided");
     });
 
+    it("Can register a TLD with characters [a-z0-9-]", async () => {
+      const letters = "world";
+      const lettersHash = hashDomainLabel(letters);
+
+      const alphaNumeric = "0x0dwidler0x0";
+      const alphaNumericHash = hashDomainLabel(alphaNumeric);
+
+      const withHyphen = "0x0-dwidler-0x0";
+      const withHyphenHash = hashDomainLabel(withHyphen);
+
+      const tx1 = zns.rootRegistrar.connect(deployer).registerRootDomain(
+        letters,
+        ethers.constants.AddressZero,
+        defaultTokenURI,
+        distrConfigEmpty
+      );
+
+      await expect(tx1).to.emit(zns.rootRegistrar, "DomainRegistered").withArgs(
+        ethers.constants.HashZero,
+        lettersHash,
+        BigNumber.from(lettersHash),
+        letters,
+        deployer.address,
+        ethers.constants.AddressZero,
+      );
+
+      const tx2 = zns.rootRegistrar.connect(deployer).registerRootDomain(
+        alphaNumeric,
+        ethers.constants.AddressZero,
+        defaultTokenURI,
+        distrConfigEmpty
+      );
+
+      await expect(tx2).to.emit(zns.rootRegistrar, "DomainRegistered").withArgs(
+        ethers.constants.HashZero,
+        alphaNumericHash,
+        BigNumber.from(alphaNumericHash),
+        alphaNumeric,
+        deployer.address,
+        ethers.constants.AddressZero,
+      );
+
+      const tx3 = zns.rootRegistrar.connect(deployer).registerRootDomain(
+        withHyphen,
+        ethers.constants.AddressZero,
+        defaultTokenURI,
+        distrConfigEmpty
+      );
+
+      await expect(tx3).to.emit(zns.rootRegistrar, "DomainRegistered").withArgs(
+        ethers.constants.HashZero,
+        withHyphenHash,
+        BigNumber.from(withHyphenHash),
+        withHyphen,
+        deployer.address,
+        ethers.constants.AddressZero,
+      );
+    });
+
+    it("Fails for domains that use any invalid character", async () => {
+      // Valid names must match the pattern [a-z0-9]
+      const nameA = "WILDER";
+      const nameB = "!?w1Id3r!?";
+      const nameC = "!%$#^*?!#👍3^29";
+      const nameD = "wo.rld";
+
+      await expect(
+        defaultRootRegistration({
+          user: deployer,
+          zns,
+          domainName: nameA,
+        })
+      ).to.be.revertedWith(INVALID_NAME_ERR);
+
+      await expect(
+        defaultRootRegistration({
+          user: deployer,
+          zns,
+          domainName: nameB,
+        })
+      ).to.be.revertedWith(INVALID_NAME_ERR);
+
+      await expect(
+        defaultRootRegistration({
+          user: deployer,
+          zns,
+          domainName: nameC,
+        })
+      ).to.be.revertedWith(INVALID_NAME_ERR);
+
+      await expect(
+        defaultRootRegistration({
+          user: deployer,
+          zns,
+          domainName: nameD,
+        })
+      ).to.be.revertedWith(INVALID_NAME_ERR);
+    });
+
     // eslint-disable-next-line max-len
     it("Successfully registers a domain without a resolver or resolver content and fires a #DomainRegistered event", async () => {
       const tokenURI = "https://example.com/817c64af";
@@ -458,31 +557,6 @@ describe("ZNSRootRegistrar", () => {
       await expect(tx).to.be.revertedWith("ERC20: transfer amount exceeds balance");
     });
 
-    // eslint-disable-next-line max-len
-    it("Allows unicode characters in domain names and matches the hash of normalized string acquired from namehash library", async () => {
-      const unicodeDomainLabel = "œ柸þ€§ﾪ";
-
-      const normalizedDomainLabel = normalizeName(unicodeDomainLabel);
-
-      const tx = await defaultRootRegistration({
-        user,
-        zns,
-        domainName: normalizedDomainLabel,
-      });
-
-      const domainHash = await getDomainHashFromReceipt(tx);
-      // validate that namehash lib works the same way as our contract hashing
-      // TODO: a security issue with namehash lib is the usage of non-ASCII characters
-      //  this should be handled at the SDK/dApp level!
-      const namehashRef = hashDomainLabel(unicodeDomainLabel);
-      expect(domainHash).to.eq(namehashRef);
-      expect(await zns.registry.exists(domainHash)).to.be.true;
-
-      const expectedStaked = await calcCurvePrice(normalizedDomainLabel, priceConfigDefault);
-      const { amount: staked } = await zns.treasury.stakedForDomain(domainHash);
-      expect(expectedStaked).to.eq(staked);
-    });
-
     it("Disallows creation of a duplicate domain", async () => {
       await defaultRootRegistration({
         user,
@@ -596,14 +670,14 @@ describe("ZNSRootRegistrar", () => {
       await zns.domainToken.connect(deployer).transferFrom(deployer.address, user.address, tokenId);
 
       // Verify owner in registry
-      const originalOwner  = await zns.registry.connect(deployer).getDomainOwner(domainHash);
+      const originalOwner = await zns.registry.connect(deployer).getDomainOwner(domainHash);
       expect(originalOwner).to.equal(deployer.address);
 
       // Reclaim the Domain
       await zns.rootRegistrar.connect(user).reclaimDomain(domainHash);
 
       // Verify domain token is still owned
-      const owner  = await zns.domainToken.connect(user).ownerOf(tokenId);
+      const owner = await zns.domainToken.connect(user).ownerOf(tokenId);
       expect(owner).to.equal(user.address);
 
       // Verify domain is owned in registry
@@ -674,7 +748,7 @@ describe("ZNSRootRegistrar", () => {
       // Reclaim the Domain
       await zns.rootRegistrar.connect(user).reclaimDomain(domainHash);
       // Verify domain token is still owned
-      let owner  = await zns.domainToken.connect(user).ownerOf(tokenId);
+      let owner = await zns.domainToken.connect(user).ownerOf(tokenId);
       expect(owner).to.equal(user.address);
 
       // Transfer the domain token back
@@ -684,7 +758,7 @@ describe("ZNSRootRegistrar", () => {
       await zns.rootRegistrar.connect(deployer).reclaimDomain(domainHash);
 
       // Verify domain token is owned
-      owner  = await zns.domainToken.connect(deployer).ownerOf(tokenId);
+      owner = await zns.domainToken.connect(deployer).ownerOf(tokenId);
       expect(owner).to.equal(deployer.address);
 
       // Verify domain is owned in registrar
@@ -778,7 +852,7 @@ describe("ZNSRootRegistrar", () => {
     });
 
     it("Cannot revoke a domain that doesnt exist", async () => {
-    // Register Top level
+      // Register Top level
       const fakeHash = "0xd34cfa279afd55afc6aa9c00aa5d01df60179840a93d10eed730058b8dd4146c";
       const exists = await zns.registry.exists(fakeHash);
       expect(exists).to.be.false;
@@ -789,7 +863,7 @@ describe("ZNSRootRegistrar", () => {
     });
 
     it("Revoking domain unstakes", async () => {
-    // Verify Balance
+      // Verify Balance
       const balance = await zns.zeroToken.balanceOf(user.address);
       expect(balance).to.eq(userBalanceInitial);
 
@@ -826,7 +900,7 @@ describe("ZNSRootRegistrar", () => {
     });
 
     it("Cannot revoke if Name is owned by another user", async () => {
-    // Register Top level
+      // Register Top level
       const topLevelTx = await defaultRootRegistration({ user: deployer, zns, domainName: defaultDomain });
       const parentDomainHash = await getDomainHashFromReceipt(topLevelTx);
       const owner = await zns.registry.connect(user).getDomainOwner(parentDomainHash);