Audit M-03: RabbitTreasury Burrow param bounds, finalizeEpoch liveness, and single-epochId doc clarity #119
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#119
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
This issue tracks audit finding M-03 from
audits/audit_smartcontract_1777813071.md:RabbitTreasuryBurrow parameter setters lack on-chain bounds for several math-critical WAD values.PARAMS_ROLEcan setcStarWad,alphaWad,betaWad,lamWad,deltaMaxFracWadto arbitraryuint256values. Some other setters already validate (e.g.setMBoundsWadenforcesmMin < mMax, fee / min-efficiency setters enforce< WADor(0, WAD]), but the unbounded group can still causefinalizeEpochto revert, produce extreme repricing, or yield unintuitive redemption economics.Privileged impact:
finalizeEpochis permissionless. After a params update, keepers or any caller rely on stable math; griefing or misconfiguration risks liveness breaks (finalize reverts) or sharp e/dynamics even without attacker control of roles.Product / docs correction (maintainer input): Yieldomega v1 is not a multi-epoch product in the sense of distinct “Epoch 2 governance phases” or “defer changes until the next era.” The audit’s suggestion to use “delayed activation by next epoch” is not the right framing here. On-chain,
finalizeEpochstill incrementsepochIdfor rolling snapshot / cooldown accounting only—that is not “a second product epoch.” Documentation and runbooks must state this clearly so readers do not assume a multi-epoch roadmap. Preferred mitigations are hard bounds on setters, regression tests, and, if governance wants delay, timelock / two-step admin—not “wait for the nextepochId” as if that were a separate product stage.Affected on-chain surface
setCStarWadcMaxWad; interacts withmultiplierWadsetAlphaWadalphaWad >= WAD,BurrowMath.multiplierWadcan hitinner<=0revertsetBetaWadsetLamWadnextEWadstepsetDeltaMaxFracWademove;nextEWade<=0if step drives non-positiveeReference:
contracts/src/RabbitTreasury.sol(parameter setters ~L441–472),contracts/src/libraries/BurrowMath.sol(multiplierWadrequire(i > 0, "BurrowMath: inner<=0"),nextEWadrequire(en > 0, "BurrowMath: e<=0")).Deployed defaults and intended envelopes are already listed in
contracts/PARAMETERS.md(§ Rabbit Treasury (Burrow)), e.g.c_star1.05,alpha0.02,beta2.0,lambda0.5,delta_max_frac0.02,m_min/m_max0.98–1.02,c_max2.0—setters should enforce a documented safe band consistent with simulations (andsimulations/bounded_formulas/model.py/ BurrowMath NatSpec cross-link).Recommended fix (implementation)
Add
requireguards onsetCStarWad,setAlphaWad,setBetaWad,setLamWad,setDeltaMaxFracWadmatching the simulation envelope inPARAMETERS.md(tight min/max WAD constants or shared internal library). Examples of constraints to encode in design review:alphaWad < WAD(strict) so1 + α·tanh(·)stays positive inmultiplierWad.0 < lamWad ≤ WAD(or tighter band per sims).0 < deltaMaxFracWad ≤ WAD(or tighter, e.g. cap at a few percent per epoch step per product).cStarWadin(0, cMaxWad]or consistent band vs initializercMaxWad.betaWadupper bound to limit exp/tanh path stress (coordinate withBurrowMath.t.solfuzz envelope).Single source of truth: Either duplicate documented constants in Solidity with a comment “must match PARAMETERS.md table” or extract named constants in
RabbitTreasury/BurrowMathand syncPARAMETERS.md+ product docs to those names.Deferred activation: Do not implement “apply on next epoch” as the primary fix for v1 framing. If delayed effect is still desired operationally, use OpenZeppelin Timelock, two-step governance, or explicit “pending params + admin apply” pattern—documented as governance, not as “next epoch” product language.
Recommended regression tests (Foundry)
Add to
contracts/test/RabbitTreasury.t.sol/contracts/test/BurrowMath.t.solas appropriate:vm.expectRevertwhenvalis just below min or just above max (per chosen constants).epochEnd, assertfinalizeEpochsucceeds after each boundary-legal update (can be a parametrized loop or small Cartesian sample).alphaWad = WADwould makemultiplierWadrevert—after fix,setAlphaWad(WAD)must revert at the treasury setter, not at finalize.finalizeEpoch+ invariants still hold (extendcontracts/test/RabbitTreasuryInvariant.t.solonly if it adds clear value).Map new tests in
docs/testing/invariants-and-business-logic.md§ RabbitTreasury and BurrowMath if they introduce new named scenarios.Files likely to change
Solidity / tests
contracts/src/RabbitTreasury.sol— bounds onsetCStarWad,setAlphaWad,setBetaWad,setLamWad,setDeltaMaxFracWad; NatSpec referencing envelope.contracts/src/libraries/BurrowMath.sol— optional shared constants or comments if bounds are centralized here.contracts/test/RabbitTreasury.t.sol— bounds + finalize liveness tests.contracts/test/BurrowMath.t.sol— numerical edge cases if needed.contracts/test/RabbitTreasuryInvariant.t.sol— only if extending invariants.Documentation
contracts/PARAMETERS.md— replace vague “Per simulations” with explicit min/max bounds columns aligned with on-chainrequires.docs/product/rabbit-treasury.md— clarify thatepochId/finalizeEpochare rolling accounting windows (snapshots, cooldown indexing), not a multi-phase “Epoch 2” product; avoid reader confusion with governance delay language.docs/testing/invariants-and-business-logic.md— new/updated test map rows for param bounds + finalize liveness.docs/onchain/fee-routing-and-governance.md(and any operator / Stage 3 runbook referencing Burrow params) — governance: bounds + optional timelock; not “defer to next epoch” as product semantics.audits/audit_smartcontract_1777813071.md— optional maintainer note linking this issue and correcting the “next epoch” mitigation for v1 alignment.Other
simulations/bounded_formulas/model.py(or notebooks) — verify stated parameter ranges match enforced bounds; update comments if needed.No indexer / frontend change expected unless copy surfaces raw param names and needs a one-line clarification (unlikely).
Acceptance criteria
setCStarWad,setAlphaWad,setBetaWad,setLamWad,setDeltaMaxFracWadrejects out-of-envelope values with a clear revert (prefer stable short reason strings consistent with existing"RT: …"style).contracts/PARAMETERS.mdRabbit Treasury table (single documented min/max or fixed allowed set).alphaWad < WAD(or tighter) soBurrowMath: inner<=0cannot be reached via governance alone from a previously valid state without violating a setter (document in NatSpec).finalizeEpochsuccess after legal updates under representative reserve/supply conditions.forge testpasses for the touched packages.epochIdrolling forward is accounting/snapshot/cooldown indexing only; governance delay should be described as timelock / process, not “next epoch” ambiguity.Verification checklist (implementer)
forge test --match-contract RabbitTreasury(or fullforge testperdocs/testing/strategy.md).PARAMETERS.mdtable.Verification checklist (reviewer / operator)
cast send/ script with illegal value on testnet—revert at setter, not atfinalizeEpoch.finalizeEpochstill succeeds on a healthy deployment after max-legal params (optional Anvil script).References
audits/audit_smartcontract_1777813071.mdmentioned in commit
547e9e1ae9/cc @brouie
Merged to
main(includes RabbitTreasury Burrow param envelopes oninitialize+setCStarWad/setAlphaWad/setBetaWad/setLamWad/setDeltaMaxFracWad, Foundry coverage, docs ↔ invariants, PARAMETERS / simulation docstring, skills cross-links).Verification checklist
cd contracts && forge test --match-contract 'RabbitTreasury|BurrowMath'— expect 42 tests green (includes invariant fuzz).contracts/PARAMETERS.mdRabbit Treasury (Burrow) row vs on-chainMAX_*/ revert strings.docs/testing/invariants-and-business-logic.md#rabbit-treasury-burrow-param-bounds-gitlab-119matches implementation.Leaving the issue open for your sign-off on audit M-03.
mentioned in commit
9905b25c9cmentioned in commit
717c7fd6a1verified on commit
547e9e1(merged viad12bb99). all bound checks land + finalizeEpoch liveness covered.setter bound rejections — your AC #1:
test_setAlphaWad_reverts_when_not_strictly_below_wad— alphaWad < WAD enforced (closes the BurrowMathinner<=0revert path you flagged)test_setBetaWad_reverts_out_of_envelope— beta upper bound enforcedtest_setCStarWad_reverts_out_of_envelope— cStar in (0, cMaxWad] bandtest_setDeltaMaxFracWad_reverts_out_of_envelope— deltaMax boundedtest_setLamWad_reverts_out_of_envelope— lambda boundedfinalizeEpoch liveness — your AC #2:
test_finalizeEpoch_succeeds_after_extreme_legal_params— finalize succeeds across boundary-legal updatesexisting invariants intact:
invariant_rabbitTreasury_doubSupplyMatchesGhostMintBurn(256 runs, 6400 calls, 57 reverts) — supply ledger preservedinvariant_rabbitTreasury_reservesMatchTokenBalance(256 runs, 6400 calls, 43 reverts) — backing ledger preservedtest_protocolOwned_notExtracted_viaOrdinaryWithdraw— protocol-owned bucket isolation preservedtest_receiveFee_doesNotMintDoub_andDoesNotIncreaseRedeemable— fee accounting invariant preservedsetters now reject pre-fix BurrowMath revert modes at the treasury level, not at finalize. matches your spec — single source of truth in
PARAMETERS.mddocumented bounds.mentioned in commit
166fd6fa3a