From 73305482e2322696d47d516cb18a20fc90a77c33 Mon Sep 17 00:00:00 2001 From: Philippe Date: Thu, 4 Jun 2026 01:26:11 +0200 Subject: [PATCH 1/2] Bug: NodeStaking DID squatting + nodeIds memory leak + slashing scope gap --- BUG_REPORT_nodestaking_did_squatting.md | 111 ++++++++++++++++++++++++ 1 file changed, 111 insertions(+) create mode 100644 BUG_REPORT_nodestaking_did_squatting.md diff --git a/BUG_REPORT_nodestaking_did_squatting.md b/BUG_REPORT_nodestaking_did_squatting.md new file mode 100644 index 0000000..de6e8e0 --- /dev/null +++ b/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. From cc75f8a3f4ac33c34e82448dc12cef18c22ee83c Mon Sep 17 00:00:00 2001 From: Philippe Date: Thu, 4 Jun 2026 01:46:11 +0200 Subject: [PATCH 2/2] fix: prevent DID squatting + plug nodeIds memory leak (PR #9) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 #9 bug report. --- src/GitlawbNodeStaking.sol | 53 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/src/GitlawbNodeStaking.sol b/src/GitlawbNodeStaking.sol index a8610c8..f7c1945 100644 --- a/src/GitlawbNodeStaking.sol +++ b/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();