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) 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);