diff --git a/BUG_REPORT_feedist_centralization_and_griefing.md b/BUG_REPORT_feedist_centralization_and_griefing.md new file mode 100644 index 0000000..b7e52d7 --- /dev/null +++ b/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. diff --git a/src/GitlawbFeeDistributor.sol b/src/GitlawbFeeDistributor.sol index 6d502bb..c75db3b 100644 --- a/src/GitlawbFeeDistributor.sol +++ b/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,6 +187,10 @@ 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; @@ -174,12 +198,40 @@ contract GitlawbFeeDistributor { 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 {