Skip to content

Security : FeeDistributor centralization risks + grief surface#10

Open
philpof102-svg wants to merge 2 commits into
Gitlawb:mainfrom
philpof102-svg:fix-feedist-centralization
Open

Security : FeeDistributor centralization risks + grief surface#10
philpof102-svg wants to merge 2 commits into
Gitlawb:mainfrom
philpof102-svg:fix-feedist-centralization

Conversation

@philpof102-svg

Copy link
Copy Markdown

Three combined FeeDistributor findings :

  1. Owner can rug keeperShareBps to 0 in one tx (MAX_BPS_CHANGE=500, current=100, diff to 0 is 100). No timelock. Keepers built expecting 1% suddenly get 0.
  2. distribute() bricks if either sink reverts — combined with PR Security HIGH: NodeStaking.depositRevenue O(n) loops gas DoS #7's O(n) DoS, weekly distributions lock up entirely.
  3. setSinks no timelock — owner can redirect 99% of protocol revenue with single tx.

Full PoC + 7-day timelock fix pattern in BUG_REPORT_feedist_centralization_and_griefing.md.

6th PR in continuous Gitlawb audit (#5 #6 #7 #8 #9 #10). Reporter @philpof102-svg.

@philpof102-svg

Copy link
Copy Markdown
Author

Upgraded with merge-ready fix (src/GitlawbFeeDistributor.sol, +56/-4).

Two of the three issues fixed in this PR ; the third (sink revert grief) is deferred to a follow-up to keep this diff reviewable.

1) Keeper-share floor

uint256 public constant MIN_KEEPER_SHARE_BPS = 25; // 0.25 %

function setSplit(uint256 _n, uint256 _u, uint256 _k) external {
    ...
    if (_k < MIN_KEEPER_SHARE_BPS) revert KeeperShareTooLow();
    ...
}

Stops the multi-update walk-to-zero where owner shifts keeperShareBps by MAX_BPS_CHANGE per call until permissionless cron stops being profitable.

2) Sink timelock (replace setSinks with queue / execute / cancel)

uint256 public constant SINK_TIMELOCK = 2 days;
address public pendingNodeStaking;
address public pendingUserStaking;
uint256 public pendingSinksEta;

function queueSinks(address _n, address _u) external { ... } // emits SinksQueued
function executeSinks() external { ... } // only after eta
function cancelSinks() external { ... }

Stakers get a 2-day window to exit if the queued sink is malicious. Monitor SinksQueued events. cancelSinks lets the owner abort a queued change safely.

Migration : scripts/UI that called setSinks(a, b) must split into queueSinks(a, b) → wait → executeSinks().

The sink-revert grief (where a malicious sink reverts on depositRevenue and bricks the whole distribute() call) is a separate diff — needs try/catch wrapping in distribute and a partial-failure event. Happy to do that as a follow-up PR if you want to merge this one first.

…itlawb#10)

Three centralization risks in GitlawbFeeDistributor admin path :

1. **Keeper rug** : owner could drop keeperShareBps to 0 over multiple
   updates (5% per step, 4 weeks to zero), stopping the permissionless
   cron and forcing distributions through owner-controlled paths.
   Fix: MIN_KEEPER_SHARE_BPS = 25 (0.25 %) hard floor in setSplit().

2. **Sink swap with no notice** : setSinks() was instantaneous. Owner
   could swap nodeStaking/userStaking to a malicious contract that
   silently captures the next distribute() payout, with stakers having
   no exit window.
   Fix: replaced setSinks with queue/execute/cancel pattern gated by
   SINK_TIMELOCK = 2 days. Owner queues a swap; it executes only
   after the timelock. Stakers can monitor SinksQueued events and
   exit during the 2-day window.

3. (Sink revert grief is addressed by callers wrapping in try/catch in
   a follow-up — out of scope for this minimal fix to keep the diff
   reviewable. Tracked separately.)

New surface :
- constant: MIN_KEEPER_SHARE_BPS, SINK_TIMELOCK
- storage:  pendingNodeStaking, pendingUserStaking, pendingSinksEta
- events:   SinksQueued, SinksCancelled
- errors:   KeeperShareTooLow, NoPendingSinks, TimelockNotElapsed
- removed:  setSinks(address, address)
- added:    queueSinks, executeSinks, cancelSinks

Migration : front-end / scripts that called setSinks must split into
queueSinks → wait 2 days → executeSinks.

Related to PR Gitlawb#10 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