fix: bug related to pop name reservation - #236
Merged
Merged
Conversation
Contributor
CI Summary
4naly3er AnalysisMedium (5)
Low (10)
Gas (14)
Informational (11)
Slither AnalysisHigh (3)
Medium (38)
Low (54)
Informational (61)
Contract Tests (Unit + Fuzz)BasicDotnsIntegrationReverts (test/intergration/BasicDotns.reverts.t.sol)
BasicDotnsIntegration (test/intergration/BasicDotns.t.sol)
DeployCreate3FactoryTest (test/unit/deploy/DeployCreate3Factory.t.sol)
DeterministicDeploymentTest (test/unit/deploy/DeterministicDeployment.t.sol)
DotnsContentResolverTests (test/unit/resolver/DotnsContentResolver.t.sol)
DotnsNameEscrowTest (test/unit/escrow/DotnsNameEscrow.t.sol)
DotnsNameEscrowFuzzTest (test/fuzz/escrow/DotnsNameEscrowFuzz.t.sol)
DotnsNameEscrowRefundsTest (test/unit/escrow/DotnsNameEscrowRefunds.t.sol)
DotnsPopControllerTests (test/unit/registrar/DotnsPopController.t.sol)
DotnsPopControllerFuzz (test/fuzz/registrar/DotnsPopControllerFuzz.t.sol)
DotnsPopResolverTests (test/unit/resolver/DotnsPopResolver.t.sol)
DotnsProtocolRegistryTldTests (test/unit/registry/DotnsProtocolRegistry.t.sol)
DotnsProtocolRegistryFuzzTest (test/fuzz/registry/DotnsProtocolRegistryFuzz.t.sol)
DotnsRegistrarTests (test/unit/registrar/DotnsRegistrar.t.sol)
DotnsRegistrarControllerTest (test/unit/registrar/DotnsRegistrarController.t.sol)
DotnsRegistrarControllerFuzzTest (test/fuzz/registrar/DotnsRegistrarControllerFuzz.t.sol)
DotnsRegistrarControllerLifecycleTest (test/unit/registrar/DotnsRegistrarControllerLifecycle.t.sol)
DotnsRegistryTests (test/unit/registry/DotnsRegistry.t.sol)
DotnsRegistryFuzzTest (test/fuzz/registry/DotnsRegistryFuzz.t.sol)
DotnsResolverTests (test/unit/resolver/DotnsResolver.t.sol)
DotnsReverseResolverTests (test/unit/resolver/DotnsReverseResolver.t.sol)
LabelStoreTests (test/unit/store/LabelStore.t.sol)
LabelStoreFuzzTest (test/fuzz/store/LabelStoreFuzz.t.sol)
NoStatusDepositLifecycle (test/intergration/NoStatusDepositLifecycle.t.sol)
PopRulesFuzzTest (test/fuzz/pop/PopFuzz.t.sol)
PopLifecycleFlow (test/intergration/PopLifecycleFlow.t.sol)
PopRulesTests (test/unit/pop/PopRules.t.sol)
RootGatewayDispatcherTests (test/unit/registrar/RootGatewayDispatcher.t.sol)
StoreFactoryTests (test/unit/store/StoreFactory.t.sol)
StoreIntegrationTest (test/intergration/StoreIntegration.t.sol)
StoreStressTest (test/stress/store/StoreStress.t.sol)
UserStoreTests (test/unit/store/UserStore.t.sol)
UserStoreFuzzTest (test/fuzz/store/UserStoreFuzz.t.sol)
WhitelistOperatorFlow (test/intergration/WhitelistOperatorFlow.t.sol)
Contract Tests (Invariant)DotnsNameEscrowInvariantTest (test/invariant/escrow/DotnsNameEscrowInvariant.t.sol)
DotnsPopControllerInvariant (test/invariant/registrar/DotnsPopControllerInvariant.t.sol)
DotnsRegistrarControllerInvariantTest (test/invariant/registrar/DotnsRegistrarControllerInvariant.t.sol)
DotnsRegistrarControllerRoleInvariantTest (test/invariant/registrar/DotnsRegistrarControllerRoleInvariant.t.sol)
DotnsRegistryInvariantTest (test/invariant/registry/DotnsRegistryInvariant.t.sol)
StoreInvariantTest (test/invariant/store/StoreInvariant.t.sol)
Gas ReportDotnsProtocolRegistry
DotnsRegistrar
DotnsRegistrarController
DotnsRegistry
DotnsReverseResolver
PopRules
Coverage
Deploy ContractsDeployed addresses vs DEPLOYMENTS.mdExpected is the committed manifest; actual is this CI deployment of the same pipeline.
Labelssmartcontracts, scope: registration, type: test |
github-actions
Bot
added
smartcontracts
re-gius
approved these changes
Aug 17, 2026
re-gius
left a comment
re-gius
left a comment
Contributor
There was a problem hiding this comment.
LGTM! Just a few docs / test nits
| ); | ||
| // No reservation was recorded, so the stem family stays open to other candidates. | ||
| (bool reserved,) = dotnsPopController.isReservedForClaim("longnamebob"); | ||
| assertFalse(reserved); |
Contributor
There was a problem hiding this comment.
Cheap addition :
assertFalse(dotnsRegistrar.exists(uint256(_nodeOf(LITE_LABEL_A))));
| /// @custom:emits ReservationQueued). The enqueue rejects with @custom:reverts | ||
| /// AlreadyReserved when the user already holds a reservation that was not cleared by the | ||
| /// prior removal and with @custom:reverts QueueFull when the per-label queue has reached | ||
| /// `MAX_RESERVATION_QUEUE`. Cross-chain callers pass the ABI-encoded reservation tuple as |
Contributor
There was a problem hiding this comment.
I would add something like "the base-name validation runs before the lite mint, 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."
Contributor
There was a problem hiding this comment.
and also requires the label to have no owner on the registrar (otherwise @Custom:reverts BaseNameAlreadyRegistered)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
reserveBaseNameandreserveBaseNameOnlyvalidated the reserved base label for tier and shape but never checked whether the name already had an owner. Because the reservation queue keys by stem, an unclaimable reservation queued over an already-registered name locked every two-digit variant of that stem for the full reservation window, with no operator or owner revoke to release it — only holder relinquish or expiry.The fix extracts
_validateReservableBaseLabel, shared by both entrypoints so the guard cannot drift between them, and adds a registrar existence check that revertsBaseNameAlreadyRegisteredbefore any reservation state is mutated.exists(owner set) mirrors exactly what makes the eventual claim's mint revert, so a label that passes the guard is one a claim can still register.Type
Scope
Related Issues
#235
Fixes
Fixes #235
Checklist
Code
forge buildpassesforge testpassesTesting
Security
selfdestructordelegatecallDocumentation
Breaking Changes
How to test
Notes
exists()is the correct authority here rather thanavailable():available()returns true for escrow-held names, but the claim path (registrar.register) reverts on any existing token and never routes through escrow reclaim, soexists()mirrors claim-time behaviour exactly — no over- or under-blocking.