Audit L-04: FeeRouter permissionless distributeFees + governed ERC-20 rescue / docs #122
Labels
No labels
agent:fix_bugfix
agent:fix_conflicts
agent:fix_security
agent:gap_analysis
agent:implement
agent:open_issues
agents
agent:security_audit
agent:verify
area
timecurve
arena
arena-v2
audit
block:feature_review
block:security
bug
contracts
design
devops
docs
documentation
enhancement
epic
feature
frontend
indexer
low priority
megaeth
operations
ops
performance
QA
qa checklist
referrals
removal
security
smart-contracts
tech-debt
testing
time-arena
timecurve
visual-qa
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
code/yieldomega#122
Loading…
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
Track remediation for L-04 in
audits/audit_smartcontract_1777813071.md: publicFeeRouter.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
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 andsafeTransfers to the five sink destinations — includingPodiumPool, burn address,DoubLPIncentives,RabbitTreasury, etc., depending on deployment.FeeRouteris 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 runsdistributeFees, with no governance step.Sink asymmetry: Concrete sinks that
is FeeSinkexposewithdraw(token, to, amount)gated byWITHDRAWER_ROLE(governed recovery of balances at the sink).PodiumPoolis not aFeeSink: it only exposespayPodiumPayout(wired toTimeCurveasprizePusher). So stray ERC-20 that lands inPodiumPoolvia wrong-token distribution or direct transfer has no symmetricFeeSink.withdrawpath today.Goals (both audit recommendations are in scope)
Per L-04, both directions apply:
Documentation: Make it unambiguous that
FeeRouteris not a custody contract and that permissionlessdistributeFeescan move any token balance held by the router.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.withdrawhandles ERC-20 at sinks (WITHDRAWER_ROLE/ admin hierarchy).Recommended fixes (design options — implementer picks per product tolerance)
A. Documentation hardening (required baseline)
FeeRouter; onlyTimeCurve(and documented paths) should fund fee routing for the accepted asset.FeeRouter/ sinks; who holdsGOVERNOR_ROLE,WITHDRAWER_ROLE,DEFAULT_ADMIN_ROLEon proxies (GitLab #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):
FeeRouter— token allowlist fordistributeFees:GOVERNOR_ROLE-managed allowlist (or the canonical sale asset address registered fromTimeCurve) may be split viadistributeFees.rescueERC20(IERC20 token, address to, uint256 amount)(or batch variant)onlyRole(GOVERNOR_ROLE)(orDEFAULT_ADMIN_ROLEif that matches other modules — must match repo governance doc).FeeRouter— rescue-only, keep permissionless distribute for allowlisted token:distributeFeesrestricted to one canonical token;rescueERC20for everything else underGOVERNOR_ROLE.PodiumPool— parity withFeeSinkfor non-prize stray assets:withdraw(IERC20 token, address to, uint256 amount)withWITHDRAWER_ROLE(or reuseDEFAULT_ADMIN_ROLEpattern fromFeeSink) only for tokens that are not the intended prize asset / only when not conflicting withdistributePrizesaccounting — 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)
distributeFeesstill splits correctly; sinks receive expected sharescontracts/test/FeeRouter.t.sol,contracts/test/FeeRouterInvariant.t.soldistributeFeesreverts; governed rescue succeeds; no fee-split side effectsFeeRouter.t.solPodiumPoolstray-token withdraw (if implemented): cannot drain legitimate CL8Y prize balance or tests lock only wrong tokencontracts/test/FeeSinks.t.sol/ newPodiumPoolRescue.t.solforge testFiles likely to need updates
Solidity / tests
contracts/src/FeeRouter.sol— allowlist and/orrescueERC20; events; NatSpec.contracts/src/sinks/PodiumPool.sol— optional governedwithdraw(if spec’d).contracts/test/FeeRouter.t.sol,contracts/test/FeeRouterInvariant.t.solcontracts/test/FeeSinks.t.sol— PodiumPool changes.contracts/script/UUPSDeployLib.sol,contracts/script/DeployDev.s.sol— initializer args / wiring if new storage.contracts/test/TimeCurve.t.sol,contracts/test/DevStackIntegration.t.sol) ifdistributeFeessignature or behavior changes.Documentation
docs/onchain/fee-routing-and-governance.md— anti-pattern: “do not send tokens toFeeRouter”; governance for allowlist/rescue; post-update invariants.docs/product/primitives.md— short integrator warning if needed.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— update “No hidden rescue” bullet if governed rescue is added (must remain accurate).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/— decodeSinksUpdated–class / newTokenAllowed/Rescuedevents if operators need receipts (docs/indexer/design.md).frontend/src/components/FeeTransparency.tsx— only if UX should surface allowlist state (optional).Acceptance criteria
FeeRouteris explicitly non-custodial; permissionlessdistributeFeesbehavior and risks are ** operator- and integrator-visible** in canonical docs.FeeRouter(andPodiumPoolif in scope) can be recovered only via roles consistent withFeeSink.withdrawgovernance expectations (no new permissionless value extraction beyond today’sdistributeFeessemantics for intended assets).TimeCurve→FeeRouter→ sinks for the accepted asset remains correct, test-covered, and not materially more gas-fragile for happy path without governance justification.docs/onchain/cl8y-flow-audit.mdand related security statements remain factually true after the change set.Verification checklists
Pre-merge (engineering)
cd contracts && forge testpasses (or CI green) including newFeeRouter/PodiumPooltests.FeeRouterstorage changes: UUPS layout check / OpenZeppelin upgrades workflow completed;__gappreserved.initialize/DeployDev: noaddress(0)admin; regression for #54 proxy canonical addressing.eventemissions for any rescue or allowlist mutation (indexer/ops observability).Post-deploy / operations (staging or checklist for mainnet)
GOVERNOR_ROLE,WITHDRAWER_ROLE,DEFAULT_ADMIN_ROLEholders documented forFeeRouterand each sink proxy.FeeRouter→ rescue path (or documented manual sink withdrawal if onlyFeeSinksinks receive dust).distributeFeescannot misroute past allowlist rules (negative test evidence captured in CI).Cross-functional
FeeRouter— escalation and irreversibility messaging.References
audits/audit_smartcontract_1777813071.md— § L-04: Public fee distribution and sink withdrawals depend on correct token/sink choicescontracts/src/sinks/FeeSink.sol—withdraw+WITHDRAWER_ROLEcontracts/src/FeeRouter.sol—distributeFees,GOVERNOR_ROLE,updateSinksmentioned in commit
8255f4344e@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
Leaving #122 open per instructions.
verified on commit
8255f43(merged viaebbf58e). 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 routetest_setDistributableToken_emits_and_gate— governance configures + emits eventtest_setDistributableToken_unauthorized_reverts— non-governance can't add tokenstest_setDistributableToken_zero_token_reverts— zero-token guardgoverned rescue:
test_rescueERC20_moves_balance— moves stranded balance to ops wallettest_rescueERC20_unauthorized_reverts— only governancetest_rescueERC20_zero_to_reverts— zero-recipient guardinvariants intact (256 runs, 6400 calls, 0 reverts each):
invariant_feeRouter_routerBalanceMatchesGhostinvariant_feeRouter_sinksSumEqualsDistributedindexer visibility: new migration
20260504130000_fee_router_governance_events.up.sqlships 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).
mentioned in issue #123
indexer surface verified for the new FeeRouter governance events.
migration apply on fresh DB:
20260504130000_fee_router_governance_events.up.sqlapplies clean, both tables created (idx_fee_router_distributable_token_updated,idx_fee_router_erc20_rescued)idx_fee_router_fees_distributed,idx_fee_router_sinks_updated)full plumbing wired end-to-end:
event DistributableTokenUpdatedABI definition + dispatch at line 1087FeeRouterDistributableTokenUpdatedat line 520indexer test suite: 19 unit + 1 integration PASS, no failures, 0.73s integration test runtime against fresh DB.
mentioned in issue #207
mentioned in issue #217