Skip to content

fix(contracts): error on malformed DeFindex valuation, add vault zero-assets backstop - #597

Open
Danielkallala wants to merge 1 commit into
drydocs:mainfrom
Danielkallala:fix/defindex-total-assets-malformed-response
Open

fix(contracts): error on malformed DeFindex valuation, add vault zero-assets backstop#597
Danielkallala wants to merge 1 commit into
drydocs:mainfrom
Danielkallala:fix/defindex-total-assets-malformed-response

Conversation

@Danielkallala

Copy link
Copy Markdown

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

  • [ADD] MalformedProtocolResponse = 2 error variant to defindex-adapter/src/lib.rs
  • [MODIFY] DefindexAdapter::total_assets(): replace amounts.get(0).unwrap_or(0) with a match that panics with MalformedProtocolResponse when the DeFindex vault returns an empty or malformed vector
  • [ADD] set_asset_amounts_per_shares() to MockDefindexVault for simulating malformed responses in tests
  • [ADD] Test: total_assets_panics_on_malformed_defindex_response — verifies the adapter panics instead of silently returning zero

Vault — Zero-assets backstop

  • [ADD] AdapterReportedNoAssets = 15 error variant to vault/src/lib.rs
  • [MODIFY] deposit(): add guard that rejects deposits with AdapterReportedNoAssets when total_shares > 0 && total_assets <= 0, preventing any adapter from driving the pricing denominator to zero while shares are outstanding
  • [ADD] Test: deposit_fails_when_adapter_reports_zero_assets_with_shares_outstanding — verifies the vault rejects deposits when adapter reports zero assets with existing holders

Verification Results

cargo check --all --tests (entire workspace)
✅ Compiles cleanly — no errors, no warnings

All existing tests unchanged and passing:
✅ 44 vault tests (including new AdapterReportedNoAssets test)
✅ 14 defindex adapter tests (including new MalformedProtocolResponse test)

Acceptance Criteria

Criteria Status
Adapter no longer silently returns zero on malformed DeFindex response MalformedProtocolResponse panic on shape mismatch
Vault rejects deposits when adapter reports zero assets with shares outstanding AdapterReportedNoAssets error returned
New error variants do not break existing contracts or ABI ✅ New variants only — no breaking changes
Regression test for the pricing-path failure mode total_assets_panics_on_malformed_defindex_response
Vault-side backstop guards against any future adapter exhibiting the same failure class deposit_fails_when_adapter_reports_zero_assets_with_shares_outstanding

…-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
@drips-wave

drips-wave Bot commented Aug 25, 2026

Copy link
Copy Markdown

@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! 🚀

Learn more about application limits

@vercel

vercel Bot commented Aug 25, 2026

Copy link
Copy Markdown

@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 collinsezedike left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 exceeds CONTRIBUTING.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")]

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@collinsezedike

Copy link
Copy Markdown
Collaborator

@Danielkallala this PR also has merge conflicts against main now, please rebase and resolve those alongside the review comments above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] DefindexAdapter::total_assets() silently returns zero on a malformed response

2 participants