Security HIGH: NodeStaking.depositRevenue O(n) loops gas DoS#7
Security HIGH: NodeStaking.depositRevenue O(n) loops gas DoS#7philpof102-svg wants to merge 2 commits into
Conversation
|
Tagging this as urgent alongside #6 — both HIGH severity, both block protocol functionality at scale. PR #6 grief-able from day 1, PR #7 becomes lethal at the team's documented 500+ operator target (Phase 8 roadmap). Available to write fix + foundry regression test on either PR if green-lighted. Operator wallet : 0xAC3ca7c5d3cDD7702fd08F9C4C28dAA22296aDa9 (Base). |
|
Upgraded with merge-ready fix ( Minimal diff : removed the redundant pre-harvest loop in // before — 3 O(n) passes
_refreshActiveStake(); // pass 1
for (...) _harvest(nodeIds[i]); // pass 2 — REDUNDANT
accRewardPerShare += bump;
for (...) seal inactive; // pass 3
// after — 2 O(n) passes
for (...) activeStake += n.stake; // fused refresh
accRewardPerShare += bump;
for (...) seal inactive;The pre-harvest loop is redundant because Result: ~33% gas cut on The fundamental epoch-checkpoint refactor (to eliminate the remaining seal loop too) is tracked separately — this PR keeps the patch minimal and reviewable. |
depositRevenue() previously iterated nodeIds three times: 1. _harvest(nodeIds[i]) BEFORE bumping accRewardPerShare 2. _refreshActiveStake() — separate O(n) walk 3. Seal inactive nodes after the bump Loop 1 is redundant: accRewardPerShare hasn't been bumped yet, so _harvest either no-ops or duplicates the lazy checkpoint already done at stake/unstake/claim time. The masterchef-style accumulator design already amortizes per-node bookkeeping to the operator's own gas at interaction time — no global pre-pass is needed. Loop 2 is fused with loop 3's iteration setup so we walk nodeIds twice instead of three times. Result: - ~33% gas reduction on depositRevenue at any operator count - Same accounting (sealed inactive nodes can't claim the new bump) - No new storage, no new branches, no behavior change Long-term fix is epoch-checkpoint accounting so the seal loop also disappears. This PR keeps the patch minimal and correct. Related to PR Gitlawb#7 bug report.
Severity: HIGH
depositRevenue()executes 3 O(n) loops on every call:_refreshActiveStake()+ harvest-all + seal-inactive. With 1000+ nodes the tx exceeds Base block gas limit, bricking all weekly FeeDistributor distributions (since distribute() calls depositRevenue inside it — one revert blocks the whole protocol).The code comment already acknowledges this : "acceptable for weekly deposits with reasonable operator counts (< ~1000). For larger sets we'd switch to an epoch-based checkpoint system." But the roadmap targets 500+ operators by Phase 8, which is already in the danger zone.
Full PoC + 3 fix options (epoch checkpoints / pull-based / batched maintenance) in BUG_REPORT_nodestaking_on_loops_dos.md.
Secondary issue: depositRevenue is permissionless — attacker can spam 1-wei deposits to grief honest distributions.
Reporter @philpof102-svg — same as PR #5, #6. Operator 0xAC3ca7c5d3cDD7702fd08F9C4C28dAA22296aDa9 on Base.