fix(defindex-adapter): apply real slippage floor on deposit/withdraw - #622
Conversation
Fixes drydocs#558. DefindexAdapter::deposit/withdraw hardcoded 0 as the minimum acceptable amount on both legs of their calls into the underlying DeFindex vault, so the adapter accepted any execution price the vault happened to offer. drydocs#117 and drydocs#432 fixed the identical bug class off-chain, in packages/stellar-sdk-helpers/src/defindex.ts's transaction builder, but never touched this on-chain path -- the one MeridianVault::deposit/ withdraw actually invoke, carrying pooled depositor funds directly. - Added SLIPPAGE_BPS (50 bps / 0.5%, matching the issue's suggested tolerance) and a min_after_slippage() helper. - deposit() now passes amount floored by 0.5% as amounts_min, instead of 0. - withdraw() now quotes the expected payout via DeFindex's own get_asset_amounts_per_shares() immediately before withdrawing, and passes that floored by 0.5% as min_amounts_out, instead of 0. - The floor leaves headroom below the exact expected amount rather than matching it exactly, so ordinary rounding doesn't cause spurious reverts -- the same constraint drydocs#117 established off-chain. - Extended MockDefindexVault to record the amounts_min/min_amounts_out it's called with, and added two regression tests (deposit_passes_a_real_slippage_floor_not_zero, withdraw_passes_a_real_slippage_floor_not_zero) asserting both legs send a real floor rather than 0. No API/ABI changes: deposit()/withdraw() keep their existing signatures, so this is not a breaking change.
|
@habnark Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
|
@habnark is attempting to deploy a commit to the Collins' projects Team on Vercel. A member of the Team first needs to authorize it. |
collinsezedike
left a comment
collinsezedike
left a comment
There was a problem hiding this comment.
deposit()'s fix is correct, min_after_slippage(amount) is a real floor on the exact requested amount, and it's well-tested. One gap in withdraw() needed before this can merge, noted inline.
| let expected = client | ||
| .get_asset_amounts_per_shares(&shares) | ||
| .get(0) | ||
| .unwrap_or(0); |
There was a problem hiding this comment.
This reuses the "safe and intentional" comment from a different site below, but it isn't safe here. A malformed or empty response collapses expected to 0, which feeds directly into min_after_slippage(0) == 0, so withdraw() calls DeFindex with min_amounts_out=[0], reintroducing exactly the "accept any price" bug this PR exists to close. The new regression test doesn't cover this, MockDefindexVault::get_asset_amounts_per_shares isn't overridden there and defaults to a normal 1:1 quote. Panic on a malformed response here instead (matching the pattern from #555), rather than silently defaulting to 0.
|
@habnark checking in, this has been sitting on REQUEST_CHANGES for over two days with no update. Let me know if you're still working through the feedback or need any help. |
|
@habnark this PR now has merge conflicts with main after today's vault contract changes. Could you rebase onto the latest main and resolve them? |
|
@habnark this PR now has a merge conflict with |
collinsezedike
left a comment
collinsezedike
left a comment
There was a problem hiding this comment.
@habnark Two issues, see inline comments.
| let expected = client | ||
| .get_asset_amounts_per_shares(&shares) | ||
| .get(0) | ||
| .unwrap_or(0); |
There was a problem hiding this comment.
@habnark withdraw()'s price quote falls back to .unwrap_or(0) if get_asset_amounts_per_shares returns an empty vector, the same malformed-response condition total_assets() explicitly guards against a few lines below in this same file. That makes expected = 0, so the slippage floor computes to 0 too, silently reintroducing the exact "accept any price" bug this PR exists to fix, precisely when the DeFindex vault is misbehaving. Neither of the two new regression tests covers this path, only the malformed-response case's sibling function (total_assets) has a test for it.
|
|
||
| /// Floors `amount` by `SLIPPAGE_BPS`, giving the minimum acceptable amount to | ||
| /// pass as the DeFindex vault's `amounts_min` / `min_amounts_out` leg. | ||
| fn min_after_slippage(amount: i128) -> i128 { |
There was a problem hiding this comment.
@habnark min_after_slippage uses raw -/*// instead of checked_mul/checked_div, unlike the equivalent slippage-floor computation in vault/src/lib.rs's migrate_adapter, which returns a typed Overflow error on the same kind of arithmetic. An intermediate overflow here traps the whole contract call with an unrecoverable panic instead of a catchable error. Worth matching the existing convention for consistency, even though the actual overflow threshold is astronomically unlikely to hit for real USDC amounts.
collinsezedike
left a comment
collinsezedike
left a comment
There was a problem hiding this comment.
@habnark Thank you for this contribution. I pushed a few follow-up commits directly to this branch to get it across the line (a merge with main had accidentally reverted the slippage floor, plus a couple of gaps from review), so credit for the fix stays with you.
Merging now.
|
@habnark If you enjoyed working on this, consider starring the repo, it helps more than you'd think. |
2 participants
Fixes #558.
DefindexAdapter::deposit/withdraw hardcoded 0 as the minimum acceptable amount on both legs of their calls into the underlying DeFindex vault, so the adapter accepted any execution price the vault happened to offer. #117 and #432 fixed the identical bug class off-chain, in packages/stellar-sdk-helpers/src/defindex.ts's transaction builder, but never touched this on-chain path -- the one MeridianVault::deposit/ withdraw actually invoke, carrying pooled depositor funds directly.
No API/ABI changes: deposit()/withdraw() keep their existing signatures, so this is not a breaking change.
Summary
Test plan
pnpm lint && pnpm typecheck && pnpm testpass locallyCloses #