From 9507a821fa9e7bb39ca75a055626d18bc30efc50 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=87R=C5=B8ST=C3=83=C5=81?= Date: Fri, 23 Oct 2026 16:58:07 +0100 Subject: [PATCH] fix: correct slippage bound docs to 500 bps --- apps/docs/architecture/vault-contract.md | 2 +- packages/contracts/vault/src/errors.rs | 9 ++++++++- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/apps/docs/architecture/vault-contract.md b/apps/docs/architecture/vault-contract.md index 79002906..5099eafa 100644 --- a/apps/docs/architecture/vault-contract.md +++ b/apps/docs/architecture/vault-contract.md @@ -108,7 +108,7 @@ Phase 2. Must be preceded by `begin_migration(new_adapter)`, with `MIN_LEDGER_GA Fails up front with `InvalidSlippageBps` if `max_slippage_bps > MAX_ADMIN_SLIPPAGE_BPS` (500 bps, 5%, as of [#557](https://github.com/drydocs/meridian/issues/557); previously 10,000, unlimited), `SameAdapter` if `new_adapter` is the vault's current adapter, `NoAdapterPosition` if the current adapter has no position (`ADPT_SH <= 0`, distinct from `NoSharesOutstanding`, which checks `TOTAL_SH` [vault mUSDC shares] instead and can desync from it), or `MigrationNotInitialized`/`MigrationCooldownNotMet` if no matching, sufficiently-aged `begin_migration` snapshot exists for `new_adapter`. -Refreshes and reads the old adapter's `total_assets()` as the pre-migration value (an independent measurement taken before extraction, so it can catch loss on the withdrawal leg itself, not just the deposit leg), also baselines `new_adapter`'s `total_assets()` before landing any funds on it (so a pre-existing balance the target already held, e.g. residue stranded by the `set_adapter` wrong-counter case above, isn't later counted as value this migration delivered), withdraws the vault's entire adapter-share position into the vault itself, deposits it into `new_adapter`, and requires the new adapter to report a positive share count (`DepositTooSmall` otherwise, this is what stops the vault's own bookkeeping from ever being pointed at zero adapter shares while `TOTAL_SH` is still positive). Two independent checks then have to pass: a post-migration `total_assets()` delta over the pre-deposit baseline no lower than `(10_000 - max_slippage_bps) / 10_000` of the _pre-migration_ value (`MigrationValueDrift` otherwise), and the target's pre-deposit baseline itself no lower than `(10_000 - max_slippage_bps) / 10_000` of the `begin_migration` _snapshot_ value (`MigrationStabilityDrift` otherwise). Together, these prove the target wasn't drained or manipulated during the cooldown. The second check deliberately reads the pre-deposit baseline, not the post-migration total: the post-migration total always includes the funds this call itself just delivered, so checking it against the snapshot would pass regardless of how much the target's pre-existing balance had been manipulated. `10_000` itself is a valid, if extreme, choice of `max_slippage_bps`, an admin explicitly accepting no value-preservation protection, e.g. when recovering from an old adapter already known to be broken. Since Soroban transactions are atomic, a failed check leaves no partial state and nothing moves. The `begin_migration` snapshot also survives a failed attempt and can be reused. On success, `ADAPTER`, `ADPT_SH`, and the migration snapshot are cleared; `TOTAL_SH`, every holder's mUSDC balance, and every depositor's `Principal`/`Entry` are untouched, since they're denominated in vault mUSDC shares, not adapter shares. +Refreshes and reads the old adapter's `total_assets()` as the pre-migration value (an independent measurement taken before extraction, so it can catch loss on the withdrawal leg itself, not just the deposit leg), also baselines `new_adapter`'s `total_assets()` before landing any funds on it (so a pre-existing balance the target already held, e.g. residue stranded by the `set_adapter` wrong-counter case above, isn't later counted as value this migration delivered), withdraws the vault's entire adapter-share position into the vault itself, deposits it into `new_adapter`, and requires the new adapter to report a positive share count (`DepositTooSmall` otherwise, this is what stops the vault's own bookkeeping from ever being pointed at zero adapter shares while `TOTAL_SH` is still positive). Two independent checks then have to pass: a post-migration `total_assets()` delta over the pre-deposit baseline no lower than `(10_000 - max_slippage_bps) / 10_000` of the _pre-migration_ value (`MigrationValueDrift` otherwise), and the target's pre-deposit baseline itself no lower than `(10_000 - max_slippage_bps) / 10_000` of the `begin_migration` _snapshot_ value (`MigrationStabilityDrift` otherwise). Together, these prove the target wasn't drained or manipulated during the cooldown. The second check deliberately reads the pre-deposit baseline, not the post-migration total: the post-migration total always includes the funds this call itself just delivered, so checking it against the snapshot would pass regardless of how much the target's pre-existing balance had been manipulated. `max_slippage_bps` is capped at `500` (5%) by the `InvalidSlippageBps` check above, so `10_000` is not an accepted value; the most permissive accepted setting is `500`, which still requires the migration to preserve at least 95% of value. Since Soroban transactions are atomic, a failed check leaves no partial state and nothing moves. The `begin_migration` snapshot also survives a failed attempt and can be reused. On success, `ADAPTER`, `ADPT_SH`, and the migration snapshot are cleared; `TOTAL_SH`, every holder's mUSDC balance, and every depositor's `Principal`/`Entry` are untouched, since they're denominated in vault mUSDC shares, not adapter shares. **This does not protect against a malicious or compromised admin key.** The admin chooses `new_adapter`, and a fake adapter could report whatever `total_assets()` it likes to pass the slippage check and then keep the funds. The invariant guards against accidental value loss (slippage, a buggy new adapter), not against the admin key itself. That is a key-custody problem (see the shared testnet admin/deployer/mUSDC-issuer key warning in the deploy scripts), not something this function can close on its own. diff --git a/packages/contracts/vault/src/errors.rs b/packages/contracts/vault/src/errors.rs index 8524b1c0..1ebe673d 100644 --- a/packages/contracts/vault/src/errors.rs +++ b/packages/contracts/vault/src/errors.rs @@ -42,7 +42,8 @@ pub enum ContractError { /// `ADPT_SH` (adapter-side shares), not `TOTAL_SH` (vault mUSDC shares), /// and the two can desync. NoAdapterPosition = 13, - /// `migrate_adapter` was called with `max_slippage_bps > 10_000`. + /// `migrate_adapter` was called with `max_slippage_bps > 500` (the + /// `MAX_ADMIN_SLIPPAGE_BPS` cap). InvalidSlippageBps = 14, /// `withdraw` was called with a `min_usdc_out` floor and the actual /// amount out fell below it. Distinct from `WithdrawalTooSmall` (which @@ -87,4 +88,10 @@ pub enum ContractError { /// Distinct from `Overflow`: this points to a degenerate adapter state /// (e.g. zero `total_assets`) rather than a genuine arithmetic overflow. DivisionByZero = 24, + /// `migrate_adapter` read a zero or negative `total_assets()` from the + /// old adapter while the vault still holds adapter shares. That value is + /// the baseline for the withdrawal-leg slippage floor; a non-positive + /// baseline collapses the floor to zero (or below) so the loss guard + /// would always pass. + MigrationBaselineInvalid = 25, }