Skip to content

Security MEDIUM: NameRegistry reverse-lookup poisoning#8

Open
philpof102-svg wants to merge 2 commits into
Gitlawb:mainfrom
philpof102-svg:fix-name-registry-reverse-poisoning
Open

Security MEDIUM: NameRegistry reverse-lookup poisoning#8
philpof102-svg wants to merge 2 commits into
Gitlawb:mainfrom
philpof102-svg:fix-name-registry-reverse-poisoning

Conversation

@philpof102-svg

Copy link
Copy Markdown

Severity: MEDIUM — identity confusion / phishing surface

NameRegistry.register() and update() write the didToName reverse mapping without checking whether the DID is already mapped. Attacker registers a different name with victim's DID → poisons reverseLookup(victimDID) to return attacker-chosen name → social engineering on any UI displaying agent names.

The forward mapping (name → did) is fine. The reverse direction is the only impacted surface, but it IS the canonical display direction for agent UIs.

Full PoC + fix in BUG_REPORT_name_reverse_lookup_poison.md.

Reporter @philpof102-svg — 4th PR in this audit run. Related : #5, #6, #7.

)

Previously register() and update() blindly wrote didToName[didHash] without
checking whether the DID was already mapped to a different name. This let
anyone register a fresh name pointing at a victim's already-claimed DID,
overwriting the reverse mapping and turning consumers'
reverseLookup(victimDid) → 'attacker-name' for phishing.

Attack walkthrough :
  1. Legit user registers 'alice' → did:gitlawb:alice
     didToName[hash(did:gitlawb:alice)] = 'alice'  ✓
  2. Attacker calls register('alice-pay', 'did:gitlawb:alice')
     - Name 'alice-pay' is free, so NameTaken does not fire
     - didToName overwritten to 'alice-pay'  ✗
  3. Any consumer trusting reverseLookup(did:gitlawb:alice) sees
     'alice-pay' instead of 'alice' — pays the attacker, signs the
     attacker's contracts, etc.

Fix :
  - register(): require didToName[didHash] empty before write.
    New error DIDAlreadyClaimed(bytes32 didHash).
  - update(): require didToName[newDidHash] empty UNLESS the caller is
    re-pointing to the SAME did (no-op refresh of updatedAt allowed).
  - Both functions reject empty-string DIDs (EmptyDID error) — a separate
    edge case that lets attackers spam the '' reverse slot.

Semantics now match the documented intent : a name owns a DID, and a DID
can be owned by at most one name at a time.

Backward compatible : existing collision-free deployments see no change.
Names that legitimately re-point to a fresh DID continue to work because
the old DID's mapping is deleted before the new one is written.

Suggested regression test (Foundry) :

    function test_DIDPoisoning_Reverts() public {
        registry.register('alice', 'did:gitlawb:alice');
        vm.prank(attacker);
        vm.expectRevert(abi.encodeWithSelector(
            GitlawbNameRegistry.DIDAlreadyClaimed.selector,
            keccak256(bytes('did:gitlawb:alice'))
        ));
        registry.register('alice-pay', 'did:gitlawb:alice');
    }

    function test_SameOwner_SameDID_UpdateIsNoOp() public {
        registry.register('alice', 'did:gitlawb:alice');
        registry.update('alice', 'did:gitlawb:alice'); // refresh updatedAt
        assertEq(registry.reverseLookup('did:gitlawb:alice'), 'alice');
    }

Related to PR Gitlawb#8 bug report.
@philpof102-svg

Copy link
Copy Markdown
Author

Upgraded with merge-ready fix (src/GitlawbNameRegistry.sol, +25/-6) plus regression tests.

Attack walkthrough

  1. Alice : register('alice', 'did:gitlawb:alice')didToName[hash]=alice
  2. Attacker : register('alice-pay', 'did:gitlawb:alice') — name 'alice-pay' is free, no NameTaken
  3. didToName[hash] overwritten to 'alice-pay' — consumers using reverseLookup(did) are now poisoned

Fix

// register()
bytes32 didHash = keccak256(bytes(did));
if (bytes(didToName[didHash]).length != 0) revert DIDAlreadyClaimed(didHash);

// update() — allow same-DID self-refresh
if (newDidHash != oldDidHash && bytes(didToName[newDidHash]).length != 0) {
    revert DIDAlreadyClaimed(newDidHash);
}

Also: both functions now reject empty-string DIDs (EmptyDID()) — separate edge case that lets attackers spam the '' reverse slot.

Regression tests (Foundry)

function test_DIDPoisoning_Reverts() public {
    registry.register('alice', 'did:gitlawb:alice');
    vm.prank(attacker);
    vm.expectRevert(abi.encodeWithSelector(
        GitlawbNameRegistry.DIDAlreadyClaimed.selector,
        keccak256(bytes('did:gitlawb:alice'))
    ));
    registry.register('alice-pay', 'did:gitlawb:alice');
}

function test_SameOwner_SameDID_UpdateIsNoOp() public {
    registry.register('alice', 'did:gitlawb:alice');
    registry.update('alice', 'did:gitlawb:alice');
    assertEq(registry.reverseLookup('did:gitlawb:alice'), 'alice');
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant