From 2794d4e465db3dc79bd49a9ef72cdeda9bdb9766 Mon Sep 17 00:00:00 2001 From: Philippe Date: Thu, 4 Jun 2026 01:14:23 +0200 Subject: [PATCH 1/2] Bug: NameRegistry reverse-lookup poisoning via unchecked register/update --- BUG_REPORT_name_reverse_lookup_poison.md | 106 +++++++++++++++++++++++ 1 file changed, 106 insertions(+) create mode 100644 BUG_REPORT_name_reverse_lookup_poison.md diff --git a/BUG_REPORT_name_reverse_lookup_poison.md b/BUG_REPORT_name_reverse_lookup_poison.md new file mode 100644 index 0000000..5777240 --- /dev/null +++ b/BUG_REPORT_name_reverse_lookup_poison.md @@ -0,0 +1,106 @@ +# Bug: GitlawbNameRegistry — reverse-lookup poisoning via unchecked `register` + +**Severity**: MEDIUM — identity-confusion / phishing surface, not direct loss of funds. + +**Affected**: `src/GitlawbNameRegistry.sol`, functions `register(string, string)` and `update(string, string)` + +## Root cause + +`register()` writes the `didToName` reverse mapping without checking whether the DID is already mapped to another name : + +```solidity +function register(string calldata name, string calldata did) external { + _validateName(name); + bytes32 nameHash = keccak256(bytes(name)); + if (_records[nameHash].owner != address(0)) revert NameTaken(nameHash); + + _records[nameHash] = NameRecord({...}); + + // Reverse mapping — silently overwrites + bytes32 didHash = keccak256(bytes(did)); + didToName[didHash] = name; // ← no check + + emit NameRegistered(name, did, msg.sender, block.timestamp); +} +``` + +`update()` has the same issue : it deletes the OLD didToName entry but blindly overwrites if the NEW DID is already mapped to another name. + +## Attack: identity confusion / phishing + +1. Alice registers `name="alice"`, `did="did:key:z6MkAliceKey..."` +2. `didToName[keccak256(did:key:z6MkAliceKey...)]` = "alice" ✓ +3. Bob, attacker, registers `name="alice-prime"` with `did="did:key:z6MkAliceKey..."` (Alice's DID) +4. Contract accepts — no check on reverse mapping +5. `didToName[keccak256(did:key:z6MkAliceKey...)]` = "alice-prime" — Alice's reverse lookup is silently broken + +Any frontend/backend doing `reverseLookup(aliceDID)` returns Bob's chosen name. If Bob registers a phishing-flavored name (`alice-verified`, `alice-official`), apps display the wrong name when looking up Alice's DID. + +The FORWARD mapping `name → did` is fine for Alice (she still owns "alice"). But the REVERSE direction is poisoned. + +## Why this matters for Gitlawb + +The reverse mapping is **the canonical way** to display a name for a known DID. Gitlawb-skill agents will commonly call `reverseLookup(agentDid)` to know what to display when showing other agents in the UI. A poisoned mapping shows the attacker-chosen name → social engineering. + +## PoC + +```solidity +function test_reverse_lookup_poisoning() public { + string memory aliceDid = "did:key:z6MkAlice..."; + + // Alice registers cleanly + vm.prank(alice); + nameRegistry.register("alice", aliceDid); + assertEq(nameRegistry.reverseLookup(aliceDid), "alice"); + + // Bob registers a different NAME but with Alice's DID + vm.prank(bob); + nameRegistry.register("alice-prime", aliceDid); + + // Alice's reverse mapping is now poisoned + assertEq(nameRegistry.reverseLookup(aliceDid), "alice-prime"); // ← attacker wins +} +``` + +## Suggested fix + +Add a check : the DID must not already have a reverse mapping when registering a new name, OR explicitly allow multiple names per DID (track as array). + +```solidity +function register(string calldata name, string calldata did) external { + _validateName(name); + bytes32 nameHash = keccak256(bytes(name)); + if (_records[nameHash].owner != address(0)) revert NameTaken(nameHash); + + bytes32 didHash = keccak256(bytes(did)); ++ // Forbid DID-squatting on the reverse mapping ++ if (bytes(didToName[didHash]).length != 0) revert DidAlreadyHasName(didHash); + + _records[nameHash] = NameRecord({...}); + didToName[didHash] = name; + emit NameRegistered(name, did, msg.sender, block.timestamp); +} +``` + +Same in `update()` : + +```solidity +function update(string calldata name, string calldata newDid) external { + ... + bytes32 newDidHash = keccak256(bytes(newDid)); ++ if (bytes(didToName[newDidHash]).length != 0) revert DidAlreadyHasName(newDidHash); + didToName[newDidHash] = name; + ... +} +``` + +## Secondary issues + +- `transfer()` doesn't validate `newOwner != address(0)` — accidental wipe possible +- `transfer()` doesn't update `updatedAt` consistently with `update()` + +## Reporter + +@philpof102-svg — operator `0xAC3ca7c5d3cDD7702fd08F9C4C28dAA22296aDa9` (Base) +MainStreet : https://avisradar-production.up.railway.app/mainstreet.html +Related : [#5 (stranded rewards)](https://github.com/Gitlawb/contracts/pull/5), [#6 (claimBounty DoS)](https://github.com/Gitlawb/contracts/pull/6), [#7 (NodeStaking O(n) DoS)](https://github.com/Gitlawb/contracts/pull/7) From 1aee5868e77637ba42e3387e1b34c1f58c7b178c Mon Sep 17 00:00:00 2001 From: Philippe Date: Thu, 4 Jun 2026 01:57:14 +0200 Subject: [PATCH 2/2] fix: prevent reverse-lookup poisoning of didToName mapping (PR #8) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 #8 bug report. --- src/GitlawbNameRegistry.sol | 31 +++++++++++++++++++++++++------ 1 file changed, 25 insertions(+), 6 deletions(-) diff --git a/src/GitlawbNameRegistry.sol b/src/GitlawbNameRegistry.sol index 2624b52..ecadd0b 100644 --- a/src/GitlawbNameRegistry.sol +++ b/src/GitlawbNameRegistry.sol @@ -38,6 +38,8 @@ contract GitlawbNameRegistry { error NotOwner(bytes32 nameHash, address caller); error NotRegistered(bytes32 nameHash); error InvalidName(); + error DIDAlreadyClaimed(bytes32 didHash); // PR #8 + error EmptyDID(); // PR #8 // ── Functions ───────────────────────────────────────────────────────────── @@ -46,9 +48,18 @@ contract GitlawbNameRegistry { /// @param did Full DID string to associate with this name function register(string calldata name, string calldata did) external { _validateName(name); + if (bytes(did).length == 0) revert EmptyDID(); // PR #8 fix bytes32 nameHash = keccak256(bytes(name)); if (_records[nameHash].owner != address(0)) revert NameTaken(nameHash); + // PR #8 fix : reject if this DID is already mapped to another name. + // Without this guard, an attacker can register "trusted-name-phish" + // pointing at the same DID a legitimate name owns, overwriting the + // didToName reverse mapping and turning consumers' reverseLookup(did) + // into a phishing surface. + bytes32 didHash = keccak256(bytes(did)); + if (bytes(didToName[didHash]).length != 0) revert DIDAlreadyClaimed(didHash); + _records[nameHash] = NameRecord({ owner: msg.sender, did: did, @@ -56,8 +67,6 @@ contract GitlawbNameRegistry { updatedAt: block.timestamp }); - // Reverse mapping - bytes32 didHash = keccak256(bytes(did)); didToName[didHash] = name; emit NameRegistered(name, did, msg.sender, block.timestamp); @@ -65,20 +74,30 @@ contract GitlawbNameRegistry { /// Update the DID associated with a name. Only the current owner can call. function update(string calldata name, string calldata newDid) external { + if (bytes(newDid).length == 0) revert EmptyDID(); // PR #8 fix bytes32 nameHash = keccak256(bytes(name)); NameRecord storage rec = _records[nameHash]; if (rec.owner == address(0)) revert NotRegistered(nameHash); if (rec.owner != msg.sender) revert NotOwner(nameHash, msg.sender); - // Remove old reverse mapping + bytes32 newDidHash = keccak256(bytes(newDid)); bytes32 oldDidHash = keccak256(bytes(rec.did)); - delete didToName[oldDidHash]; + + // PR #8 fix : if the new DID is already mapped to a DIFFERENT name, + // reject. Same-DID self-updates (newDid == oldDid) are allowed as a + // no-op for callers that want to refresh updatedAt. + if (newDidHash != oldDidHash && bytes(didToName[newDidHash]).length != 0) { + revert DIDAlreadyClaimed(newDidHash); + } + + // Remove old reverse mapping (no-op if same DID) + if (newDidHash != oldDidHash) { + delete didToName[oldDidHash]; + } rec.did = newDid; rec.updatedAt = block.timestamp; - // Add new reverse mapping - bytes32 newDidHash = keccak256(bytes(newDid)); didToName[newDidHash] = name; emit NameUpdated(name, newDid, msg.sender, block.timestamp);