fix: close two paths that can permanently strand a name

README.md

@@ -234,7 +234,7 @@ Reads are open but fail-closed: nameOf(address) re-validates current ownership a
 
 ### DotnsContentResolver
 
-Stores contenthash and text records per node. This is where external content links (for example IPFS hashes) and arbitrary key-value text records (for example social handles, verification metadata) live. Writes accept the node owner, any address the registry recognises as authorised for the node through isAuthorised (the ERC-721 holder, a single-token approvee, or an operator-for-all on the registrar), or an operator approved directly on this resolver; reads are open. The registry-recognised path lets a registrar-level name admin manage records without a separate grant, while the resolver-local operator is a narrower record-only delegation that confers no power over ownership or transfer. Authority is evaluated against the current owner on every write, so transferring the name reassigns write access automatically.
+Stores contenthash and text records per node. This is where external content links (for example IPFS hashes) and arbitrary key-value text records (for example social handles, verification metadata) live. Writes accept the node owner, any address the registry recognises as authorised for the node through isAuthorised (the ERC-721 holder, a single-token approvee, or an operator-for-all on the registrar), or an operator approved directly on this resolver; reads are open. The registry-recognised path lets a registrar-level name admin manage records without a separate grant, while the resolver-local operator is a narrower record-only delegation that confers no power over ownership or transfer. Authority is evaluated against the current owner on every write, so transferring a name reassigns write access to it automatically. That covers the node transferred and not the nodes beneath it: a subname carries its own owner in the registry, and transferring the parent does not change it. A seller therefore keeps write access to every subname they minted until the buyer reassigns each one with `setSubnodeOwner`. The set is derivable from the registry's `NewOwner` events, and reclaiming is one call per subname.
 
 Choosing a delegation mechanism:
 

contracts/escrow/DotnsNameEscrow.sol

@@ -770,10 +770,9 @@ contract DotnsNameEscrow is
         returns (bytes4 selector)
     {
         require(msg.sender == address(_registrar()), NotAcceptedTransfer(msg.sender));
-        // Only accept transfers that this contract itself initiated via `release`. A holder calling
-        // `registrar.safeTransferFrom(holder, escrow, tokenId)` directly would otherwise land the
-        // NFT in custody with no `released` position, leaving the token (and any prior deposit)
-        // permanently unreachable through `withdraw` / `reclaim`.
+        // Only accept transfers this contract initiated via `release`; custody with no `released`
+        // position leaves the token and any prior deposit unreachable. The registrar enforces the
+        // same rule, which is what covers the plain `transferFrom` spelling this hook never sees.
         require(_positions[tokenId].released, UnsolicitedDeposit(tokenId));
         selector = IERC721Receiver.onERC721Received.selector;
     }

contracts/registrars/DotnsRegistrar.sol

@@ -289,6 +289,9 @@ contract DotnsRegistrar is
         IDotnsProtocolRegistry registry = protocolRegistry;
         address escrow = registry.get(DotnsConstants.NAME_ESCROW);
         require(escrow != address(0), EscrowNotConfigured());
+        // `release` is the only caller that moves a name into custody, so any other sender is a
+        // deposit the escrow holds no position for.
+        require(to != escrow || msg.sender == escrow, UnsolicitedEscrowDeposit(tokenId));
         IStoreFactory factory = IStoreFactory(registry.get(DotnsConstants.STORE_FACTORY));
 
         bool isEscrowTouching = to == escrow || from == escrow;
@@ -348,7 +351,11 @@ contract DotnsRegistrar is
             // downstream writes are demand-deploy through `StoreUtils.ensureLabelStore`.
             return;
         }
-        factory.writeLabel(to, bytes32(tokenId), fullName);
+        // A slot holding a different string was written by someone else, and `storeLabel` has no
+        // delete, so mirroring nothing would hand over a name `_quoteTransferFeeFor` rejects on
+        // every onward transfer. A matching entry is still a no-op, so a transfer back to a prior
+        // owner passes.
+        factory.writeNewLabel(to, bytes32(tokenId), fullName);
     }
 
     /// @notice Reads the full name (`label.tld`) for `tokenId` from `holder`'s `LabelStore` using

contracts/registrars/IDotnsRegistrar.sol

@@ -47,6 +47,15 @@ interface IDotnsRegistrar is IERC721 {
     /// @custom:function quoteTransferFee.
     error NameSoulbound(uint256 tokenId);
 
+    /// @notice Thrown when a name is transferred into escrow custody by anything other than the
+    /// escrow's own release path.
+    /// @dev `redeem` and `reclaim`, the two calls that return a name from custody, gate on the
+    /// release position the escrow records before it moves the name, so custody reached any other
+    /// way has no exit.
+    /// @custom:function IDotnsNameEscrow.onERC721Received refuses such a deposit but does not run
+    /// on a plain `transferFrom`, which is why the gate is enforced here too.
+    error UnsolicitedEscrowDeposit(uint256 tokenId);
+
     /// @notice Emitted when a name is registered.
     /// @param id The token id (namehash node) that was minted.
     /// @param owner The address that received the name.

contracts/utils/RegistrationUtils.sol

@@ -62,7 +62,7 @@ library RegistrationUtils {
     ///      protocol registry on every write (@custom:function StoreAuth.isStoreWriter), so no
     ///      per-store allowlist bookkeeping is needed here.
     /// @dev The registrar writes the `LabelStore` entry directly inside `register`
-    ///      so this helper deliberately does not call `StoreUtils.writeLabel`.
+    ///      so this helper deliberately does not call `StoreUtils.writeNewLabel`.
     ///      Doing it twice would deploy or touch the store on every flow and
     ///      could conflict with the registrar's locked-entry semantics.
     /// @param context Registration inputs. See @custom:struct RegistrationContext.

contracts/utils/StoreUtils.sol

@@ -7,8 +7,8 @@ import {IStoreFactory} from "../store/IStoreFactory.sol";
 /// @title DotNS Store Utilities Library
 /// @notice Canonical helpers for protocol writes into per-user `LabelStore` instances.
 /// @dev One auth rule, one write path. Every DotNS consumer (controller, registrar,
-///      registry, PoP controller) funnels label writes through `writeLabel` so
-///      authorisation and deploy-on-first-use semantics are identical across flows.
+///      registry, PoP controller) funnels label writes through `writeNewLabel` so
+///      authorisation, deploy-on-first-use and conflict handling are identical across flows.
 /// @custom:security-contact admin@parity.io
 library StoreUtils {
     /// @notice Thrown when a name being registered already has a different label entry.
@@ -36,13 +36,11 @@ library StoreUtils {
     }
 
     /// @notice Writes `label` for a name being registered, rejecting a conflicting entry.
-    /// @dev Same as @custom:function writeLabel except that an existing entry is only tolerated
-    ///      when it already holds `label`. A registration is the first time the protocol names a
-    ///      node in this user's store, so an entry saying something else was put there by
-    ///      someone else and must not be silently honoured: `storeLabel` is single-write with no
-    ///      delete, so accepting it would leave the name permanently mislabelled and
-    ///      untransferable. Matching entries stay a no-op, which keeps re-registration by a
-    ///      previous holder working.
+    /// @dev An existing entry is tolerated only when it already holds `label`. An entry saying
+    ///      something else was put there by someone else and must not be silently honoured:
+    ///      `storeLabel` is single-write with no delete, so accepting it would leave the name
+    ///      permanently mislabelled and untransferable. Matching entries stay a no-op, which keeps
+    ///      re-registration by a previous holder, and a transfer back to one, working.
     /// @param factory The store factory.
     /// @param user The label store owner.
     /// @param labelhash The labelhash key.
@@ -68,30 +66,4 @@ library StoreUtils {
         }
         ILabelStore(store).storeLabel(labelhash, label);
     }
-
-    /// @notice Writes `label` under `labelhash` for `user`, deploying their `LabelStore` if needed.
-    /// @dev Idempotent on locked entries: once a label is locked the call is a no-op rather
-    ///      than a revert, so retried protocol flows (e.g. an ERC721 transfer back to a prior
-    ///      owner) pass through without failing on the existing lock. Inherits the factory's
-    ///      writer authorisation: callers that are not the factory owner and not
-    ///      a store writer @custom:reverts NotAuthorised when the user has no store yet.
-    /// @param factory The store factory.
-    /// @param user The label store owner.
-    /// @param labelhash The labelhash key.
-    /// @param label The label string (typically the full name, e.g. "alice.dot").
-    /// @return store The resolved or newly deployed store address.
-    function writeLabel(
-        IStoreFactory factory,
-        address user,
-        bytes32 labelhash,
-        string memory label
-    )
-        internal
-        returns (address store)
-    {
-        store = ensureLabelStore(factory, user);
-        if (!ILabelStore(store).isLocked(labelhash)) {
-            ILabelStore(store).storeLabel(labelhash, label);
-        }
-    }
 }

test/unit/escrow/DotnsNameEscrow.t.sol

@@ -5,6 +5,7 @@ import {ERC721} from "@openzeppelin/contracts/token/ERC721/ERC721.sol";
 
 import {BaseDotns, IDotnsRegistrarController} from "../../base/BaseDotns.t.sol";
 import {IDotnsNameEscrow} from "../../../contracts/escrow/IDotnsNameEscrow.sol";
+import {IDotnsRegistrar} from "../../../contracts/registrars/IDotnsRegistrar.sol";
 import {IPopRules} from "../../../contracts/pop/IPopRules.sol";
 
 /// @title ForceSender
@@ -84,6 +85,43 @@ contract DotnsNameEscrowTest is BaseDotns {
         assertFalse(pos.claimed, "claimed should be false");
     }
 
+    /// @notice A name cannot be moved into escrow custody except by `release`, on either transfer
+    ///         spelling.
+    /// @dev `onERC721Received` covers only the safe spelling, since it is the only one that runs a
+    ///      receiver hook. Both are asserted here so the plain `transferFrom` cannot regress.
+    function test_direct_transfer_into_escrow_is_rejected_on_both_spellings() public {
+        uint256 tokenId = _registerNoStatus(LABEL, ed);
+        address escrow = address(dotnsNameEscrow);
+
+        vm.prank(ed);
+        vm.expectRevert(
+            abi.encodeWithSelector(IDotnsRegistrar.UnsolicitedEscrowDeposit.selector, tokenId)
+        );
+        dotnsRegistrar.transferFrom(ed, escrow, tokenId);
+
+        vm.prank(ed);
+        vm.expectRevert(
+            abi.encodeWithSelector(IDotnsRegistrar.UnsolicitedEscrowDeposit.selector, tokenId)
+        );
+        dotnsRegistrar.safeTransferFrom(ed, escrow, tokenId);
+
+        // An approved operator is refused on the same terms: approval carries no authority to
+        // strand the name.
+        vm.prank(ed);
+        dotnsRegistrar.approve(leonardo, tokenId);
+        vm.prank(leonardo);
+        vm.expectRevert(
+            abi.encodeWithSelector(IDotnsRegistrar.UnsolicitedEscrowDeposit.selector, tokenId)
+        );
+        dotnsRegistrar.transferFrom(ed, escrow, tokenId);
+
+        assertEq(dotnsRegistrar.ownerOf(tokenId), ed, "the name never left its holder");
+
+        // The legitimate route still works, so the guard reads the caller rather than the target.
+        _approveAndRelease(tokenId, ed);
+        assertEq(dotnsRegistrar.ownerOf(tokenId), escrow, "release still reaches custody");
+    }
+
     function test_release_transfers_token_to_escrow() public {
         uint256 tokenId = _registerNoStatus(LABEL, ed);
         _approveAndRelease(tokenId, ed);

test/unit/registrar/DotnsRegistrar.t.sol

@@ -9,6 +9,7 @@ import {IDotnsProtocolRegistry} from "../../../contracts/registry/IDotnsProtocol
 import {IDotnsNameEscrow} from "../../../contracts/escrow/IDotnsNameEscrow.sol";
 import {ILabelStore} from "../../../contracts/store/ILabelStore.sol";
 import {DotnsConstants} from "../../../contracts/utils/DotnsConstants.sol";
+import {StoreUtils} from "../../../contracts/utils/StoreUtils.sol";
 import {IPopRules} from "../../../contracts/pop/IPopRules.sol";
 import {IERC721} from "@openzeppelin/contracts/token/ERC721/IERC721.sol";
 import {IERC721Errors} from "@openzeppelin/contracts/interfaces/draft-IERC6093.sol";
@@ -21,6 +22,48 @@ import {
 ///         availability tracking, registration, transfer fee gate, label readback,
 ///         upgrade authorisation, and approval surfaces.
 contract DotnsRegistrarTests is BaseDotns {
+    /// @notice A transfer into a recipient whose slot for that name already holds a different
+    ///         string reverts, rather than mirroring nothing and handing over a stranded name.
+    /// @dev Skipping the write would strand the name: `storeLabel` has no delete, so the wrong
+    ///      label stands and every onward transfer reverts. Reverting is the recoverable outcome.
+    function test_transfer_into_a_poisoned_recipient_slot_reverts() public {
+        string memory label = "conflictname";
+        _register(label, ed, IPopRules.PopStatus.NoStatus);
+        uint256 tokenId = _tokenIdForLabel(label);
+
+        // A store writer occupies the recipient's slot for this name with a string that is not the
+        // name, the way a rogue or buggy controller could.
+        vm.prank(owner);
+        address store = storeFactory.deployLabelStoreFor(leonardo);
+        vm.prank(address(dotnsRegistrar));
+        ILabelStore(store).storeLabel(bytes32(tokenId), "not-the-name");
+
+        vm.prank(ed);
+        vm.expectRevert(
+            abi.encodeWithSelector(
+                StoreUtils.LabelEntryConflict.selector, store, bytes32(tokenId), "not-the-name"
+            )
+        );
+        dotnsRegistrar.transferFrom(ed, leonardo, tokenId);
+
+        assertEq(dotnsRegistrar.ownerOf(tokenId), ed, "the name stayed with its holder");
+    }
+
+    /// @notice A recipient slot already holding the same name stays a no-op, so a transfer back to
+    ///         a prior holder still passes.
+    function test_transfer_back_to_a_prior_holder_still_passes() public {
+        string memory label = "roundtripname";
+        _register(label, ed, IPopRules.PopStatus.NoStatus);
+        uint256 tokenId = _tokenIdForLabel(label);
+
+        vm.prank(ed);
+        dotnsRegistrar.transferFrom(ed, leonardo, tokenId);
+        vm.prank(leonardo);
+        dotnsRegistrar.transferFrom(leonardo, ed, tokenId);
+
+        assertEq(dotnsRegistrar.ownerOf(tokenId), ed, "the name returned to its prior holder");
+    }
+
     function test_add_controller() public {
         address additionalController = makeAddr("additionalController");
 

test/unit/store/LabelEntryConflict.t.sol

@@ -22,18 +22,6 @@ contract StoreWriter {
     {
         return factory.writeNewLabel(user, labelhash, label);
     }
-
-    function write(
-        IStoreFactory factory,
-        address user,
-        bytes32 labelhash,
-        string memory label
-    )
-        external
-        returns (address)
-    {
-        return factory.writeLabel(user, labelhash, label);
-    }
 }
 
 /// @title LabelEntryConflictTests
@@ -81,16 +69,4 @@ contract LabelEntryConflictTests is BaseDotns {
         address store = factory.getLabelStore(ed);
         assertEq(ILabelStore(store).getLabel(NODE), CANONICAL, "the stored label changed");
     }
-
-    /// @notice The transfer mirror keeps its tolerant behaviour: it must not revert when the
-    ///         recipient already holds an entry, or a transfer back to a prior holder would fail.
-    function test_transfer_path_still_tolerates_an_existing_entry() public {
-        writer.writeNew(factory, ed, NODE, CANONICAL);
-        writer.write(factory, ed, NODE, "something-else");
-
-        address store = factory.getLabelStore(ed);
-        assertEq(
-            ILabelStore(store).getLabel(NODE), CANONICAL, "the tolerant path overwrote an entry"
-        );
-    }
 }