fix(contracts): error on malformed DeFindex valuation, add vault zero-assets backstop - #597
Conversation
…-assets backstop Replace the silent unwrap_or(0) in DefindexAdapter::total_assets() with a panic on MalformedProtocolResponse so a held position that cannot be valued fails loudly instead of reporting a zero price. Add a vault-side backstop in deposit() that rejects with AdapterReportedNoAssets when shares are outstanding but the adapter reports zero or negative total assets, preventing share dilution regardless of which adapter is active. Closes drydocs#555
|
@Danielkallala 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! 🚀 |
|
@Danielkallala 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
There was a problem hiding this comment.
The vault-side fix (AdapterReportedNoAssets guard in deposit()) is sound and well-tested. Three CI checks are failing and need fixing before this can merge:
Commit Messages: the commit subject exceedsCONTRIBUTING.md's 72-character limit.Soroban Contract Tests: see the inline comment.
One more thing worth fixing while you're in this file: withdraw() (packages/contracts/defindex-adapter/src/lib.rs:155) still has the same amounts.get(0).unwrap_or(0) pattern you just fixed for total_assets() a few lines up. Not a fund-safety issue, the vault's WithdrawalTooSmall guard catches the resulting zero, but it's the same failure mode this PR set out to make loud, left unpatched in the same file. Worth applying the same MalformedProtocolResponse fix there too.
| } | ||
|
|
||
| #[test] | ||
| #[should_panic(expected = "MalformedProtocolResponse")] |
There was a problem hiding this comment.
panic_with_error! produces a panic message like HostError: Error(Contract, #2), it never contains the variant name MalformedProtocolResponse, so this should_panic(expected = ...) assertion doesn't actually match and the test fails as written (confirmed both by reading this and by the current CI failure). Use try_total_assets() instead and assert on the returned ContractError directly, rather than string-matching a panic message.
|
@Danielkallala this PR also has merge conflicts against |
Overview
This PR fixes a critical share-dilution vulnerability in
DefindexAdapter::total_assets(). When the DeFindex vault returns a malformed response, the adapter now fails loudly instead of silently returning zero, which would have caused massive share dilution on any subsequent deposit.Related Issue
Closes #555
Changes
DeFindex Adapter — Error on malformed valuation
MalformedProtocolResponse = 2error variant todefindex-adapter/src/lib.rsDefindexAdapter::total_assets(): replaceamounts.get(0).unwrap_or(0)with amatchthat panics withMalformedProtocolResponsewhen the DeFindex vault returns an empty or malformed vectorset_asset_amounts_per_shares()toMockDefindexVaultfor simulating malformed responses in teststotal_assets_panics_on_malformed_defindex_response— verifies the adapter panics instead of silently returning zeroVault — Zero-assets backstop
AdapterReportedNoAssets = 15error variant tovault/src/lib.rsdeposit(): add guard that rejects deposits withAdapterReportedNoAssetswhentotal_shares > 0 && total_assets <= 0, preventing any adapter from driving the pricing denominator to zero while shares are outstandingdeposit_fails_when_adapter_reports_zero_assets_with_shares_outstanding— verifies the vault rejects deposits when adapter reports zero assets with existing holdersVerification Results
Acceptance Criteria
MalformedProtocolResponsepanic on shape mismatchAdapterReportedNoAssetserror returnedtotal_assets_panics_on_malformed_defindex_responsedeposit_fails_when_adapter_reports_zero_assets_with_shares_outstanding