Introduce SafeYieldManager (handler pattern) with Uniswap V3 / Aerodrome / Uniswap V4 support, staking + reward harvest, and heavy clean up - #16
Open
junta wants to merge 55 commits into
Open
Conversation
…script - Simplified the contract's notice and developer documentation for clarity. - Updated constructor validation to reject maxFeeBps above 100%. - Enhanced test coverage with new cases for constructor and slippage checks. - Adjusted deployment script to use the correct environment variable for the registry address.
…onstructor args - Implemented a new function to recursively convert Ignition serialized bigint constructor arguments into native bigints. - Updated the getConstructorArgsFromJournal function to utilize reviveIgnitionValues for correct encoding of constructor arguments. - This change addresses issues with ABI encoding of bigint values, ensuring compatibility with hardhat-verify.
Rename Protocol -> DebtProtocol (Types.sol, contractAddresses.ts, ABIs)
so debt and yield protocol enums can coexist unambiguously.
Review fixes:
- closeLp performance-fee check reads the transfer return word in
memory-safe assembly instead of abi.decode, so a malformed return
value from a non-compliant token can never block an exit
- 3_DeployAerodromeHelper read RHP_TREASURY for the registry address;
now reads RHA_REGISTRY as documented
- restore revert strings mangled by the rename ("DebtProtocol handler")
Simplify pass:
- yield handlers build swap calldata from IV3SwapRouter /
ISlipstreamSwapRouter via abi.encodeCall instead of hand-pinned
selectors and mirrored structs
- dedupe _safeApprove/_safeMintLp into _safeExec (mint now bubbles
inner revert reasons) and extract _swapWethDeltaToUsdc shared by
closeLp/collectLp
- SafeYieldManager: shared _pinnedPosition guard for closeLp/collectLp,
cheap owner check before the external safeOperator() call
- share requireAddress across ignition modules; export YieldProtocol
enum from contractAddresses.ts instead of per-file constants
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MTbcUbFL6Q65YJmFSBPC2c
junta
changed the title
…olders SafeYieldManager's AerodromeYieldHandler supersedes the standalone Aerodrome module, which was only ever a test deployment with no live positions - delete the contract, its test suite, deploy module, ABI, and env/doc surface. Merge contracts/mock/ into contracts/mocks/ and contracts/protocolsSafeDebt/ into contracts/protocolsDebt/ so each concern lives in one folder.
Delete unreferenced files (TransferHelper, ISwapRouter02 family, Aerodrome IRouter/IWETH, IDebtToken, MaliciousContract, the broken deployRolesProxy script), unused modifiers/imports/struct fields, and unused mock knobs surfaced by a dead-code sweep. Hoist the five V3-shaped hooks into an abstract V3StyleYieldHandler so UniV3YieldHandler and the future-protocol test mock share one implementation, extract _trySafeTransfer in SafeYieldManager to match the legacy helper's shape, and collapse the derivable exitBps branch. Document why _validateHandler's code-length pre-check is load-bearing: return-data decoding errors escape try/catch, so it cannot be replaced by the catch clause.
junta
changed the title
safeYieldManagerUniV3Fork.ts mirrors the Aerodrome fork suite so the new yield stack is exercised against the real Uniswap V3 contracts too, closing the asymmetry where only the legacy module had live-V3 coverage. Group tests by area — debt/, registry/, yield/, legacy/ (deployed standalone modules) — with fixtures and utils under helpers/, and update import paths, the coverage:rhp glob, and CLAUDE.md conventions accordingly.
Mirror the test/ layout: common/ (Types, ProtocolRegistry, Imports), debt/ + debt/handlers/, yield/ + yield/handlers/, and legacy/ (RatehopperUniV3Positions and the excluded upgradeable draft), keeping interfaces/, dependencies/ and mocks/ where they were. Update import paths, the ABI exporter's artifact paths, the coverage gate path, and the architecture docs.
The deployed RatehopperUniV3Positions keeps serving legacy positions but will never be deployed again, so drop deploy:2_univ3_helper, its ignition module, and the wipe shortcut. Rename 4_DeployYieldManager to 2_DeployYieldManager (deploy:2_yield_manager) to close the numbering gap, and update env/docs references accordingly.
…n tests SafeDebtManager already returns leftover dust to the user at the end of the flash callback, so the extra refund inside the repay branch (and its LeveragedPosition copy) was redundant. On the test side, getParaswapData accepts a preferredDEX hint so the EURC route can pin Uniswap V3, and the Fluid assertion tolerates the sub-10,000-unit dust its vault refuses to accept on repay.
Collaborator
|
A few things so far:
|
Collaborator
|
also current branch coverage is 68.4% (156/228) can we check that and bring it up closer to 100%? |
Collaborator
Author
timelock duration was changed to 8 hours by this commit. (but live timelock is 2 days) I have changed the Ignition module default to 2 days |
Review response for PR #16. The Ignition timelock default had been silently weakened to 8 hours in an unrelated deploy-script commit; a future deploy without RHP_TIMELOCK would have shipped it. Restore the original 2-day default and align docs. Bring the yield stack from 156/228 to 228/228 branch coverage: assert the previously untested defensive branches (no-fee-on-loss, partial- close rounding guard, LpNotOnSafe, ModuleCallFailed, pool validation, reentrancy paths, fee-transfer returndata shapes) directly against SafeYieldManager, and add the yield contracts to the CI coverage gate via coverage:gated. Replace the degenerate swapAmountOutMin=1 params in the fork tests with spot-price-derived values, plus a negative case proving an over-tight min-out reverts inside the real router.
Collaborator
Author
I've increased branch coverage --------------------------------------------|----------|----------|----------|----------|----------------|
|
…ypes Cover the last two gaps in the yield stack: the minPositionLiquidity getter and the _validateHandler catch path (a contract without PROTOCOL()), bringing statements/functions/lines to 100% alongside branches. Cast slot0 sqrtPriceX96 to bigint in the fork tests so the spot-price helpers typecheck, and default openLpBySafe to DRY_RUN.
Collaborator
Author
Addressed in be874f5 (with a follow-up in 95b8ef8). Point by point: Defensive branches now asserted directly against
No-fee-on-loss is asserted: a close realizing less than basis emits Partial-close rounding guard is asserted: a 50% close of a liquidity-1 position reverts with Fork tests no longer use degenerate slippage params: |
Pool params now carry the pair (abi.encode(token0, token1, feeTier|tickSpacing))
so SafeYieldManager can manage LPs beyond WETH/USDC (e.g. WBTC/USDT) without new
handlers. USDC stays the sole funding/accounting currency: open/close route each
non-USDC side through its own SwapLeg (USDC sides need no swap), keeping
basisUsd6, the performance fee and minUsdcOut semantics unchanged. Swap legs are
pinned on-chain to a {token, USDC} pool so routes cannot be diverted.
Breaking ABI change (param structs, PositionOpened field names, handler
constructors) — acceptable while the only deployment is the test stack; scripts
and ignition target the NEXT deployment. Also adds a fee-tier switch fork test
(0.3% -> 0.05%) and fixes the carried-basis assertions to account for
undeployed mint leftovers.
_safeApprove now decodes a bool return when present and reverts TokenApprovalFailed on false, so a token that rejects the zero-reset cannot leave a live router allowance behind an otherwise-successful operation (no-return tokens keep working). collectLp on a gauge-staked position previously stopped after claiming AERO, silently deferring all LP trading fees to the final close. It now claims the gauge reward, temporarily unstakes the NFT to the Safe, runs the normal fee collect (treasury skim included), and restakes.
Quality pass over the arbitrary-pair work (no behavior change; 95 yield unit+fork tests green): - The USDC-side skip now lives inside _validateSwapLeg/_swapDeltaToUsdc, so closeLp/collectLp lose their eight per-call-site token guards; a leg whose token IS USDC is ignored in exactly one layer. - closeLp reuses the entry _position read for liquidity; balance snapshots are taken only for sides that will actually swap; _acquireSide drops its information-free spentUsdc return; the LpNotOnSafe check and the Aerodrome staked-gauge predicate each get a single definition. - Pool-param encoding is centralized in contractAddresses (encodeUniV3PoolParam/encodeAerodromePoolParam) and SwapLeg builders in test/helpers, replacing ~10 hand-rolled abi.encode sites and 4 ZERO_LEG copies; the Aerodrome fork gauge test reuses extracted stack/pool/funding helpers instead of ~60 duplicated lines. - New scripts/lpSafeShared.ts holds the ABI fragments, deployment lookup, tick/liquidity math, owner-key resolution, and receipt polling that the three ops scripts had each copied (already drifting). - MockCLGauge.withdraw now exercises the Safe harness's onERC721Received; the redundant local ERC721 interface is replaced by OZ's IERC721.
Staked Slipstream liquidity earns AERO emissions instead of trading fees, so collectLp on a staked position now claims via the stakePool only and leaves the NFT staked - the temporary unstake/collect/restake round-trip is gone. New opt-in swapRewardToUsdc + rewardSwap leg swaps exactly the claimed reward delta to USDC through an allowed pool. Also carried in this working set: gauge -> stakePool / stakeInGauge -> stake renames, switchLp no longer takes a performance fee (deferred to the real exit via closeLp), Uniswap V4 address/enum scaffolding, and regenerated exported ABIs. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Fejj9oBtCgvbri98eCtmbL
The returndata-bool interpretation (empty = success, <32 bytes = malformed, word must be canonical true) was hand-rolled in four places across the manager and handlers; it is the one piece of Safe-module token-call handling that must never diverge between them. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VUYKcbbReA6FpHtkiXWu6m
The separate module existed only to avoid invalidating the recorded deployment of a live SafeYieldManager; with a fresh redeploy planned, constructor registration replaces the multi-step timelock runbook and brings V4 up enabled with the canonical native ETH/USDC 0.05% pool allow-listed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VUYKcbbReA6FpHtkiXWu6m
SafeYieldManager accepted any pool pair the admin allow-listed, with no tie to the ProtocolRegistry token whitelist the debt side enforces. Ask each handler to decode its pool param (new IYieldHandler.poolTokens, via staticcall with the same revert-wrapping as delegatecalls) and require both tokens whitelisted on openLp and the switchLp open leg. Exits stay ungated so a later de-listing can never trap a position, and V4's address(0) native-ETH sentinel is skipped. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H2JcTqWxLq45x4vJc1ZaHF
collectLpBySafe.ts completes the ops-script set (open/close/switch already existed). All four scripts now resolve the Safe from TESTING_SAFE_WALLET_ADDRESS and the ignition deployment from IGNITION_DEPLOYMENT_ID, so runs against a secondary deployment (yield-v2, now gitignored) need no source edits. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H2JcTqWxLq45x4vJc1ZaHF
README/.env.sample still described Uniswap V4 as a future protocol, a two-handler deploy, an 8-hour timelock default (actual: 2 days), and an RHP_TIMELOCK shared by all modules (actual: yield module only — the registry deploy reads REGISTRY_TIMELOCK). Also documents the LP ops scripts and V3StyleYieldHandler layer, fixes the abis count, and repairs the folder-reorg import paths the excluded legacy upgradeable file missed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VUYKcbbReA6FpHtkiXWu6m
Collaborator
Author
junta
changed the title
The V4 fork suite only exercised the native ETH/USDC pool, whose currency0 rides as call value and is swept back — the Permit2 two-step approval and reset for an ERC20 currency0 (steps 51/52/56/57) was validated only against mocks. Add a WETH/USDC (currency0 == WETH, fee 0.3%/ts 60, the deeper real pool) lifecycle test that asserts both the WETH and USDC Permit2 hops reset to zero after the mint, and that closes swap only the delta they withdraw so the open-mint WETH dust is left untouched. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H2JcTqWxLq45x4vJc1ZaHF
…sting Add setPoolParamAllowed.ts (admin-signed, role-checked, DRY_RUN-guarded) to toggle manager pool-param allow-list entries across the three protocols. openLpBySafe gains a STAKE flag; collectLpBySafe gains a swapRewardToUsdc path that prices the gauge's earned AERO through an allow-listed AERO/USDC pool; switchLpBySafe accepts a source position owned by its Aerodrome gauge (the close leg unstakes it on-chain) instead of requiring Safe ownership. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H2JcTqWxLq45x4vJc1ZaHF
Every Aerodrome fork test used the WETH/USDC pool, where USDC is token1 and swap0 does the work — the token1-side acquisition (swap1 leg) and delta-swap close were only exercised against mocks. Add a USDC/AERO lifecycle test (USDC < AERO, so the funding token is token0) through the deep tickSpacing-200 pool; the ts-50 pool the reward swap uses is too shallow for LP-sized swaps.
The treasury-facing money flows were only ever exercised where they round to zero: every fork collect ran right after open (nothing collected, no skim) and every close realized at a loss (no performance fee), so feeCollectBps, the swapFeesToUsdc leg, and performanceFeeBps never actually moved tokens on a fork. Wash-trade real volume through the live pools to accrue genuine fees, then pin the in-kind feeCollectBps skim, the fee-to-USDC swap, and the profit-only performance fee to exact event amounts. The performance-fee case runs on the USDC/AERO ts-200 pool - the WETH/USDC pool is deep enough that a small position's fee share can never outrun its own swap costs.
Collaborator
Author
|
test coverage ┌───────────────────────────────────────────────────────┬───────────┬─────────┬───────┬────────┐ |
The old switchLp realized the position to USDC on the close leg and swapped half back on the open leg, paying two swap legs (pool fee + up to the slippage tolerance) on ~half the notional per switch. For a same-pair move the required destination token ratio is what the withdrawal just delivered, so the swaps bought nothing. switchLp (same name, new SwitchLpParams without swap legs) now composes two new handler primitives: - withdrawLp: full decrease + collect + burn, tokens land on the Safe as-is; fees harvested first so feeCollectBps still applies to fees only. Native ETH is wrapped to WETH so callers see ERC20s. - openLpInKind: mint straight from the withdrawn amounts; the handler rejects a pair mismatch (WrongTokenPair) so a switch can never deploy unrelated Safe funds. The V4 handler unwraps WETH for native pools (new WETH constructor arg, wired in ignition). Basis accounting: an in-kind switch realizes nothing, so the basis is carried onto the replacement UNCHANGED - no oracle needed and still no performance fee. Mint residue stays in the Safe and only under-states later realized profit. PositionSwitched now reports carriedBasisUsd6 + withdrawn/used amounts. Price protection reduces to decrease minimums (withdraw leg) and mint minimums (open leg); the decrease mocks now enforce amount0Min/1Min like the real position managers. Unit suites rewritten for the in-kind flow (incl. V3->native-V4 unwrap and reentrancy via an NPM callback); the Base-fork suite verifies all four protocol pairings carry the basis 1:1. switchLpBySafe.ts follows the new param shape. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QKobiCuUa3k6sHvyy7sS5q
Two gates were failing on the switchLp rework: - format:check — prettier reflowed the long IYieldHandler import lists and a few wrapped expressions in the manager and both handlers. - coverage:check — the new withdrawLp / openLpInKind branches dragged UniV4YieldHandler to 92.47% and BaseYieldHandler to 93.75%, under the 95% gate. Added the missing cases: delegatecall-only entry for both new functions, the WrongTokenPair rejection on each side of the pair, a zero-liquidity withdrawal (no decrease to make), an ERC20-currency0 V4 withdrawal (the no-wrap path), the destination position-liquidity floor, and the per-side mint minimums. Gated branch coverage is back over the bar: UniV4 98.63%, Base 99.11%, 624 branches at 98.56% overall. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QKobiCuUa3k6sHvyy7sS5q
The in-kind switch left each handler with two copies of the same mint half. In UniV4YieldHandler that was ~28 lines repeated verbatim, including the two-step Permit2 grant and its matching reset — the pairing you must never get wrong — plus the "measure what the mint consumed as a Safe balance delta" trick and the liquidity floor. BaseYieldHandler repeated ~20 lines the same way. Both now call a single `_mintFromAmounts`: derive/spend the amounts, enforce the floor, mint, reset every allowance hop, report used0/used1, confirm the Safe owns the NFT. `openLp` keeps what is genuinely its own (acquire the sides by swapping USDC, then value the basis at the executed rate); `openLpInKind` keeps the destination-pair check and, on V4, the WETH unwrap for a native pool. Base's `_safeMintLp` is folded in — it had no other callers. Arguments travel in a `MintArgs` struct rather than flat: the flat form is 10-11 parameters and pushes both callers past the stack limit once solidity-coverage instruments the branches (viaIR, see .solcover.js). Behaviour, step codes and revert reasons are unchanged. Gated branch coverage improves because the duplicated branches collapse: 624 -> 610 branches, 98.56% -> 98.69%; UniV4 98.63% -> 99.25%. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QKobiCuUa3k6sHvyy7sS5q
_unstakeIfStaked deleted the pin on every unstake, so a partial close left the surviving NFT sitting on the Safe, earning nothing, with the manager believing it was never staked. Restaking it outside the manager then made later operations fail: the gauge owns the NFT but stakePoolOf reads zero. The pin now survives the temporary unstake. _unstakeIfStaked returns the pool it took the NFT out of; a close that leaves liquidity behind puts it straight back into that same pool, and only a full close (NFT burned) clears the pin. The restake deliberately does not re-read Voter.gauges — governance may have rotated the mapping since the position was staked, and a rotation must not silently move a user's position. A failed restake reverts the whole partial close rather than leaving custody and state disagreeing. Adds stakePoolOf(protocol, tokenId) for tests and operations, and folds the deposit half of _stake into the shared _restakeInto. Tests: partial close restakes and preserves the pin with correct residual liquidity; a later collect still claims; the final close burns and clears the pin; a never-staked position is not staked by a partial close; gauge rotation does not change the restake destination; a failing deposit reverts the whole close with pre-call state intact. The Aerodrome fork suite proves the real gauge takes the survivor back and keeps paying emissions across a five-day warp. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QKobiCuUa3k6sHvyy7sS5q
Staked Slipstream positions earn AERO instead of trading fees, and none of it was taxed. A staked collect claimed rewards straight to the Safe with no fee, and the gauge withdrawal inside a close or a switch paid out accrued emissions outside every existing fee and valuation snapshot — so "close instead of collect" was a way to take emissions for free. Every route that can claim now funnels through _settleStakedReward: it measures the delta credited to the Safe, sends feeCollectBps of it to the treasury, and emits StakedRewardCollected with the gross claimed and the fee actually paid. Measuring a delta rather than a balance leaves any reward the Safe already held untouched. The reward snapshot in _collectStakedRewardIfStaked is now unconditional — the claim is fee-bearing whether or not the caller wants it swapped — and the swap that follows moves only the net, because it reads the delta from the same pre-claim snapshot the fee already reduced. _unstakeIfStaked settles the withdrawal reward itself, which covers close and switch in one place. Both callers snapshot their pool-token and USDC balances after it returns, so a reward can never leak into the close swap delta or the performance-fee valuation — proven by making the reward token USDC, where a mis-ordered settle would inflate currentValueUsd6. A failed treasury transfer waives the fee, emits CollectFeeTransferFailed and reports feePaid = 0, and never blocks the collect or the exit. A gauge that reports no reward token is skipped rather than reverting: an exit must not brick on a misbehaving gauge. Tests: fee charged on an unswapped claim; only the newly claimed delta is taxed; full close, partial close (once, then restake) and switch each charge it; a USDC reward stays out of both close value and switch amounts; zero rate, rounds-to-zero and zero reward; revert and false-return treasury failures waive without blocking. The Aerodrome fork suite pins the real treasury AERO delta across collect, partial close and full close, sharing the gauge's remaining epoch so each step really accrues. The mock gauge now pays on withdraw like the real one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QKobiCuUa3k6sHvyy7sS5q
Every ops script shipped mintAmount0Min/1Min and decreaseAmount0Min/1Min as literal 0n, so any execution-time ratio was acceptable, and the one piece of tick math they did have went through `Math.sqrt(1.0001 ** tick) * 2 ** 96` — a double, for a 160-bit quantity. scripts/lpMath.ts is now the single implementation: bigint ports of TickMath, LiquidityAmounts and the amounts-from-liquidity direction, with nothing routed through a JS number, plus the three floor builders the scripts need. lpSafeShared re-exports them so open, close and switch provably share one copy. The floors are derived the only way that keeps them achievable: take the liquidity the supplied amounts can actually fund (the smaller side), then floor the amounts THAT liquidity consumes. A side the range cannot consume at the current price stays zero, because any positive floor there is unsatisfiable. Close and switch prorate by exitBps first. The switch's mint floors come from the withdraw leg's floors — the worst withdrawal the transaction will accept — so a switch that satisfies the withdraw leg cannot then revert on the open leg; collected fees only make it easier. `getLiquidityForAmount0/1` now refuse a result above uint128, matching where LiquidityAmounts reverts: returning it would hand the caller a minimum the position manager could never satisfy. Tests (test/scripts/lpMath.ts) diff the helpers against the pinned Uniswap libraries through a new LpMathProbe rather than against copied constants: tick vectors at both extremes, zero, both signs and every power-of-two bit; liquidity conversions in, below and above range; 1e24 values that a double would round; and the uint128 boundary. Then the floor cases the audit asks for, the encoded openLp/closeLp/switchLp calldata carrying the exact helper output for in-, below- and above-range fixtures, and a wiring check that no script has a 0n minimum or floating-point tick math left. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QKobiCuUa3k6sHvyy7sS5q
I-02 asked for a choice: document a bounded dust maximum, or refund the residue. Refund. There is no single bound worth documenting — a handler declines part of a repayment for its own reason (Aave skips 1 wei to dodge InvalidBurnAmount, Fluid skips below its minimum operate amount, Moonwell caps at the live debt), and Moonwell's leftover is `amount - actualDebt`, not a constant. Worse, residue in the module is not inert: the next operation reads balanceOf(address(this)), so one user's leftovers would be swept into the next user's position. SafeDebtManager._executeDebtSwap and LeveragedPosition._handleCreateCallback now hand the post-repayment balance back to the Safe, which is what _handleCloseCallback already did — the two paths that skipped it were the outliers. That buys a single invariant in place of a per-protocol dust table: after any debt operation the module holds zero of every asset it touched. debtSwapBySafe asserts it in afterEach for every case, across all five protocols. I-01 and I-03 are properties of the Safe-module custody model rather than defects, so they are written down in docs/SECURITY_MODEL.md instead of patched. The performance fee is cooperative: the Safe owns the position NFT and can exit around the module, drop the module, or simply hold no USDC (the treasury transfer is best-effort by design, since it must never block an exit) — enforcing it would mean taking custody. The pauser can stop module-mediated exits through setProtocolEnabledForClose, kept for the case of a compromised handler, and the same NFT ownership that weakens the fee is what bounds that power: it delays exits, it cannot trap a position. The doc records the operational rules and the recovery sequence. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QKobiCuUa3k6sHvyy7sS5q
…upply
`SwapLeg` carried an `expectedOut` that the handler checked `amountOutMin`
against. The same caller supplied both, so {amountOutMin: 1, expectedOut: 1}
satisfied it — and `collectLpBySafe.ts` was passing exactly that, because
the fee amounts a collect swaps are not known until it executes. A
compromised operator key needed nothing cleverer to drain a swap.
The handlers now derive the router minimum themselves:
minOut = max(caller's amountOutMin, twapQuote(amountIn) * (1 - slippageBps))
Deriving rather than validating is what makes the unknown-size case honest:
such callers pass 0 and get the floor. The check sits in `_swapViaSafe` /
`_swapV4ViaSafe`, the single funnel each handler routes every call through,
so it holds structurally. `expectedOut` is gone from the struct.
The reference is a Uniswap V3 pool per TOKEN, deliberately not the pool a
swap executes in — one price per token, so Aerodrome and V4 swaps are
floored by the same V3 observation history, and native ETH prices under
WETH. TwapOracle fails closed on every degradation and never falls back to
spot; a fallback IS the attack, since whoever can degrade the oracle would
choose the degraded path.
Three checks, and the third is the one worth explaining. Cardinality must
meet the configured floor; the pool's own OLD revert propagates; and the
NEWEST observation must be recent. A bare `observe()` call cannot replace
that last one: a cardinality-1 pool idle longer than the window answers
without reverting, because every point in the window resolves after its
single stored observation, so the "average" is exactly the live tick.
Measured on Base at block 50197687, four pools behave this way — the
Aerodrome WETH/USDC tickSpacing-200 pool and three Uniswap V3 AERO pools,
one holding no liquidity at all. What the check defends is staleness, not
manipulation: moving a tick writes an observation carrying the PRE-move
tick, so an attacker's own trade adds nothing to the average that block.
An abandoned reference stuck below the market is the real hazard.
`setTwapConfig` stays admin-settable rather than timelocked, because a
reference that degrades must be repointable immediately — a stale oracle
blocks closeLp. The dangerous direction is closed by construction instead:
MIN_TWAP_WINDOW and MIN_TWAP_CARDINALITY are constants no role can lower,
the pair is verified against the pool's immutable token0/token1, and the
setter calls the oracle so a reference that cannot answer today is refused
at configuration time.
Flooring closeLp on an oracle would otherwise let a broken reference strand
a position, which is why the audit tied an oracle-free exit to this item.
`withdrawLp` was a handler primitive the manager never exposed; it is now an
entry point. It takes a position out in kind, reads no price, and is gated
only by protocolEnabledForClose. No performance fee, for the same reason
switchLp charges none — nothing is realized in USDC, so there is no profit
to measure.
Tests assert the value the router actually received rather than that a call
reverted: the mocks now record `lastAmountOutMinimum`, and each case pins
raise-to-floor, tighten-above-floor, zero-means-floor, and scaling with
slippageBps. TwapOracle is under the 95% branch gate at 100%. The fork
suites configure real Base references (both cardinality 2000) and needed a
`pokeTwapPool` helper: a warped fork stops writing observations, and the
poke has to move the tick, since V3 writes one only when the tick changes.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QKobiCuUa3k6sHvyy7sS5q
M-01 named the collect fee swap, which shipped `amountOutMin: 1` because the amount collected is unknowable before the collect runs. H-01's derived floor removed the category rather than the instance: `_swapViaSafe` and `_swapV4ViaSafe` are the only two places in the handlers that reach a router, both size the minimum from the amount actually being swapped, and a caller passing 1 — or 0 — now simply gets the floor. A test pins that on the exact call the audit named, asserting the value the router received rather than that something reverted. M-02 is the real work. A concentrated-liquidity mint consumes its two sides only in the ratio its range demands, so it stops at whichever side runs out and hands the rest back to the Safe. On a Base fork that residue is 4.8%-5.3% of the position's basis. Unaccounted, it is a repeatable fee leak: switch, take profit out as residue, switch again, close a position that looks break-even and pay nothing. So the residue is treated as what it is — a withdrawal. It repays cost basis first; whatever exceeds the basis is profit already taken and is carried onto the replacement position, prorated on partial exits exactly like the basis it mirrors. `closeLp` charges on lifecycle profit, `currentValue + carryForExit - basisForExit`, so the fee follows the position rather than the last leg of it. Valued at the H-01 reference TWAP, never spot: spot would let anyone able to nudge a pool under-report the residue and shrink the fee the eventual close charges. A switch that redeploys everything reads no price at all, and a switch that cannot price its residue reverts rather than guessing. `PositionClosed` gained `carryForExitUsd6` because without it the emitted numbers no longer explain the emitted fee, and `PositionWithdrawn` gained `releasedCarryUsd6` so the fee-free in-kind exit is visible rather than silent. `PositionSwitched.carriedBasisUsd6` now reports the basis actually stored, which is the old basis minus the residue. The fork tests assert conservation on real numbers — newBasis + residue == previousBasis — instead of the old "basis rides along unchanged", which was only ever true because the residue was being dropped. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QKobiCuUa3k6sHvyy7sS5q
…mment Reviewing the branch as a whole rather than finding by finding turned up four things worth correcting before it goes out. The deploy module could not have run. `setTwapConfig` needs DEFAULT_ADMIN_ROLE, which the constructor grants to `initialAdmin` — an env-supplied address, normally a multisig — while ignition executes `m.call` from the deploying key, which never holds the role. The same block also called `envString(TWAP_REF_WETH_USDC_POOL, "SYM_...")`, but `envString` takes only env var NAMES, so the pool address was being looked up as a variable and the parameter resolved to "". Both problems disappear with the call: TWAP configuration is a post-deploy admin step, exactly like the pool allow-listing the module already documents that way. The module header now spells out that it is required before first use, why it cannot be done at deploy time, and where the reference addresses live. Nothing is silently unsafe in the gap — an unconfigured token makes openLp, closeLp and collectLp revert `TwapNotConfigured`, which fails closed. `SwapMinBelowTwapFloor` was declared and never thrown, left over from the first H-01 draft that validated the caller's minimum before the design moved to deriving it. It was reaching the ABI as a phantom error. `PositionWithdrawn` was documented as "no swap, no oracle, no fee". The first two hold; the third does not. `withdrawLp` harvests fees on the way out through the same `_collectLpFees` path as everything else, so `feeCollectBps` is charged — only the PERFORMANCE fee is waived. The event doc and the manager NatSpec now say which fee they mean. Two paths the new entry point made reachable had no test. `withdrawLp` on a STAKED Aerodrome position is the case the emergency exit exists for — the stakePool owns the NFT, so it has to unstake, burn, and clear the pin — and it was only covered indirectly through switchLp's withdraw leg. And nothing asserted that the carry a switch accrued is released without being charged, which is the one place the fee waiver is observable. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QKobiCuUa3k6sHvyy7sS5q
The config-integrity work added 36 branches to SafeYieldManager and covered 22 of them, taking the file from 97.00% to 91.53% and putting CI below its 95% gate. Most of the gap sits on machinery that nothing else would catch if it were wrong. The allow-list is no longer a bare mapping: it carries a parallel array and an index map so protocol re-enablement can walk it, removal is swap-and-pop, and the index of the entry that gets moved has to follow it. A test now drives add, duplicate-add, remove-something-never-added, remove-from-the- middle and remove-the-last, checking count, enumeration and membership at each step — the middle removal is the one that would silently corrupt the list, and the never-added removal is the one that would underflow the index map. Also covered: allow-listing against a protocol with no handler, the same-token pool param whose second reference lookup is skipped rather than repeated, the slippage bounds and missing-reference revert on `twapMinimumOut`, and quoting the native key in both directions. One comment in the V4 fixture still described native ETH as "priced under WETH", which stopped being true when native got its own address(0) key. It now says what actually holds, including why BOTH keys have to be configured: swaps on a native pool read address(0), while the in-kind switch values its residue under WETH, because `withdrawLp` hands a native side back wrapped. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QKobiCuUa3k6sHvyy7sS5q
…diately `setTwapConfig` being timelock-critical is right for CHANGES — repointing a reference moves the price boundary under every managed Safe. It was wrong as the only way IN: the constructor enables every protocol it registers, so a freshly deployed manager sat open-enabled and unable to swap anything until a timelock proposal executed, days later. The constructor now takes `_twapTokens` / `_twapConfigs` and installs them through `_storeTwapConfig`, extracted from the setter. Both paths run the same window and cardinality floors, the same immutable-pair check against the pool's own token0/token1, the same code and initialization checks, and the same live `meanTick` read — a seeded reference is held to exactly the standard a replaced one is, so seeding does not open a weaker door. A reference that cannot answer fails the deploy rather than being installed. Seeding happens before any protocol is registered, which lets the constructor's own allow-listing run the reference requirement `setPoolParamAllowed` enforces. "Allow-listed implies a live reference" now holds from block one instead of starting at the first post-deploy change — the gap flagged in the pre-PR review. That check earned its keep immediately: the switchLp fork fixture allow-lists a Uniswap V4 native pool but had never configured the `address(0)` reference. Nothing noticed, because an in-kind switch performs no swap; the first native swap would have reverted. The fixture now seeds it. The deploy module seeds WETH, the native `address(0)` key and AERO. Only the first two are load-bearing for the pool params it allow-lists; AERO is there so staked Aerodrome emissions can be sold without a timelock round trip, and can be dropped if that pool happens to be quiet at deploy time. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QKobiCuUa3k6sHvyy7sS5q
The constructor was made to enforce the same reference requirement as `setPoolParamAllowed`, but nothing asserted it. An invariant that only a comment claims is one refactor away from being untrue, and this one is the reason seeding is safe to allow at all. Two tests now hold it. A deployment that allow-lists a pool param whose non-USDC side has no seeded reference must revert `TwapNotConfigured` — otherwise the constructor would be the single path into an allow-listed-but-unpriceable pool, discovered at the first swap rather than at deploy. And a seeded reference is put through every rejection the timelocked setter applies: window and cardinality below their floors, a pool with no code, a pool that does not trade the pair, and a pool that trades it but cannot answer over the window. Checked the same way for the rest of the constructor: treasury, fee ceilings, pauser and handler validation all run the identical checks their setters do, `maxSlippageBps` is seeded at 300 against a settable ceiling of 1000, and a seeded pool of address(0) is caught by the code-length check rather than needing the setter's separate removal guard. `setYieldHandler` does not re-derive references for an existing allow-list, which is fine: a replacement handler is timelock-installed and pinned to the same protocol id, so it decodes the same params to the same tokens. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QKobiCuUa3k6sHvyy7sS5q
Seeding TWAP references took the constructor to 17 parameters. That compiles fine normally, but `yarn coverage:gated` instruments every function with an extra local and the constructor then overran by exactly one slot: YulException: Cannot swap Slot RET with Variable ret_param: too deep in the stack by 1 slots in [ RET ret_param_1 ... ret_param_16 ret_param ] `_twapTokens` and `_twapConfigs` are now a single `TwapSeed[]`, which is the pattern the repo already uses for exactly this reason (`MintArgs`, `SwapSteps`). Sixteen parameters clears the limit. Pairing the key with its config also makes the two impossible to supply at different lengths, so the `LengthMismatch` check for that pair — and the test pinning it — are gone: the invariant is structural now rather than enforced. I had reported this gate as passing twice. It was not: I ran `yarn coverage:gated >/dev/null 2>&1`, which swallowed the compilation failure, and `yarn coverage:check` then read a `coverage.json` left over from an earlier successful run. Verified this time by reproducing CI's own sequence with output visible and with its pinned BASE_FORK_BLOCK_NUMBER of 49470000 — 322 tests, SafeYieldManager 96.19%, overall 97.66%, on a coverage.json written seconds before the check. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QKobiCuUa3k6sHvyy7sS5q
…${chainId}
deploy:2_yield_manager runs the ignition deploy under whatever --deployment-id
the caller picks (e.g. yield-v2 for the current generation, kept separate
from the chain-8453 default so the retired generation's addresses stay
recoverable). The sync step ignored that and always read
ignition/deployments/chain-${chainId}, so it kept writing that OLD
generation's addresses back over the reviewable deployments/base.json
manifest instead of the one actually deployed. Reading IGNITION_DEPLOYMENT_ID
first — the same override scripts/lpSafeShared.ts already uses — fixes that.
deployments/base.json is corrected as a result: it had been stuck on the
retired 0x7d92... manager from the chain-8453 record and never picked up
0x1D5B..., the generation actually in production.
Output of `node scripts/exportAbis.js` and `IGNITION_DEPLOYMENT_ID=yield-v2 node scripts/syncDeploymentAddresses.js`, run right after the redeploy in 3a13043. abis/SafeYieldManager.json is the same 132 entries reordered (the compiled artifact's own ordering); deployments/base.json now points at the new manager + handlers instead of the retired 0x1D5B... generation.
test/debt/*.ts moved collateral onto the Safe with a real `transfer` from a test EOA, so every run's outcome depended on what that wallet still held on mainnet at the pinned fork block — a balance that only ever goes down. Once it ran dry the transfers reverted; WETH9's transfer has no reason string, so this surfaced as the opaque "reverted without a reason string" rather than anything pointing at the actual cause. dealToken/dealTokenAmount (test/helpers/utils.ts) write the recipient's balance directly via hardhat_setStorageAt instead, probing for the token's balance slot rather than hardcoding it per token (a hardcoded table breaks silently on a layout it wasn't built for). sendCollateralToSafe and the two collateral transfers in createLeveragedPositionBySafe.ts now deal instead of transferring; SafeExecTransactionWrapper.ts follows the same pattern for its Fluid-vs-token-transfer branch. dealTokenAmount reads the token's own decimals rather than assuming 18 — the prior code sized every deal with parseEther, which asked cbBTC (8 decimals) for ten million cbBTC. Separately, the shared test Safe's Fluid position on a given vault is not unique: past runs leave their emptied NFT behind, so `getPosition` (which picked the FIRST match) kept returning an old zero-collateral position instead of the one just funded, and borrowing against zero collateral divides by zero inside Fluid (Panic 0x12, surfaced as GS013 through the Safe). It now prefers a funded match, falling back to the newest. createLeveragedPositionBySafe.ts's defaultTargetSupplyAmount is now derived from DEFAULT_SUPPLY_AMOUNT (2x) instead of an independent literal — the two were the same value by coincidence, and the leveraged-position diff amount (target - principle) goes negative the moment they drift, which Paraswap rejects as "Invalid Amount" with no hint that the constants are the cause. These fixes verifiably eliminate the two failure modes named above (grepped for their exact signatures — "reverted without a reason string" and "divide or modulo by zero" both drop to 0 occurrences) and introduce no new one (no "Invalid Amount" either). They do NOT fix everything in test/debt/*.ts: full-suite counts still land in the 30s-40s and move between runs of the *same* code, because getParaswapData (also in test/helpers/utils.ts, pre-existing) calls the live api.paraswap.io swap endpoint for every debt-swap test — real-time market routing layered on a block-pinned fork, so slippage and revert behavior aren't reproducible run to run. CI already excludes this whole directory (pre-existing, since before this remediation branch).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR rebuilds RateHopper's yield infrastructure around a unified handler pattern and extends it well beyond the original Aerodrome scope: the stack now covers Uniswap V3, Aerodrome Slipstream (with staking + AERO reward harvest), and Uniswap V4 (including native ETH pools) over any allow-listed token pair, with atomic position switching between protocols. The debt side is renamed/reorganized for symmetry, and a dead-code sweep plus documentation pass keep the repo consistent with what actually ships.
Architecture
Unified yield module —
SafeYieldManageris the single Gnosis Safe module users enable for all yield (LP) protocols, the yield-side counterpart ofSafeDebtManager. Per-protocol mechanics live in stateless handlers executed via delegatecall (BaseYieldHandler→V3StyleYieldHandler→UniV3YieldHandler;AerodromeYieldHandler;UniV4YieldHandler). Shared mutable state lives in an ERC-7201 namespace (YieldStorage), so handler delegatecode can never collide with manager storage; handlers declare no storage of their own. Basis bookkeeping, performance / collect fees, pause and per-protocol kill-switches, and the timelocked setter surface stay in the manager.Extensible protocol IDs — protocols are plain
uint8ids end-to-end (YIELD_PROTOCOL_*constants document the canonical assignment). A new protocol registers on a deployed manager via timelockedsetYieldHandlerwith no redeploy. Pool selection params are ABI-encoded bytes carrying the pair + protocol key, allow-listed bykeccak256(poolParam)— which is exactly what lets V4's richerPoolKeyfit without interface changes.Protocol support
Uniswap V4 handler (
UniV4YieldHandler) — implementsIYieldHandlerdirectly because the V4 singleton/actions model doesn't fit the V3-shaped hooks:PoolKeytuple, sokeccak256(poolParam)IS the V4 PoolId; hooked pools are gated exclusively by the admin allow-list.currency0 == address(0)) supported: ETH rides as call value, needs no approvals, and excess mint value is swept back to the Safe.@uniswap/v4-core@1.0.2,@uniswap/v4-periphery@1.0.3) instead of vendored sources.Aerodrome staking & AERO rewards (
AerodromeYieldHandler):openLp: the minted Slipstream NFT is deposited into its pool's stake pool (resolved via the Voter) for AERO emissions; the stake pool is pinned per position inYieldStorage.closeLp/switchLpauto-unstake when needed; unstaked and no-stake-pool positions close unchanged.collectLpon a staked position claims the stake-pool reward to the Safe, with an optional reward→USDC swap leg (swapRewardToUsdc+rewardSwap) sized to exactly the claimed amount; harvested LP fees can likewise be swapped to USDC (swapFeesToUsdc).Arbitrary pairs & LP switching:
SwapLeg(a side that IS USDC needs no swap).switchLpatomically closes a full position and opens its replacement in another allowed pool/protocol (V3 ↔ Aerodrome ↔ V4), carrying only the basis attributable to value actually redeployed.openLp/switchLppool tokens are gated on the registry token whitelist (checked via staticcallpoolTokensbefore any funds move).Hardening
TokenReturnLib(empty return = success for USDT-style tokens, strict canonical-true otherwise, never reverts the waive-on-failure fee paths); approvals are hardened with zero-resets after use; constructor validates timelock wiring (getMinDelay() > 0); min pool/position liquidity floors and pool-param allow-lists guard spot-price reads and dust positions; slippage floors are enforced per swap leg against a manager-cappedmaxSlippageBps.Debt side & repo reorganization
Debt-side rename —
Protocol→DebtProtocol, handlers renamed to<Protocol>DebtHandler, folders reorganized into area folders (contracts/debt/,contracts/yield/,contracts/legacy/,contracts/common/); deployed-contract ABIs unchanged.Legacy coexistence — the standalone
RatehopperAerodromePositionsmodule is deleted from the repo;RatehopperUniV3Positionsremains frozen, serving existing positions (no basis migration — new/old reject each other's tokenIds).Code cleanup — dead-code sweep removed ~1,400 lines (unused interfaces, modifiers, struct fields, orphaned scripts); duplicated handler flows, test helpers, and ops scripts deduplicated.
Deployment & operations
Single yield deploy module —
2_DeployYieldManager.tsdeploys all three handlers +SafeYieldManager; constructor registration enables the protocols and seeds default pool allow-lists (WETH/USDC on V3 fee tiers {100, 500, 3000} and Aerodrome tick spacings {100, 200}; the hookless native ETH/USDC 0.05% pool on V4). Layered env-var scheme (SYM_*override → shared name → legacyRHP_*→ default). Deploys sync a reviewable manifest todeployments/base.json.LP ops scripts —
openLpBySafe/closeLpBySafe/collectLpBySafe/switchLpBySafedrive a deployed manager from the user's own Safe.Testing & docs
Tests — mock-driven
SafeYieldManagerunit suites plus per-protocol Base-fork suites (Uniswap V3, Aerodrome incl. staking/reward flows, Uniswap V4 incl. native-ETH and USDC-as-currency0 pairs, cross-protocolswitchLp), fork tests running against a real Safe; statement/function coverage completed with a branch-coverage gate in CI (pinned fork block for determinism). Test tree reorganized intotest/{debt,yield,registry,legacy,helpers}/.Docs — README /
.env.sample/ CLAUDE.md brought in line with the shipped stack (V4 deploy + env vars, staking semantics, 2-day timelock default, handler hierarchy, ops-script runbook).