diff --git a/README.md b/README.md index 91c475eca..3787b6d09 100644 --- a/README.md +++ b/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: diff --git a/contracts/escrow/DotnsNameEscrow.sol b/contracts/escrow/DotnsNameEscrow.sol index 1ef846915..79d817f85 100644 --- a/contracts/escrow/DotnsNameEscrow.sol +++ b/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; } diff --git a/contracts/registrars/DotnsRegistrar.sol b/contracts/registrars/DotnsRegistrar.sol index 060260fdf..35f9c8e1b 100644 --- a/contracts/registrars/DotnsRegistrar.sol +++ b/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 diff --git a/contracts/registrars/IDotnsRegistrar.sol b/contracts/registrars/IDotnsRegistrar.sol index 04d9fbbfe..81fe86763 100644 --- a/contracts/registrars/IDotnsRegistrar.sol +++ b/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. diff --git a/contracts/utils/RegistrationUtils.sol b/contracts/utils/RegistrationUtils.sol index 3c213fb21..85451a794 100644 --- a/contracts/utils/RegistrationUtils.sol +++ b/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. diff --git a/contracts/utils/StoreUtils.sol b/contracts/utils/StoreUtils.sol index dcd9a611c..957859df4 100644 --- a/contracts/utils/StoreUtils.sol +++ b/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); - } - } } diff --git a/test/unit/escrow/DotnsNameEscrow.t.sol b/test/unit/escrow/DotnsNameEscrow.t.sol index e753449e4..87227ca7b 100644 --- a/test/unit/escrow/DotnsNameEscrow.t.sol +++ b/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); diff --git a/test/unit/registrar/DotnsRegistrar.t.sol b/test/unit/registrar/DotnsRegistrar.t.sol index 3ae27b744..02d8b8690 100644 --- a/test/unit/registrar/DotnsRegistrar.t.sol +++ b/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"); diff --git a/test/unit/store/LabelEntryConflict.t.sol b/test/unit/store/LabelEntryConflict.t.sol index 9e8a98c5f..b76e1ea65 100644 --- a/test/unit/store/LabelEntryConflict.t.sol +++ b/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" - ); - } }