Security : NodeStaking DID squatting + nodeIds growth + slashing gap

BUG_REPORT_nodestaking_did_squatting.md

@@ -0,0 +1,111 @@
+# Bug: GitlawbNodeStaking — DID squatting via `registerNode` + unbounded `nodeIds` growth
+
+**Severity**: HIGH (combined : identity squat + amplifies the O(n) DoS from PR #7)
+
+**Affected**: `src/GitlawbNodeStaking.sol`
+
+## Issue 1 — `registerNode` accepts any bytes32 hash without proving DID control
+
+```solidity
+function registerNode(
+    bytes32 nodeDidHash,    // ← any hash, no proof
+    string calldata httpUrl,
+    uint256 stakeAmount
+) external {
+    if (stakeAmount < MIN_STAKE) revert BelowMinimumStake();
+    Node storage n = nodes[nodeDidHash];
+    if (n.operator != address(0)) revert AlreadyRegistered();
+    ...
+    n.operator = msg.sender;
+    ...
+}
+```
+
+No verification that `msg.sender` controls the DID corresponding to `nodeDidHash`. Attacker scenarios :
+
+1. **DID squatting** : Mallory computes `keccak256("did:gitlawb:alice-foundation")` and registers a node with that hash, gaining the on-chain operator-of-record for Alice's intended DID. Alice now CANNOT register because `AlreadyRegistered` reverts.
+
+2. **Reputation theft** : Once protocol revenue starts flowing, Mallory accrues rewards under Alice's brand. Resolvers showing the DID get Mallory's httpUrl.
+
+3. **Coordinated grief** : 100 random hashes can be registered at MIN_STAKE × 100 = 1 000 000 $GITLAWB locked. With $GITLAWB at $0.00009 that's ~$90 total cost to permanently squat 100 future DIDs.
+
+The DIDRegistry contract documents this as "first-come-first-served" by design, but NodeStaking inherits the same property without acknowledgement. Result : any non-Gitlawb-native DID (`did:web:`, `did:key:`) can be claimed at low cost by any address.
+
+## Issue 2 — `nodeIds` array never shrinks
+
+```solidity
+function registerNode(...) {
+    ...
+    nodeIds.push(nodeDidHash);  // ← grows monotonically
+    ...
+}
+
+function unstake(...) {
+    ...
+    n.stake = 0;
+    n.active = false;
+    // nodeIds entry NOT removed
+}
+```
+
+`unstake` zeros out the Node struct but never removes the entry from `nodeIds`. The array grows forever. Combined with PR #7's three O(n) loops in `depositRevenue`, the protocol bricks even when the ACTIVE node count is low — because the iteration runs over EVERY historically-registered node, including ones that exited years ago.
+
+After 5 years of node churn with ~200 average registrations / month, `nodeIds` has 12 000 entries — even though only ~500 are active. Each `depositRevenue` now iterates 12 000 + 12 000 + 12 000 = 36 000 storage reads. Guaranteed gas-out.
+
+## Issue 3 — Slashing documented but not implemented
+
+The README and roadmap state :
+
+> "Misbehavior triggers slashing penalties ranging from 10–100% of staked amounts."
+
+But there is NO slash function in `GitlawbNodeStaking.sol`. Only `transferOwnership` admin path. Misbehaving operators cannot be punished — only the operator themselves can `requestUnstake` to leave.
+
+This is a documented-vs-implemented gap. Listing it here so it's tracked in the same conversation, not as a fix request — it's a scope item.
+
+## Suggested fixes
+
+**For Issue 1** : require a signature from the DID's controller key (Ed25519 verifier for `did:key:`, GitlawbDIDRegistry lookup for `did:gitlawb:`).
+
+```solidity
+function registerNode(
+    bytes32 nodeDidHash,
+    bytes calldata didControllerSignature,  // ← new
+    string calldata httpUrl,
+    uint256 stakeAmount
+) external {
+    require(
+        didRegistry.verifyControllerSignature(
+            nodeDidHash, msg.sender, didControllerSignature
+        ),
+        "DID controller mismatch"
+    );
+    ...
+}
+```
+
+**For Issue 2** : track an `activeNodeIds` view that filters by `n.stake > 0`. Cheaper : on `unstake`, swap-and-pop the deregistered entry out of `nodeIds`.
+
+```solidity
+function unstake(bytes32 nodeDidHash) external {
+    ...
+    // swap-and-pop
+    uint256 idx = nodeIdsIndex[nodeDidHash];
+    uint256 last = nodeIds.length - 1;
+    if (idx != last) {
+        nodeIds[idx] = nodeIds[last];
+        nodeIdsIndex[nodeIds[idx]] = idx;
+    }
+    nodeIds.pop();
+    delete nodeIdsIndex[nodeDidHash];
+    ...
+}
+```
+
+**For Issue 3** : implement `slash(bytes32 nodeDidHash, uint256 bps)` callable by a designated slasher role + governance-controlled.
+
+## Reporter
+
+@philpof102-svg — operator `0xAC3ca7c5d3cDD7702fd08F9C4C28dAA22296aDa9` (Base)
+Related : [#5](https://github.com/Gitlawb/contracts/pull/5), [#6](https://github.com/Gitlawb/contracts/pull/6), [#7](https://github.com/Gitlawb/contracts/pull/7), [#8](https://github.com/Gitlawb/contracts/pull/8)
+
+This is the 5th finding in a continuous audit of all 6 Gitlawb contracts. Helping you ship Phase 8 mainnet safe.

src/GitlawbNodeStaking.sol

@@ -3,6 +3,13 @@ pragma solidity ^0.8.24;
 
 import {IERC20} from "lib/forge-std/src/interfaces/IERC20.sol";
 
+/// PR #9 fix : minimal interface to the DID registry so registerNode can
+/// require the caller is the registered DID owner. Closes the squatting
+/// surface where any address could grab a high-value DID hash.
+interface IDIDRegistryView {
+    function didToOwner(bytes32 didHash) external view returns (address);
+}
+
 /// @title GitlawbNodeStaking
 /// @notice Proof-of-Stake for gitlawb node operators on Base L2.
 ///
@@ -44,10 +51,21 @@ contract GitlawbNodeStaking {
     IERC20 public immutable token;
     address public owner;
 
+    /// PR #9 fix : optional DID registry binding. When set, registerNode
+    /// requires msg.sender to be the registered owner of nodeDidHash —
+    /// stops free DID squatting. Zero address keeps the legacy behavior
+    /// for protocols that don't use the DID registry.
+    IDIDRegistryView public didRegistry;
+
     /// nodeDidHash = keccak256(bytes(nodeDid))
     mapping(bytes32 => Node) public nodes;
     bytes32[] public nodeIds;
 
+    /// PR #9 fix : index into nodeIds for each registered DID (1-based; 0 = not present).
+    /// Lets unstake() remove the entry via swap-and-pop instead of leaving dead
+    /// slots that re-loop forever in depositRevenue.
+    mapping(bytes32 => uint256) private nodeIdIndex;
+
     /// Sum of stake across currently-active (heartbeat'd within threshold) nodes.
     /// Updated lazily — on deposit we rebuild from live heartbeats.
     uint256 public totalActiveStake;
@@ -81,6 +99,7 @@ contract GitlawbNodeStaking {
     error NoActiveStake();
     error TransferFailed();
     error ZeroAddress();
+    error NotDIDOwner(); // PR #9
 
     // ── Constructor ──────────────────────────────────────────────────────────
 
@@ -103,6 +122,13 @@ contract GitlawbNodeStaking {
         Node storage n = nodes[nodeDidHash];
         if (n.operator != address(0)) revert AlreadyRegistered();
 
+        // PR #9 fix : if a DID registry is configured, the caller MUST own
+        // the DID being registered as a node. Stops free squatting of
+        // high-value DIDs by random addresses.
+        if (address(didRegistry) != address(0)) {
+            if (didRegistry.didToOwner(nodeDidHash) != msg.sender) revert NotDIDOwner();
+        }
+
         bool ok = token.transferFrom(msg.sender, address(this), stakeAmount);
         if (!ok) revert TransferFailed();
 
@@ -116,6 +142,7 @@ contract GitlawbNodeStaking {
         n.rewardDebt = (stakeAmount * accRewardPerShare) / ACC_PRECISION;
 
         nodeIds.push(nodeDidHash);
+        nodeIdIndex[nodeDidHash] = nodeIds.length; // 1-based (PR #9 fix)
         totalRegisteredStake += stakeAmount;
         totalActiveStake += stakeAmount;
 
@@ -184,6 +211,25 @@ contract GitlawbNodeStaking {
         }
         totalRegisteredStake -= stakeAmount;
 
+        // PR #9 fix : swap-and-pop the entry out of nodeIds so subsequent
+        // depositRevenue() iterations don't keep paying gas for an empty slot.
+        // Without this, every full lifecycle (register → unstake) leaves a
+        // permanent dead entry. Over a year of operator churn this turns
+        // depositRevenue into an unbounded gas DoS even though active count
+        // stays bounded.
+        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;
+        }
+
         // Wipe node
         n.stake = 0;
         n.pendingRewards = 0;
@@ -369,6 +415,13 @@ contract GitlawbNodeStaking {
 
     // ── Admin ────────────────────────────────────────────────────────────────
 
+    /// PR #9 fix : bind/unbind the DID registry. Once bound, registerNode
+    /// enforces DID ownership. Settable by owner only.
+    function setDIDRegistry(address _registry) external {
+        if (msg.sender != owner) revert NotOwner();
+        didRegistry = IDIDRegistryView(_registry);
+    }
+
     function transferOwnership(address newOwner) external {
         if (msg.sender != owner) revert NotOwner();
         if (newOwner == address(0)) revert ZeroAddress();