diff --git a/BUG_REPORT_nodestaking_on_loops_dos.md b/BUG_REPORT_nodestaking_on_loops_dos.md new file mode 100644 index 0000000..d11fbbd --- /dev/null +++ b/BUG_REPORT_nodestaking_on_loops_dos.md @@ -0,0 +1,112 @@ +# Bug: GitlawbNodeStaking.depositRevenue() O(n) gas exhaustion DoS + +**Severity**: HIGH — protocol becomes unusable once `~3000+` nodes register. Base block gas limit hit, all weekly distributions fail. + +**Affected**: `src/GitlawbNodeStaking.sol`, function `depositRevenue(uint256)` + +## Root cause + +`depositRevenue()` executes THREE O(n) loops over `nodeIds` in the same call : + +```solidity +function depositRevenue(uint256 amount) external { + if (amount == 0) revert InvalidAmount(); + + // Loop 1 : _refreshActiveStake() iterates ALL nodes + _refreshActiveStake(); // O(n) + + uint256 activeStake = totalActiveStake; + if (activeStake == 0) revert NoActiveStake(); + bool ok = token.transferFrom(msg.sender, address(this), amount); + if (!ok) revert TransferFailed(); + + uint256 len = nodeIds.length; + + // Loop 2 : harvest every node + for (uint256 i = 0; i < len; i++) { + _harvest(nodeIds[i]); // O(n) × _harvest + } + + accRewardPerShare += (amount * ACC_PRECISION) / activeStake; + totalRewardsDistributed += amount; + + // Loop 3 : seal inactive nodes against the new acc + for (uint256 i = 0; i < len; i++) { + Node storage n = nodes[nodeIds[i]]; + if (n.stake > 0 && !_isActive(n)) { + n.rewardDebt = (n.stake * accRewardPerShare) / ACC_PRECISION; + } + } +} +``` + +Each loop iteration : storage read + arithmetic + maybe storage write. Roughly 8-15k gas per node per loop. With 3 loops : + +- n=100 nodes → ~3M gas (OK) +- n=500 nodes → ~15M gas (tight on Base) +- n=1000 nodes → ~30M+ gas (exceeds Base block limit, varies by load) +- n=3000+ nodes → guaranteed revert + +Once n exceeds the block-gas-limit threshold, **every weekly FeeDistributor.distribute() call reverts** because `nodeStaking.depositRevenue(...)` is inside `distribute()` and one revert nukes the whole tx. The protocol becomes permanently broken without an upgrade. + +## The doc comment already acknowledges this + +```solidity +/// Walk the nodes array and rebuild totalActiveStake based on live heartbeats. +/// O(n) in total registered nodes — acceptable for weekly deposits with +/// reasonable operator counts (< ~1000). For larger sets we'd switch to an +/// epoch-based checkpoint system. +function _refreshActiveStake() internal { +``` + +The team has acknowledged the limit but shipped it anyway, planning a future fix. With the project targeting "500+ independent operators" by Phase 8 (per the roadmap), the cap is already in the planned operating range. + +## Recommended fix + +**Option A — Epoch checkpoints** (recommended) : + +Track active stake per epoch (week). Snapshot once per epoch. Reward accrual reads the active stake at deposit-epoch, not refreshed at deposit time. + +**Option B — Pull-based accounting**: + +Instead of looping over all nodes on every deposit, store `accRewardPerShareSnapshot[epoch]`. Each node calculates its earnings lazily on `claimRewards()` or `_harvest()`. No O(n) on deposit. + +**Option C — Batched maintenance** (lowest disruption to current shape) : + +Split `depositRevenue` into two calls : +- `depositRevenue(amount)` — transfer in + update `accRewardPerShare`. O(1). +- `processEpoch(uint256 from, uint256 to)` — anyone can call, processes a slice of nodes. Permissionless, anyone gets a small reward per processed node. + +## Secondary DoS surface + +`depositRevenue` is **permissionless** — any address with allowance can call. An attacker can spam tiny deposits (1 wei via transferFrom) to trigger the O(n) loops, burning gas for honest harvests. Adding a minimum amount filter (≥ 1 token, like the `MIN_DISTRIBUTION` already in `FeeDistributor`) helps. + +## PoC + +```solidity +function test_depositRevenue_dos_at_1000_nodes() public { + // Register 1000 nodes (need 1000 wallets with 10k stake each = 10M GITLAWB locked) + for (uint256 i = 0; i < 1000; i++) { + address operator = makeAddr(string(abi.encodePacked("op", i))); + deal(address(token), operator, 10_000 ether); + vm.startPrank(operator); + token.approve(address(nodeStaking), 10_000 ether); + bytes32 nodeDid = keccak256(abi.encodePacked("did:gitlawb:", i)); + nodeStaking.registerNode(nodeDid, "https://x", 10_000 ether); + vm.stopPrank(); + } + + // FeeDistributor tries to distribute 100 GITLAWB + deal(address(token), feeDistributor, 100 ether); + + uint256 gasBefore = gasleft(); + vm.expectRevert(); // out of gas + feeDistributor.distribute(); +} +``` + +## Reporter + +@philpof102-svg — operator wallet `0xAC3ca7c5d3cDD7702fd08F9C4C28dAA22296aDa9` (Base) +MainStreet : https://avisradar-production.up.railway.app/mainstreet.html +Related : PR [#5 (stranded rewards)](https://github.com/Gitlawb/contracts/pull/5), [#6 (claimBounty DoS)](https://github.com/Gitlawb/contracts/pull/6) diff --git a/src/GitlawbNodeStaking.sol b/src/GitlawbNodeStaking.sol index a8610c8..abc5c5d 100644 --- a/src/GitlawbNodeStaking.sol +++ b/src/GitlawbNodeStaking.sol @@ -226,35 +226,35 @@ contract GitlawbNodeStaking { function depositRevenue(uint256 amount) external { if (amount == 0) revert InvalidAmount(); - // Refresh active stake snapshot before distributing - _refreshActiveStake(); - uint256 activeStake = totalActiveStake; - if (activeStake == 0) revert NoActiveStake(); - bool ok = token.transferFrom(msg.sender, address(this), amount); if (!ok) revert TransferFailed(); + // PR #7 fix : was 3 separate O(n) loops over nodeIds. The pre-harvest + // loop is redundant — accRewardPerShare hasn't been bumped yet, so + // _harvest would either no-op or duplicate the lazy checkpoint + // already done at stake/unstake/claim time. Removing it cuts gas by + // ~33% per deposit at scale. The two remaining passes are fused + // logically: one computes activeStake, the other seals inactive + // nodes against the new bump. uint256 len = nodeIds.length; - - // 1. Harvest every node with the PRE-update accRewardPerShare. Active - // nodes get their prior accrual credited to pendingRewards; inactive - // nodes with no prior accrual no-op (their debt already equals acc). + uint256 activeStake = 0; for (uint256 i = 0; i < len; i++) { - _harvest(nodeIds[i]); + Node storage n = nodes[nodeIds[i]]; + if (_isActive(n)) activeStake += n.stake; } + if (activeStake == 0) revert NoActiveStake(); + totalActiveStake = activeStake; - // 2. Bump accRewardPerShare for this deposit. - accRewardPerShare += (amount * ACC_PRECISION) / activeStake; + uint256 bump = (amount * ACC_PRECISION) / activeStake; + accRewardPerShare += bump; totalRewardsDistributed += amount; - // 3. Seal inactive nodes against the new acc — advance their rewardDebt - // so this deposit's bump is NOT counted as their earnings when they - // next interact. Active nodes are left alone; they'll earn their - // share at next harvest. + // Seal inactive nodes against this deposit's bump so they can't claim + // its share at next harvest. for (uint256 i = 0; i < len; i++) { Node storage n = nodes[nodeIds[i]]; if (n.stake > 0 && !_isActive(n)) { - n.rewardDebt = (n.stake * accRewardPerShare) / ACC_PRECISION; + n.rewardDebt += (n.stake * bump) / ACC_PRECISION; } }