fix(security): minor security bugs

.github/workflows/release-metadata.yml

@@ -7,6 +7,13 @@ name: Release Metadata
 # resolved against sources.
 on:
   pull_request:
+    # Includes labeled and unlabeled so adding `deployment-record` re-runs the manifest guard
+    # below. Listing types at all replaces the default set, so the defaults are repeated here.
+    types: [opened, edited, synchronize, reopened, labeled, unlabeled, ready_for_review]
+    # A pull request touching none of the paths below never starts this workflow, so no job in
+    # it reports a conclusion on that pull request. Harmless while these are optional checks,
+    # and the thing to settle before marking any of them required: a required check that never
+    # reports blocks the merge.
     paths:
       - "deployments/**"
       - ".github/abi-contracts.txt"
@@ -29,3 +36,39 @@ 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.
+  # Runs on every pull request rather than being skipped for a labelled one: a skipped job
+  # reports no conclusion, so making this a required check would leave a correctly labelled
+  # deploy pull request unable to merge. The label is read inside the step instead.
+  live-manifests-unchanged:
+    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 }}
+          IS_DEPLOY_RECORD: ${{ contains(github.event.pull_request.labels.*.name, 'deployment-record') }}
+        run: |
+          changed=$(git diff --name-only "$BASE_SHA"...HEAD -- 'deployments/*/*.json')
+          if [ -z "$changed" ]; then
+            echo "No live network manifest was edited."
+            exit 0
+          fi
+          if [ "$IS_DEPLOY_RECORD" = "true" ]; then
+            echo "Live network manifest edited under the 'deployment-record' label:"
+            printf '  %s\n' $changed
+            exit 0
+          fi
+          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

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/<network>/<chainId>.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/<network>/<chainId>.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:
 
@@ -277,7 +277,7 @@ forge test --no-match-path 'test/fork/**'
 2. Delete the paired fork test under `test/fork/`.
 3. Delete every `*Old.sol` and `I*Old.sol` referenced only by the upgrade script.
 4. Delete temporary forge artefacts: `broadcast/<Script>.s.sol/` and `cache/<Script>.s.sol/`.
-5. Update `deployments/<network>/<chainid>.json` with any new addresses. It is the only place they are recorded, so nothing else needs editing.
+5. Update `deployments/<network>/<chainid>.json` with any new addresses. It is the only place they are recorded, so nothing else needs editing. That edit is the one case where touching a live manifest is correct, so label the pull request `deployment-record`; without it the `live-manifests-unchanged` check refuses the diff.
 
 ## Code of conduct
 

DEPLOYMENTS.md

@@ -283,7 +283,7 @@ If the deployment was intended to update a public environment, update the addres
 
 ### Network manifests and the expected set
 
-`deployments/<network>/<chainId>.json` is a **network record**: what is deployed on that live network right now. It is updated only by a real deploy or migration on that network, never by a code change. Everything that answers for reality reads these files: releases copy their addresses verbatim, and pointing tooling or the wire stage at an address with nothing behind it breaks whatever reads it.
+`deployments/<network>/<chainId>.json` is a **network record**: what is deployed on that live network right now. It is updated only by a real deploy or migration on that network, never by a code change. Everything that answers for reality reads these files: releases copy their addresses verbatim, and pointing tooling or the wire stage at an address with nothing behind it breaks whatever reads it. The CREATE3 parity gates compare `expected.json` rather than these files, so the rule that a code change leaves them alone is enforced in CI instead: `release-metadata.yml` fails a pull request that edits one unless it carries the `deployment-record` label.
 
 `deployments/expected.json` is the **expected set**: the addresses a fresh deploy of the current revision lands through the pinned CREATE3 factory. It is a property of the code, not of any network; the CI deploy job and `scripts/genesis/build-genesis.sh` verify against it, and releases never publish it.
 

README.md

@@ -179,7 +179,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. That is a protocol limit rather than the RFC 1035 figure it resembles, since the RFC bound counts a wire-format name including its length bytes and its TLD. 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.
 

contracts/registry/DotnsRegistry.sol

@@ -141,6 +141,12 @@ contract DotnsRegistry is Initializable, UUPSUpgradeable, OwnableUpgradeable, ID
         // prior owner's resolver cannot follow the name across that recycle. This does not cover a
         // secondary-market ERC-721 `transferFrom`: that path does not call the registry, so a name
         // sold directly carries the seller's resolver pointer until the buyer overwrites it.
+        // Nor does it cover the nodes beneath: a subname carries its own owner in `records`,
+        // and `_isAuthorised` returns on that owner before it ever consults the registrar, so a
+        // seller keeps write authority over every subname whose stored owner is still the
+        // seller, until the buyer reassigns each one through `setSubnodeOwner`. A subname
+        // created for someone else stays with that owner and leaves the seller no write path.
+        // The set is derivable from this contract's `NewOwner` events, which index the parent.
         // Owner remains the zero sentinel so reads delegate to the registrar's ERC-721 holder.
         records[node] = Record({
             owner: address(0),

contracts/registry/IDotnsRegistry.sol

@@ -11,6 +11,9 @@ 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. The bound is on the path in
+    ///        octets, so it limits nesting only through how much of that budget each level
+    ///        spends.
     /// @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 +68,22 @@ 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`. That last bound is on the path in octets, so
+    ///      it limits nesting only through how much of the budget each level spends. It exists
+    ///      because 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. The bound is on the path in
+    ///        octets, so it limits nesting only through how much of that budget each level
+    ///        spends.
     /// @param resolver Resolver contract address (zero clears).
     struct SubnodeResolverRecord {
         bytes32 parentNode;
@@ -84,7 +97,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 +124,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.
@@ -126,8 +141,15 @@ interface IDotnsRegistry {
     ///      prior owner's resolver pointer (and the records keyed under it) cannot be inherited
     ///      by the next holder across that recycle. A secondary-market ERC-721 `transferFrom` does
     ///      not call the registry, so a name sold directly keeps the seller's resolver pointer
-    ///      until the buyer overwrites it. Stores `owner = address(0)` as a sentinel so reads
-    ///      delegate to `IDotnsRegistrar.ownerOf` and ERC-721 transfers remain authoritative. Emits
+    ///      until the buyer overwrites it. That covers the node itself and not the nodes beneath
+    ///      it: a subname stores its own owner, and @custom:function isAuthorised returns on that
+    ///      owner before consulting the registrar, so after the sale the seller keeps write
+    ///      authority over every subname whose stored owner is still the seller, until the buyer
+    ///      reassigns each one through @custom:function setSubnodeOwner. A subname created for
+    ///      someone else stays with that owner and leaves the seller no write path. The set is
+    ///      derivable from @custom:emits NewOwner, which indexes the parent node. Stores
+    ///      `owner = address(0)` as a sentinel so reads delegate to `IDotnsRegistrar.ownerOf` and
+    ///      ERC-721 transfers remain authoritative. Emits
     ///      @custom:emits NodeTransferred on success.
     function setOwner(bytes32 node, address newOwner) external;
 

contracts/utils/StringUtils.sol

@@ -26,8 +26,25 @@ 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 Cap on the dotted parent path a caller submits, in octets.
+    /// @dev Not an RFC 1035 figure, despite the value. That ceiling is 255 octets of *wire* name,
+    ///      where each label carries a length prefix and the name ends in a zero byte, and it
+    ///      covers the fully qualified name. This bounds the dotted presentation string the caller
+    ///      passes in, which also carries no TLD, so the two are not the same quantity and this
+    ///      one must not be retuned to "match RFC 1035". What reaches a `LabelStore` row is longer
+    ///      again, and depends on the network's TLD: `subLabel` + "." + path + the TLD, so about
+    ///      323 octets at the maximum where the TLD is four 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.
@@ -213,18 +230,26 @@ 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;
-        if (length == 0) return false;
+        // Measured on calldata and rejected before the copy: an oversized path is exactly the
+        // input this bound exists for, so it must not be paid for in memory first.
+        uint256 length = bytes(value).length;
+        if (length == 0 || length > MAX_NAME_PATH_OCTETS) return false;
 
+        bytes memory path = bytes(value);
         uint256 start;
         for (uint256 i = 0; i < length; ++i) {
             if (path[i] != bytes1(0x2e)) continue;