Skip to content

Security LOW: GitlawbDIDRegistry.transfer accepts address(0), enables DID burn + hostile re-registration #14

Description

@philpof102-svg

Gitlawb/contracts — LOW — DIDRegistry.transfer accepts address(0), enables DID burn + hostile re-registration

Phil-ready issue body. Severity LOW — 1-line fix, no funds at risk
but enables silent DID hijack via phishing or fat-finger.

Severity — LOW

Requires owner action (intentional or fat-finger / phishing-induced).
Fix is 1 line. Cost/benefit strongly favors patching even though
exploit isn't drive-by.

Mechanism

GitlawbDIDRegistry.transfer(string did, address newOwner) has no
zero-address guard:

function transfer(string calldata did, address newOwner) external {
    bytes32 didHash = keccak256(bytes(did));
    if (didToOwner[didHash] == address(0)) revert NotRegistered(didHash);
    if (didToOwner[didHash] != msg.sender) revert NotOwner(didHash, msg.sender);
    didToOwner[didHash] = newOwner;   // newOwner == address(0) accepted
    // ...
}

After transfer(did, address(0)):

  • didToOwner[didHash] is now zero
  • isRegistered(did) returns false (the next caller sees the DID as
    unregistered)
  • register(did, maliciousDoc) succeeds for any caller, with whatever
    DID document they choose

The legitimate owner's on-chain identity is destroyed and any party
can claim the DID name with a hostile DID document. Off-chain
verifiers that trust the registry as a source of truth may then
follow the attacker's document.

Attack surface

  • Phishing call: malicious frontend calls transfer(yourDid, 0x0)
    through a wallet popup that looks innocuous
  • Fat-finger: owner pastes 0x0 or an empty address in a CLI tool
    that doesn't validate
  • Smart-wallet UX that auto-populates fields with address(0) defaults

Suggested fix (1 line)

 function transfer(string calldata did, address newOwner) external {
+    if (newOwner == address(0)) revert ZeroAddress();
     bytes32 didHash = keccak256(bytes(did));

If the protocol wants a deactivation primitive, expose it explicitly
(deactivate(did) that sets a tombstone, not address(0)). The
NotRegistered revert path is the natural tombstone marker — should
not coincide with the active-but-burned state.

Why this is distinct from prior disclosures

Scanned titles + bodies of Gitlawb/contracts PRs #5-#12. None
address GitlawbDIDRegistry zero-address handling. Phil's prior
DID-related work was the NodeStaking DID-squatting issue (#9), a
different contract.

Deployment

Mainnet: 0x8046284116C5ac6724adbBf860feBeA85692d574

Foundry test (drop into test/GitlawbDIDRegistry.t.sol)

function test_transfer_rejectsZeroAddress() public {
    string memory did = "did:key:test";
    didRegistry.register(did, "doc-uri");
    vm.expectRevert();
    didRegistry.transfer(did, address(0));
}

function test_burnedDidCannotBeReclaimedByAttacker() public {
    // Without fix, this test PASSES (proving the bug)
    // With fix, the inner transfer reverts and the attacker never gets a chance
    string memory did = "did:key:victim";
    vm.prank(VICTIM);
    didRegistry.register(did, "victim-doc");
    vm.prank(VICTIM);
    didRegistry.transfer(did, address(0));  // burn
    vm.prank(ATTACKER);
    didRegistry.register(did, "attacker-doc");  // succeeds — bug
    assertEq(didRegistry.documentOf(did), "attacker-doc");
}

3-skeptic pass log

  1. "DID owners should be able to deactivate." → Normal deactivation
    updates the document to a tombstone, does NOT free the DID for
    hostile re-registration. Conflating "deactivated" with "free to
    claim" is the bug.

  2. "Requires intentional owner call." → Correct. Hence LOW, not
    MEDIUM. But UI/phishing-induced calls and fat-finger inputs ARE
    real attack surface, and 1-line fix has no downside.

  3. "Already covered by another PR." → Confirmed not. Scanned all 12
    PR titles + bodies. No DIDRegistry transfer-related PR.

Phil-side filing checklist

  • Cooling-off (file after dispute-bounty MEDIUM lands or
    ~30 min later)
  • gh issue create --repo Gitlawb/contracts --title 'Security LOW: GitlawbDIDRegistry.transfer accepts address(0), enables DID burn + hostile re-registration' --body-file distribution/gitlawb-round9-didRegistry-low.md

Activity

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions