Security : FeeDistributor centralization risks + grief surface

BUG_REPORT_feedist_centralization_and_griefing.md

@@ -0,0 +1,106 @@
+# Bug: GitlawbFeeDistributor — owner can rug keeperShare to 0; griefing surface on weekly distribute
+
+**Severity**: MEDIUM (centralization) + LOW (griefing)
+
+**Affected**: `src/GitlawbFeeDistributor.sol`
+
+## Issue 1 — Owner can drop keeperShareBps to 0 in one tx
+
+`setSplit` enforces `MAX_BPS_CHANGE = 500` per call but with no timelock. Current `keeperShareBps = 100`. From the constants :
+
+```solidity
+uint256 public constant MAX_BPS_CHANGE = 500; // owner can shift at most 5% per update
+```
+
+Diff calculation `_diff(100, 0) = 100 ≤ 500`. Owner can call `setSplit(7600, 2400, 0)` in a single tx with no notice :
+- nodeBps : 7500 → 7600 (diff 100 ≤ 500) ✓
+- userBps : 2400 → 2400 (diff 0) ✓
+- keeperBps : 100 → 0 (diff 100 ≤ 500) ✓
+- sum = 10000 ✓
+
+Keepers who built infrastructure expecting 1% caller reward suddenly get 0. Worse, the owner can front-run an inflight `distribute()` in the mempool with the rug-`setSplit` tx (higher gas) so the caller pays gas for 0 reward.
+
+## Issue 2 — `distribute()` reverts on sink failure → griefing
+
+```solidity
+nodeStaking.depositRevenue(nodeShare);   // ← if reverts, whole distribute() reverts
+userStaking.depositRevenue(userShare);   // ← same
+```
+
+If either sink reverts (paused, gas exhaust, or PR #7's O(n) DoS), the entire weekly distribution is bricked. Keepers lose gas trying. The fee pool grows uncollected until owner intervenes.
+
+Combined with PR #7's O(n) DoS in `GitlawbNodeStaking.depositRevenue`, this is a guaranteed lockup once the node count crosses the gas-limit threshold.
+
+## Issue 3 — `setSinks` has no timelock
+
+```solidity
+function setSinks(address _nodeStaking, address _userStaking) external {
+    if (msg.sender != owner) revert NotOwner();
+    ...
+    nodeStaking = IRevenueSink(_nodeStaking);
+    userStaking = IRevenueSink(_userStaking);
+}
+```
+
+Owner can redirect 99% of the protocol's revenue to a wallet they control by setting both sinks to an attacker contract. No timelock, no governance, no event the community can react to before the next distribute() fires.
+
+The doc claims protocol is "permissionless weekly distribution" but the owner can drain the next distribution into their own pocket.
+
+## Issue 4 — `lastDistribution = block.timestamp` set BEFORE balance check
+
+```solidity
+function distribute() external {
+    uint256 next = lastDistribution + DISTRIBUTION_PERIOD;
+    if (block.timestamp < next) revert TooSoon(next);
+
+    uint256 bal = token.balanceOf(address(this));
+    if (bal < MIN_DISTRIBUTION) revert NothingToDistribute();
+
+    lastDistribution = block.timestamp;  // ← set after bal check, OK
+    ...
+}
+```
+
+Actually checked — `lastDistribution` is set AFTER the `NothingToDistribute` check. Safe. (Including this so audit log shows it was reviewed.)
+
+## Suggested fixes
+
+**Issue 1+3 — timelock on admin functions** :
+
+```solidity
+mapping(bytes32 => uint256) public pendingChange;
+uint256 public constant TIMELOCK = 7 days;
+
+function proposeSplit(uint256 _nodeBps, uint256 _userBps, uint256 _keeperBps) external {
+    if (msg.sender != owner) revert NotOwner();
+    bytes32 h = keccak256(abi.encode(_nodeBps, _userBps, _keeperBps));
+    pendingChange[h] = block.timestamp + TIMELOCK;
+    emit ChangeProposed(h, _nodeBps, _userBps, _keeperBps);
+}
+
+function applySplit(uint256 _nodeBps, uint256 _userBps, uint256 _keeperBps) external {
+    bytes32 h = keccak256(abi.encode(_nodeBps, _userBps, _keeperBps));
+    require(pendingChange[h] != 0 && block.timestamp >= pendingChange[h], "not ready");
+    delete pendingChange[h];
+    // existing setSplit logic
+}
+```
+
+Same pattern for `setSinks`. 7-day timelock gives stakers and keepers time to exit if they disagree.
+
+**Issue 2 — graceful sink failure** :
+
+```solidity
+try nodeStaking.depositRevenue(nodeShare) {
+    // ok
+} catch {
+    emit SinkFailed("node");
+    // accumulate for next epoch instead of bricking
+    nodeShareCarryover += nodeShare;
+}
+```
+
+## Reporter
+
+@philpof102-svg — operator `0xAC3ca7c5d3cDD7702fd08F9C4C28dAA22296aDa9` (Base)
+6th PR in the continuous Gitlawb audit. Related : #5, #6, #7, #8, #9.

src/GitlawbFeeDistributor.sol

@@ -42,10 +42,25 @@ contract GitlawbFeeDistributor {
     uint256 public constant MAX_BPS_CHANGE = 500; // owner can shift at most 5% per update
     uint256 public constant MIN_DISTRIBUTION = 1e18; // 1 token — dust gate
 
+    /// PR #10 fix : floor on keeperShareBps so owner can't rug keepers to 0
+    /// (which would stop the permissionless cron and force distributions
+    /// through owner-controlled paths).
+    uint256 public constant MIN_KEEPER_SHARE_BPS = 25; // 0.25% — small but non-zero
+
+    /// PR #10 fix : timelock on setSinks. Owner queues a swap; it executes
+    /// only after SINK_TIMELOCK has elapsed. Gives stakers a window to exit
+    /// if the new sink is malicious.
+    uint256 public constant SINK_TIMELOCK = 2 days;
+
     uint256 public lastDistribution;
     uint256 public totalDistributed;
     uint256 public distributionCount;
 
+    /// PR #10 fix : queued sink swap. pendingSinksEta == 0 means no pending change.
+    address public pendingNodeStaking;
+    address public pendingUserStaking;
+    uint256 public pendingSinksEta;
+
     // ── Events ───────────────────────────────────────────────────────────────
 
     event FeesDistributed(
@@ -58,6 +73,8 @@ contract GitlawbFeeDistributor {
     );
     event SplitUpdated(uint256 nodeShareBps, uint256 userShareBps, uint256 keeperShareBps);
     event SinksUpdated(address nodeStaking, address userStaking);
+    event SinksQueued(address nodeStaking, address userStaking, uint256 eta); // PR #10
+    event SinksCancelled(); // PR #10
 
     // ── Errors ───────────────────────────────────────────────────────────────
 
@@ -68,6 +85,9 @@ contract GitlawbFeeDistributor {
     error ChangeTooLarge();
     error ZeroAddress();
     error TransferFailed();
+    error KeeperShareTooLow(); // PR #10
+    error NoPendingSinks(); // PR #10
+    error TimelockNotElapsed(); // PR #10
 
     // ── Constructor ──────────────────────────────────────────────────────────
 
@@ -167,19 +187,51 @@ contract GitlawbFeeDistributor {
         if (_diff(_userBps, userShareBps) > MAX_BPS_CHANGE) revert ChangeTooLarge();
         if (_diff(_keeperBps, keeperShareBps) > MAX_BPS_CHANGE) revert ChangeTooLarge();
 
+        // PR #10 fix : enforce a non-zero floor on keeperShareBps so the
+        // owner can't strand the permissionless cron over multiple updates.
+        if (_keeperBps < MIN_KEEPER_SHARE_BPS) revert KeeperShareTooLow();
+
         nodeShareBps = _nodeBps;
         userShareBps = _userBps;
         keeperShareBps = _keeperBps;
 
         emit SplitUpdated(_nodeBps, _userBps, _keeperBps);
     }
 
-    function setSinks(address _nodeStaking, address _userStaking) external {
+    /// PR #10 fix : sink swap is now a two-step process gated by SINK_TIMELOCK.
+    /// Step 1 (queue) sets pendingSinks + eta. Step 2 (execute) applies them
+    /// once eta has passed. Stakers can use the window to exit if the new
+    /// sink is malicious. Owner can cancel a pending swap before execution.
+    function queueSinks(address _nodeStaking, address _userStaking) external {
         if (msg.sender != owner) revert NotOwner();
         if (_nodeStaking == address(0) || _userStaking == address(0)) revert ZeroAddress();
-        nodeStaking = IRevenueSink(_nodeStaking);
-        userStaking = IRevenueSink(_userStaking);
-        emit SinksUpdated(_nodeStaking, _userStaking);
+        pendingNodeStaking = _nodeStaking;
+        pendingUserStaking = _userStaking;
+        pendingSinksEta = block.timestamp + SINK_TIMELOCK;
+        emit SinksQueued(_nodeStaking, _userStaking, pendingSinksEta);
+    }
+
+    function executeSinks() external {
+        if (msg.sender != owner) revert NotOwner();
+        if (pendingSinksEta == 0) revert NoPendingSinks();
+        if (block.timestamp < pendingSinksEta) revert TimelockNotElapsed();
+        address _node = pendingNodeStaking;
+        address _user = pendingUserStaking;
+        nodeStaking = IRevenueSink(_node);
+        userStaking = IRevenueSink(_user);
+        pendingNodeStaking = address(0);
+        pendingUserStaking = address(0);
+        pendingSinksEta = 0;
+        emit SinksUpdated(_node, _user);
+    }
+
+    function cancelSinks() external {
+        if (msg.sender != owner) revert NotOwner();
+        if (pendingSinksEta == 0) revert NoPendingSinks();
+        pendingNodeStaking = address(0);
+        pendingUserStaking = address(0);
+        pendingSinksEta = 0;
+        emit SinksCancelled();
     }
 
     function transferOwnership(address newOwner) external {