From cd7761f5daa3039b6d5b2b16d1345332422f6a2b Mon Sep 17 00:00:00 2001
From: James Earle <james@zero.tech>
Date: Tue, 24 Oct 2023 18:33:07 -0400
Subject: [PATCH 01/18] WIP work for addressing ZNS-2, lack of string
 validation

---
 contracts/price/ZNSCurvePricer.sol       |  9 ++++---
 contracts/registrar/ZNSRootRegistrar.sol | 30 +++++++++++++++++++++---
 contracts/utils/StringUtils.sol          | 23 ++++++++++++++++++
 hardhat.config.ts                        |  2 +-
 test/ZNSCurvePricer.test.ts              | 21 +++++++++++++++++
 test/ZNSRootRegistrar.test.ts            | 19 +++++++++++++++
 6 files changed, 97 insertions(+), 7 deletions(-)

diff --git a/contracts/price/ZNSCurvePricer.sol b/contracts/price/ZNSCurvePricer.sol
index a5a647c00..9c535e673 100644
--- a/contracts/price/ZNSCurvePricer.sol
+++ b/contracts/price/ZNSCurvePricer.sol
@@ -291,9 +291,12 @@ 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;
+        // To avoid an issue where the config might return less than the min price at a point,
+        // if (price < config.minPrice) return config.minPrice;
     }
 
     /**
diff --git a/contracts/registrar/ZNSRootRegistrar.sol b/contracts/registrar/ZNSRootRegistrar.sol
index c5afde960..2e3d5fa72 100644
--- a/contracts/registrar/ZNSRootRegistrar.sol
+++ b/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
@@ -82,18 +85,39 @@ contract ZNSRootRegistrar is
      *      but all the parameters inside are required.
      */
     function registerRootDomain(
-        string calldata name,
+        string memory name,
         address domainAddress,
         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);
+        // uint256 length = nameBytes.length;
+        // // if length < max int, do unchecked math on increment
+        // // only does the above check once instead of every increment
+        // uint256 MAX_INT = 0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff;
+        // require(length < MAX_INT, "ZNSRootRegistrar: Domain name too long");
+
+        // for(uint256 i; i < length;) {
+        //     require(
+        //         nameBytes[i] > 0x60 && nameBytes[i] < 0x7B,
+        //         "ZNSRootRegistrar: Invalid domain name"
+        //     );
+        //     unchecked {
+        //         ++i;
+        //     }
+        // }
+
+        // perform validation of the name
+        // no capitals or . characters
         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),
diff --git a/contracts/utils/StringUtils.sol b/contracts/utils/StringUtils.sol
index a02703d9b..932311f51 100644
--- a/contracts/utils/StringUtils.sol
+++ b/contracts/utils/StringUtils.sol
@@ -33,4 +33,27 @@ 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];
+            require(
+                (b > 0x60 && b < 0x7B) || (b > 0x2F && b < 0x3A),
+                "ZNSRootRegistrar: Invalid domain name"
+            );
+            unchecked {
+                ++i;
+            }
+        }
+    }
 }
\ No newline at end of file
diff --git a/hardhat.config.ts b/hardhat.config.ts
index a93f8c633..354b135bf 100644
--- a/hardhat.config.ts
+++ b/hardhat.config.ts
@@ -71,7 +71,7 @@ const config : HardhatUserConfig = {
     timeout: 5000000,
   },
   gasReporter: {
-    enabled: true
+    enabled: false
   },
   networks: {
     mainnet: {
diff --git a/test/ZNSCurvePricer.test.ts b/test/ZNSCurvePricer.test.ts
index 8dd6b0c48..83c610690 100644
--- a/test/ZNSCurvePricer.test.ts
+++ b/test/ZNSCurvePricer.test.ts
@@ -257,6 +257,27 @@ describe("ZNSCurvePricer", () => {
   });
 
   describe("#setPriceConfig", () => {
+    // it.only("Specific scenario from the audit", async () => {
+    //   const newConfig = {
+    //     baseLength: BigNumber.from("5"),
+    //     maxLength: BigNumber.from("10"),
+    //     maxPrice: parseEther("10"),
+    //     minPrice: parseEther("5.5"),
+    //     precisionMultiplier: precisionMultiDefault,
+    //     feePercentage: registrationFeePercDefault,
+    //   };
+
+    //   await zns.curvePricer.connect(user).setPriceConfig(domainHash, newConfig);
+
+    //   const nineLength = "abcdefghi";
+    //   const tenLength = "abcdefghij";
+
+    //   const nPrice = await zns.curvePricer.getPrice(domainHash, nineLength);
+    //   const tPrice = await zns.curvePricer.getPrice(domainHash, tenLength);
+
+    //   console.log(nPrice.toString());
+    //   console.log(tPrice.toString());
+    // });
     it("Should set the config for any existing domain hash, including 0x0", async () => {
       const newConfig = {
         baseLength: BigNumber.from("6"),
diff --git a/test/ZNSRootRegistrar.test.ts b/test/ZNSRootRegistrar.test.ts
index 9594d1fc0..0369d3541 100644
--- a/test/ZNSRootRegistrar.test.ts
+++ b/test/ZNSRootRegistrar.test.ts
@@ -330,6 +330,25 @@ describe("ZNSRootRegistrar", () => {
       ).to.be.revertedWith("ZNSRootRegistrar: Domain Name not provided");
     });
 
+    it.only("Can only register a TLD with characters [a-z]", async () => {
+      const longname = "wilderwilderwilderwilderwilderwilderwilderwilderwilder";
+      const shortname = "dwidler";
+
+      let tx1 = await defaultRootRegistration({
+        user: deployer,
+        zns,
+        domainName: shortname,
+      });
+      console.log(`short: ${tx1.gasUsed.toString()}`);
+
+      let tx2 = await defaultRootRegistration({
+        user: deployer,
+        zns,
+        domainName: longname,
+      });
+      console.log(`long: ${tx2.gasUsed.toString()}`);
+    });
+
     // 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";

From 68ceeb9623a98545aa75463e7e9c2a909d4f1f34 Mon Sep 17 00:00:00 2001
From: James Earle <james@zero.tech>
Date: Wed, 25 Oct 2023 11:36:54 -0400
Subject: [PATCH 02/18] audit fix for string validation

---
 contracts/price/ZNSCurvePricer.sol       |   3 +
 contracts/registrar/ZNSRootRegistrar.sol |  20 +---
 contracts/registrar/ZNSSubRegistrar.sol  |   5 +
 contracts/utils/StringUtils.sol          |   2 +-
 test/ZNSCurvePricer.test.ts              |  49 +++------
 test/ZNSRootRegistrar.test.ts            | 134 ++++++++++++++---------
 test/ZNSSubRegistrar.test.ts             |  80 +++++++++++++-
 test/gas/gas-costs.json                  |   4 +-
 test/helpers/errors.ts                   |   3 +
 9 files changed, 195 insertions(+), 105 deletions(-)

diff --git a/contracts/price/ZNSCurvePricer.sol b/contracts/price/ZNSCurvePricer.sol
index 9c535e673..9da499a45 100644
--- a/contracts/price/ZNSCurvePricer.sol
+++ b/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;
diff --git a/contracts/registrar/ZNSRootRegistrar.sol b/contracts/registrar/ZNSRootRegistrar.sol
index 2e3d5fa72..d7e721746 100644
--- a/contracts/registrar/ZNSRootRegistrar.sol
+++ b/contracts/registrar/ZNSRootRegistrar.sol
@@ -92,25 +92,9 @@ contract ZNSRootRegistrar is
     ) external override returns (bytes32) {
         // Confirms string values are only [a-z0-9]
         name.validate();
+
         bytes memory nameBytes = bytes(name);
-        // uint256 length = nameBytes.length;
-        // // if length < max int, do unchecked math on increment
-        // // only does the above check once instead of every increment
-        // uint256 MAX_INT = 0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff;
-        // require(length < MAX_INT, "ZNSRootRegistrar: Domain name too long");
-
-        // for(uint256 i; i < length;) {
-        //     require(
-        //         nameBytes[i] > 0x60 && nameBytes[i] < 0x7B,
-        //         "ZNSRootRegistrar: Invalid domain name"
-        //     );
-        //     unchecked {
-        //         ++i;
-        //     }
-        // }
-
-        // perform validation of the name
-        // no capitals or . characters
+
         require(
             nameBytes.length != 0,
             "ZNSRootRegistrar: Domain Name not provided"
diff --git a/contracts/registrar/ZNSSubRegistrar.sol b/contracts/registrar/ZNSSubRegistrar.sol
index dd0dd03fe..b663b2684 100644
--- a/contracts/registrar/ZNSSubRegistrar.sol
+++ b/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),
diff --git a/contracts/utils/StringUtils.sol b/contracts/utils/StringUtils.sol
index 932311f51..30ab44370 100644
--- a/contracts/utils/StringUtils.sol
+++ b/contracts/utils/StringUtils.sol
@@ -49,7 +49,7 @@ library StringUtils {
             bytes1 b = nameBytes[i];
             require(
                 (b > 0x60 && b < 0x7B) || (b > 0x2F && b < 0x3A),
-                "ZNSRootRegistrar: Invalid domain name"
+                "StringUtils: Invalid domain name"
             );
             unchecked {
                 ++i;
diff --git a/test/ZNSCurvePricer.test.ts b/test/ZNSCurvePricer.test.ts
index 83c610690..1a75caa39 100644
--- a/test/ZNSCurvePricer.test.ts
+++ b/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,27 +246,19 @@ describe("ZNSCurvePricer", () => {
   });
 
   describe("#setPriceConfig", () => {
-    // it.only("Specific scenario from the audit", async () => {
-    //   const newConfig = {
-    //     baseLength: BigNumber.from("5"),
-    //     maxLength: BigNumber.from("10"),
-    //     maxPrice: parseEther("10"),
-    //     minPrice: parseEther("5.5"),
-    //     precisionMultiplier: precisionMultiDefault,
-    //     feePercentage: registrationFeePercDefault,
-    //   };
-
-    //   await zns.curvePricer.connect(user).setPriceConfig(domainHash, newConfig);
-
-    //   const nineLength = "abcdefghi";
-    //   const tenLength = "abcdefghij";
-
-    //   const nPrice = await zns.curvePricer.getPrice(domainHash, nineLength);
-    //   const tPrice = await zns.curvePricer.getPrice(domainHash, tenLength);
-
-    //   console.log(nPrice.toString());
-    //   console.log(tPrice.toString());
-    // });
+    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"),
diff --git a/test/ZNSRootRegistrar.test.ts b/test/ZNSRootRegistrar.test.ts
index 0369d3541..921fda4b0 100644
--- a/test/ZNSRootRegistrar.test.ts
+++ b/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,23 +330,84 @@ describe("ZNSRootRegistrar", () => {
       ).to.be.revertedWith("ZNSRootRegistrar: Domain Name not provided");
     });
 
-    it.only("Can only register a TLD with characters [a-z]", async () => {
-      const longname = "wilderwilderwilderwilderwilderwilderwilderwilderwilder";
-      const shortname = "dwidler";
+    it("Can register a TLD with characters [a-z0-9]", async () => {
+      const letters = "world";
+      const lettersHash = hashDomainLabel(letters);
 
-      let tx1 = await defaultRootRegistration({
-        user: deployer,
-        zns,
-        domainName: shortname,
-      });
-      console.log(`short: ${tx1.gasUsed.toString()}`);
+      const alphaNumeric = "0x0dwidler0x0";
+      const alphaNumericHash = hashDomainLabel(alphaNumeric);
 
-      let tx2 = await defaultRootRegistration({
-        user: deployer,
-        zns,
-        domainName: longname,
-      });
-      console.log(`long: ${tx2.gasUsed.toString()}`);
+      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,
+      );
+    });
+
+    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
@@ -477,31 +538,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,
@@ -615,14 +651,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
@@ -693,7 +729,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
@@ -703,7 +739,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
@@ -797,7 +833,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;
@@ -808,7 +844,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);
 
@@ -845,7 +881,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);
diff --git a/test/ZNSSubRegistrar.test.ts b/test/ZNSSubRegistrar.test.ts
index 3c53b7c37..f12500f9c 100644
--- a/test/ZNSSubRegistrar.test.ts
+++ b/test/ZNSSubRegistrar.test.ts
@@ -13,6 +13,7 @@ import {
   getStakingOrProtocolFee,
   GOVERNOR_ROLE,
   INITIALIZED_ERR,
+  INVALID_NAME_ERR,
   INVALID_TOKENID_ERC_ERR,
   ONLY_NAME_OWNER_REG_ERR,
   PaymentType,
@@ -26,7 +27,7 @@ import { BigNumber } from "ethers";
 import { expect } from "chai";
 import { registerDomainPath, validatePathRegistration } from "./helpers/flows/registration";
 import assert from "assert";
-import { registrationWithSetup } from "./helpers/register-setup";
+import { defaultSubdomainRegistration, registrationWithSetup } from "./helpers/register-setup";
 import { getDomainHashFromEvent } from "./helpers/events";
 import { time } from "@nomicfoundation/hardhat-network-helpers";
 import { CustomDecimalTokenMock, ZNSSubRegistrarUpgradeMock__factory } from "../typechain";
@@ -139,6 +140,83 @@ describe("ZNSSubRegistrar", () => {
       expect(tokenURI).to.eq(subTokenURI);
     });
 
+    it("Can register a subdomain with characters [a-z0-9]", async () => {
+      const alphaNumeric = "0x0dwidler0x0";
+
+      // Add allowance
+      await zns.zeroToken.connect(lvl2SubOwner).approve(zns.treasury.address, ethers.constants.MaxUint256);
+
+      // While "to.not.be.reverted" isn't really a full "test"
+      // we don't emit a custom event here, only in the `rootRegistrar.coreRegister`
+      // call. So we can't use the `.to.emit` syntax
+      await expect(defaultSubdomainRegistration(
+        {
+          user: lvl2SubOwner,
+          zns,
+          parentHash: rootHash,
+          subdomainLabel: alphaNumeric,
+          domainContent: lvl2SubOwner.address,
+          tokenURI: subTokenURI,
+          distrConfig: distrConfigEmpty,
+        }
+      )).to.not.be.reverted;
+    });
+
+    it("Fails for a subdomain that uses any invalid characters", async () => {
+      const nameA = "WILDER";
+      const nameB = "!?w1Id3r!?";
+      const nameC = "!%$#^*?!#👍3^29";
+      const nameD = "wo.rld";
+
+      await expect(defaultSubdomainRegistration(
+        {
+          user: lvl2SubOwner,
+          zns,
+          parentHash: rootHash,
+          subdomainLabel: nameA,
+          domainContent: lvl2SubOwner.address,
+          tokenURI: subTokenURI,
+          distrConfig: distrConfigEmpty,
+        }
+      )).to.be.revertedWith(INVALID_NAME_ERR);
+
+      await expect(defaultSubdomainRegistration(
+        {
+          user: lvl2SubOwner,
+          zns,
+          parentHash: rootHash,
+          subdomainLabel: nameB,
+          domainContent: lvl2SubOwner.address,
+          tokenURI: subTokenURI,
+          distrConfig: distrConfigEmpty,
+        }
+      )).to.be.revertedWith(INVALID_NAME_ERR);
+
+      await expect(defaultSubdomainRegistration(
+        {
+          user: lvl2SubOwner,
+          zns,
+          parentHash: rootHash,
+          subdomainLabel: nameC,
+          domainContent: lvl2SubOwner.address,
+          tokenURI: subTokenURI,
+          distrConfig: distrConfigEmpty,
+        }
+      )).to.be.revertedWith(INVALID_NAME_ERR);
+
+      await expect(defaultSubdomainRegistration(
+        {
+          user: lvl2SubOwner,
+          zns,
+          parentHash: rootHash,
+          subdomainLabel: nameD,
+          domainContent: lvl2SubOwner.address,
+          tokenURI: subTokenURI,
+          distrConfig: distrConfigEmpty,
+        }
+      )).to.be.revertedWith(INVALID_NAME_ERR);
+    });
+
     it("should revert when trying to register a subdomain under a non-existent parent", async () => {
       // check that 0x0 hash can NOT be passed as parentHash
       await expect(
diff --git a/test/gas/gas-costs.json b/test/gas/gas-costs.json
index 52b8fd0d0..a6a1bc9c4 100644
--- a/test/gas/gas-costs.json
+++ b/test/gas/gas-costs.json
@@ -1,4 +1,4 @@
 {
-	"Root Domain Price": "425483",
-	"Subdomain Price": "419495"
+	"Root Domain Price": "427687",
+	"Subdomain Price": "423989"
 }
\ No newline at end of file
diff --git a/test/helpers/errors.ts b/test/helpers/errors.ts
index e582d6846..30cbc32d7 100644
--- a/test/helpers/errors.ts
+++ b/test/helpers/errors.ts
@@ -28,6 +28,9 @@ export const NOT_BOTH_OWNER_RAR_ERR = "ZNSRootRegistrar: Not the owner of both N
 // eslint-disable-next-line max-len
 export const DISTRIBUTION_LOCKED_NOT_EXIST_ERR = "ZNSSubRegistrar: Parent domain's distribution is locked or parent does not exist";
 
+// StringUtils
+export const INVALID_NAME_ERR = "StringUtils: Invalid domain name";
+
 // OpenZeppelin
 export const INVALID_TOKENID_ERC_ERR = "ERC721: invalid token ID";
 export const INITIALIZED_ERR = "Initializable: contract is already initialized";

From b07031af484a3d838464da5e406328ad68a8b2a4 Mon Sep 17 00:00:00 2001
From: James Earle <james@zero.tech>
Date: Wed, 25 Oct 2023 11:43:41 -0400
Subject: [PATCH 03/18] remove comment in curve pricer

---
 contracts/price/ZNSCurvePricer.sol | 2 --
 1 file changed, 2 deletions(-)

diff --git a/contracts/price/ZNSCurvePricer.sol b/contracts/price/ZNSCurvePricer.sol
index 9da499a45..54e8954a2 100644
--- a/contracts/price/ZNSCurvePricer.sol
+++ b/contracts/price/ZNSCurvePricer.sol
@@ -298,8 +298,6 @@ contract ZNSCurvePricer is AAccessControlled, ARegistryWired, UUPSUpgradeable, I
             / config.precisionMultiplier * config.precisionMultiplier;
 
         return price;
-        // To avoid an issue where the config might return less than the min price at a point,
-        // if (price < config.minPrice) return config.minPrice;
     }
 
     /**

From f7ca2a2a461975d13fdf09a9961c886906e695ec Mon Sep 17 00:00:00 2001
From: James Earle <james@zero.tech>
Date: Wed, 25 Oct 2023 11:45:46 -0400
Subject: [PATCH 04/18] change string  back to calldata

---
 contracts/registrar/ZNSRootRegistrar.sol | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contracts/registrar/ZNSRootRegistrar.sol b/contracts/registrar/ZNSRootRegistrar.sol
index d7e721746..e03616b2f 100644
--- a/contracts/registrar/ZNSRootRegistrar.sol
+++ b/contracts/registrar/ZNSRootRegistrar.sol
@@ -85,7 +85,7 @@ contract ZNSRootRegistrar is
      *      but all the parameters inside are required.
      */
     function registerRootDomain(
-        string memory name,
+        string calldata name,
         address domainAddress,
         string calldata tokenURI,
         DistributionConfig calldata distributionConfig

From 34ffddae1cb928140c62703c08798af26eadf17a Mon Sep 17 00:00:00 2001
From: James Earle <james@zero.tech>
Date: Wed, 25 Oct 2023 14:07:34 -0400
Subject: [PATCH 05/18] allow hyphen character as well

---
 contracts/utils/StringUtils.sol |  3 ++-
 test/ZNSRootRegistrar.test.ts   | 21 ++++++++++++++++++++-
 2 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/contracts/utils/StringUtils.sol b/contracts/utils/StringUtils.sol
index 30ab44370..e07af49cf 100644
--- a/contracts/utils/StringUtils.sol
+++ b/contracts/utils/StringUtils.sol
@@ -47,8 +47,9 @@ library StringUtils {
 
         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 > 0x60 && b < 0x7B) || (b > 0x2F && b < 0x3A) || b == 0x2D,
                 "StringUtils: Invalid domain name"
             );
             unchecked {
diff --git a/test/ZNSRootRegistrar.test.ts b/test/ZNSRootRegistrar.test.ts
index 921fda4b0..d6b1dd9ac 100644
--- a/test/ZNSRootRegistrar.test.ts
+++ b/test/ZNSRootRegistrar.test.ts
@@ -330,13 +330,16 @@ describe("ZNSRootRegistrar", () => {
       ).to.be.revertedWith("ZNSRootRegistrar: Domain Name not provided");
     });
 
-    it("Can register a TLD with characters [a-z0-9]", async () => {
+    it.only("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,
@@ -368,6 +371,22 @@ describe("ZNSRootRegistrar", () => {
         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 () => {

From 02f22a51ef63f428e158a217f1617bf1154c3a0c Mon Sep 17 00:00:00 2001
From: James Earle <james@zero.tech>
Date: Wed, 25 Oct 2023 14:11:17 -0400
Subject: [PATCH 06/18] remove .only

---
 test/ZNSRootRegistrar.test.ts | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/test/ZNSRootRegistrar.test.ts b/test/ZNSRootRegistrar.test.ts
index d6b1dd9ac..10d74de26 100644
--- a/test/ZNSRootRegistrar.test.ts
+++ b/test/ZNSRootRegistrar.test.ts
@@ -330,7 +330,7 @@ describe("ZNSRootRegistrar", () => {
       ).to.be.revertedWith("ZNSRootRegistrar: Domain Name not provided");
     });
 
-    it.only("Can register a TLD with characters [a-z0-9-]", async () => {
+    it("Can register a TLD with characters [a-z0-9-]", async () => {
       const letters = "world";
       const lettersHash = hashDomainLabel(letters);
 

From 497298cb4ecbe0611b62b307d26f2e715c36d2bd Mon Sep 17 00:00:00 2001
From: Kirill <dor7an@gmail.com>
Date: Mon, 30 Oct 2023 14:34:10 -0700
Subject: [PATCH 07/18] remove extra variable step in CurvePricer.getPrice()

---
 contracts/price/ZNSCurvePricer.sol | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/contracts/price/ZNSCurvePricer.sol b/contracts/price/ZNSCurvePricer.sol
index 54e8954a2..b12aae182 100644
--- a/contracts/price/ZNSCurvePricer.sol
+++ b/contracts/price/ZNSCurvePricer.sol
@@ -294,10 +294,8 @@ contract ZNSCurvePricer is AAccessControlled, ARegistryWired, UUPSUpgradeable, I
         if (length <= config.baseLength) return config.maxPrice;
         if (length > config.maxLength) return config.minPrice;
 
-        uint256 price = (config.baseLength * config.maxPrice / length) 
+        return (config.baseLength * config.maxPrice / length)
             / config.precisionMultiplier * config.precisionMultiplier;
-
-        return price;
     }
 
     /**

From 203260cb7c6782dafef4913c008804870ded58b1 Mon Sep 17 00:00:00 2001
From: Kirill <dor7an@gmail.com>
Date: Mon, 30 Oct 2023 14:36:16 -0700
Subject: [PATCH 08/18] fix revert msg in StringUtils

---
 contracts/utils/StringUtils.sol | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/contracts/utils/StringUtils.sol b/contracts/utils/StringUtils.sol
index e07af49cf..f678e9c8b 100644
--- a/contracts/utils/StringUtils.sol
+++ b/contracts/utils/StringUtils.sol
@@ -35,7 +35,7 @@ library StringUtils {
     }
 
     /**
-     * @dev Confirm that a given string has only alphanumeric characters [a-z0-9]
+     * @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 {
@@ -43,7 +43,7 @@ library StringUtils {
         uint256 length = nameBytes.length;
 
         uint256 MAX_INT = 0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff;
-        require(length < MAX_INT, "ZNSRootRegistrar: Domain name too long");
+        require(length < MAX_INT, "StringUtils: Domain name too long");
 
         for (uint256 i; i < length;) {
             bytes1 b = nameBytes[i];

From 55f41e46408045abe35e8ed1fcb827c4721fafff Mon Sep 17 00:00:00 2001
From: Kirill <dor7an@gmail.com>
Date: Mon, 30 Oct 2023 15:28:24 -0700
Subject: [PATCH 09/18] add zero length check to StringUtils.validate(), fix
 revert messages

---
 contracts/utils/StringUtils.sol | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/contracts/utils/StringUtils.sol b/contracts/utils/StringUtils.sol
index f678e9c8b..f40955ac0 100644
--- a/contracts/utils/StringUtils.sol
+++ b/contracts/utils/StringUtils.sol
@@ -43,14 +43,17 @@ library StringUtils {
         uint256 length = nameBytes.length;
 
         uint256 MAX_INT = 0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff;
-        require(length < MAX_INT, "StringUtils: Domain name too long");
+        require(
+            length > 0 && length < MAX_INT,
+            "StringUtils: Domain label too long or nonexistent"
+        );
 
         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"
+                "StringUtils: Invalid domain label"
             );
             unchecked {
                 ++i;

From 951e64e58f634bac0666e3c4b344fbaa3d42beef Mon Sep 17 00:00:00 2001
From: Kirill <dor7an@gmail.com>
Date: Mon, 30 Oct 2023 15:29:24 -0700
Subject: [PATCH 10/18] add a bool flag to CurvePricer.getPrice() to skip
 double validation when registering, add comments

---
 contracts/price/ZNSCurvePricer.sol | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/contracts/price/ZNSCurvePricer.sol b/contracts/price/ZNSCurvePricer.sol
index b12aae182..fa503cf21 100644
--- a/contracts/price/ZNSCurvePricer.sol
+++ b/contracts/price/ZNSCurvePricer.sol
@@ -54,15 +54,26 @@ contract ZNSCurvePricer is AAccessControlled, ARegistryWired, UUPSUpgradeable, I
 
     /**
      * @notice Get the price of a given domain name
+     * @dev `skipValidityCheck` param is added to provide proper revert when the user is
+     * calling this to find out the price of a domain that is not valid. But in Registrar contracts
+     * we want to do this explicitly and before we get the price to have lower tx cost for reverted tx.
+     * So Registrars will pass this bool as "true" to not repeat the validity check.
+     * Note that if calling this function directly to find out the price, a user should always pass "false"
+     * as `skipValidityCheck` param, otherwise, the price will be returned for an invalid label that is not
+     * possible to register.
      * @param parentHash The hash of the parent domain under which price is determined
      * @param label The label of the subdomain candidate to get the price for before/during registration
+     * @param skipValidityCheck If true, skips the validity check for the label
      */
     function getPrice(
         bytes32 parentHash,
-        string calldata label
+        string calldata label,
+        bool skipValidityCheck
     ) public view override returns (uint256) {
-        // Confirms string values are only [a-z0-9]
-        label.validate();
+        if (!skipValidityCheck) {
+            // Confirms string values are only [a-z0-9-]
+            label.validate();
+        }
 
         uint256 length = label.strlen();
         // No pricing is set for 0 length domains
@@ -94,9 +105,10 @@ contract ZNSCurvePricer is AAccessControlled, ARegistryWired, UUPSUpgradeable, I
     */
     function getPriceAndFee(
         bytes32 parentHash,
-        string calldata label
+        string calldata label,
+        bool skipValidityCheck
     ) external view override returns (uint256 price, uint256 stakeFee) {
-        price = getPrice(parentHash, label);
+        price = getPrice(parentHash, label, skipValidityCheck);
         stakeFee = getFeeForPrice(parentHash, price);
         return (price, stakeFee);
     }

From f0c63187239f7953b887643fe27196efa2a97fb7 Mon Sep 17 00:00:00 2001
From: Kirill <dor7an@gmail.com>
Date: Mon, 30 Oct 2023 15:29:47 -0700
Subject: [PATCH 11/18] bring validity check parity for FixedPricer

---
 contracts/price/ZNSFixedPricer.sol | 30 +++++++++++++++++++++++++-----
 1 file changed, 25 insertions(+), 5 deletions(-)

diff --git a/contracts/price/ZNSFixedPricer.sol b/contracts/price/ZNSFixedPricer.sol
index 6b1611bb8..f393f03a1 100644
--- a/contracts/price/ZNSFixedPricer.sol
+++ b/contracts/price/ZNSFixedPricer.sol
@@ -5,6 +5,7 @@ import { AAccessControlled } from "../access/AAccessControlled.sol";
 import { ARegistryWired } from "../registry/ARegistryWired.sol";
 import { IZNSFixedPricer } from "./IZNSFixedPricer.sol";
 import { UUPSUpgradeable } from "@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol";
+import { StringUtils } from "../utils/StringUtils.sol";
 
 
 /**
@@ -12,6 +13,7 @@ import { UUPSUpgradeable } from "@openzeppelin/contracts-upgradeable/proxy/utils
  * that doesn't depend on the length of the label.
 */
 contract ZNSFixedPricer is AAccessControlled, ARegistryWired, UUPSUpgradeable, IZNSFixedPricer {
+    using StringUtils for string;
 
     uint256 public constant PERCENTAGE_BASIS = 10000;
 
@@ -25,7 +27,6 @@ contract ZNSFixedPricer is AAccessControlled, ARegistryWired, UUPSUpgradeable, I
         setRegistry(_registry);
     }
 
-    // TODO audit question: should we add onlyProxy modifiers for every function in proxied contracts ??
     /**
      * @notice Sets the price for a domain. Only callable by domain owner/operator. Emits a `PriceSet` event.
      * @param domainHash The hash of the domain who sets the price for subdomains
@@ -37,16 +38,33 @@ contract ZNSFixedPricer is AAccessControlled, ARegistryWired, UUPSUpgradeable, I
 
     /**
      * @notice Gets the price for a subdomain candidate label under the parent domain.
+     * @dev `skipValidityCheck` param is added to provide proper revert when the user is
+     * calling this to find out the price of a domain that is not valid. But in Registrar contracts
+     * we want to do this explicitly and before we get the price to have lower tx cost for reverted tx.
+     * So Registrars will pass this bool as "true" to not repeat the validity check.
+     * Note that if calling this function directly to find out the price, a user should always pass "false"
+     * as `skipValidityCheck` param, otherwise, the price will be returned for an invalid label that is not
+     * possible to register.
      * @param parentHash The hash of the parent domain to check the price under
      * @param label The label of the subdomain candidate to check the price for
+     * @param skipValidityCheck If true, skips the validity check for the label
     */
     // solhint-disable-next-line no-unused-vars
-    function getPrice(bytes32 parentHash, string calldata label) public override view returns (uint256) {
+    function getPrice(
+        bytes32 parentHash,
+        string calldata label,
+        bool skipValidityCheck
+    ) public override view returns (uint256) {
+        if (!skipValidityCheck) {
+            // Confirms string values are only [a-z0-9-]
+            label.validate();
+        }
+
         return priceConfigs[parentHash].price;
     }
 
     /**
-     * @notice Sets the feePercentage for a domain. Only callable by domain owner/operator. 
+     * @notice Sets the feePercentage for a domain. Only callable by domain owner/operator.
      * Emits a `FeePercentageSet` event.
      * @dev `feePercentage` is set as a part of the `PERCENTAGE_BASIS` of 10,000 where 1% = 100
      * @param domainHash The hash of the domain who sets the feePercentage for subdomains
@@ -94,12 +112,14 @@ contract ZNSFixedPricer is AAccessControlled, ARegistryWired, UUPSUpgradeable, I
      * under the given parent.
      * @param parentHash The hash of the parent domain under which price and fee are determined
      * @param label The label of the subdomain candidate to get the price and fee for before/during registration
+     * @param skipValidityCheck If true, skips the validity check for the label
     */
     function getPriceAndFee(
         bytes32 parentHash,
-        string calldata label
+        string calldata label,
+        bool skipValidityCheck
     ) external view override returns (uint256 price, uint256 fee) {
-        price = getPrice(parentHash, label);
+        price = getPrice(parentHash, label, skipValidityCheck);
         fee = getFeeForPrice(parentHash, price);
         return (price, fee);
     }

From ba0f5cf5df9fd28a021d5e8114e23aa826ea1d45 Mon Sep 17 00:00:00 2001
From: Kirill <dor7an@gmail.com>
Date: Mon, 30 Oct 2023 15:30:25 -0700
Subject: [PATCH 12/18] add a bool to all getPrice calls from Registrars

---
 contracts/registrar/ZNSRootRegistrar.sol | 13 +++----------
 contracts/registrar/ZNSSubRegistrar.sol  | 11 +++++------
 2 files changed, 8 insertions(+), 16 deletions(-)

diff --git a/contracts/registrar/ZNSRootRegistrar.sol b/contracts/registrar/ZNSRootRegistrar.sol
index e03616b2f..290c663bc 100644
--- a/contracts/registrar/ZNSRootRegistrar.sol
+++ b/contracts/registrar/ZNSRootRegistrar.sol
@@ -90,18 +90,11 @@ contract ZNSRootRegistrar is
         string calldata tokenURI,
         DistributionConfig calldata distributionConfig
     ) external override returns (bytes32) {
-        // Confirms string values are only [a-z0-9]
+        // Confirms string values are only [a-z0-9-]
         name.validate();
 
-        bytes memory nameBytes = bytes(name);
-
-        require(
-            nameBytes.length != 0,
-            "ZNSRootRegistrar: Domain Name not provided"
-        );
-
         // Create hash for given domain name
-        bytes32 domainHash = keccak256(nameBytes);
+        bytes32 domainHash = keccak256(bytes(name));
 
         require(
             !registry.exists(domainHash),
@@ -109,7 +102,7 @@ contract ZNSRootRegistrar is
         );
 
         // Get price for the domain
-        uint256 domainPrice = rootPricer.getPrice(0x0, name);
+        uint256 domainPrice = rootPricer.getPrice(0x0, name, true);
 
         _coreRegister(
             CoreRegisterArgs(
diff --git a/contracts/registrar/ZNSSubRegistrar.sol b/contracts/registrar/ZNSSubRegistrar.sol
index b663b2684..53266456c 100644
--- a/contracts/registrar/ZNSSubRegistrar.sol
+++ b/contracts/registrar/ZNSSubRegistrar.sol
@@ -76,7 +76,7 @@ contract ZNSSubRegistrar is AAccessControlled, ARegistryWired, UUPSUpgradeable,
         string calldata tokenURI,
         DistributionConfig calldata distrConfig
     ) external override returns (bytes32) {
-        // Confirms string values are only [a-z0-9]
+        // Confirms string values are only [a-z0-9-]
         label.validate();
 
         bytes32 domainHash = hashWithParent(parentHash, label);
@@ -117,13 +117,15 @@ contract ZNSSubRegistrar is AAccessControlled, ARegistryWired, UUPSUpgradeable,
                 (coreRegisterArgs.price, coreRegisterArgs.stakeFee) = IZNSPricer(address(parentConfig.pricerContract))
                     .getPriceAndFee(
                         parentHash,
-                        label
+                        label,
+                        true
                     );
             } else {
                 coreRegisterArgs.price = IZNSPricer(address(parentConfig.pricerContract))
                     .getPrice(
                         parentHash,
-                        label
+                        label,
+                        true
                     );
             }
         }
@@ -192,9 +194,6 @@ contract ZNSSubRegistrar is AAccessControlled, ARegistryWired, UUPSUpgradeable,
     */
     function setPricerContractForDomain(
         bytes32 domainHash,
-        // TODO audit question: is this a problem that we expect the simplest interface
-        //  but are able set any of the derived ones ??
-        //  Can someone by setting their own contract here introduce a vulnerability ??
         IZNSPricer pricerContract
     ) public override {
         require(

From 943bbdb2882f35c10d5814ebf5afe9ec7190c1c0 Mon Sep 17 00:00:00 2001
From: Kirill <dor7an@gmail.com>
Date: Mon, 30 Oct 2023 15:30:50 -0700
Subject: [PATCH 13/18] fix IZNSPricer and update SubRegistrarMock

---
 contracts/types/IZNSPricer.sol                         |  6 ++++--
 .../distribution/ZNSSubRegistrarMock.sol               | 10 ++++++++--
 2 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/contracts/types/IZNSPricer.sol b/contracts/types/IZNSPricer.sol
index 848afcea2..e6c6aaabe 100644
--- a/contracts/types/IZNSPricer.sol
+++ b/contracts/types/IZNSPricer.sol
@@ -13,7 +13,8 @@ interface IZNSPricer {
      */
     function getPrice(
         bytes32 parentHash,
-        string calldata label
+        string calldata label,
+        bool skipValidityCheck
     ) external view returns (uint256);
 
     /**
@@ -23,7 +24,8 @@ interface IZNSPricer {
      */
     function getPriceAndFee(
         bytes32 parentHash,
-        string calldata label
+        string calldata label,
+        bool skipValidityCheck
     ) external view returns (uint256 price, uint256 fee);
 
     /**
diff --git a/contracts/upgrade-test-mocks/distribution/ZNSSubRegistrarMock.sol b/contracts/upgrade-test-mocks/distribution/ZNSSubRegistrarMock.sol
index f51b19fe2..572ca39bf 100644
--- a/contracts/upgrade-test-mocks/distribution/ZNSSubRegistrarMock.sol
+++ b/contracts/upgrade-test-mocks/distribution/ZNSSubRegistrarMock.sol
@@ -9,6 +9,8 @@ import { IZNSRootRegistrar, CoreRegisterArgs } from "../../registrar/IZNSRootReg
 import { AAccessControlled } from "../../access/AAccessControlled.sol";
 import { ARegistryWired } from "../../registry/ARegistryWired.sol";
 import { UUPSUpgradeable } from "@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol";
+import { StringUtils } from "../../utils/StringUtils.sol";
+
 
 enum AccessType {
     LOCKED,
@@ -72,6 +74,8 @@ contract ZNSSubRegistrarUpgradeMock is
         string memory tokenURI,
         DistributionConfig calldata distrConfig
     ) external returns (bytes32) {
+        label.validate();
+
         DistributionConfig memory parentConfig = distrConfigs[parentHash];
 
         bool isOwnerOrOperator = registry.isOwnerOrOperator(parentHash, msg.sender);
@@ -109,13 +113,15 @@ contract ZNSSubRegistrarUpgradeMock is
                 (coreRegisterArgs.price, coreRegisterArgs.stakeFee) = IZNSPricer(address(parentConfig.pricerContract))
                 .getPriceAndFee(
                     parentHash,
-                    label
+                    label,
+                    true
                 );
             } else {
                 coreRegisterArgs.price = IZNSPricer(address(parentConfig.pricerContract))
                     .getPrice(
                     parentHash,
-                    label
+                    label,
+                    true
                 );
             }
         }

From d5ce419a655074e56502de765eb45049a1a972d6 Mon Sep 17 00:00:00 2001
From: Kirill <dor7an@gmail.com>
Date: Mon, 30 Oct 2023 15:34:01 -0700
Subject: [PATCH 14/18] fix interfaces

---
 contracts/price/IZNSCurvePricer.sol                      | 6 ++++--
 contracts/price/IZNSFixedPricer.sol                      | 9 +++++++--
 .../distribution/ZNSSubRegistrarMock.sol                 | 2 ++
 3 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/contracts/price/IZNSCurvePricer.sol b/contracts/price/IZNSCurvePricer.sol
index fb8840b6e..5e541102f 100644
--- a/contracts/price/IZNSCurvePricer.sol
+++ b/contracts/price/IZNSCurvePricer.sol
@@ -69,7 +69,8 @@ interface IZNSCurvePricer is ICurvePriceConfig, IZNSPricer {
 
     function getPrice(
         bytes32 parentHash,
-        string calldata label
+        string calldata label,
+        bool skipValidityCheck
     ) external view returns (uint256);
 
     function getFeeForPrice(
@@ -79,7 +80,8 @@ interface IZNSCurvePricer is ICurvePriceConfig, IZNSPricer {
 
     function getPriceAndFee(
         bytes32 parentHash,
-        string calldata label
+        string calldata label,
+        bool skipValidityCheck
     ) external view returns (
         uint256 price,
         uint256 stakeFee
diff --git a/contracts/price/IZNSFixedPricer.sol b/contracts/price/IZNSFixedPricer.sol
index 9cab0e115..2e33cf7d7 100644
--- a/contracts/price/IZNSFixedPricer.sol
+++ b/contracts/price/IZNSFixedPricer.sol
@@ -38,7 +38,11 @@ interface IZNSFixedPricer is IZNSPricer {
 
     function setPrice(bytes32 domainHash, uint256 _price) external;
 
-    function getPrice(bytes32 parentHash, string calldata label) external view returns (uint256);
+    function getPrice(
+        bytes32 parentHash,
+        string calldata label,
+        bool skipValidityCheck
+    ) external view returns (uint256);
 
     function setFeePercentage(
         bytes32 domainHash,
@@ -52,7 +56,8 @@ interface IZNSFixedPricer is IZNSPricer {
 
     function getPriceAndFee(
         bytes32 parentHash,
-        string calldata label
+        string calldata label,
+        bool skipValidityCheck
     ) external view returns (uint256 price, uint256 fee);
 
     function setPriceConfig(
diff --git a/contracts/upgrade-test-mocks/distribution/ZNSSubRegistrarMock.sol b/contracts/upgrade-test-mocks/distribution/ZNSSubRegistrarMock.sol
index 572ca39bf..a1cecacc3 100644
--- a/contracts/upgrade-test-mocks/distribution/ZNSSubRegistrarMock.sol
+++ b/contracts/upgrade-test-mocks/distribution/ZNSSubRegistrarMock.sol
@@ -48,6 +48,8 @@ contract ZNSSubRegistrarUpgradeMock is
     ZNSSubRegistrarMainState,
     UpgradeMock {
 
+    using StringUtils for string;
+
     modifier onlyOwnerOperatorOrRegistrar(bytes32 domainHash) {
         require(
             registry.isOwnerOrOperator(domainHash, msg.sender)

From b74a46bfa65a1a919f67c4dbe2a517e2c96b0c24 Mon Sep 17 00:00:00 2001
From: Kirill <dor7an@gmail.com>
Date: Mon, 30 Oct 2023 15:50:20 -0700
Subject: [PATCH 15/18] add new tests and fix existing tests

---
 test/ZNSCurvePricer.test.ts        | 96 ++++++++++++++++--------------
 test/ZNSFixedPricer.test.ts        | 16 +++--
 test/ZNSRootRegistrar.test.ts      |  4 +-
 test/ZNSSubRegistrar.test.ts       |  5 +-
 test/ZNSTreasury.test.ts           |  3 +-
 test/helpers/errors.ts             |  3 +-
 test/helpers/flows/registration.ts |  2 +-
 test/helpers/register-setup.ts     |  4 +-
 8 files changed, 75 insertions(+), 58 deletions(-)

diff --git a/test/ZNSCurvePricer.test.ts b/test/ZNSCurvePricer.test.ts
index 1a75caa39..8287e55b4 100644
--- a/test/ZNSCurvePricer.test.ts
+++ b/test/ZNSCurvePricer.test.ts
@@ -12,7 +12,7 @@ import {
   validateUpgrade,
   PaymentType,
   NOT_AUTHORIZED_REG_WIRED_ERR,
-  CURVE_NO_ZERO_PRECISION_MULTIPLIER_ERR,
+  CURVE_NO_ZERO_PRECISION_MULTIPLIER_ERR, INVALID_LENGTH_ERR,
 } from "./helpers";
 import { decimalsDefault, priceConfigDefault, registrationFeePercDefault } from "./helpers/constants";
 import {
@@ -101,21 +101,29 @@ describe("ZNSCurvePricer", () => {
   });
 
   describe("#getPrice", async () => {
-    it("Returns 0 price for a root name with no length", async () => {
+    it("Returns 0 price for a label with no length if label validation is skipped", async () => {
       const {
         price,
         stakeFee,
-      } = await zns.curvePricer.getPriceAndFee(domainHash, "");
+      } = await zns.curvePricer.getPriceAndFee(domainHash, "", true);
       expect(price).to.eq(0);
       expect(stakeFee).to.eq(0);
     });
 
+    it("Reverts for a label with no length if label validation is not skipped", async () => {
+      await expect(zns.curvePricer.getPrice(domainHash, "", false)).to.be.revertedWith(INVALID_LENGTH_ERR);
+    });
+
+    it("Reverts for invalid label if label validation is not skipped", async () => {
+      await expect(zns.curvePricer.getPrice(domainHash, "wilder!", false)).to.be.revertedWith(INVALID_NAME_ERR);
+    });
+
     it("Returns the base price for domains that are equal to the base length", async () => {
       // Using the default length of 3
       const domain = "eth";
       const params = await zns.curvePricer.priceConfigs(domainHash);
 
-      const domainPrice = await zns.curvePricer.getPrice(domainHash, domain);
+      const domainPrice = await zns.curvePricer.getPrice(domainHash, domain, true);
       expect(domainPrice).to.eq(params.maxPrice);
     });
 
@@ -124,10 +132,10 @@ describe("ZNSCurvePricer", () => {
       const domainB = "e";
       const params = await zns.curvePricer.priceConfigs(domainHash);
 
-      let domainPrice = await zns.curvePricer.getPrice(domainHash, domainA);
+      let domainPrice = await zns.curvePricer.getPrice(domainHash, domainA, true);
       expect(domainPrice).to.eq(params.maxPrice);
 
-      (domainPrice = await zns.curvePricer.getPrice(domainHash, domainB));
+      (domainPrice = await zns.curvePricer.getPrice(domainHash, domainB, true));
       expect(domainPrice).to.eq(params.maxPrice);
     });
 
@@ -145,8 +153,8 @@ describe("ZNSCurvePricer", () => {
       const domainOneExpPrice = await calcCurvePrice(domainOne, priceConfigDefault);
       const domainTwoExpPrice = await calcCurvePrice(domainTwo, priceConfigDefault);
 
-      const domainOnePriceSC = await zns.curvePricer.getPrice(domainHash, domainOne);
-      const domainTwoPriceSC = await zns.curvePricer.getPrice(domainHash, domainTwo);
+      const domainOnePriceSC = await zns.curvePricer.getPrice(domainHash, domainOne, true);
+      const domainTwoPriceSC = await zns.curvePricer.getPrice(domainHash, domainTwo, true);
 
       expect(domainOnePriceSC).to.eq(domainOneRefValue);
       expect(domainOnePriceSC).to.eq(domainOneExpPrice);
@@ -164,7 +172,7 @@ describe("ZNSCurvePricer", () => {
         "abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstu";
 
       const expectedPrice = await calcCurvePrice(domain, priceConfigDefault);
-      const domainPrice = await zns.curvePricer.getPrice(domainHash, domain);
+      const domainPrice = await zns.curvePricer.getPrice(domainHash, domain, true);
 
       expect(domainPrice).to.eq(expectedPrice);
     });
@@ -177,15 +185,15 @@ describe("ZNSCurvePricer", () => {
       const long = "wilderworldbeastspetsnftscatscalicosteve";
 
       const expectedShortPrice = await calcCurvePrice(short, priceConfigDefault);
-      const shortPrice = await zns.curvePricer.getPrice(domainHash, short);
+      const shortPrice = await zns.curvePricer.getPrice(domainHash, short, true);
       expect(expectedShortPrice).to.eq(shortPrice);
 
       const expectedMediumPrice = await calcCurvePrice(medium, priceConfigDefault);
-      const mediumPrice = await zns.curvePricer.getPrice(domainHash, medium);
+      const mediumPrice = await zns.curvePricer.getPrice(domainHash, medium, true);
       expect(expectedMediumPrice).to.eq(mediumPrice);
 
       const expectedLongPrice = await calcCurvePrice(long, priceConfigDefault);
-      const longPrice = await zns.curvePricer.getPrice(domainHash, long);
+      const longPrice = await zns.curvePricer.getPrice(domainHash, long, true);
       expect(expectedLongPrice).to.eq(longPrice);
     });
 
@@ -198,7 +206,7 @@ describe("ZNSCurvePricer", () => {
         "abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyz" +
         "a";
       const expectedPrice = calcCurvePrice(domain, priceConfigDefault);
-      const domainPrice = await zns.curvePricer.getPrice(domainHash, domain);
+      const domainPrice = await zns.curvePricer.getPrice(domainHash, domain, true);
       expect(domainPrice).to.eq(expectedPrice);
     });
 
@@ -213,7 +221,7 @@ describe("ZNSCurvePricer", () => {
 
       // baseLength = 0 is a special case
       await zns.curvePricer.connect(user).setBaseLength(domainHash, 0);
-      const domainPrice = await zns.curvePricer.getPrice(domainHash, domain);
+      const domainPrice = await zns.curvePricer.getPrice(domainHash, domain, true);
       expect(domainPrice).to.eq(config.maxPrice);
 
       let outer = 1;
@@ -227,7 +235,7 @@ describe("ZNSCurvePricer", () => {
         config = await zns.curvePricer.priceConfigs(domainHash);
 
         while (config.maxLength.gt(inner)) {
-          const priceTx = zns.curvePricer.getPrice(domainHash, domain);
+          const priceTx = zns.curvePricer.getPrice(domainHash, domain, true);
           promises.push(priceTx);
 
           domain += "a";
@@ -253,10 +261,10 @@ describe("ZNSCurvePricer", () => {
       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);
+      await expect(zns.curvePricer.getPrice(domainHash, labelA, false)).to.be.revertedWith(INVALID_NAME_ERR);
+      await expect(zns.curvePricer.getPrice(domainHash, labelB, false)).to.be.revertedWith(INVALID_NAME_ERR);
+      await expect(zns.curvePricer.getPrice(domainHash, labelC, false)).to.be.revertedWith(INVALID_NAME_ERR);
+      await expect(zns.curvePricer.getPrice(domainHash, labelD, false)).to.be.revertedWith(INVALID_NAME_ERR);
     });
 
     it("Should set the config for any existing domain hash, including 0x0", async () => {
@@ -416,8 +424,8 @@ describe("ZNSCurvePricer", () => {
       const shortDomain = "a";
       const longDomain = "abcdefghijklmnopqrstuvwxyz";
 
-      const shortPrice = await zns.curvePricer.getPrice(domainHash, shortDomain);
-      const longPrice = await zns.curvePricer.getPrice(domainHash, longDomain);
+      const shortPrice = await zns.curvePricer.getPrice(domainHash, shortDomain, true);
+      const longPrice = await zns.curvePricer.getPrice(domainHash, longDomain, true);
 
       expect(shortPrice).to.eq(BigNumber.from("0"));
       expect(longPrice).to.eq(BigNumber.from("0"));
@@ -427,7 +435,7 @@ describe("ZNSCurvePricer", () => {
       const newMaxPrice = priceConfigDefault.maxPrice.add(parseEther("9"));
 
       const expectedPriceBefore = await calcCurvePrice(defaultDomain, priceConfigDefault);
-      const priceBefore= await zns.curvePricer.getPrice(domainHash, defaultDomain);
+      const priceBefore= await zns.curvePricer.getPrice(domainHash, defaultDomain, true);
 
       expect(expectedPriceBefore).to.eq(priceBefore);
 
@@ -439,7 +447,7 @@ describe("ZNSCurvePricer", () => {
       };
 
       const expectedPriceAfter = await calcCurvePrice(defaultDomain, newConfig);
-      const priceAfter = await zns.curvePricer.getPrice(domainHash, defaultDomain);
+      const priceAfter = await zns.curvePricer.getPrice(domainHash, defaultDomain, true);
 
       expect(expectedPriceAfter).to.eq(priceAfter);
       expect(expectedPriceAfter).to.be.gt(expectedPriceBefore);
@@ -496,9 +504,9 @@ describe("ZNSCurvePricer", () => {
       const long = "abcdefghijklmnoabcdefghijklmnoabcdefghijklmnoabcdefghijklmno";
 
       const priceCalls = [
-        zns.curvePricer.getPrice(domainHash, short),
-        zns.curvePricer.getPrice(domainHash, medium),
-        zns.curvePricer.getPrice(domainHash, long),
+        zns.curvePricer.getPrice(domainHash, short, true),
+        zns.curvePricer.getPrice(domainHash, medium, true),
+        zns.curvePricer.getPrice(domainHash, long, true),
       ];
 
       const [
@@ -556,7 +564,7 @@ describe("ZNSCurvePricer", () => {
     it("Verifies new prices are affected after changing the precision multiplier", async () => {
       const atIndex = 7;
 
-      const before = await zns.curvePricer.getPrice(domainHash, defaultDomain);
+      const before = await zns.curvePricer.getPrice(domainHash, defaultDomain, true);
       const beforePriceString = before.toString();
 
       expect(beforePriceString.charAt(atIndex)).to.eq("0");
@@ -568,7 +576,7 @@ describe("ZNSCurvePricer", () => {
 
       await zns.curvePricer.connect(user).setPrecisionMultiplier(domainHash, newPrecisionMultiplier);
 
-      const after = await zns.curvePricer.getPrice(domainHash, defaultDomain);
+      const after = await zns.curvePricer.getPrice(domainHash, defaultDomain, true);
       const afterPriceString = after.toString();
 
       expect(afterPriceString.charAt(atIndex)).to.not.eq("0");
@@ -630,9 +638,9 @@ describe("ZNSCurvePricer", () => {
       const long = "abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyz";
 
       const priceCalls = [
-        zns.curvePricer.getPrice(domainHash, short),
-        zns.curvePricer.getPrice(domainHash, medium),
-        zns.curvePricer.getPrice(domainHash, long),
+        zns.curvePricer.getPrice(domainHash, short, true),
+        zns.curvePricer.getPrice(domainHash, medium, true),
+        zns.curvePricer.getPrice(domainHash, long, true),
       ];
 
       const [shortPrice, mediumPrice, longPrice] = await Promise.all(priceCalls);
@@ -650,8 +658,8 @@ describe("ZNSCurvePricer", () => {
       const shortDomain = "a";
       const longDomain = "abcdefghijklmnopqrstuvwxyz";
 
-      const shortPrice = await zns.curvePricer.getPrice(domainHash, shortDomain);
-      const longPrice = await zns.curvePricer.getPrice(domainHash, longDomain);
+      const shortPrice = await zns.curvePricer.getPrice(domainHash, shortDomain, true);
+      const longPrice = await zns.curvePricer.getPrice(domainHash, longDomain, true);
 
       expect(shortPrice).to.eq(params.maxPrice);
       expect(longPrice).to.eq(params.maxPrice);
@@ -662,7 +670,7 @@ describe("ZNSCurvePricer", () => {
       const paramsBefore = await zns.curvePricer.priceConfigs(domainHash);
 
       const expectedPriceBefore = await calcCurvePrice(defaultDomain, priceConfigDefault);
-      const priceBefore = await zns.curvePricer.getPrice(domainHash, defaultDomain);
+      const priceBefore = await zns.curvePricer.getPrice(domainHash, defaultDomain, true);
       expect(priceBefore).to.eq(expectedPriceBefore);
       expect(priceBefore).to.not.eq(paramsBefore.maxPrice);
 
@@ -676,7 +684,7 @@ describe("ZNSCurvePricer", () => {
       };
 
       const expectedPriceAfter = await calcCurvePrice(defaultDomain, newConfig);
-      const priceAfter = await zns.curvePricer.getPrice(domainHash, defaultDomain);
+      const priceAfter = await zns.curvePricer.getPrice(domainHash, defaultDomain, true);
       expect(priceAfter).to.eq(expectedPriceAfter);
       expect(priceAfter).to.eq(paramsAfter.maxPrice);
     });
@@ -693,7 +701,7 @@ describe("ZNSCurvePricer", () => {
       const paramsBefore = await zns.curvePricer.priceConfigs(domainHash);
 
       const expectedPriceBefore = await calcCurvePrice(defaultDomain, newConfig1);
-      const priceBefore = await zns.curvePricer.getPrice(domainHash, defaultDomain);
+      const priceBefore = await zns.curvePricer.getPrice(domainHash, defaultDomain, true);
       expect(priceBefore).to.eq(expectedPriceBefore);
       expect(priceBefore).to.eq(paramsBefore.maxPrice);
 
@@ -708,7 +716,7 @@ describe("ZNSCurvePricer", () => {
       const paramsAfter = await zns.curvePricer.priceConfigs(domainHash);
 
       const expectedPriceAfter = await calcCurvePrice(defaultDomain, newConfig2);
-      const priceAfter = await zns.curvePricer.getPrice(domainHash, defaultDomain);
+      const priceAfter = await zns.curvePricer.getPrice(domainHash, defaultDomain, true);
       expect(priceAfter).to.eq(expectedPriceAfter);
       expect(priceAfter).to.not.eq(paramsAfter.maxPrice);
     });
@@ -718,7 +726,7 @@ describe("ZNSCurvePricer", () => {
       await zns.curvePricer.connect(user).setBaseLength(domainHash, newRootLength);
 
       let config = await zns.curvePricer.priceConfigs(domainHash);
-      let price = await zns.curvePricer.getPrice(domainHash, defaultDomain);
+      let price = await zns.curvePricer.getPrice(domainHash, defaultDomain, true);
 
       expect(config.maxPrice).to.eq(price);
 
@@ -729,7 +737,7 @@ describe("ZNSCurvePricer", () => {
       );
 
       config = await zns.curvePricer.priceConfigs(domainHash);
-      price = await zns.curvePricer.getPrice(domainHash, defaultDomain);
+      price = await zns.curvePricer.getPrice(domainHash, defaultDomain, true);
 
       expect(config.maxPrice).to.eq(price);
     });
@@ -743,7 +751,7 @@ describe("ZNSCurvePricer", () => {
       };
 
       const expectedRootPrice = await calcCurvePrice(defaultDomain, newConfig);
-      const rootPrice = await zns.curvePricer.getPrice(domainHash, defaultDomain);
+      const rootPrice = await zns.curvePricer.getPrice(domainHash, defaultDomain, true);
 
       expect(rootPrice).to.eq(expectedRootPrice);
     });
@@ -793,9 +801,9 @@ describe("ZNSCurvePricer", () => {
       const beyondBaseLength = "abcde";
 
       const priceCalls = [
-        zns.curvePricer.getPrice(domainHash, short),
-        zns.curvePricer.getPrice(domainHash, long),
-        zns.curvePricer.getPrice(domainHash, beyondBaseLength),
+        zns.curvePricer.getPrice(domainHash, short, true),
+        zns.curvePricer.getPrice(domainHash, long, true),
+        zns.curvePricer.getPrice(domainHash, beyondBaseLength, true),
       ];
 
       const [shortPrice, longPrice, beyondPrice] = await Promise.all(priceCalls);
@@ -950,7 +958,7 @@ describe("ZNSCurvePricer", () => {
         zns.curvePricer.registry(),
         zns.curvePricer.getAccessController(),
         zns.curvePricer.priceConfigs(domainHash),
-        zns.curvePricer.getPrice(domainHash, "wilder"),
+        zns.curvePricer.getPrice(domainHash, "wilder", true),
       ];
 
       await validateUpgrade(deployer, zns.curvePricer, newCurvePricer, factory, contractCalls);
diff --git a/test/ZNSFixedPricer.test.ts b/test/ZNSFixedPricer.test.ts
index 05d97bff4..4bc067c0a 100644
--- a/test/ZNSFixedPricer.test.ts
+++ b/test/ZNSFixedPricer.test.ts
@@ -4,7 +4,7 @@ import {
   deployZNS,
   getAccessRevertMsg,
   GOVERNOR_ROLE,
-  INITIALIZED_ERR,
+  INITIALIZED_ERR, INVALID_NAME_ERR,
   NOT_AUTHORIZED_REG_WIRED_ERR,
   PaymentType,
   PERCENTAGE_BASIS,
@@ -134,7 +134,7 @@ describe("ZNSFixedPricer", () => {
     await expect(tx).to.emit(zns.fixedPricer, "PriceSet").withArgs(domainHash, newPrice);
 
     expect(
-      await zns.fixedPricer.getPrice(domainHash, "testname")
+      await zns.fixedPricer.getPrice(domainHash, "testname", true)
     ).to.equal(newPrice);
   });
 
@@ -143,10 +143,16 @@ describe("ZNSFixedPricer", () => {
     await zns.fixedPricer.connect(user).setPrice(domainHash, newPrice);
 
     expect(
-      await zns.fixedPricer.getPrice(domainHash, "testname")
+      await zns.fixedPricer.getPrice(domainHash, "testname", false)
     ).to.equal(newPrice);
   });
 
+  it("#getPrice() should revert for invalid label when not skipping the label validation", async () => {
+    await expect(
+      zns.fixedPricer.getPrice(domainHash, "tEstname", false)
+    ).to.be.revertedWith(INVALID_NAME_ERR);
+  });
+
   it("#getPriceAndFee() should return the correct price and fee", async () => {
     const newPrice = ethers.utils.parseEther("3213");
     const newFee = BigNumber.from(1234);
@@ -156,7 +162,7 @@ describe("ZNSFixedPricer", () => {
     const {
       price,
       fee,
-    } = await zns.fixedPricer.getPriceAndFee(domainHash, "testname");
+    } = await zns.fixedPricer.getPriceAndFee(domainHash, "testname", false);
 
     expect(price).to.equal(newPrice);
     expect(fee).to.equal(newPrice.mul(newFee).div(PERCENTAGE_BASIS));
@@ -351,7 +357,7 @@ describe("ZNSFixedPricer", () => {
         zns.fixedPricer.registry(),
         zns.fixedPricer.getAccessController(),
         zns.fixedPricer.priceConfigs(domainHash),
-        zns.fixedPricer.getPrice(domainHash, "wilder"),
+        zns.fixedPricer.getPrice(domainHash, "wilder", false),
       ];
 
       await validateUpgrade(deployer, zns.fixedPricer, newFixedPricer, factory, contractCalls);
diff --git a/test/ZNSRootRegistrar.test.ts b/test/ZNSRootRegistrar.test.ts
index 10d74de26..55003bb19 100644
--- a/test/ZNSRootRegistrar.test.ts
+++ b/test/ZNSRootRegistrar.test.ts
@@ -829,7 +829,7 @@ describe("ZNSRootRegistrar", () => {
 
       const ogPrice = BigNumber.from(135);
       await zns.fixedPricer.connect(user).setPrice(domainHash, ogPrice);
-      expect(await zns.fixedPricer.getPrice(domainHash, defaultDomain)).to.eq(ogPrice);
+      expect(await zns.fixedPricer.getPrice(domainHash, defaultDomain, false)).to.eq(ogPrice);
 
       const tokenId = await getTokenIdFromReceipt(topLevelTx);
 
@@ -1160,7 +1160,7 @@ describe("ZNSRootRegistrar", () => {
         zns.treasury.stakedForDomain(domainHash),
         zns.domainToken.name(),
         zns.domainToken.symbol(),
-        zns.curvePricer.getPrice(ethers.constants.HashZero, domainName),
+        zns.curvePricer.getPrice(ethers.constants.HashZero, domainName, false),
       ];
 
       await validateUpgrade(deployer, zns.rootRegistrar, registrar, registrarFactory, contractCalls);
diff --git a/test/ZNSSubRegistrar.test.ts b/test/ZNSSubRegistrar.test.ts
index f12500f9c..e461497eb 100644
--- a/test/ZNSSubRegistrar.test.ts
+++ b/test/ZNSSubRegistrar.test.ts
@@ -2079,7 +2079,8 @@ describe("ZNSSubRegistrar", () => {
         stakeFee: feeFromSC,
       } = await zns.curvePricer.getPriceAndFee(
         subdomainParentHash,
-        label
+        label,
+        true
       );
       const protocolFeeFromSC = await zns.curvePricer.getFeeForPrice(
         ethers.constants.HashZero,
@@ -3252,7 +3253,7 @@ describe("ZNSSubRegistrar", () => {
         zns.treasury.stakedForDomain(domainHash),
         zns.domainToken.name(),
         zns.domainToken.symbol(),
-        zns.fixedPricer.getPrice(ethers.constants.HashZero, domainLabel),
+        zns.fixedPricer.getPrice(ethers.constants.HashZero, domainLabel, true),
       ];
 
       await validateUpgrade(deployer, zns.subRegistrar, registrar, registrarFactory, contractCalls);
diff --git a/test/ZNSTreasury.test.ts b/test/ZNSTreasury.test.ts
index 744ba2c9f..6c4b499d1 100644
--- a/test/ZNSTreasury.test.ts
+++ b/test/ZNSTreasury.test.ts
@@ -126,7 +126,8 @@ describe("ZNSTreasury", () => {
 
       const expectedStake = await zns.curvePricer.getPrice(
         ethers.constants.HashZero,
-        domainName
+        domainName,
+        false
       );
       const fee = await zns.curvePricer.getFeeForPrice(ethers.constants.HashZero, expectedStake);
 
diff --git a/test/helpers/errors.ts b/test/helpers/errors.ts
index 30cbc32d7..11780f78a 100644
--- a/test/helpers/errors.ts
+++ b/test/helpers/errors.ts
@@ -29,7 +29,8 @@ export const NOT_BOTH_OWNER_RAR_ERR = "ZNSRootRegistrar: Not the owner of both N
 export const DISTRIBUTION_LOCKED_NOT_EXIST_ERR = "ZNSSubRegistrar: Parent domain's distribution is locked or parent does not exist";
 
 // StringUtils
-export const INVALID_NAME_ERR = "StringUtils: Invalid domain name";
+export const INVALID_NAME_ERR = "StringUtils: Invalid domain label";
+export const INVALID_LENGTH_ERR = "StringUtils: Domain label too long or nonexistent";
 
 // OpenZeppelin
 export const INVALID_TOKENID_ERC_ERR = "ERC721: invalid token ID";
diff --git a/test/helpers/flows/registration.ts b/test/helpers/flows/registration.ts
index d98341851..0367805d5 100644
--- a/test/helpers/flows/registration.ts
+++ b/test/helpers/flows/registration.ts
@@ -157,7 +157,7 @@ export const validatePathRegistration = async ({
         ({
           price: expectedPrice,
           fee: stakeFee,
-        } = await zns.fixedPricer.getPriceAndFee(parentHashFound, domainLabel));
+        } = await zns.fixedPricer.getPriceAndFee(parentHashFound, domainLabel, false));
       } else {
         const {
           maxPrice,
diff --git a/test/helpers/register-setup.ts b/test/helpers/register-setup.ts
index 01fc2e612..112a856d5 100644
--- a/test/helpers/register-setup.ts
+++ b/test/helpers/register-setup.ts
@@ -54,9 +54,9 @@ export const approveForParent = async ({
   let price = BigNumber.from(0);
   let parentFee = BigNumber.from(0);
   if (pricerContract === zns.curvePricer.address) {
-    [price, parentFee] = await zns.curvePricer.getPriceAndFee(parentHash, domainLabel);
+    [price, parentFee] = await zns.curvePricer.getPriceAndFee(parentHash, domainLabel, false);
   } else if (pricerContract === zns.fixedPricer.address) {
-    [price, parentFee] = await zns.fixedPricer.getPriceAndFee(parentHash, domainLabel);
+    [price, parentFee] = await zns.fixedPricer.getPriceAndFee(parentHash, domainLabel, false);
   }
 
   const { token: tokenAddress } = await zns.treasury.paymentConfigs(parentHash);

From 7b3d418e41d46a20ace3df99111ed078b95e8cff Mon Sep 17 00:00:00 2001
From: Kirill <dor7an@gmail.com>
Date: Mon, 30 Oct 2023 16:03:06 -0700
Subject: [PATCH 16/18] add comment to IZNSPricer

---
 contracts/types/IZNSPricer.sol | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/contracts/types/IZNSPricer.sol b/contracts/types/IZNSPricer.sol
index e6c6aaabe..ebc24e3f1 100644
--- a/contracts/types/IZNSPricer.sol
+++ b/contracts/types/IZNSPricer.sol
@@ -10,6 +10,13 @@ interface IZNSPricer {
     /**
      * @dev `parentHash` param is here to allow pricer contracts
      *  to have different price configs for different subdomains
+     * `skipValidityCheck` param is added to provide proper revert when the user is
+     * calling this to find out the price of a domain that is not valid. But in Registrar contracts
+     * we want to do this explicitly and before we get the price to have lower tx cost for reverted tx.
+     * So Registrars will pass this bool as "true" to not repeat the validity check.
+     * Note that if calling this function directly to find out the price, a user should always pass "false"
+     * as `skipValidityCheck` param, otherwise, the price will be returned for an invalid label that is not
+     * possible to register.
      */
     function getPrice(
         bytes32 parentHash,

From f73970f2fc376df316dcd310f3d0d0cef99b57b4 Mon Sep 17 00:00:00 2001
From: Kirill <dor7an@gmail.com>
Date: Mon, 30 Oct 2023 16:15:28 -0700
Subject: [PATCH 17/18] fix test

---
 test/ZNSRootRegistrar.test.ts | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/test/ZNSRootRegistrar.test.ts b/test/ZNSRootRegistrar.test.ts
index 55003bb19..1a31e9fc1 100644
--- a/test/ZNSRootRegistrar.test.ts
+++ b/test/ZNSRootRegistrar.test.ts
@@ -5,7 +5,7 @@ import {
   AccessType, defaultTokenURI,
   deployZNS,
   distrConfigEmpty,
-  hashDomainLabel,
+  hashDomainLabel, INVALID_LENGTH_ERR,
   INVALID_TOKENID_ERC_ERR,
   normalizeName,
   NOT_AUTHORIZED_REG_ERR,
@@ -327,7 +327,7 @@ describe("ZNSRootRegistrar", () => {
           zns,
           domainName: emptyName,
         })
-      ).to.be.revertedWith("ZNSRootRegistrar: Domain Name not provided");
+      ).to.be.revertedWith(INVALID_LENGTH_ERR);
     });
 
     it("Can register a TLD with characters [a-z0-9-]", async () => {

From 54983448803d054a5807c4204e624a8075d2c90e Mon Sep 17 00:00:00 2001
From: Kirill <dor7an@gmail.com>
Date: Tue, 31 Oct 2023 15:08:19 -0700
Subject: [PATCH 18/18] add missing argument to `getPrice()`

---
 contracts/price/ZNSFixedPricer.sol | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/contracts/price/ZNSFixedPricer.sol b/contracts/price/ZNSFixedPricer.sol
index 30722a9c2..e79d3103d 100644
--- a/contracts/price/ZNSFixedPricer.sol
+++ b/contracts/price/ZNSFixedPricer.sol
@@ -55,7 +55,11 @@ contract ZNSFixedPricer is AAccessControlled, ARegistryWired, UUPSUpgradeable, I
      * @param skipValidityCheck If true, skips the validity check for the label
     */
     // solhint-disable-next-line no-unused-vars
-    function getPrice(bytes32 parentHash, string calldata label) public override view returns (uint256) {
+    function getPrice(
+        bytes32 parentHash,
+        string calldata label,
+        bool skipValidityCheck
+    ) public override view returns (uint256) {
         require(
             priceConfigs[parentHash].isSet,
             "ZNSFixedPricer: parent's price config has not been set properly through IZNSPricer.setPriceConfig()"