fix(contracts): error on malformed DeFindex valuation, add vault zero-assets backstop - #597
Conversation
|
@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 |
|
@Danielkallala 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. |
a37297e to
62812de
Compare
Fix DeFindex adapter to return a typed MalformedProtocolResponse error instead of silently defaulting to zero when the protocol returns a malformed or empty response from total_assets() and withdraw(). In the vault contract, add an AdapterReportedNoAssets guard that rejects deposits when the adapter reports zero total assets with shares outstanding, preventing share dilution. 🤖 Generated with Codebuff Co-Authored-By: Codebuff <noreply@codebuff.com>
62812de to
546f4cc
Compare
collinsezedike
left a comment
There was a problem hiding this comment.
Both CI failures and the extra unwrap are fixed: the commit message is within the length limit, withdraw()/total_assets() now assert on try_withdraw()/try_total_assets()'s returned ContractError directly instead of string-matching a panic message, and the same MalformedProtocolResponse guard now covers both amounts.get(0) sites in this file. The new AdapterReportedNoAssets = 17 on the vault side doesn't collide with anything on current main. Merging now.
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