fix: repair fee-anchor lifecycle accounting#428
Draft
carrion256 wants to merge 14 commits into
Draft
Conversation
Contributor
|
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:
📝 WalkthroughWalkthroughAdds kernel bounded conversion helpers with cap enforcement, advances/initializes fee anchors during deposits/refresh, reconciles idle on-ledger asset balances into state, introduces a VaultCommand::RefreshFees path, updates Soroban entrypoints/preview logic to use bounded conversions, and adds unit/property/integration tests. ChangesBounded Conversion APIs and Kernel Errors
Fee Anchor Advancement and Early Initialization
Withdrawal and Redemption with Bounded Conversions
Virtual Offsets Locking Infrastructure
Idle Asset Reconciliation Against Live Token Balance
RefreshFees Command Architecture
Proxy View Bounded Conversion Updates
Property Tests, Documentation, and Integration Test Setup
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 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 |
This was referenced May 18, 2026
carrion256
force-pushed
the
audit/fee-anchor-lifecycle-spec
branch
from
d31bf3c to
ba08a36
Compare
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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/kernel/src/actions/mod.rs (1)
732-758:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winReject zero-share deposits before mutating state.
A nonzero
assets_incan still floor toshares_out == 0here. In that case the vault will pull assets, increasetotal_assets/idle_assets, advance the fee anchor, and mint nothing to the receiver. That is a user-funds loss path on small deposits against a high share price.🐛 Proposed fix
let shares_out = convert_to_shares_bounded( &state, config, assets_in, u128::MAX.saturating_sub(state.total_shares), InvalidStateCode::MintOverflowTotalShares, )?; + if shares_out == 0 { + return Err(KernelError::ZeroAmount); + } if shares_out < min_shares_out { return Err(KernelError::Slippage { min: min_shares_out, actual: shares_out, });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contract/vault/kernel/src/actions/mod.rs` around lines 732 - 758, After computing shares_out via convert_to_shares_bounded, add an early check that rejects zero-share deposits before mutating state: if shares_out == 0 return an error (e.g., KernelError::Slippage { min: min_shares_out, actual: shares_out }) so we never update state.total_assets, state.idle_assets, state.total_shares or advance FeeAccrualAnchor::new when a nonzero assets_in would mint zero shares; place this check immediately after the convert_to_shares_bounded call and before any checked_add mutations.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@contract/vault/soroban/src/contract/entrypoints.rs`:
- Around line 406-413: refresh_fees_impl currently reconciles idle assets via
reconcile_current_idle_assets(env, vault, now_ns) which may update
fee_anchor.timestamp_ns to now_ns and then calls vault.refresh_fees(now_ns),
causing FeeRefreshTimestampMustAdvance to trigger; fix by checking whether
reconcile_current_idle_assets advanced the fee anchor to now_ns (compare
pre-reconcile fee_anchor.timestamp_ns with post-reconcile value) and if it did,
skip calling vault.refresh_fees(now_ns) (or instead call refresh_fees with the
preserved pre-reconcile timestamp) so the FeeRefreshTimestampMustAdvance path is
avoided; locate this logic in refresh_fees_impl and use
with_contract_vault_contract_error and ContractVault methods to read/preserve
fee_anchor.timestamp_ns around the reconciliation call.
In `@contract/vault/soroban/tests/integration_tests.rs`:
- Around line 1862-1865: The current assertion only enforces an upper bound on
minted_shares and would allow minted_shares == 0 to slip by; update the test
around the minted_shares check (the assertion that currently verifies
minted_shares <= 62) to also assert a positive lower bound (e.g., minted_shares
>= 1) so the test enforces that a nonzero payment yields nonzero shares while
still preventing surplus capture; keep the failure message clear and reference
minted_shares in the message.
---
Outside diff comments:
In `@contract/vault/kernel/src/actions/mod.rs`:
- Around line 732-758: After computing shares_out via convert_to_shares_bounded,
add an early check that rejects zero-share deposits before mutating state: if
shares_out == 0 return an error (e.g., KernelError::Slippage { min:
min_shares_out, actual: shares_out }) so we never update state.total_assets,
state.idle_assets, state.total_shares or advance FeeAccrualAnchor::new when a
nonzero assets_in would mint zero shares; place this check immediately after the
convert_to_shares_bounded call and before any checked_add mutations.
🪄 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: 9befdd90-2370-4733-b7d9-515556c96d0e
📒 Files selected for processing (16)
contract/vault/kernel/src/actions/mod.rscontract/vault/kernel/src/actions/tests.rscontract/vault/kernel/src/error.rscontract/vault/kernel/src/lib.rscontract/vault/kernel/src/state/vault/mod.rscontract/vault/kernel/tests/property_tests.rscontract/vault/soroban/README.mdcontract/vault/soroban/shared-types/src/lib.rscontract/vault/soroban/src/contract/curator_vault.rscontract/vault/soroban/src/contract/entrypoints.rscontract/vault/soroban/src/contract/helpers.rscontract/vault/soroban/src/contract/mod.rscontract/vault/soroban/src/contract/types.rscontract/vault/soroban/src/fungible_vault.rscontract/vault/soroban/src/tests.rscontract/vault/soroban/tests/integration_tests.rs
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
contract/vault/soroban/tests/integration_tests.rs (1)
1862-1865:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAssert the exact post-donation mint here.
(1..=62)still lets severe under-minting regressions pass. In this setup the result is deterministic: after reconciling to 800 assets, a 100-asset deposit against 500 shares should mint exactly62shares, so this should be an equality assertion instead of a range.As per coding guidelines, "`**/tests/**/*.rs`: Write comprehensive unit tests for new, non-trivial Rust logic; prefer `#[rstest]` parameterization when it improves coverage and keeps cases readable`".Suggested patch
- assert!( - (1..=62).contains(&minted_shares), - "deposit minted {minted_shares} shares outside expected post-donation bounds" - ); + assert_eq!( + minted_shares, 62, + "deposit minted {minted_shares} shares instead of pricing against the reconciled donated NAV" + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contract/vault/soroban/tests/integration_tests.rs` around lines 1862 - 1865, The test currently uses a range assertion for minted_shares which allows under-minting to pass; replace the loose range check with an exact equality assertion (assert_eq!(minted_shares, 62)) so the test verifies the deterministic outcome after reconciling to 800 assets and depositing 100 assets against 500 shares; update any failure message accordingly to reflect the expected exact value and keep the assertion referencing the minted_shares variable.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@contract/vault/soroban/tests/integration_tests.rs`:
- Around line 1862-1865: The test currently uses a range assertion for
minted_shares which allows under-minting to pass; replace the loose range check
with an exact equality assertion (assert_eq!(minted_shares, 62)) so the test
verifies the deterministic outcome after reconciling to 800 assets and
depositing 100 assets against 500 shares; update any failure message accordingly
to reflect the expected exact value and keep the assertion referencing the
minted_shares variable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d036ebe8-aa3f-495c-bf99-3a30dccd8832
📒 Files selected for processing (4)
contract/vault/kernel/src/actions/mod.rscontract/vault/kernel/src/actions/tests.rscontract/vault/soroban/src/contract/entrypoints.rscontract/vault/soroban/tests/integration_tests.rs
…0-4470-9921-b7ffe2741079) Trace: 1227f46
…12c4-99e3-4abf-8188-2c4083600cd9) Trace: a258c5f
…61637-ec7c-4abf-b964-b51d18988521) Trace: 90413f2
…f85-0147-424e-91c3-a9dd6cea1f6e) Trace: 9c1cf2c
…921-b7ffe2741079) Trace: 2c7def2
…3-4abf-8188-2c4083600cd9) Trace: 4b4232f
…0261637-ec7c-4abf-b964-b51d18988521) Trace: abfd943
…7-424e-91c3-a9dd6cea1f6e) Trace: 12916e1
…bec-769c-4475-b256-ca22013de56b) Trace: 9febe56
…5d-594c-4565-9b5e-726657416c19) Trace: 3163b84
…65-9b5e-726657416c19) Trace: 236acee
…fa-595b-42a3-a1a0-cf7cba5e3f93) Trace: d31bf3c
…8 Nexus 20f09f85-0147-424e-91c3-a9dd6cea1f6e Nexus d0261637-ec7c-4abf-b964-b51d18988521 Nexus 68a5a45d-594c-4565-9b5e-726657416c19)
carrion256
force-pushed
the
audit/fee-anchor-lifecycle-spec
branch
from
8b940a2 to
3c4abaf
Compare
…9 Nexus ca9eb389-f5c4-41b6-88cb-57ba837928f4 Nexus fb2a0ba4-33cc-48b2-98bc-1f090dfa0391 Nexus 16754f09-e5da-4838-81d9-1ded6598f171) Normalize legacy zero fee anchors during migration, make zero-anchor donation growth non-feeable until the anchor is advanced, and replace unchecked fee-policy account indexing with checked input validation. Verification: cargo fmt --all; CARGO_INCREMENTAL=0 CARGO_TARGET_DIR=/data/tmp/contracts-fee-a021-target cargo test -p templar-vault-kernel refresh_fees_zero_anchor_excludes_uncapped_donation_growth -- --nocapture; CARGO_INCREMENTAL=0 CARGO_TARGET_DIR=/data/tmp/contracts-fee-a021-target cargo test -p templar-soroban-runtime soroban_contract_proxy_view_does_not_inflate_from_zero_fee_anchor -- --nocapture; CARGO_INCREMENTAL=0 CARGO_TARGET_DIR=/data/tmp/contracts-fee-a021-target cargo test -p templar-soroban-runtime soroban_contract_refresh_fees_command_updates_anchor -- --nocapture; git diff --check.
carrion256
force-pushed
the
audit/fee-anchor-lifecycle-spec
branch
from
acfd8e7 to
aeba3fc
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
6c6f1936-9740-4470-9921-b7ffe2741079: Soroban deposits now advance the fee anchor to post-deposit assets and deposit timestamp.e95e12c4-99e3-4abf-8188-2c4083600cd9: zero(0,0)fee anchors are treated as uninitialized instead of minting fees against existing supply.d0261637-ec7c-4abf-b964-b51d18988521: direct underlying transfers are reconciled before pricing so donated idle balance cannot be captured by the next depositor.20f09f85-0147-424e-91c3-a9dd6cea1f6e:RefreshFeesis reachable through the Soroban command ABI.0659bbec-769c-4475-b256-ca22013de56b:ResyncIdleBalancekeepsfee_anchor.total_assetsandfee_anchor.timestamp_nscoherent, so later fee refreshes use the resync observation time.68a5a45d-594c-4565-9b5e-726657416c19: share/asset conversion and fee-mint paths now bound full-width quotients against operation caps before converting tou128.21aa4dfa-595b-42a3-a1a0-cf7cba5e3f93: virtual conversion offsets are locked after capitalization / first public deposit, including later full unwind to zero state.Notes
The first four commits are the original red/spec commits for the audited invariants. The production fixes are kept one commit per Nexus ID for auditor traceability. Commit
3163b84pins the A-031 red specs; commit236aceeis the A-031 remediation commit.The Soroban runtime now uses a lazy actual-balance reconciliation policy for preview-sensitive and pricing-sensitive paths.
DepositWithMin,RefreshFees, andResyncIdleBalanceread the live asset token balance, reconcile idle assets, recompute total assets, and reset the fee anchor onto the reconciled baseline before continuing. This is documented incontract/vault/soroban/README.md.A-031 uses bounded-domain math rather than saturation: deposit, withdrawal request planning, atomic withdraw/redeem, fee-share minting, and Soroban
proxy_viewpreviews compare the fullNumberquotient against the operation legal cap beforeu128conversion. Overflowing actions fail closed with operation-specific errors; signed ABI previews reject values abovei128::MAX.A-039 is folded into this fee-anchor cluster PR from superseded PR #435. The Soroban runtime now rejects
SetGovernanceConfig(VIRTUAL_OFFSETS)while stored vault accounting is nonzero, persists aVirtualOffsetsLockedinstance flag after the first successful public deposit, and keeps virtual offsets immutable even after a later full unwind to zero state.Verification
cargo fmt --all -- --checkgit diff --checkCARGO_TARGET_DIR=/data/projects/templar/contracts/.shared-target/fee-anchor-spec cargo test -p templar-vault-kernel --lib -- --nocapture(302 passed)CARGO_TARGET_DIR=/data/projects/templar/contracts/.shared-target/fee-anchor-spec cargo test -p templar-soroban-shared-types -- --nocapture(6 passed)CARGO_INCREMENTAL=0 CARGO_TARGET_DIR=/data/projects/templar/contracts/.shared-target/fee-anchor-spec cargo test -p templar-vault-kernel prop_a031 --test property_tests -- --nocapture(2 passed)CARGO_INCREMENTAL=0 CARGO_TARGET_DIR=/data/projects/templar/contracts/.shared-target/fee-anchor-spec cargo test -p templar-soroban-runtime soroban_contract_resync_idle_balance_anchors_fee_refresh_window --test integration_tests -- --nocapture(1 passed)CARGO_INCREMENTAL=0 CARGO_TARGET_DIR=/data/projects/templar/contracts/.shared-target/fee-anchor-spec cargo test -p templar-soroban-runtime --test integration_tests -- --nocapture(56 passed)CARGO_INCREMENTAL=0 CARGO_TARGET_DIR=/data/projects/templar/contracts/.shared-target/fee-anchor-spec just -f contract/vault/soroban/justfile size-budget-check(passed, deploy wasm 95397 bytes)CARGO_INCREMENTAL=0 CARGO_TARGET_DIR=/data/tmp/contracts-audit-fee-anchor-spec/.target-a039-fold cargo test -p templar-soroban-runtime virtual_offset -- --nocapture(passed)CARGO_INCREMENTAL=0 CARGO_TARGET_DIR=/data/tmp/contracts-audit-fee-anchor-spec/.target-a039-fold cargo test -p templar-soroban-runtime test_phase1_deposit_with_min_resource_probe -- --nocapture(passed)CARGO_INCREMENTAL=0 CARGO_TARGET_DIR=/data/tmp/contracts-audit-fee-anchor-spec/.target-a039-fold cargo test -p templar-soroban-runtime -- --nocapture(passed)CARGO_INCREMENTAL=0 CARGO_TARGET_DIR=/data/tmp/contracts-audit-fee-anchor-spec/.target-a039-fold just -f contract/vault/soroban/justfile size-budget-check(passed, deploy wasm 95716 bytes)Tracker context:
contracts-audit-clusters/fee-anchor-lifecycle.audit.md.Halborn Finding IDs
Included for Halborn SSC GitHub remediation detection:
6c6f1936-9740-4470-9921-b7ffe2741079— Soroban deposits do not advance fee accrual anchor while NEAR doese95e12c4-99e3-4abf-8188-2c4083600cd9— Initial fee anchor at zero inflatesproxy_viewonce fees are enabledd0261637-ec7c-4abf-b964-b51d18988521— Direct underlying transfers are mispriced until manual resync20f09f85-0147-424e-91c3-a9dd6cea1f6e— Fee accrual is unreachable from the deployed Soroban ABI0659bbec-769c-4475-b256-ca22013de56b—resync_idle_balanceadvancesfee_anchor.total_assetswithout updatingtimestamp_ns68a5a45d-594c-4565-9b5e-726657416c19— Share-conversion helpers truncate 256-bit quotients to u12821aa4dfa-595b-42a3-a1a0-cf7cba5e3f93— Virtual conversion offsets can be changed after the vault is capitalizedThis change is