From 94db427311a02dc0625697a0e278632cd76155c4 Mon Sep 17 00:00:00 2001 From: Siphamandla Mjoli Date: Mon, 17 Aug 2026 12:43:48 +0100 Subject: [PATCH 1/2] fix: bug related to pop name reservation --- contracts/registrars/DotnsPopController.sol | 42 +++++++++----- contracts/registrars/IDotnsPopController.sol | 4 ++ test/unit/registrar/DotnsPopController.t.sol | 59 +++++++++++++++----- 3 files changed, 77 insertions(+), 28 deletions(-) diff --git a/contracts/registrars/DotnsPopController.sol b/contracts/registrars/DotnsPopController.sol index 7baf162d..0a920698 100644 --- a/contracts/registrars/DotnsPopController.sol +++ b/contracts/registrars/DotnsPopController.sol @@ -215,13 +215,7 @@ contract DotnsPopController is bytes32 reservedHash; bool hasReservation = bytes(params.reservedBaseLabel).length != 0; if (hasReservation) { - (IPopRules.PopStatus required,) = rules.classifyName(params.reservedBaseLabel); - require( - required != IPopRules.PopStatus.Reserved - && rules.isBaseName(params.reservedBaseLabel), - InvalidBaseLabel() - ); - (reservedHash,) = _validateBaseLabel(params.reservedBaseLabel); + (reservedHash,) = _validateReservableBaseLabel(rules, params.reservedBaseLabel); } _reserveLite(rules, params.lite); @@ -245,13 +239,7 @@ contract DotnsPopController is onlyGateway { IPopRules rules = _popRules(); - (IPopRules.PopStatus required,) = rules.classifyName(params.reservedBaseLabel); - require( - required != IPopRules.PopStatus.Reserved && rules.isBaseName(params.reservedBaseLabel), - InvalidBaseLabel() - ); - - (bytes32 reservedHash,) = _validateBaseLabel(params.reservedBaseLabel); + (bytes32 reservedHash,) = _validateReservableBaseLabel(rules, params.reservedBaseLabel); _advanceExpiredHead(reservedHash); _removeUserFromQueue(params.user); _enqueueReservation(rules, reservedHash, params.reservedBaseLabel, params.user); @@ -814,6 +802,32 @@ contract DotnsPopController is (labelhash, node) = LabelUtils.deriveNode(protocolRegistry.tldNode(), baseLabel); } + /// @notice Validates a base label as reservable and returns its hashes. + /// @dev Shared by both reservation entrypoints so the guard cannot drift between them. Runs + /// three checks and reverts on the first failure, before any reservation state is mutated: the + /// label must classify outside the governance-reserved tier and be a base name, be a canonical + /// single label, and have no owner on the registrar. The last check is the fix for a + /// reservation queued over an already-registered name: the queue keys by stem, so such a + /// reservation could never be redeemed yet would lock every two-digit variant of the stem for + /// the full reservation window. `exists` (owner set) mirrors exactly what makes the eventual + /// claim's mint revert, so a label that passes here is one a claim can still register. + function _validateReservableBaseLabel( + IPopRules rules, + string calldata baseLabel + ) + internal + view + returns (bytes32 labelhash, bytes32 node) + { + (IPopRules.PopStatus required,) = rules.classifyName(baseLabel); + require( + required != IPopRules.PopStatus.Reserved && rules.isBaseName(baseLabel), + InvalidBaseLabel() + ); + (labelhash, node) = _validateBaseLabel(baseLabel); + require(!_registrar().exists(uint256(node)), BaseNameAlreadyRegistered()); + } + /// @notice Reverts when a non-empty chat key is not exactly `CHAT_KEY_LENGTH` bytes. /// @dev Mirrors the resolver's own length gate so the gateway sees a controller-local /// `InvalidChatKey` revert before any mint state is written. diff --git a/contracts/registrars/IDotnsPopController.sol b/contracts/registrars/IDotnsPopController.sol index 590615d4..b020ddae 100644 --- a/contracts/registrars/IDotnsPopController.sol +++ b/contracts/registrars/IDotnsPopController.sol @@ -203,6 +203,10 @@ interface IDotnsPopController is IDotnsController { /// @notice Thrown when a supplied base label is not a canonical DNS label. error InvalidBaseLabel(); + /// @notice Thrown when a reserved base label already has an owner on the registrar, so the + /// queued reservation could never be redeemed at mint time. + error BaseNameAlreadyRegistered(); + /// @notice Thrown when a supplied chat key is non-empty and not exactly 65 bytes long. /// @dev Mirrors the resolver's `InvalidChatKeyLength` so the controller surfaces a /// controller-local error before the mint runs. diff --git a/test/unit/registrar/DotnsPopController.t.sol b/test/unit/registrar/DotnsPopController.t.sol index 65ec014c..b17df51b 100644 --- a/test/unit/registrar/DotnsPopController.t.sol +++ b/test/unit/registrar/DotnsPopController.t.sol @@ -521,25 +521,20 @@ contract DotnsPopControllerTests is BaseDotns { dotnsRegistry.setSubnodeOwner(subnodeRecord); } - function test_pop_reservation_of_already_public_minted_name_fails_on_claim() public { + function test_pop_reservation_of_already_public_minted_name_reverts_at_reserve_time() public { + // A name minted through the public commit-reveal flow already has an owner, so a PoP + // reservation over it could never be redeemed. The guard rejects it at reserve time + // rather than admitting it and only failing at claim, which would have locked every + // two-digit variant of the stem for the full reservation window. _commitAndRegister("longnamebob", ed, true); _grantPopFull(tiago); + vm.expectRevert(IDotnsPopController.BaseNameAlreadyRegistered.selector); _reservePop(tiago, LITE_LABEL_A, _validChatKey(0xaa), "longnamebob"); - (bool reserved, address holder) = dotnsPopController.isReservedForClaim("longnamebob"); - assertTrue(reserved); - assertEq(holder, tiago); - - IDotnsPopController.Link memory link = _linkWithLite(LITE_LABEL_A); - vm.expectRevert( - abi.encodeWithSelector( - IDotnsRegistrar.NameNotAvailable.selector, uint256(_nodeOf("longnamebob")) - ) - ); - _gatewayRegisterBaseName( - IDotnsPopController.FullRegistration({label: "longnamebob", user: tiago, link: link}) - ); + // No reservation was recorded, so the stem family stays open to other candidates. + (bool reserved,) = dotnsPopController.isReservedForClaim("longnamebob"); + assertFalse(reserved); } function test_enqueue_becomesHead_writes_popRules_reservation() public { @@ -592,6 +587,24 @@ contract DotnsPopControllerTests is BaseDotns { _reservePop(ed, LITE_LABEL_A, _validChatKey(0xaa), "longnamebob01"); } + function test_reserveBaseName_reverts_when_reserved_label_already_registered() public { + // George worked example: ed reserves and then claims the base name, which frees the + // stem slot on PopRules. A later lite candidate must not be able to queue a reservation + // over the now-registered name; the queue keys by stem, so an unclaimable reservation + // would lock every two-digit variant of the stem for the full reservation window. + _grantPopFull(ed); + _reservePop(ed, LITE_LABEL_A, _validChatKey(0xaa), "longnamebob"); + _gatewayRegisterBaseName( + IDotnsPopController.FullRegistration({ + label: "longnamebob", user: ed, link: _linkWithLite(LITE_LABEL_A) + }) + ); + + _grantPopFull(tiago); + vm.expectRevert(IDotnsPopController.BaseNameAlreadyRegistered.selector); + _reservePop(tiago, LITE_LABEL_B, _validChatKey(0xbb), "longnamebob"); + } + function test_public_stranger_can_mint_after_claim_clears_reservation() public { _grantPopFull(ed); _reservePop(ed, LITE_LABEL_A, _validChatKey(0xaa), "longnamebob"); @@ -920,6 +933,24 @@ contract DotnsPopControllerTests is BaseDotns { ); } + function test_reserveBaseNameOnly_reverts_when_label_already_registered() public { + // The standalone reservation entrypoint shares the same guard: a base name that already + // has an owner on the registrar can never be redeemed, so the reservation is rejected up + // front rather than discovered to be unusable at claim time. + _grantPopFull(ed); + _reservePop(ed, LITE_LABEL_A, _validChatKey(0xaa), "longnamebob"); + _gatewayRegisterBaseName( + IDotnsPopController.FullRegistration({ + label: "longnamebob", user: ed, link: _linkWithLite(LITE_LABEL_A) + }) + ); + + vm.expectRevert(IDotnsPopController.BaseNameAlreadyRegistered.selector); + _gatewayReserveBaseNameOnly( + IDotnsPopController.BaseNameReservation({user: tiago, reservedBaseLabel: "longnamebob"}) + ); + } + function test_reserveBaseNameOnly_does_not_mint_lite_or_base_name() public { _gatewayReserveBaseNameOnly( IDotnsPopController.BaseNameReservation({user: ed, reservedBaseLabel: BASE_LABEL_A}) From fdc191fd1738bd51448aec9935d5cd229643fb07 Mon Sep 17 00:00:00 2001 From: Siphamandla Mjoli Date: Mon, 17 Aug 2026 16:25:06 +0100 Subject: [PATCH 2/2] fix: address PR comments --- contracts/registrars/IDotnsPopController.sol | 16 ++++++++++++---- test/unit/registrar/DotnsPopController.t.sol | 4 ++++ 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/contracts/registrars/IDotnsPopController.sol b/contracts/registrars/IDotnsPopController.sol index b020ddae..0206ae66 100644 --- a/contracts/registrars/IDotnsPopController.sol +++ b/contracts/registrars/IDotnsPopController.sol @@ -263,8 +263,13 @@ interface IDotnsPopController is IDotnsController { /// @custom:emits PendingClaimStashed, with @custom:emits NameRegistered deferred to /// @custom:function claimLabelStore when the user settles. The base-name leg only runs /// when `reservedBaseLabel` is non-empty: it validates the DNS-label shape and requires a - /// true base label with no trailing digits (otherwise @custom:reverts InvalidBaseLabel) - /// before any queue mutation so a bad reservation never touches the queue, advances the + /// true base label with no trailing digits (otherwise @custom:reverts InvalidBaseLabel) and + /// with no owner on the registrar (otherwise @custom:reverts BaseNameAlreadyRegistered), + /// since a name that already has an owner could never be claimed. This validation runs + /// before both the lite mint and any queue mutation, so an already-registered + /// `reservedBaseLabel` aborts the whole call and the candidate receives no lite username + /// either; callers should validate the reserved label before attesting rather than relying + /// on this revert. It then advances the /// head past expired entries (emitting @custom:emits ReservationExpired for each one), /// removes the user from any prior queue position so a single user holds at most one live /// reservation across all labels, and enqueues a fresh entry (emitting @@ -284,7 +289,8 @@ interface IDotnsPopController is IDotnsController { /// typed overload's full revert surface bubbles up byte-for-byte: gateway-only access /// (otherwise @custom:reverts NotGateway), lite-label shape (otherwise /// @custom:reverts InvalidLiteLabel), base-label shape (otherwise - /// @custom:reverts InvalidBaseLabel), duplicate-reservation guard (otherwise + /// @custom:reverts InvalidBaseLabel), already-registered base label (otherwise + /// @custom:reverts BaseNameAlreadyRegistered), duplicate-reservation guard (otherwise /// @custom:reverts AlreadyReserved), and queue capacity (otherwise /// @custom:reverts QueueFull). The success path likewise emits the same events as the /// typed call: @custom:emits LiteNameReserved and @custom:emits NameRegistered on the lite @@ -300,7 +306,9 @@ interface IDotnsPopController is IDotnsController { /// gateway flow: @custom:function reserveLiteName mints the lite username first, then this /// function reserves the full/base label in a separate transaction so proof-size stays below /// per-call limits. Reverts with @custom:reverts InvalidBaseLabel when the label is empty, - /// non-canonical, digit-suffixed, or governance-reserved. The caller remains agnostic about + /// non-canonical, digit-suffixed, or governance-reserved, and with + /// @custom:reverts BaseNameAlreadyRegistered when the label already has an owner on the + /// registrar and so could never be claimed. The caller remains agnostic about /// backend batching; it simply exposes a small retryable primitive. /// @param params Reservation request; see @custom:struct BaseNameReservation. function reserveBaseNameOnly(BaseNameReservation calldata params) external; diff --git a/test/unit/registrar/DotnsPopController.t.sol b/test/unit/registrar/DotnsPopController.t.sol index b17df51b..48817ca8 100644 --- a/test/unit/registrar/DotnsPopController.t.sol +++ b/test/unit/registrar/DotnsPopController.t.sol @@ -532,6 +532,10 @@ contract DotnsPopControllerTests is BaseDotns { vm.expectRevert(IDotnsPopController.BaseNameAlreadyRegistered.selector); _reservePop(tiago, LITE_LABEL_A, _validChatKey(0xaa), "longnamebob"); + // The guard runs before the lite mint, so the whole call aborts and no lite name is + // minted for the candidate. + assertFalse(dotnsRegistrar.exists(uint256(_nodeOf(LITE_LABEL_A)))); + // No reservation was recorded, so the stem family stays open to other candidates. (bool reserved,) = dotnsPopController.isReservedForClaim("longnamebob"); assertFalse(reserved);