Audit remediations for Inflow Vault#388
Conversation
Code Review - PR #388: Audit Remediations for Inflow VaultI've completed a comprehensive review of this PR addressing the Q1 2026 audit findings. Overall, the remediations are well-implemented and production-ready with strong test coverage. ✅ Successfully Addressed FindingsCritical: Share Price ManipulationStatus: Fixed The pre-minting mechanism effectively prevents share price inflation attacks:
contracts/inflow/vault/src/contract.rs:64-121 Medium: Deployment Tracking SynchronizationStatus: Fixed The toggle synchronization logic is robust:
contracts/inflow/vault/src/contract.rs:1364-1440 Medium: Adapter Unregistration ProtectionStatus: Fixed Position check prevents fund loss:
contracts/inflow/vault/src/contract.rs:1293-1327 Low: Silent Adapter Query FailuresStatus: Mitigated Failed queries are now tracked and reported:
contracts/inflow/vault/src/contract.rs:1750-1840 Informational: Zero Shares & Config ValidationStatus: Fixed Edge cases properly handled with early validation and clear error messages. 🔍 Issues & Recommendations1. Token Ratio Provider Trust (Medium Priority)Location: The pre-minting validation depends on Example: If ratio = 1000:1, depositor only needs 1000 tokens instead of 1M base tokens. Recommendation:
// Suggested validation
if ratio > Decimal::from_ratio(10u128, 1u128) || ratio < Decimal::from_ratio(1u128, 10u128) {
return Err(new_generic_error("token ratio out of expected range"));
}2. Adapter Query Failure During Admin Operations (Low Priority)Locations:
If Recommendation:
3. Race Condition with Tracked Adapters (Documented)The code documents this limitation in the
Recommendation:
4. No On-Chain Failed Adapter Registry (Low Priority)Failed adapter queries are reported via events but not persisted on-chain. Consistently-failing adapters aren't automatically disabled. Recommendation: // Add state tracking
const ADAPTER_FAILURE_COUNT: Map<String, u32> = Map::new("adapter_failures");
const DISABLED_ADAPTERS: Map<String, Empty> = Map::new("disabled_adapters");
// In calculate_venues_allocation
Err(_) => {
failed_adapters.push(adapter_name.clone());
let count = ADAPTER_FAILURE_COUNT
.may_load(deps.storage, adapter_name.clone())?
.unwrap_or(0) + 1;
if count >= 10 { // Threshold
DISABLED_ADAPTERS.save(deps.storage, adapter_name.clone(), &Empty {})?;
}
ADAPTER_FAILURE_COUNT.save(deps.storage, adapter_name, &count)?;
}
// Add admin function to re-enable adapters5. Test Coverage GapsMissing negative-path tests:
Recommendation: #[test]
fn unregister_adapter_fails_when_query_fails() {
// Setup adapter that returns errors on queries
// Attempt unregister
// Verify appropriate error handling
}
#[test]
fn toggle_tracking_fails_when_query_fails() {
// Setup adapter with query failures
// Attempt to toggle tracking mode
// Verify transaction reverts safely
}📋 Minor Code Quality Notes1. Magic Number DocumentationLocation: const MINIMUM_INITIAL_SHARES: u128 = 1_000_000;Consider adding a comment explaining why 1M was chosen: // 1M shares prevent share price manipulation via donation attacks.
// This value makes the attack cost prohibitive (requires donating
// millions of base tokens to meaningfully inflate share price).
const MINIMUM_INITIAL_SHARES: u128 = 1_000_000;2. Error Message ConsistencySome errors use 🧪 Test Coverage SummaryStrong coverage for:
Gaps identified above. 📚 Documentation QualityThe new ADAPTER_SPEC.md is excellent:
Minor suggestion: Add a "Security Considerations" section highlighting:
✅ Pre-Merge ChecklistBefore merging, ensure:
🎯 Overall AssessmentThis PR successfully remediates all critical and medium audit findings with well-designed, testable solutions. The code is production-ready with the following caveats: Risk Level: LOW for critical findings, MEDIUM for operational monitoring Blockers: None Recommendations: Address token ratio provider validation before mainnet deployment. Other issues are operational concerns that can be addressed in follow-up PRs. Approval Status: Approved with minor recommendations for future improvements. Excellent work on the comprehensive remediation! The systematic approach to testing and documentation significantly improves the security posture of the Inflow Vault. |
There was a problem hiding this comment.
Pull request overview
This PR implements security remediations for findings from the Q1 2026 Control Center & Vault audit. The primary focus is preventing share price manipulation attacks through pre-minting shares at instantiation, along with several medium and low severity fixes to improve vault safety and observability.
Changes:
- Critical: Pre-mints minimum 1,000,000 shares during instantiation to prevent share price manipulation via donation attacks
- Medium: Syncs DEPLOYED_AMOUNT when toggling adapter tracking modes and blocks adapter unregistration when positions are non-zero
- Low/Informational: Adds failed adapter query visibility, zero shares checks, config validation, and comprehensive adapter interface documentation
- Additional: Fixes withdrawal cancellation hedging by using min() logic for share reminting
Reviewed changes
Copilot reviewed 35 out of 35 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| contracts/inflow/vault/src/contract.rs | Core remediation logic including pre-mint, tracking sync, adapter checks, failed query tracking, zero shares validation, and withdrawal cancellation fix |
| contracts/inflow/vault/src/msg.rs | Added initial_shares_recipient field to InstantiateMsg and ReplyPayload |
| contracts/inflow/vault/src/testing.rs | Updated tests for new instantiation requirements and added withdrawal cancellation tests |
| contracts/inflow/vault/src/testing_adapters.rs | Updated adapter tests and added tests for tracking toggle and unregistration checks |
| contracts/inflow/vault/ADAPTER_SPEC.md | New comprehensive adapter interface specification document |
| docs/plans/audit-remediation-plan.md | Detailed implementation plan for all audit remediations |
| docs/audit/MyComments.md | Audit reviewer's comments on findings and remediation approaches |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Uint128::zero(), | ||
| Uint128::zero(), |
There was a problem hiding this comment.
After instantiation with MINIMUM_INITIAL_DEPOSIT (1,000,000 tokens), the contract will have 1,200,000 base tokens (1,000,000 * 1.2 ratio) and 1,200,000 shares minted to the initial_shares_recipient. The control center mock should be initialized with these values, not zero. This affects all share price calculations in subsequent deposits. Initialize the mock with: setup_control_center_mock(..., Uint128::new(1_200_000), Uint128::new(1_200_000)).
| Uint128::zero(), | |
| Uint128::zero(), | |
| Uint128::new(1_200_000), | |
| Uint128::new(1_200_000), |
| let mut total_pool_value = 1200u128; | ||
| let total_shares_issued_before = 0u128; | ||
| let mut total_shares_issued_after = 1200u128; | ||
|
|
||
| execute_deposit( | ||
| &mut deps, | ||
| &env, | ||
| &wasm_querier, | ||
| &vault_contract_addr, | ||
| USER1, | ||
| &vault_shares_denom_str, | ||
| user1_deposit, | ||
| user1_shares, | ||
| user1_shares, | ||
| user1_deposit, | ||
| total_pool_value, | ||
| total_shares_issued_before, | ||
| total_shares_issued_after, | ||
| ); |
There was a problem hiding this comment.
The total_pool_value, total_shares_issued_before, and total_shares_issued_after values don't account for the initial deposit made during instantiation. After instantiation, the pool has 1,200,000 base tokens and 1,200,000 shares. User1's deposit should be: total_pool_value = 1,201,200 (not 1200), total_shares_issued_before = 1,200,000 (not 0), total_shares_issued_after = 1,201,200 (not 1200). This error propagates through all subsequent assertions in this test.
| Uint128::zero(), | ||
| Uint128::zero(), |
There was a problem hiding this comment.
After instantiation with MINIMUM_INITIAL_DEPOSIT (1,000,000 tokens), the contract will have 1,200,000 base tokens (1,000,000 * 1.2 ratio) and 1,200,000 shares minted. The control center mock should be initialized with these values, not zero. Initialize with: setup_control_center_mock(..., Uint128::new(1_200_000), Uint128::new(1_200_000)).
| Uint128::zero(), | |
| Uint128::zero(), | |
| Uint128::new(1_200_000), | |
| Uint128::new(1_200_000), |
| let mut total_pool_value = 1200u128; | ||
| let total_shares_issued_before = 0u128; | ||
| let mut total_shares_issued_after = 1200u128; | ||
|
|
||
| execute_deposit( | ||
| &mut deps, | ||
| &env, | ||
| &wasm_querier, | ||
| &vault_contract_addr, | ||
| USER1, | ||
| &vault_shares_denom_str, | ||
| user1_deposit, | ||
| user1_shares, | ||
| user1_shares, | ||
| user1_deposit, | ||
| total_pool_value, | ||
| total_shares_issued_before, | ||
| total_shares_issued_after, | ||
| ); |
There was a problem hiding this comment.
The initial pool state values don't account for the 1,000,000 token initial deposit made during instantiation. The values should be: total_pool_value = 1,201,200, total_shares_issued_before = 1,200,000, total_shares_issued_after = 1,201,200. This affects all subsequent assertions.
| .add_attribute("adapter_name", name) | ||
| .add_attribute("deployment_tracking", format!("{:?}", deployment_tracking))) | ||
| .add_attribute("deployment_tracking", format!("{:?}", deployment_tracking)) | ||
| .add_attribute("synced_amount", adapter_position)) |
There was a problem hiding this comment.
The synced_amount attribute reports the position in deposit tokens, but the actual message sent to the control center uses the converted base token amount (adapter_position_base_tokens). For consistency and clarity, consider reporting the base token amount in the attribute: add_attribute("synced_amount", adapter_position_base_tokens).
There was a problem hiding this comment.
This one makes sense. I would also add a direction (add/subtract).
Code reviewFound 1 issue:
Lines 172 to 175 in 80b8be3 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
…tiation The previous message conflated base tokens and shares as interchangeable quantities. The new message clearly states how many shares the deposit would mint vs. how many are required. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Freezing withdrawals is a legitimate operational need (e.g. during market turbulence). Remove the >= 1 validation on max_withdrawals_per_user in both instantiate and update_config, and update tests and remediation plan accordingly. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
dusan-maksimovic
left a comment
dusan-maksimovic
left a comment
There was a problem hiding this comment.
LGTM! Are we sure that we want everything under docs to go into main?
| .add_attribute("adapter_name", name) | ||
| .add_attribute("deployment_tracking", format!("{:?}", deployment_tracking))) | ||
| .add_attribute("deployment_tracking", format!("{:?}", deployment_tracking)) | ||
| .add_attribute("synced_amount", adapter_position)) |
There was a problem hiding this comment.
This one makes sense. I would also add a direction (add/subtract).
|
Yeah I think keeping the audit here makes sense for future reference |
Summary
Addresses findings from the Q1 2026 Control Center & Vault audit.
Audit Findings Addressed
Critical:
Medium:
DEPLOYED_AMOUNTwhen toggling betweenTrackedandNotTrackedmodes. Queries adapter position and adjusts accounting accordingly.Low:
failed_adapter_queries). Operators now have visibility into malfunctioning adapters.Informational:
max_withdrawals_per_user >= 1on both instantiation and config updates. Prevents withdrawal freeze.ADAPTER_SPEC.mddocumenting required adapter behaviors and trust assumptions.Additional Fixes
Files Changed
contracts/inflow/vault/src/contract.rs- Core remediation logiccontracts/inflow/vault/src/msg.rs- Addedinitial_shares_recipientfieldcontracts/inflow/vault/src/testing.rs- Tests for remediationscontracts/inflow/vault/src/testing_adapters.rs- Adapter-related testscontracts/inflow/vault/ADAPTER_SPEC.md- New adapter interface documentationdocs/audit/- Audit documentationdocs/plans/audit-remediation-plan.md- Implementation planTest plan
make test-unitto verify all tests passmake clippyfor lintingmake compileto regenerate artifacts🤖 Generated with Claude Code