Skip to content

Security : NodeStaking DID squatting + nodeIds growth + slashing gap#9

Open
philpof102-svg wants to merge 2 commits into
Gitlawb:mainfrom
philpof102-svg:fix-nodestaking-did-squatting
Open

Security : NodeStaking DID squatting + nodeIds growth + slashing gap#9
philpof102-svg wants to merge 2 commits into
Gitlawb:mainfrom
philpof102-svg:fix-nodestaking-did-squatting

Conversation

@philpof102-svg

Copy link
Copy Markdown

Three combined NodeStaking findings :

  1. DID squatting (HIGH) — registerNode accepts any bytes32 hash, no DID controller proof. ~$90 in $GITLAWB squats 100 future DIDs.
  2. nodeIds memory leak (MEDIUM, amplifies Security HIGH: NodeStaking.depositRevenue O(n) loops gas DoS #7) — unstake never removes the entry. After 5 years of churn → guaranteed gas-out even at low active count.
  3. Slashing scope gap (INFO) — docs promise 10-100% slashing but no slash() function exists in code.

Full PoC + fix patterns in BUG_REPORT_nodestaking_did_squatting.md.

5th PR in continuous Gitlawb audit. Helping ship Phase 8 mainnet safe. Reporter @philpof102-svg.

@philpof102-svg

Copy link
Copy Markdown
Author

Upgraded with merge-ready fix (src/GitlawbNodeStaking.sol, +53 lines).

Two fixes bundled :

1) DID squatting (opt-in binding)

interface IDIDRegistryView {
    function didToOwner(bytes32) external view returns (address);
}

IDIDRegistryView public didRegistry;

function registerNode(...) external {
    ...
    if (address(didRegistry) != address(0)) {
        if (didRegistry.didToOwner(nodeDidHash) != msg.sender) revert NotDIDOwner();
    }
    ...
}

function setDIDRegistry(address _r) external { ... } // onlyOwner

Zero-address default preserves legacy behavior — existing deployments don't break, and you opt in via setDIDRegistry(GitlawbDIDRegistry).

2) nodeIds swap-and-pop (memory leak → O(1) cleanup)

mapping(bytes32 => uint256) private nodeIdIndex; // 1-based

// registerNode
nodeIds.push(nodeDidHash);
nodeIdIndex[nodeDidHash] = nodeIds.length;

// unstake
uint256 idx1 = nodeIdIndex[nodeDidHash];
if (idx1 != 0) {
    uint256 last = nodeIds.length - 1;
    uint256 idx = idx1 - 1;
    if (idx != last) {
        bytes32 moved = nodeIds[last];
        nodeIds[idx] = moved;
        nodeIdIndex[moved] = idx + 1;
    }
    nodeIds.pop();
    nodeIdIndex[nodeDidHash] = 0;
}

Pre-fix nodes have nodeIdIndex == 0 so the cleanup is a no-op for them — backward compatible.

Combined with the PR #7 loop-reduction in depositRevenue, this gets the protocol to a place where revenue distribution stays cheap even after long operator churn.

Two related fixes to GitlawbNodeStaking:

1. **DID squatting** — registerNode() previously accepted any nodeDidHash
   from any caller, letting an attacker grab high-value DIDs (e.g. the
   DID hash of a known agent) and either ransom it or front-run the
   legitimate operator into AlreadyRegistered() reverts forever.

   Fix: optional binding to GitlawbDIDRegistry. When set, registerNode
   requires msg.sender == didToOwner(nodeDidHash). Zero address keeps
   the legacy behavior so existing deployments don't break — opt-in
   via setDIDRegistry(address) by owner.

   - new interface IDIDRegistryView (single view function)
   - new storage: didRegistry
   - new error: NotDIDOwner
   - registerNode now enforces ownership when registry bound
   - new admin: setDIDRegistry

2. **nodeIds memory leak** — unstake() wiped the Node struct but never
   removed the entry from nodeIds[]. Every register→unstake lifecycle
   left a permanent dead slot. depositRevenue's O(n) loops re-paid gas
   for these slots on every deposit. Over a year of operator churn this
   turns the protocol into an unbounded gas DoS even though the active
   operator count stays bounded.

   Fix: swap-and-pop. New mapping nodeIdIndex tracks each DID's position
   in nodeIds (1-based; 0 = not present). unstake() removes the entry
   by swapping with the last and popping. O(1) instead of O(n) cleanup.

   - new mapping: nodeIdIndex (private)
   - registerNode records the 1-based index
   - unstake performs swap-and-pop

Backward compatible: existing nodes pre-fix have nodeIdIndex=0, so the
swap-and-pop is a no-op for them (handled by the idx1 != 0 guard).

Related to PR Gitlawb#9 bug report.
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