Audit L-04: FeeRouter permissionless distributeFees + governed ERC-20 rescue / docs #122

Closed
opened 2026-05-04 04:19:37 +00:00 by PlasticDigits · 7 comments
PlasticDigits commented 2026-05-04 04:19:37 +00:00 (Migrated from gitlab.com)

Summary

Track remediation for L-04 in audits/audit_smartcontract_1777813071.md: public FeeRouter.distributeFees + sink design means any ERC-20 balance held by the router can be split and pushed to configured sinks by any caller. That supports liveness and dust cleanup for the canonical sale asset but creates a misrouting risk if third-party or accidental tokens are sent to the router address.

Affected onchain surface (per audit): FeeRouter, FeeSink (and concrete sinks), PodiumPool.


Problem statement

  1. FeeRouter.distributeFees(IERC20 token, uint256 amount) is permissionless. Anyone who knows the router’s balance can invoke it with a token + amount the router holds. The router then splits per bps and safeTransfers to the five sink destinations — including PodiumPool, burn address, DoubLPIncentives, RabbitTreasury, etc., depending on deployment.

  2. FeeRouter is not a custody / deposit contract — integrators and users must not treat it as a wallet. Today, accidental transfers (wrong token, airdrops, phishing dust) can still be mechanically “absorbed” into the fee-split plumbing by the first caller that runs distributeFees, with no governance step.

  3. Sink asymmetry: Concrete sinks that is FeeSink expose withdraw(token, to, amount) gated by WITHDRAWER_ROLE (governed recovery of balances at the sink). PodiumPool is not a FeeSink: it only exposes payPodiumPayout (wired to TimeCurve as prizePusher). So stray ERC-20 that lands in PodiumPool via wrong-token distribution or direct transfer has no symmetric FeeSink.withdraw path today.


Goals (both audit recommendations are in scope)

Per L-04, both directions apply:

  1. Documentation: Make it unambiguous that FeeRouter is not a custody contract and that permissionless distributeFees can move any token balance held by the router.

  2. Policy / mechanism: If accidental third-party tokens are plausible, add an allowlist and/or governed rescue path so recovery is explicit, reviewable, and role-governed — consistent with how FeeSink.withdraw handles ERC-20 at sinks (WITHDRAWER_ROLE / admin hierarchy).


A. Documentation hardening (required baseline)

  • Participant + integrator docs: Never deposit to FeeRouter; only TimeCurve (and documented paths) should fund fee routing for the accepted asset.
  • Operator runbooks: What to do if wrong token appears on FeeRouter / sinks; who holds GOVERNOR_ROLE, WITHDRAWER_ROLE, DEFAULT_ADMIN_ROLE on proxies (GitLab #54 — UUPS proxy as canonical address).

Pick one or combine (security review required):

  1. FeeRouter — token allowlist for distributeFees:

    • Only tokens in an GOVERNOR_ROLE-managed allowlist (or the canonical sale asset address registered from TimeCurve) may be split via distributeFees.
    • Stray tokens require rescueERC20(IERC20 token, address to, uint256 amount) (or batch variant) onlyRole(GOVERNOR_ROLE) (or DEFAULT_ADMIN_ROLE if that matches other modules — must match repo governance doc).
  2. FeeRouter — rescue-only, keep permissionless distribute for allowlisted token:

    • Minimal change: distributeFees restricted to one canonical token; rescueERC20 for everything else under GOVERNOR_ROLE.
  3. PodiumPool — parity with FeeSink for non-prize stray assets:

    • Add withdraw(IERC20 token, address to, uint256 amount) with WITHDRAWER_ROLE (or reuse DEFAULT_ADMIN_ROLE pattern from FeeSink) only for tokens that are not the intended prize asset / only when not conflicting with distributePrizes accounting — exact rules need a spec to avoid prize theft (e.g. allowlist recovery tokens, or pause prize path during recovery — product/security tradeoff).

Non-goals: Do not introduce permissionless sweeps of user funds; any new path must be governed and evented.


Regression / new tests (Forge)

Test intent Suggested location / pattern
Allowlisted token: distributeFees still splits correctly; sinks receive expected shares Extend contracts/test/FeeRouter.t.sol, contracts/test/FeeRouterInvariant.t.sol
Non-allowlisted token: distributeFees reverts; governed rescue succeeds; no fee-split side effects New tests in FeeRouter.t.sol
Rescue only callable by governor; withdrawer / random EOA reverts Access control tests
PodiumPool stray-token withdraw (if implemented): cannot drain legitimate CL8Y prize balance or tests lock only wrong token contracts/test/FeeSinks.t.sol / new PodiumPoolRescue.t.sol
Upgrade / storage layout: new state (allowlist, pause flags) respects UUPS gap OZ upgrades validation / forge test

Files likely to need updates

Solidity / tests

Documentation

Indexer / frontend (if new events or governance params are product-visible)


Acceptance criteria

  1. Documented: FeeRouter is explicitly non-custodial; permissionless distributeFees behavior and risks are ** operator- and integrator-visible** in canonical docs.
  2. Governed recovery: If implementation proceeds, stray / wrong ERC-20 on FeeRouter (and PodiumPool if in scope) can be recovered only via roles consistent with FeeSink.withdraw governance expectations (no new permissionless value extraction beyond today’s distributeFees semantics for intended assets).
  3. Canonical sale path: TimeCurve → FeeRouter → sinks for the accepted asset remains correct, test-covered, and not materially more gas-fragile for happy path without governance justification.
  4. Tests: New behavior has Forge coverage; invariants updated or extended where balances are ghost-tracked.
  5. Accuracy: docs/onchain/cl8y-flow-audit.md and related security statements remain factually true after the change set.

Verification checklists

Pre-merge (engineering)

  • cd contracts && forge test passes (or CI green) including new FeeRouter / PodiumPool tests.
  • If FeeRouter storage changes: UUPS layout check / OpenZeppelin upgrades workflow completed; __gap preserved.
  • initialize / DeployDev: no address(0) admin; regression for #54 proxy canonical addressing.
  • NatSpec + event emissions for any rescue or allowlist mutation (indexer/ops observability).

Post-deploy / operations (staging or checklist for mainnet)

  • Role inventory: GOVERNOR_ROLE, WITHDRAWER_ROLE, DEFAULT_ADMIN_ROLE holders documented for FeeRouter and each sink proxy.
  • Dry run: Accidental token on FeeRouter → rescue path (or documented manual sink withdrawal if only FeeSink sinks receive dust).
  • Wrong-token distributeFees cannot misroute past allowlist rules (negative test evidence captured in CI).

Cross-functional

  • Product / risk signoff that allowlist does not block future asset migrations (governance process to update allowlist).
  • Support playbook: User sent tokens to FeeRouter — escalation and irreversibility messaging.

References

  • Audit: audits/audit_smartcontract_1777813071.md — § L-04: Public fee distribution and sink withdrawals depend on correct token/sink choices
  • Governance pattern: contracts/src/sinks/FeeSink.sol — withdraw + WITHDRAWER_ROLE
  • Current router: contracts/src/FeeRouter.sol — distributeFees, GOVERNOR_ROLE, updateSinks
## Summary Track remediation for **L-04** in [`audits/audit_smartcontract_1777813071.md`](audits/audit_smartcontract_1777813071.md): **public `FeeRouter.distributeFees` + sink design** means **any ERC-20 balance held by the router** can be **split and pushed to configured sinks** by **any caller**. That supports **liveness and dust cleanup** for the **canonical sale asset** but creates a **misrouting** risk if **third-party or accidental tokens** are sent to the router address. **Affected onchain surface (per audit):** `FeeRouter`, `FeeSink` (and concrete sinks), `PodiumPool`. --- ## Problem statement 1. **`FeeRouter.distributeFees(IERC20 token, uint256 amount)`** is **permissionless**. Anyone who knows the router’s balance can invoke it with a **token + amount** the router holds. The router then **splits per bps** and **`safeTransfer`s** to the **five sink destinations** — including **`PodiumPool`**, burn address, **`DoubLPIncentives`**, **`RabbitTreasury`**, etc., depending on deployment. 2. **`FeeRouter` is not a custody / deposit contract** — integrators and users **must not** treat it as a wallet. Today, **accidental transfers** (wrong token, airdrops, phishing dust) can still be **mechanically “absorbed” into the fee-split plumbing** by the **first** caller that runs `distributeFees`, with **no governance step**. 3. **Sink asymmetry:** Concrete sinks that **`is FeeSink`** expose **`withdraw(token, to, amount)`** gated by **`WITHDRAWER_ROLE`** (governed recovery of balances **at the sink**). **`PodiumPool`** is **not** a `FeeSink`: it only exposes **`payPodiumPayout`** (wired to **`TimeCurve`** as **`prizePusher`**). So **stray ERC-20** that lands in **`PodiumPool`** via **wrong-token distribution** or **direct transfer** has **no** symmetric **`FeeSink.withdraw`** path today. --- ## Goals (both audit recommendations are in scope) Per L-04, **both** directions apply: 1. **Documentation:** Make it **unambiguous** that **`FeeRouter` is not a custody contract** and that **permissionless `distributeFees`** can move **any** token balance held by the router. 2. **Policy / mechanism:** If **accidental third-party tokens** are plausible, add an **allowlist and/or governed rescue path** so **recovery** is **explicit, reviewable, and role-governed** — **consistent with** how **`FeeSink.withdraw`** handles **ERC-20** at sinks (**`WITHDRAWER_ROLE`** / admin hierarchy). --- ## Recommended fixes (design options — implementer picks per product tolerance) ### A. Documentation hardening (required baseline) - **Participant + integrator docs:** Never deposit to **`FeeRouter`**; only **`TimeCurve`** (and documented paths) should fund fee routing for the **accepted asset**. - **Operator runbooks:** What to do if wrong token appears on **`FeeRouter`** / sinks; **who** holds **`GOVERNOR_ROLE`**, **`WITHDRAWER_ROLE`**, **`DEFAULT_ADMIN_ROLE`** on proxies ([GitLab #54](https://gitlab.com/PlasticDigits/yieldomega/-/issues/54) — UUPS proxy as canonical address). ### B. Onchain: governed rescue aligned with `FeeSink` (recommended if code changes are approved) Pick **one** or **combine** (security review required): 1. **`FeeRouter` — token allowlist for `distributeFees`:** - Only tokens in an **`GOVERNOR_ROLE`**-managed allowlist (or the **canonical sale asset** address registered from **`TimeCurve`**) may be **split** via **`distributeFees`**. - **Stray tokens** require **`rescueERC20(IERC20 token, address to, uint256 amount)`** (or batch variant) **`onlyRole(GOVERNOR_ROLE)`** (or **`DEFAULT_ADMIN_ROLE`** if that matches other modules — **must match repo governance doc**). 2. **`FeeRouter` — rescue-only, keep permissionless distribute for allowlisted token:** - Minimal change: **`distributeFees`** restricted to **one** canonical token; **`rescueERC20`** for everything else under **`GOVERNOR_ROLE`**. 3. **`PodiumPool` — parity with `FeeSink` for non-prize stray assets:** - Add **`withdraw(IERC20 token, address to, uint256 amount)`** with **`WITHDRAWER_ROLE`** (or reuse **`DEFAULT_ADMIN_ROLE`** pattern from **`FeeSink`**) **only for tokens that are not** the **intended prize asset** / **only when** not conflicting with **`distributePrizes`** accounting — exact rules need a **spec** to avoid **prize theft** (e.g. allowlist **recovery** tokens, or **pause** prize path during recovery — product/security tradeoff). **Non-goals:** Do **not** introduce **permissionless** sweeps of user funds; any new path must be **governed** and **evented**. --- ## Regression / new tests (Forge) | Test intent | Suggested location / pattern | |-------------|------------------------------| | **Allowlisted** token: **`distributeFees`** still splits correctly; sinks receive expected shares | Extend [`contracts/test/FeeRouter.t.sol`](contracts/test/FeeRouter.t.sol), [`contracts/test/FeeRouterInvariant.t.sol`](contracts/test/FeeRouterInvariant.t.sol) | | **Non-allowlisted** token: **`distributeFees` reverts**; **governed rescue** succeeds; **no** fee-split side effects | New tests in `FeeRouter.t.sol` | | **Rescue** only callable by **governor**; **withdrawer** / **random** EOA reverts | Access control tests | | **`PodiumPool`** stray-token **withdraw** (if implemented): cannot **drain** legitimate **CL8Y** prize balance **or** tests lock **only** wrong token | [`contracts/test/FeeSinks.t.sol`](contracts/test/FeeSinks.t.sol) / new `PodiumPoolRescue.t.sol` | | **Upgrade / storage layout:** new state (allowlist, pause flags) respects **UUPS gap** | OZ upgrades validation / `forge test` | --- ## Files likely to need updates ### Solidity / tests - [`contracts/src/FeeRouter.sol`](contracts/src/FeeRouter.sol) — allowlist and/or **`rescueERC20`**; events; NatSpec. - [`contracts/src/sinks/PodiumPool.sol`](contracts/src/sinks/PodiumPool.sol) — optional governed **`withdraw`** (if spec’d). - [`contracts/test/FeeRouter.t.sol`](contracts/test/FeeRouter.t.sol), [`contracts/test/FeeRouterInvariant.t.sol`](contracts/test/FeeRouterInvariant.t.sol) - [`contracts/test/FeeSinks.t.sol`](contracts/test/FeeSinks.t.sol) — PodiumPool changes. - [`contracts/script/UUPSDeployLib.sol`](contracts/script/UUPSDeployLib.sol), [`contracts/script/DeployDev.s.sol`](contracts/script/DeployDev.s.sol) — initializer args / wiring if new storage. - Any integration tests touching **buy → FeeRouter** (e.g. [`contracts/test/TimeCurve.t.sol`](contracts/test/TimeCurve.t.sol), [`contracts/test/DevStackIntegration.t.sol`](contracts/test/DevStackIntegration.t.sol)) if **`distributeFees`** signature or behavior changes. ### Documentation - [`docs/onchain/fee-routing-and-governance.md`](docs/onchain/fee-routing-and-governance.md) — **anti-pattern**: “do not send tokens to `FeeRouter`”; **governance** for allowlist/rescue; **post-update invariants**. - [`docs/product/primitives.md`](docs/product/primitives.md) — short **integrator warning** if needed. - [`docs/testing/invariants-and-business-logic.md`](docs/testing/invariants-and-business-logic.md) — new invariant rows + test map links for L-04 fee-router **allowlist/rescue**. - [`docs/onchain/cl8y-flow-audit.md`](docs/onchain/cl8y-flow-audit.md) — update **“No hidden rescue”** bullet if **governed** rescue is added (must remain accurate). - [`audits/audit_smartcontract_1777813071.md`](audits/audit_smartcontract_1777813071.md) — optional **status** line when addressed (per repo practice). ### Indexer / frontend (if new events or governance params are product-visible) - [`indexer/`](indexer/) — decode **`SinksUpdated`–class** / new **`TokenAllowed`** / **`Rescued`** events if operators need receipts ([`docs/indexer/design.md`](docs/indexer/design.md)). - [`frontend/src/components/FeeTransparency.tsx`](frontend/src/components/FeeTransparency.tsx) — only if UX should surface allowlist state (optional). --- ## Acceptance criteria 1. **Documented:** **`FeeRouter`** is explicitly **non-custodial**; permissionless **`distributeFees`** behavior and risks are ** operator- and integrator-visible** in canonical docs. 2. **Governed recovery:** If implementation proceeds, **stray / wrong ERC-20** on **`FeeRouter`** (and **`PodiumPool`** if in scope) can be **recovered only via roles** consistent with **`FeeSink.withdraw`** governance expectations (**no** new permissionless value extraction beyond today’s **`distributeFees`** semantics for **intended** assets). 3. **Canonical sale path:** **`TimeCurve` → `FeeRouter` → sinks** for the **accepted asset** remains **correct**, **test-covered**, and **not** materially more gas-fragile for happy path without governance justification. 4. **Tests:** New behavior has **Forge** coverage; **invariants** updated or extended where balances are ghost-tracked. 5. **Accuracy:** [`docs/onchain/cl8y-flow-audit.md`](docs/onchain/cl8y-flow-audit.md) and related **security** statements remain **factually true** after the change set. --- ## Verification checklists ### Pre-merge (engineering) - [ ] `cd contracts && forge test` passes (or CI green) including new **`FeeRouter`** / **`PodiumPool`** tests. - [ ] If **`FeeRouter`** storage changes: **UUPS** layout check / OpenZeppelin upgrades workflow completed; **`__gap`** preserved. - [ ] **`initialize` / `DeployDev`**: no **`address(0)`** admin; regression for **#54** proxy canonical addressing. - [ ] NatSpec + **`event`** emissions for any **rescue** or **allowlist** mutation (indexer/ops **observability**). ### Post-deploy / operations (staging or checklist for mainnet) - [ ] **Role inventory:** **`GOVERNOR_ROLE`**, **`WITHDRAWER_ROLE`**, **`DEFAULT_ADMIN_ROLE`** holders documented for **`FeeRouter`** and each **sink** proxy. - [ ] **Dry run:** Accidental token on **`FeeRouter`** → **rescue** path (or documented **manual** sink withdrawal if only **`FeeSink`** sinks receive dust). - [ ] **Wrong-token `distributeFees`** cannot misroute past allowlist rules (negative test evidence captured in CI). ### Cross-functional - [ ] Product / risk signoff that **allowlist** does not block **future** asset migrations (governance process to update allowlist). - [ ] **Support playbook:** User sent tokens to **`FeeRouter`** — escalation and **irreversibility** messaging. --- ## References - Audit: **`audits/audit_smartcontract_1777813071.md`** — **§ L-04: Public fee distribution and sink withdrawals depend on correct token/sink choices** - **Governance pattern:** [`contracts/src/sinks/FeeSink.sol`](contracts/src/sinks/FeeSink.sol) — **`withdraw`** + **`WITHDRAWER_ROLE`** - **Current router:** [`contracts/src/FeeRouter.sol`](contracts/src/FeeRouter.sol) — **`distributeFees`**, **`GOVERNOR_ROLE`**, **`updateSinks`**
PlasticDigits commented 2026-05-04 05:38:31 +00:00 (Migrated from gitlab.com)

mentioned in commit 8255f4344e

mentioned in commit 8255f4344ee8aefa431c82e5ba66050c344d729b
PlasticDigits commented 2026-05-04 05:53:58 +00:00 (Migrated from gitlab.com)

@brouie Pushed GitLab #123 ingress parity work on `main` (`a574c63`): pull paths now credit measured ERC-20 balance deltas so fee-on-transfer reverts with `TimeCurve: ERC20 parity` (and analogues) instead of opaque `ERC20InsufficientBalance`; `FeeRouter.distributeFees` / `totalRaised` use the measured segment. Complements #122 allowlist + governed rescue.

Verification

  • `FOUNDRY_PROFILE=ci forge test` (261 tests)
  • Deploy scripts that fund the router still call `setDistributableToken` for the sale asset

Leaving #122 open per instructions.

@brouie Pushed **GitLab #123** ingress parity work on \`main\` (\`a574c63\`): pull paths now credit **measured** ERC-20 balance deltas so fee-on-transfer reverts with \`TimeCurve: ERC20 parity\` (and analogues) instead of opaque \`ERC20InsufficientBalance\`; \`FeeRouter.distributeFees\` / \`totalRaised\` use the measured segment. Complements **#122** allowlist + governed rescue. **Verification** - \`FOUNDRY_PROFILE=ci forge test\` (261 tests) - Deploy scripts that fund the router still call \`setDistributableToken\` for the sale asset Leaving **#122** open per instructions.
Brouie commented 2026-05-04 06:22:58 +00:00 (Migrated from gitlab.com)

verified on commit 8255f43 (merged via ebbf58e). went the allowlist + rescue route — strongest mitigation from your audit options.

allowlist gate (closes the accidental-ERC20 routing path):

  • test_distributeFees_reverts_when_token_not_distributable — non-allowed tokens revert at distribute, not silently route
  • test_setDistributableToken_emits_and_gate — governance configures + emits event
  • test_setDistributableToken_unauthorized_reverts — non-governance can't add tokens
  • test_setDistributableToken_zero_token_reverts — zero-token guard

governed rescue:

  • test_rescueERC20_moves_balance — moves stranded balance to ops wallet
  • test_rescueERC20_unauthorized_reverts — only governance
  • test_rescueERC20_zero_to_reverts — zero-recipient guard

invariants intact (256 runs, 6400 calls, 0 reverts each):

  • invariant_feeRouter_routerBalanceMatchesGhost
  • invariant_feeRouter_sinksSumEqualsDistributed

indexer visibility: new migration 20260504130000_fee_router_governance_events.up.sql ships the event tables for ops auditability of allowlist changes + rescues.

distribute path now reverts on non-allowed tokens instead of silently routing — closes both halves of L-04 ("any accidental ERC20 sent to the router can be routed by anyone" and the rescue gap).

verified on commit `8255f43` (merged via `ebbf58e`). went the allowlist + rescue route — strongest mitigation from your audit options. **allowlist gate (closes the accidental-ERC20 routing path):** - `test_distributeFees_reverts_when_token_not_distributable` — non-allowed tokens revert at distribute, not silently route - `test_setDistributableToken_emits_and_gate` — governance configures + emits event - `test_setDistributableToken_unauthorized_reverts` — non-governance can't add tokens - `test_setDistributableToken_zero_token_reverts` — zero-token guard **governed rescue:** - `test_rescueERC20_moves_balance` — moves stranded balance to ops wallet - `test_rescueERC20_unauthorized_reverts` — only governance - `test_rescueERC20_zero_to_reverts` — zero-recipient guard **invariants intact (256 runs, 6400 calls, 0 reverts each):** - `invariant_feeRouter_routerBalanceMatchesGhost` - `invariant_feeRouter_sinksSumEqualsDistributed` **indexer visibility:** new migration `20260504130000_fee_router_governance_events.up.sql` ships the event tables for ops auditability of allowlist changes + rescues. distribute path now reverts on non-allowed tokens instead of silently routing — closes both halves of L-04 ("any accidental ERC20 sent to the router can be routed by anyone" and the rescue gap).
Brouie commented 2026-05-04 06:35:33 +00:00 (Migrated from gitlab.com)

mentioned in issue #123

mentioned in issue #123
Brouie commented 2026-05-04 06:48:04 +00:00 (Migrated from gitlab.com)

indexer surface verified for the new FeeRouter governance events.

migration apply on fresh DB:

  • 20260504130000_fee_router_governance_events.up.sql applies clean, both tables created (idx_fee_router_distributable_token_updated, idx_fee_router_erc20_rescued)
  • existing FeeRouter tables intact (idx_fee_router_fees_distributed, idx_fee_router_sinks_updated)

full plumbing wired end-to-end:

  • decoder.rs:250 event DistributableTokenUpdated ABI definition + dispatch at line 1087
  • DecodedEvent variant FeeRouterDistributableTokenUpdated at line 520
  • persist.rs:879/885/909 INSERT statements for both DistributableTokenUpdated and Erc20Rescued
  • reorg.rs:112-113 both new tables added to the rollback list

indexer test suite: 19 unit + 1 integration PASS, no failures, 0.73s integration test runtime against fresh DB.

indexer surface verified for the new FeeRouter governance events. **migration apply on fresh DB:** - `20260504130000_fee_router_governance_events.up.sql` applies clean, both tables created (`idx_fee_router_distributable_token_updated`, `idx_fee_router_erc20_rescued`) - existing FeeRouter tables intact (`idx_fee_router_fees_distributed`, `idx_fee_router_sinks_updated`) **full plumbing wired end-to-end:** - decoder.rs:250 `event DistributableTokenUpdated` ABI definition + dispatch at line 1087 - DecodedEvent variant `FeeRouterDistributableTokenUpdated` at line 520 - persist.rs:879/885/909 INSERT statements for both DistributableTokenUpdated and Erc20Rescued - reorg.rs:112-113 both new tables added to the rollback list **indexer test suite:** 19 unit + 1 integration PASS, no failures, 0.73s integration test runtime against fresh DB.
PlasticDigits (Migrated from gitlab.com) closed this issue 2026-05-04 08:19:27 +00:00
Brouie commented 2026-05-14 02:53:39 +00:00 (Migrated from gitlab.com)

mentioned in issue #207

mentioned in issue #207
PlasticDigits commented 2026-05-19 07:53:22 +00:00 (Migrated from gitlab.com)

mentioned in issue #217

mentioned in issue #217
Sign in to join this conversation.
No milestone
No project
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
code/yieldomega#122
No description provided.