fix(soroban): bind adapters by market identity#426
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe vault adapter binding system is refactored to use a stored Changes
Sequence DiagramsequenceDiagram
participant Gov as Governance System
participant Vault as Vault Contract
participant Storage as Instance Storage
Gov->>Vault: apply_supply_queue_policy(accounts, target_ids)
Vault->>Vault: Validate accounts.len() == target_ids.len()
loop For each market in target_ids
Vault->>Vault: Derive proposed adapter by index
Vault->>Storage: Check existing AdapterBindings
alt Binding exists & mismatched
Vault->>Gov: Reject (InvalidInput)
else Binding missing
Vault->>Storage: Insert market → adapter binding
end
end
Vault->>Storage: Update supply queue
Vault->>Storage: Write AdapterBindings map
Vault->>Gov: Return Ok(())
Note over Gov,Storage: Later: adapter lookup uses stored map
Gov->>Vault: adapter_for_market(market_id)
Vault->>Storage: Fetch AdapterBindings[market_id]
Storage-->>Vault: SdkAddress or InvalidInput error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai for the benefit of the human reviewers - please output a description of this PR and any critique on the usability of the design when you are reviewing. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me gather the relevant source files to provide a thorough description and design critique. 🏁 Script executed: Length of output: 422 🏁 Script executed: Length of output: 27 🏁 Script executed: Length of output: 27 🏁 Script executed: Length of output: 1309 🏁 Script executed: Length of output: 131 🏁 Script executed: Length of output: 16342 🏁 Script executed: Length of output: 16825 🏁 Script executed: Length of output: 36111 🏁 Script executed: Length of output: 40876 🏁 Script executed: Length of output: 2458 🏁 Script executed: Length of output: 9289 🏁 Script executed: Length of output: 3937
PR DescriptionProblem Addressed (Audit Finding A-001)Previously, the vault routed supply/withdraw allocation calls to adapters using positional lookup: the adapter at index What ChangedThe PR introduces a dedicated
Bindings survive
Design Critique1. The timelocked governance proposal path cannot create bindings
The only way to create a binding today is via a direct
2.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
contract/vault/soroban/src/contract/entrypoints.rs (1)
559-563: 🧹 Nitpick | 🔵 Trivial | 🏗️ Heavy lift
accountsis now carrying adapter bindings, not accounts.For
GOVERNANCE_POLICY_KIND_SUPPLY_QUEUE, the Soroban ABI still exposes a generic bag of optionals and repurposesaccountsto mean “adapter list aligned withtarget_ids”. That keeps the proxy interface opaque for curators and tooling. If codegen budget allows, a dedicated supply-queue payload or dedicatedadapter_bindingsfield would make this much harder to misuse.As per coding guidelines, prefer stronger types over loosely constrained values in Rust: enums over well-known strings, sets over vectors when uniqueness matters, dedicated newtypes over primitive obsession, and structured state machines over ad hoc flags.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contract/vault/soroban/src/contract/entrypoints.rs` around lines 559 - 563, GOVERNANCE_POLICY_KIND_SUPPLY_QUEUE is passing a loosely-typed `accounts` bag into apply_supply_queue_policy even though those values are adapter bindings, not account objects; change the call and the apply_supply_queue_policy signature to accept a strongly-typed adapter_bindings/newtype (e.g., AdapterBindings or SupplyQueueAdapterBindings) instead of `accounts`, validate that adapter_bindings length and alignment match `target_ids` and that each binding conforms to expected adapter format, update any internal uses in apply_supply_queue_policy to treat inputs as adapter bindings (not accounts), and update other callers and docs/comments to use the new type so the proxy ABI remains opaque but the Rust side enforces correctness.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contract/vault/soroban/src/contract/entrypoints.rs`:
- Around line 130-153: The code currently allows writing proposed_adapter into
AdapterBindings without checking the allowlist; before calling
bindings.set(target_id, adapter) in the entrypoint that iterates target_ids
(where proposed_adapter is computed), check that the adapter is a member of
VaultDataKey::AllowedAdapters (or call the existing membership helper used
elsewhere) and return ContractError::InvalidInput if not allowed; specifically
update the branch in entrypoints.rs that handles the else (no existing_adapter)
to validate adapter membership against AllowedAdapters prior to calling
bindings.set, so allocate_impl cannot later route funds to an unapproved
adapter.
- Around line 136-153: The loop that binds adapters must reject non-contract
addresses: call require_contract_address() on any proposed adapter (the value
produced from address_from_alloc_string() stored in proposed_adapter) before
comparing with existing_adapter or calling bindings.set; if
require_contract_address() returns an error, propagate
ContractError::InvalidInput. Update the logic in the for loop over target_ids
(around proposed_adapter, existing_adapter, and bindings.set) to validate
proposed_adapter via require_contract_address() and only proceed to compare or
set after that check.
---
Outside diff comments:
In `@contract/vault/soroban/src/contract/entrypoints.rs`:
- Around line 559-563: GOVERNANCE_POLICY_KIND_SUPPLY_QUEUE is passing a
loosely-typed `accounts` bag into apply_supply_queue_policy even though those
values are adapter bindings, not account objects; change the call and the
apply_supply_queue_policy signature to accept a strongly-typed
adapter_bindings/newtype (e.g., AdapterBindings or SupplyQueueAdapterBindings)
instead of `accounts`, validate that adapter_bindings length and alignment match
`target_ids` and that each binding conforms to expected adapter format, update
any internal uses in apply_supply_queue_policy to treat inputs as adapter
bindings (not accounts), and update other callers and docs/comments to use the
new type so the proxy ABI remains opaque but the Rust side enforces correctness.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 79f0ef35-3479-457d-a7bf-23346a761d43
📒 Files selected for processing (5)
contract/vault/soroban/src/contract/entrypoints.rscontract/vault/soroban/src/contract/helpers.rscontract/vault/soroban/src/contract/types.rscontract/vault/soroban/src/tests.rscontract/vault/soroban/tests/blend_e2e.rs
carrion256
force-pushed
the
audit/adapter-identity-and-governance-a001
branch
from
48c7f4c to
cee35b1
Compare
carrion256
force-pushed
the
audit/adapter-identity-and-governance-a001
branch
from
cee35b1 to
d2ea128
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
…f48-e0fe-4218-80c3-d631ff6c565e) Trace: cee35b1
carrion256
force-pushed
the
audit/adapter-identity-and-governance-a001
branch
from
d2ea128 to
6a6a9a9
Compare
…0fe-4218-80c3-d631ff6c565e)
1 participant
Summary
accountsAudit Context
adapter-identity-and-governancespr/refactor/vault-ergonomics/4f330057Verification
cargo test -p templar-soroban-runtime --lib -- --nocapture— 111 passedcargo test -p templar-soroban-runtime --test blend_e2e -- --nocapture— 1 passedcargo test -p templar-soroban-runtime -- --nocapture— passedNotes
AllowedAdaptersremains an allowlist/config surface, but no longer creates or rewrites market identity.adapter_for_marketnow returnsInvalidInputunless an explicit keyed binding exists.Halborn Finding IDs
Included for Halborn SSC GitHub remediation detection:
ced39f48-e0fe-4218-80c3-d631ff6c565e— Positional adapter mapping causes silent market remap on supply queue reorderThis change is