From 10ea2a453b4b4b08c514b382c98a37ff10c21c4a Mon Sep 17 00:00:00 2001 From: GHkrishna Date: Tue, 15 Sep 2026 17:32:05 +0530 Subject: [PATCH 1/8] fix: add a max ceiling for path octets Signed-off-by: GHkrishna --- contracts/utils/StringUtils.sol | 9 +++ test/unit/utils/StringUtilsNamePath.t.sol | 73 +++++++++++++++++++++++ 2 files changed, 82 insertions(+) create mode 100644 test/unit/utils/StringUtilsNamePath.t.sol diff --git a/contracts/utils/StringUtils.sol b/contracts/utils/StringUtils.sol index ad1ef02c6..84038506c 100644 --- a/contracts/utils/StringUtils.sol +++ b/contracts/utils/StringUtils.sol @@ -28,6 +28,14 @@ library StringUtils { /// on charset than a DNS label: letters only. uint256 internal constant MAX_DNS_LABEL_OCTETS = 63; + /// @notice RFC 1035 ceiling on a whole dotted name, in octets. + /// @dev @custom:constant MAX_DNS_LABEL_OCTETS bounds one segment; this bounds the path. Without + /// it a caller composes an arbitrarily long `parentLabel` out of legal 63-octet segments, + /// and `DotnsRegistry.setSubnodeOwner` stores the full name built from it verbatim in a + /// `LabelStore` row that has no delete path, so the text is a permanent multiplier on + /// every enumeration that reads the row back. + uint256 internal constant MAX_NAME_PATH_OCTETS = 255; + /// @notice ASCII full stop separating a lite label's stem from its digit suffix. /// @dev A lite label is the only label shape in DotNS that carries a separator; /// @custom:function _isDnsLabel rejects it everywhere else. @@ -224,6 +232,7 @@ library StringUtils { bytes memory path = bytes(value); uint256 length = path.length; if (length == 0) return false; + if (length > MAX_NAME_PATH_OCTETS) return false; uint256 start; for (uint256 i = 0; i < length; ++i) { diff --git a/test/unit/utils/StringUtilsNamePath.t.sol b/test/unit/utils/StringUtilsNamePath.t.sol new file mode 100644 index 000000000..06b5603f3 --- /dev/null +++ b/test/unit/utils/StringUtilsNamePath.t.sol @@ -0,0 +1,73 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.34; + +import {Test} from "forge-std/Test.sol"; + +import {StringUtils} from "../../../contracts/utils/StringUtils.sol"; + +/// @notice Exposes the calldata-only validator so the boundary can be driven directly. +contract NamePathHarness { + function isNamePath(string calldata value) external pure returns (bool isValid) { + return StringUtils.isNamePath(value); + } +} + +/// @title StringUtilsNamePathTests +/// @notice Pins the whole-path octet ceiling on @custom:function StringUtils.isNamePath. +/// @dev The per-segment bound alone leaves the path unbounded, and +/// @custom:function DotnsRegistry.setSubnodeOwner composes the stored full name out of the +/// caller's `parentLabel`. A `LabelStore` row has no delete path, so an oversized name is a +/// permanent cost on every enumeration that reads it back. Nothing else in the suite fixes +/// this ceiling, so the two cases below are what stop it drifting. +contract StringUtilsNamePathTests is Test { + NamePathHarness private harness; + + function setUp() public { + harness = new NamePathHarness(); + } + + /// @notice A path of exactly `MAX_NAME_PATH_OCTETS` octets is still accepted. + function test_accepts_a_path_at_the_ceiling() public view { + string memory path = _join(63, 63, 63, 63, 0); + assertEq(bytes(path).length, StringUtils.MAX_NAME_PATH_OCTETS, "fixture is not at the cap"); + assertTrue(harness.isNamePath(path)); + } + + /// @notice One octet over the ceiling is rejected, with every segment individually legal so + /// the refusal cannot come from the per-segment bound. + function test_rejects_a_path_one_octet_over_the_ceiling() public view { + string memory path = _join(63, 63, 63, 62, 1); + assertEq( + bytes(path).length, StringUtils.MAX_NAME_PATH_OCTETS + 1, "fixture is not one over" + ); + assertFalse(harness.isNamePath(path)); + } + + /// @notice Joins up to five `a`-runs with dots, skipping any zero-length segment. + function _join( + uint256 a, + uint256 b, + uint256 c, + uint256 d, + uint256 e + ) + private + pure + returns (string memory path) + { + path = _run(a); + if (b != 0) path = string.concat(path, ".", _run(b)); + if (c != 0) path = string.concat(path, ".", _run(c)); + if (d != 0) path = string.concat(path, ".", _run(d)); + if (e != 0) path = string.concat(path, ".", _run(e)); + } + + /// @notice A run of `count` lowercase `a` octets, which is a canonical DNS label up to 63. + function _run(uint256 count) private pure returns (string memory run) { + bytes memory buffer = new bytes(count); + for (uint256 i = 0; i < count; ++i) { + buffer[i] = "a"; + } + run = string(buffer); + } +} From db12b9e683c7266e9a633d87fca5de51a0f40209 Mon Sep 17 00:00:00 2001 From: GHkrishna Date: Tue, 15 Sep 2026 17:34:20 +0530 Subject: [PATCH 2/8] fix: update readme for max ceiling Signed-off-by: GHkrishna --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 3787b6d09..1f5b24557 100644 --- a/README.md +++ b/README.md @@ -175,7 +175,7 @@ The registrar owns transferability. A name is marked soulbound at mint when the Forward registry mapping node to (owner, resolver) and supporting subnode creation. When a base name is minted on the registrar, the matching controller wires the node to the new owner through this registry. Privileged node wiring defers to the same controllers mapping on the registrar, so both controllers can write without the registry tracking controllers of its own. -Subnames are created by the base-name owner. A subname carries its own (owner, resolver) and can in turn carry subnames, so the registry is the place the name hierarchy actually lives. +Subnames are created by the base-name owner. A subname carries its own (owner, resolver) and can in turn carry subnames, so the registry is the place the name hierarchy actually lives. Nesting is bounded by the whole path rather than by a depth counter: the dotted parent path a caller submits is capped at 255 octets, the DNS wire-format ceiling. Each level of a subname is stored as its full dotted name in the owner's LabelStore, and that row cannot be deleted, so an uncapped path would let a parent owner write an arbitrarily long permanent row into an address they chose. The registry exposes isAuthorised(node, account) as the canonical check for whether an address may manage a node: the stored owner for a subname, or the ERC-721 holder, a single-token approvee, or an operator-for-all on the registrar for a tokenised name. Sibling contracts consult this view so a single registrar-level approval delegates management across the protocol rather than each contract maintaining its own approval list. From b3d48343db8fd97ede1e47a386201012209c2523 Mon Sep 17 00:00:00 2001 From: GHkrishna Date: Tue, 15 Sep 2026 17:35:21 +0530 Subject: [PATCH 3/8] fix: update max path ceilig related dev comments Signed-off-by: GHkrishna --- contracts/registry/IDotnsRegistry.sol | 18 +++++++++++++++--- contracts/utils/StringUtils.sol | 12 ++++++++++-- 2 files changed, 25 insertions(+), 5 deletions(-) diff --git a/contracts/registry/IDotnsRegistry.sol b/contracts/registry/IDotnsRegistry.sol index 082def71d..375cd79dc 100644 --- a/contracts/registry/IDotnsRegistry.sol +++ b/contracts/registry/IDotnsRegistry.sol @@ -11,6 +11,8 @@ interface IDotnsRegistry { /// @notice Record describing a subnode creation request. /// @param subLabel Human readable subnode label e.g "alice". /// @param parentLabel Canonical parent name without the TLD suffix e.g. bob or child.bob. + /// At most `StringUtils.MAX_NAME_PATH_OCTETS` octets, which is what bounds nesting + /// depth. /// @param owner Address to assign as owner of the created subnode. /// @param persist Whether to index the subnode into the owner's `LabelStore`, deploying it on /// demand. When false the ownership and resolver record is still written but the store @@ -65,12 +67,20 @@ interface IDotnsRegistry { /// @notice Thrown when a sublabel is not a canonical lowercase ASCII DNS label. error InvalidLabel(); - /// @notice Thrown when the supplied parent label does not match the parent node. + /// @notice Thrown when the supplied parent label does not match the parent node, or is not a + /// valid name path. + /// @dev The two cases share one error because both mean the caller's `parentLabel` cannot be + /// trusted to name `parentNode`: a mismatched namehash, a malformed segment, or a path + /// over `StringUtils.MAX_NAME_PATH_OCTETS`. The last is what bounds how deep a chain of + /// subnames can nest, since the composed full name is stored in a `LabelStore` row that + /// has no delete path. error ParentLabelMismatch(); /// @notice Record describing a subnode resolver update request. /// @param subLabel Human-readable subnode label e.g "alice". /// @param parentLabel Canonical parent name without the TLD suffix e.g bob or child.bob. + /// At most `StringUtils.MAX_NAME_PATH_OCTETS` octets, which is what bounds nesting + /// depth. /// @param resolver Resolver contract address (zero clears). struct SubnodeResolverRecord { bytes32 parentNode; @@ -84,7 +94,8 @@ interface IDotnsRegistry { /// @custom:reverts NotAuthorised. The new owner address must be non-zero, otherwise /// @custom:reverts NotAllowed. `record.subLabel` must be a single canonical DNS label /// (otherwise @custom:reverts InvalidLabel) and `record.parentLabel` must be a name - /// path whose namehash matches `record.parentNode` (otherwise + /// path of at most `StringUtils.MAX_NAME_PATH_OCTETS` octets whose namehash matches + /// `record.parentNode` (otherwise /// @custom:reverts ParentLabelMismatch). Subnodes are parent-sovereign: the current /// `record.parentNode` owner may reassign or rotate a subnode's resolver at any time /// without the prior subnode owner's consent. On reassignment the resolver pointer is @@ -110,7 +121,8 @@ interface IDotnsRegistry { /// that surface trust signals to subnode owners should treat any resolver rotation as /// a re-attestation prompt. `record.subLabel` must be a single canonical DNS label /// (otherwise @custom:reverts InvalidLabel) and `record.parentLabel` must be a name - /// path whose namehash matches `record.parentNode` (otherwise + /// path of at most `StringUtils.MAX_NAME_PATH_OCTETS` octets whose namehash matches + /// `record.parentNode` (otherwise /// @custom:reverts ParentLabelMismatch). The resulting subnode must already exist, /// otherwise @custom:reverts NotAuthorised. Emits @custom:emits NewResolver on /// success. diff --git a/contracts/utils/StringUtils.sol b/contracts/utils/StringUtils.sol index 84038506c..5ff3c9c99 100644 --- a/contracts/utils/StringUtils.sol +++ b/contracts/utils/StringUtils.sol @@ -26,6 +26,8 @@ library StringUtils { /// it reaches `MAX_DNS_LABEL_OCTETS + LITE_SUFFIX_DIGITS + 1` octets. Its stem is /// bounded here but checked in @custom:function _isLitePersonLabel, which is stricter /// on charset than a DNS label: letters only. + /// This is the per-segment bound only; @custom:function isNamePath additionally bounds + /// the whole path with @custom:constant MAX_NAME_PATH_OCTETS. uint256 internal constant MAX_DNS_LABEL_OCTETS = 63; /// @notice RFC 1035 ceiling on a whole dotted name, in octets. @@ -221,13 +223,19 @@ library StringUtils { suffix = string(suffixBytes); } - /// @notice Validates that `s` is a dot-separated path of canonical DNS labels. + /// @notice Validates that `value` is a dot-separated path of canonical DNS labels, within + /// the whole-path octet ceiling. /// @dev Each segment between dots must satisfy @custom:function isSingleLabel. Empty /// segments (leading, trailing, or consecutive dots) fail. Used when /// callers submit multi-label paths (e.g. `alice.dot`) rather than /// bare labels. + /// Two bounds apply and they are not the same one: @custom:constant MAX_DNS_LABEL_OCTETS + /// caps each segment, and @custom:constant MAX_NAME_PATH_OCTETS caps the path. Without + /// the second, a caller composes an unbounded path out of legal segments, so the + /// segment bound alone does not bound what a caller can submit here. /// @param value Candidate name path. - /// @return isValid True if every dot-separated segment is a canonical DNS label. + /// @return isValid True if the path is at most @custom:constant MAX_NAME_PATH_OCTETS octets + /// and every dot-separated segment is a canonical DNS label. function isNamePath(string calldata value) internal pure returns (bool isValid) { bytes memory path = bytes(value); uint256 length = path.length; From 6994fd7b752d7befb191de432dd281bd9b58b8a0 Mon Sep 17 00:00:00 2001 From: GHkrishna Date: Tue, 15 Sep 2026 17:35:42 +0530 Subject: [PATCH 4/8] chore: test for max ceiling Signed-off-by: GHkrishna --- test/unit/registry/DotnsRegistry.t.sol | 41 ++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/test/unit/registry/DotnsRegistry.t.sol b/test/unit/registry/DotnsRegistry.t.sol index 3d29cb57a..d08960540 100644 --- a/test/unit/registry/DotnsRegistry.t.sol +++ b/test/unit/registry/DotnsRegistry.t.sol @@ -74,6 +74,47 @@ contract DotnsRegistryTests is BaseDotns { vm.stopPrank(); } + /// @notice A parent path over the whole-path octet ceiling is refused. + /// @dev Every segment here is a legal 63-octet DNS label, so the per-segment bound cannot be + /// what rejects it. `ParentLabelMismatch` is shared with a genuine namehash mismatch, so + /// this pins the user-facing surface rather than the ceiling itself; the ceiling is + /// pinned directly in `test/unit/utils/StringUtilsNamePath.t.sol`. Without the bound a + /// parent owner composes an arbitrarily long name and `setSubnodeOwner` writes it, as a + /// row that cannot be deleted, into whichever store they name as `owner`. + function test_subnode_parent_path_over_the_octet_ceiling_is_rejected() public { + string memory parentLabel = "parentnode02"; + bytes32 parentNode = _register(parentLabel, owner, IPopRules.PopStatus.NoStatus); + + // Four 63-octet segments and three dots: 255 octets, one past the ceiling once the + // fifth segment and its dot are appended below. + string memory overlong = _runOfA(63); + for (uint256 i = 0; i < 4; ++i) { + overlong = string.concat(overlong, ".", _runOfA(63)); + } + assertGt(bytes(overlong).length, 255, "fixture is not over the ceiling"); + + vm.prank(owner); + vm.expectRevert(IDotnsRegistry.ParentLabelMismatch.selector); + dotnsRegistry.setSubnodeOwner( + IDotnsRegistry.SubnodeRecord({ + parentNode: parentNode, + subLabel: "alice", + parentLabel: overlong, + owner: ed, + persist: true + }) + ); + } + + /// @notice A run of `count` lowercase `a` octets, a canonical DNS label up to 63. + function _runOfA(uint256 count) private pure returns (string memory run) { + bytes memory buffer = new bytes(count); + for (uint256 i = 0; i < count; ++i) { + buffer[i] = "a"; + } + run = string(buffer); + } + /// @notice A subname created outside the gateway carries no person provenance. /// @dev The gateway issues a lite name as the subname `michael` under `01` and records it in /// `_popIssued`. This test creates the same subname directly, without the gateway, and From 4754daab0521c307d6f5c1204efa84aa98d47a0a Mon Sep 17 00:00:00 2001 From: GHkrishna Date: Tue, 15 Sep 2026 18:10:30 +0530 Subject: [PATCH 5/8] fix: unchecked live manifest Signed-off-by: GHkrishna --- .github/workflows/release-metadata.yml | 27 ++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/.github/workflows/release-metadata.yml b/.github/workflows/release-metadata.yml index d0ff5577d..66a7bbd3e 100644 --- a/.github/workflows/release-metadata.yml +++ b/.github/workflows/release-metadata.yml @@ -29,3 +29,30 @@ jobs: - name: Validate deployment manifests and the contract list run: bun scripts/js/release-metadata.mjs validate + + # Live network manifests record what is deployed on a real chain and ship verbatim into the + # deployments.json release asset, so they are written by a deploy and never by a code change + # (CONTRIBUTING.md). The CREATE3 parity gates compare deployments/expected.json rather than + # these files, so this job is where that rule is enforced. A pull request that records a real + # deploy carries the `deployment-record` label and is exempt. + live-manifests-unchanged: + if: ${{ !contains(github.event.pull_request.labels.*.name, 'deployment-record') }} + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + with: + fetch-depth: 0 + + - name: Reject edits to a live network manifest + env: + BASE_SHA: ${{ github.event.pull_request.base.sha }} + run: | + changed=$(git diff --name-only "$BASE_SHA"...HEAD -- 'deployments/*/*.json') + if [ -n "$changed" ]; then + echo "::error::A code pull request must not edit a live network manifest." + printf ' %s\n' $changed + echo "Update deployments/expected.json instead; the live file is written by a deploy." + echo "If this pull request records a real deploy, add the 'deployment-record' label." + exit 1 + fi + echo "No live network manifest was edited." From 3ec6719e4a87982822da00a4ac4925882c8b5916 Mon Sep 17 00:00:00 2001 From: GHkrishna Date: Tue, 15 Sep 2026 18:10:59 +0530 Subject: [PATCH 6/8] fix: docs update as per manifest change Signed-off-by: GHkrishna --- CONTRIBUTING.md | 4 +++- DEPLOYMENTS.md | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 150ae8b2d..0ed444381 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -135,7 +135,7 @@ Any new contract address that other contracts need to read must be looked up thr If you are adding a new contract category, add a `bytes32` key for it in `DotnsConstants.sol`, wire it up in `WireDeployments.s.sol` (including its entry in `_registryEntries`, so its code identity is declared and verified with the rest), and list the contract and its interface in `.github/abi-contracts.txt` so their ABIs ship in the release artifact. Read it the same way every existing contract does. Give the contract the standard `version()` mirror — a `view` that returns `protocolRegistry.protocolVersion()`, copied from any existing contract — and never a hardcoded version constant: what a network runs is declared once, on the protocol registry, every contract reports that one value, and per-contract identity is the declared codehash the deploy pipeline writes, not a self-report compiled into the bytecode. -A change that moves or adds an address (a new salt, a new contract, a contract restructured behind a proxy) must update `deployments/expected.json` in the same PR — that diff is where review sees the move — and must NOT touch any `deployments//.json`. Those are records of live networks, updated only by a real deploy on that network; editing one from a code PR publishes an address nothing is deployed at. The expected set diverging from a network's manifest is normal and means a redeploy or migration is owed on that network — see "Network manifests and the expected set" in `DEPLOYMENTS.md`. +A change that moves or adds an address (a new salt, a new contract, a contract restructured behind a proxy) must update `deployments/expected.json` in the same PR — that diff is where review sees the move — and must NOT touch any `deployments//.json`. Those are records of live networks, updated only by a real deploy on that network; editing one from a code PR publishes an address nothing is deployed at. The expected set diverging from a network's manifest is normal and means a redeploy or migration is owed on that network — see "Network manifests and the expected set" in `DEPLOYMENTS.md`. CI enforces this: `release-metadata.yml` fails a pull request that edits a live manifest unless the pull request carries the `deployment-record` label, which is how a real deploy records its addresses. Bad — the registrar address is frozen at construction, so rotating it needs an upgrade: @@ -278,6 +278,8 @@ forge test --no-match-path 'test/fork/**' 3. Delete every `*Old.sol` and `I*Old.sol` referenced only by the upgrade script. 4. Delete temporary forge artefacts: `broadcast/