Audit/v3 lp fixes - #224
Conversation
Adding CLAMM liquidity is the one operation that must not be permissionless: a manipulated pool spot lets a caller force a bad-price re-add and sandwich it. Remove collectAndCompound from every permissionless user flow (deposit, withdraw, withdrawShares, supplyShares, redeemShares) and make compound() BOT-gated with a caller-supplied slippage floor (amount0Min/amount1Min forwarded to increaseLiquidity, previously hardcoded 0/0). Fee re-deployment now happens only via compound() or rebalance, both BOT-gated. No valuation regression: positionAmountsAt already simulates pending fees, so the health-check / oracle valuation stays fee-complete without an inline compound; removeLiquidity still collects the exiter's pro-rata fees. Counter-tests: compound is onlyBot; a deposit no longer deploys idle as liquidity. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…comments with BOT-gating Remove finding-ID / firm / process labels from shipped comments (no logic change): C-1, H02/Issue_04, M01, finding C/C4/D, Codex adv, AUDIT NOTE, "audit PR". Also correct two oracle @dev docstrings that still claimed peek reverts on zero total value (it now floors a dust position), and update the remaining compound comments that described the old permissionless model (compounding is now BOT-gated; user flows no longer compound). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Ceil-round amount0Used/amount1Used in the subsequent-deposit branch so the depositor pays >= their pro-rata share while shares still round down — every sub-wei rounding now favors existing holders and can never dilute the pool. Both legs stay <= the desired input (frac <= dᵢ·WAD/tᵢ), so no over-consumption. Also strip internal finding-id / auditor-name references from the V3 provider test comments (no behavior change; one test renamed accordingly). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The rebase onto master auto-merged two copies of deposit() into IStakeManager (and its mock) at different line positions — no textual conflict, but a duplicate function definition that fails to compile. Remove the redundant declaration; the interface keeps a single deposit()/instantWithdraw(). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Mirror Liquidator.sol's fund-pool integration onto V3Liquidator: - fundSource (LiquidationVault) + reflowBlacklist storage, appended (UUPS-safe). - initialize takes a fundSource arg (0 = legacy); set directly (contract check only — vault registration necessarily happens after the proxy exists) + setFundSource for later. - setFundSource validates the vault has registered this liquidator; setReflowBlacklist. - withdrawERC20/ETH gate relaxed to MANAGER or fundSource so the vault's collect* pulls. - onMoolahLiquidate pulls the exact repayment shortfall from the vault (local balance first). - liquidate/flashLiquidate/redeemV3Shares reflow the residue (loanToken + redeemed legs + native) to the vault; the transfer-restricted V3 share collateral is never reflowed. - liquidate now routes a non-redeeming callback so the pool can fund it, and emits V3Liquidation. fundSource == 0 preserves the exact legacy pre-funded behavior. Counter-tests cover initialize+setFundSource validation, withdraw gate, vault-funded liquidate, and residue reflow. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Pull Request ReviewThis Solidity DeFi PR integrates Sensitive ContentNo sensitive content detected. Security Issues🟡 [MEDIUM] Withdrawal access control is relaxed
Generated by Hashdit Bot. This tool can absolutely NOT replace manual audits. |
…wap leg
In onMoolahLiquidate the native (BNB/ETH) leg was sold with
call{ value: actualRedeemedAmount }, but the bot builds the 1inch swapData
off-chain against a pre-estimated input. Native swaps require msg.value to
equal the amount encoded in the calldata, so the variable actual amount
mismatches and the aggregator reverts — failing the whole flash liquidation.
Send the pre-agreed minTokenNAmt (the value the swapData was built for) as
msg.value instead; redeemShares guarantees actual >= min so the balance always
covers it, and the leftover stays to be wrapped/reflowed. Mirrors the
smart-collateral path in Liquidator.sol. The ERC-20 leg is unaffected (approve
is a ceiling). Counter-test uses a strict aggregator mock (msg.value == amountIn);
V3LiquidatorEth test updated to set minToken1Amt to the native swap input.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Pull Request ReviewThis Solidity DeFi PR integrates Sensitive ContentNo sensitive content detected. Security Issues🟡 [MEDIUM] Access control is relaxed for withdrawal functions
Generated by Hashdit Bot. This tool can absolutely NOT replace manual audits. |
decreaseLiquidity settles the whole position's accrued fees into tokensOwed, so collecting them together with the pro-rata burned principal paid a partial withdrawer 100% of the fees — diluting the remaining holders. Collect the fees into idle first (collect only claims owed tokens: no swap, no spot move, safe on the user path), then burn the pro-rata principal and split idle+fees pro-rata. Full redeems are unchanged (pro-rata = 100%). rebalance / _collectAndCompound already keep collected fees in the vault for all holders, so they are unaffected. Counter-test: with real accrued swap fees, a partial withdraw no longer drops the remaining holders' per-share value. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…eposit-withdraw cycle A subsequent deposit's consumed amounts stay pinned to the fair composition, but shares are now min(sharesFair, sharesSpot): the spot quote re-prices the same consumed amounts against the pool-spot composition. Withdraw settles at spot, so crediting only on fair let fair-valued shares be redeemed against a richer spot composition (the deposit-withdraw cycle leak) — the min caps the credit at what a spot exit can back. Manipulation-resistant (spot can only lower the credit, never above fair); minShares is the depositor's MEV floor. Add previewDepositShares mirroring the exact credit (shared _quoteDeposit helper so preview can't drift from the mint) so frontends size minShares correctly. NatSpec + readable names on the new/changed functions and the IV3Provider surface. Counter-tests: skewed-spot credits fewer shares, cycle no longer profitable, minShares backstops a spot squeeze, and previewDepositShares == actual mint. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A2 min(fair,spot) deposit crediting caps a large imbalanced deposit at its spot-exit value, so surplus accrues to holders and the settled peek sits above the pre-deposit price. Replace the stale "peek == normal price" check with the true invariants: peekDuring == peekAfter (no transient inflation) and peekDuring >= peekNormal (deposits only raise peek). The reentrancy attack stays fully neutralized (attacker solvent, no bad debt). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
WstETHV3Provider and WbETHV3Provider inherit V3Provider.deposit unchanged, so the min(fair,spot) credit closes the deposit-withdraw cycle on the ETH pairs too. Add cycle tests to both: a skewed spot credits fewer shares, preview == mint at a skewed spot, and the deposit->withdraw cycle extracts no value. wstETH uses the deep live Uniswap V3 pool. The only wbETH/WETH pool is empty, so the first deposit bootstraps it with our own liquidity under pure-rate mode and a wide center band; the guard relaxation is scoped to the cycle tests so the existing wiring assertions keep their defaults. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
fix(provider): credit min(fair,spot) shares on deposit to close the deposit-withdraw cycle
rebalance takes an expectedCenterRate the BOT reads via the new adapter centerRate() view and passes back; if the live LST↔native rate deviates from it by more than maxCenterRateDeviationBps the call reverts. This bounds the range anchor against a rate anomaly between build and execution — invisible to the fair-NAV loss caps, which measure in the same rate frame. Both sides opt in (expectedCenterRate != 0 per-call, maxCenterRateDeviationBps != 0 globally); 0 preserves prior behavior. Adds the MANAGER setter, the centerRate() getter, and doc-faithful counter-tests (real slippage/target floors, not zeros). Storage is append-only (upgrade-safe). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
v0.1.1 false-positived V3DexAdapter's __gap consumption (it read the __gap array shrink 45->44 as a type change at the gap slot). v0.1.2 is __gap-aware: the standard pattern — shrink __gap and place the new variable in the freed slot — validates as UPGRADE SAFE, so the committed maxCenterRateDeviationBps layout passes without contorting the storage. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…guard feat(provider): assert expectedCenterRate on rebalance
sellBNB already measures actualIn from the balance delta and validates it, but emitted the requested amountIn, so a venue that refunds part of the forwarded value was logged as if the full amount was sold. That gave SellToken different semantics per path: the ERC20 _sellToken emits the consumed amount, the native path emitted the requested one. No fund-flow impact — ExceedAmount and NoProfit already use the measured values — but off-chain accounting read the wrong input. Counter-test drives a venue that consumes 0.6 of 1.0 BNB and refunds the rest; it fails against the previous code (logs 1.0 instead of 0.6). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Pull Request ReviewThis Solidity DeFi PR hardens V3 LP deposit accounting, fee distribution, compounding, rebalancing, and liquidation flows, including integration with a shared Sensitive ContentNo sensitive content detected. Security Issues🟡 [MEDIUM] Withdrawal access control is relaxed
Generated by Hashdit Bot. This tool can absolutely NOT replace manual audits. |
…ng, range params Liquidator: - onMoolahLiquidate counts native in the before-snapshot when the loan token is the wrapped-native, so stray native from earlier redemptions or receive() donations no longer reads as fresh profit and lets a shortfall clear NoProfit. - redeemV3Shares requires receiver == self when a fundSource is set, keeping vault-funded proceeds inside the reflow path. - setReflowBlacklist rejects a no-op status change, matching the other setters. Provider: - previewDepositAmounts rounds the leg amounts UP, matching _quoteDeposit, so the preview reports exactly what deposit() consumes. - Range half-width INITIAL_RANGE_BPS 100 -> 50 (+/-0.5%), which also tightens the maxSpotDeviationBps and maxTwapDeviationBps defaults; centerRateThresholdBps drops to 1bp so the BOT, not the contract, picks the rebalance cadence. - Correct two stale comments: the zero-liquidity guard covers compound only (the rebalance re-mint has none), and the spot-vs-fair gate does not apply to the rebalance re-mint. Drop the previewDepositForToken0 suggestion to deposit a single leg when fair leaves the range -- every shape reverts until a recenter. Tests: - Counter-tests for each liquidator fix, incl. a WBNB-loan market so the wrapped-native branch is exercised. - previewDepositAmounts exact-match test; regression test pinning that deposits are closed while fair sits past tickUpper and reopen after a BOT recenter. - _deposit helpers derive a non-zero minShares from previewDepositShares: min0/min1 floor the consumed amounts, not the entry price. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Pull Request ReviewThis Solidity DeFi PR hardens V3 LP share issuance, fee distribution, compounding, rebalancing, oracle behavior, and liquidation flows. It also integrates Sensitive ContentNo sensitive content detected. Security Issues🟡 [MEDIUM] Withdrawal access control is relaxed
Generated by Hashdit Bot. This tool can absolutely NOT replace manual audits. |
Bailsec V3 Collateral final report (26 Jul, 75pp), HashDit V3 LP Collateral (29 Jun - 14 Jul, 102pp) plus its follow-up update (24 - 28 Jul, 16pp), and CertiK's preliminary comments on V3Provider (assessed 8 Jul, 93pp), filed under the existing docs/audits naming convention. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Pull Request ReviewThis Solidity DeFi PR hardens V3 LP collateral accounting by capping deposit shares at the minimum fair/spot valuation, making fee compounding BOT-gated, and distributing accrued fees pro rata on withdrawal. It also integrates Sensitive ContentNo sensitive content detected. Security Issues🟡 [MEDIUM] Withdrawal access control is relaxed
Generated by Hashdit Bot. This tool can absolutely NOT replace manual audits. |
|
@audit-agent V3Liquidator.sol V3DexAdapter.sol V3Provider.sol SlisBNBV3DexAdapter.sol SlisBNBV3Provider.sol SlisBNBV3ProviderOracle.sol SlisBnbInventoryLib.sol V3PositionLib.sol |
🤖 Cloud Audit — PR #224 (V3 LP fixes)Verdict: No Critical or High. 4 Medium, 7 Low, 20 Info. Full report attached via Telegram.
What was fixed correctly: deposit→withdraw cycle non-extractive at skewed pool spot (proven by invariant analysis + three fork tests), C-1 deposit-refund reentrancy neutralized (regression test retained), storage layouts preserved, fee sweep to idle before pro-rata burn (partial withdrawer no longer scoops 100% of fees), UUPS guards intact. Recommendation before mainnet activation: address M-1 (trivial one-liner + migration function), M-3 (mirror LiquidationVault's Historical prior art: 9 specialists dispatched. Automated audit — please validate before mainnet. |
2 participants
📄 Description
Audit remediation for the V3 LP-collateral stack (
V3Providershare token +V3DexAdapter+V3ProviderOracle+V3Liquidator) on Moolah. Hardens deposit share-crediting, fee handling, andliquidation, and adapts the V3 liquidator onto the shared
LiquidationVault. This is the delta on topof
feature/v3-lpand includes the merged deposit-withdraw-cycle fix (#225).🧠 Rationale
The V3 LP position is convex: its value at any pool spot is ≥ its value at the rate-anchored fair price.
The prior deposit path credited shares on the fair basis while withdrawals settle at spot, so a
deposit→withdraw round-trip through a skewed pool could walk out with more than was put in, and unsettled
fees / inline compounding created further leaks and manipulation surface. These changes close those gaps
with minimal, holder-favoring math and move fee deployment behind the BOT-gated, slippage-bounded path.
🧪 Example / Testing
fewer shares and the round-trip extracts no value, on all three pairs:
SlisBNBV3Provider.t.sol— slisBNB/WBNB (BSC fork)WstETHV3Provider.t.sol— wstETH/WETH (Ethereum fork, live pool)WbETHV3Provider.t.sol— wbETH/WETH (Ethereum fork; first deposit bootstraps the empty pool)V3ProviderReentrancyPoC.t.solasserts the deposit-refund window shows the settledprice (no transient inflation) under the new crediting.
V3Liquidator.t.sol/V3LiquidatorEth.t.solcover theLiquidationVaultfundingpath and the native-leg flash swap.
forge test -vvv(fork tests needBSC_RPC/ETH_RPC).🧬 Changes Summary
Provider — deposit crediting
min(fair, spot)shares on subsequent deposits: consumed amounts pinned to the faircomposition (rounded up, favoring holders), share credit capped at what a spot exit can back — closing
the deposit→withdraw cycle. Spot manipulation can only lower the credit, never over-credit.
previewDepositShares(amount0Desired, amount1Desired)view returning the exact credit, sofrontends can size
minShares(mandatory to defend against a spot-skew sandwich).Provider — fees & compounding
compoundis BOT-gated and takes caller-supplied slippage floors; deposits/withdrawals no longerinline-compound — fee redeployment happens only via
compound/rebalance.removeLiquiditysweeps accrued fees to idle before the pro-rata principal burn, then distributesidle + fees pro-rata — a partial withdrawer can no longer scoop 100% of pending fees.
Liquidator
V3Liquidatoronto the sharedLiquidationVault(fund source, shortfall pull, residuereflow), mirroring
Liquidator.sol.msg.value(native-input venuesrequire
msg.value == amountIn).liquidate()emitsV3Liquidationwith the actual redeemed leg amounts.Chore
IStakeManager.depositafter the master merge; strip leftover audit-mark comments andalign compound comments with the BOT-gating.
🤖 Generated with Claude Code