feat(proxy-curator): add Soroban curator operations proxy#419
Conversation
|
Warning Review limit reached
More reviews will be available in 8 minutes and 31 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (9)
📝 WalkthroughWalkthroughAdds a new Soroban crate ChangesSoroban Proxy Curator Contract
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 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 |
1e4a952 to
e56db7e
Compare
|
@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. It's pertinent for you to compare the external near runtime for the vaults with the soroban runtime. Now we have some codegen room the goal for this proxy contract is to have a cleaner interface into the vaults for curators, rather than the obtuse, opaque interface. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me gather the codebase details first. 🏁 Script executed: Length of output: 377 🏁 Script executed: Length of output: 4370 🏁 Script executed: Length of output: 40718 🏁 Script executed: Length of output: 3595 🏁 Script executed: Length of output: 17921
PR
|
| Surface | Methods | Destination |
|---|---|---|
| Allocator ops | allocate, refresh_markets, resync_idle_balance, extend_vault_ttl, cancel_migration |
vault via binary codec |
| Governance ops | set_paused, set_curator, set_fees, set_restrictions, submit_cap, submit_cap_group_update, accept_*, revoke_*, abdicate, … (~30 methods) |
governance contract via named calls |
| Composite views | vault_view → VaultView, preview → VaultPreview, governance_view → GovernanceView |
vault (proxy_view) + governance |
NEAR vs. Soroban Runtime Comparison
The proxy's existence is motivated by fundamental differences between the two runtimes:
| NEAR Vault | Soroban Vault | |
|---|---|---|
| Execution model | Async (Promise/callback chains, multi-transaction) | Synchronous (all effects atomic in one tx) |
| Curator API location | Embedded in the vault itself (VaultExternalInterface) — curators call the vault directly |
Vault has a single opaque binary execute(Bytes) endpoint; governance is a separate contract |
| Market identity | AccountId (human-readable NEAR account string) |
u32 integer IDs |
| Auth | predecessor_account_id() checked against RBAC roles inline |
address.require_auth() called on the typed Address argument |
| Governance timelocks | Embedded in the vault, same contract | Separate governance contract with its own ABI |
| Proxy needed? | No — named typed methods directly on vault | Yes — curator must otherwise know the binary codec and juggle two contract addresses |
On NEAR, the vault IS the curator interface. A curator calls submit_cap(market, new_cap) directly on the vault by account ID. On Soroban, the equivalent requires: encoding a VaultCommand::BeginAllocating { ... } struct using a hand-rolled byte codec, calling execute(encoded_bytes) on the vault, AND calling submit_set_cap(admin, market_id, new_cap) on a separate governance contract. The proxy eliminates this entirely.
Design Strengths
-
AllocationDeltaenum is a clear ergonomic win over the raw(market, amount, supply: bool)triple —AllocationDelta::Supply(7, 500)vsAllocationDelta::Withdraw(7, 500)reads naturally and prevents thesupplybool ambiguity. -
Composite view structs (
VaultView,VaultPreview,GovernanceView) aggregate data from multiple sources into single, richly typed read calls. The raw vault/governance APIs don't offer this. -
Auth delegation is correct — the proxy calls
allocator.require_auth()/admin.require_auth()before forwarding, respecting Soroban's caller-authorizes model. -
Test coverage is layered well: unit tests use mock vault/governance contracts to verify encoding; integration tests exercise the real vault + real governance contracts.
Design Concerns
1. Error information is lost at both boundaries.
invoke_governance collapses the entire GovernanceError enum into a single ContractError::GovernanceError. A timelock-not-elapsed violation, an unauthorized call, and an unknown proposal ID all surface identically to the caller. The same happens for vault errors (collapsed to VaultError). This makes programmatic error handling by curator tooling awkward — callers must go look at governance/vault events to understand what actually failed.
2. from_vault_error_code is a brittle numeric mapping.
pub(crate) const fn from_vault_error_code(code: u32) -> Self {
match code {
3 => Self::InvalidInput,
8 => Self::AlreadyInitialized,
_ => Self::VaultError,
}
}This maps raw u32 contract error codes to proxy errors. If the vault error enum is ever reordered or a new variant is inserted, this mapping silently breaks with no compile-time check. A re-export of the vault's error type, or at minimum a #[doc] comment naming the vault error variants being mapped, would reduce fragility here.
3. ProxyCoreView is a deeply nested anonymous tuple — positional fragility.
pub(crate) type ProxyCoreView = (
(Address, Address, Address, Address),
(i128, i128, bool),
(i128, i128, i128, i128),
(i128, u64, i128, i128),
);The vault_view_from_response unpacks this positionally (virtuals.0, totals.2, fees.1, etc.). If the vault adds a field or changes ordering in proxy_view, this breaks silently at runtime with misread values rather than a compile error. A named struct in shared-types for this response would give compile-time safety.
4. governance_view() makes 5 separate cross-contract calls.
admin, guardian, sentinel, timelocks, and pending_ids are dispatched as five independent try_invoke_contract calls. If the governance contract exposed a single governance_state() -> GovernanceView-style aggregation, this would be one call. This matters both for cost and for consistency (the view reflects 5 different moments in the ledger state if any of those calls trigger re-entrancy-adjacent side effects).
5. No TTL management for the proxy or governance contracts.
extend_vault_ttl covers the vault's instance storage TTL, but there are no extend_proxy_ttl or extend_governance_ttl equivalents. On Stellar mainnet all three contracts have TTLs. If the proxy or governance contracts expire, curator operations will fail with unhelpful errors. Either additional methods or a single extend_all_ttls sweeping all three addresses would be safer.
6. submit_other / revoke_other_pending / check_other are untyped escape hatches.
pub fn submit_other(env: Env, admin: Address, key: Symbol, payload_hash: BytesN<32>) -> Result<u64, ContractError>These methods reintroduce the opacity the proxy is supposed to eliminate. Curators using these must independently know what key means and compute the right payload_hash. This feels like it belongs in the governance contract's own ABI rather than being surfaced here, unless there is a concrete known use case requiring it at the proxy layer.
7. No accept_restrictions or accept_sentinel fast-paths.
The NEAR vault has accept_sentinel() / revoke_pending_sentinel() as immediate (non-ID-keyed) calls. The Soroban governance uses the accept(admin, proposal_id) pattern, but the proxy also exposes accept_sentinel, accept_allocators, accept_fees, etc. as separate methods. This is consistent with the governance ABI but means the accept(id) generic method and accept_fees() etc. are both present as parallel paths to the same outcome — it's worth confirming these are not accidentally doubled up.
🧠 Learnings used
Learnt from: peer2f00l
Repo: Templar-Protocol/contracts PR: 404
File: contract/vault/kernel/src/actions/mod.rs:1869-1871
Timestamp: 2026-04-14T07:35:09.869Z
Learning: In `contract/vault/kernel/src/actions/mod.rs` (Templar-Protocol/contracts, Rust), the vault kernel crate is compiled for Soroban (wasm32 target). Extracting helper functions from large dispatcher functions like `dispatch::apply_action` risks increasing wasm binary size due to additional call frames and reduced compiler inlining. Therefore, `#[allow(clippy::too_many_lines)]` suppressions on large dispatch/entrypoint functions in this file are intentional and should not be flagged as needing refactoring.
Learnt from: CR
Repo: Templar-Protocol/contracts PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-19T05:08:26.156Z
Learning: Read `contract/vault/README.md` and `contract/vault/STRIDE.md` before changing the vault contract
Learnt from: peer2f00l
Repo: Templar-Protocol/contracts PR: 411
File: contract/market/examples/config/v1/linear-usdt/proxy-collateral.json:21-23
Timestamp: 2026-04-20T03:51:24.169Z
Learning: In `contract/market/examples/config/v1/linear-usdt/proxy-collateral.json`, the `ft_price` method on `linear-protocol.near` is a valid on-chain view method. Do not flag `method_name: "ft_price"` on `account_id: "linear-protocol.near"` as non-existent in future reviews. The method exists on the deployed contract (verified via nearblocks.io) even though it is not prominently documented in the LiNEAR GitHub README.
![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
🤖 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/proxy-curator-soroban/src/contract.rs`:
- Around line 142-160: The initialize function is unsecured: anyone can call
initialize() and set ProxyDataKey::VaultAddress/ProxyDataKey::GovernanceAddress;
fix by gating initialize with an authorization check and validating targets —
add/read an admin/owner entry (e.g., ProxyDataKey::Admin) and require the caller
(env.invoker()) equals that admin (or only allow the contract deployer) before
writing storage, and additionally validate that vault_address and
governance_address are actual contract addresses (use the Address contract/host
APIs or a lightweight call to confirm contract code) before setting
ProxyDataKey::VaultAddress and ProxyDataKey::GovernanceAddress; keep the
existing is_initialized() guard but perform the auth and address-validation
checks first.
In `@contract/vault/soroban/governance/src/lib.rs`:
- Around line 394-420: The implementation of set_is_allocator currently reads
only DataKey::CurrentAllocators (committed state) and then always replaces the
pending SetAllocators proposal via
Self::submit(GovernanceAction::SetAllocators(...)), which causes successive
admin calls to overwrite rather than compose; update set_is_allocator to first
load the existing pending allocator proposal (the pending SetAllocators action
if present), merge changes into that pending set (add or remove the account),
deduplicate with dedup_addresses, and then submit the updated SetAllocators
payload so successive calls compose; reference the existing
DataKey::CurrentAllocators read, the dedup_addresses helper, and the
GovernanceAction::SetAllocators submission to locate where to change the logic.
🪄 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: 06bac26b-846c-47c4-b051-48cae762d8fc
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
.gitignoreCargo.tomlcontract/proxy-curator-soroban/Cargo.tomlcontract/proxy-curator-soroban/src/contract.rscontract/proxy-curator-soroban/src/error.rscontract/proxy-curator-soroban/src/lib.rscontract/proxy-curator-soroban/src/tests.rscontract/proxy-curator-soroban/tests/integration_tests.rscontract/vault/soroban/AGENTS.mdcontract/vault/soroban/governance/Cargo.tomlcontract/vault/soroban/governance/src/lib.rscontract/vault/soroban/governance/src/tests.rscontract/vault/soroban/governance/src/types.rscontract/vault/soroban/justfile
e56db7e to
9c8572d
Compare
3925485 to
9b352cf
Compare
9c8572d to
2da85b1
Compare
9b352cf to
57e1f69
Compare
2da85b1 to
dc019ba
Compare
b4f1402 to
411d3fd
Compare
ab7f226 to
9258b6b
Compare
91fcfc6 to
6db2fc4
Compare
e2571b8 to
4a85094
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/proxy-curator-soroban/src/contract.rs`:
- Around line 700-707: Current governance_view aggregates state by calling
admin, sentinel, timelocks_from_governance_scalars (which in turn calls
timelock_ns(kind) many times) and pending_ids, causing ~22 cross-contract calls
and potential inconsistent snapshots; add a new aggregated endpoint on the
governance contract (e.g., governance_state) that returns (admin, sentinel,
timelocks, pending_ids) in a single cross-contract call, then update
governance_view to call governance_state and unpack its returned tuple (or keep
governance_view as a thin wrapper around governance_state) so callers get a
single-call consistent snapshot and dramatically lower call cost; reference
governance_view, timelocks_from_governance_scalars, timelock_ns, admin,
sentinel, and pending_ids when making the change.
- Around line 495-507: Add concise inline documentation to the untyped
escape-hatch methods (submit_other, revoke_other_pending, check_other)
explaining their intended use cases (when typed APIs are insufficient and
governance must submit/verify opaque actions), when callers should prefer typed
submission methods, the expected Symbol key naming conventions and how to
construct the BytesN<32> payload_hash (e.g., canonical hashing scheme or
serialization expectations), and an explicit warning that misuse can create
unverifiable/unsafe governance state; apply the same documentation style to the
other occurrences of these methods to keep guidance consistent.
- Around line 18-26: The current tuple type aliases (ProxyCoreView,
ProxyPolicyView, ProxyPreviewView) are fragile; replace them with explicit
#[contracttype] structs (fields named exactly as in the review: curator,
governance, asset_token, share_token, virtual_shares, virtual_assets, paused,
total_shares, idle_assets, external_assets, total_assets,
fee_anchor_total_assets, fee_anchor_timestamp_ns, management_fee_wad,
performance_fee_wad, max_growth_rate_wad for core; supply_queue and cap_groups
for policy; the eight preview fields for preview) placed in a shared module
(templar-soroban-shared-types or new proxy-types module), change the vault
RPC/contract signature to return the structs (e.g. (ProxyCoreView,
ProxyPolicyView, ProxyPreviewView)), update the type aliases in this file to use
those structs, and refactor vault_view_from_response to stop positional tuple
unpacking and instead read fields by name (or destructure the named structs) and
update all imports/usages accordingly.
In `@contract/proxy-curator-soroban/src/tests.rs`:
- Around line 421-433: Add an explicit length assertion for the recorded
payloads before using unchecked indexing: check the number of items returned by
fixture.recorded_payloads() (e.g., assert_eq!(payloads.len(), 3) or
assert!(payloads.len() >= 3)) immediately after let payloads =
fixture.recorded_payloads(), then keep the existing
decode_command(&payloads.get_unchecked(...)) checks for
WireVaultCommand::ResyncIdleBalance, WireVaultCommand::ExtendTtl, and
WireVaultCommand::CancelMigration to avoid panics and make failures diagnostic.
🪄 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: 65b5ceea-a0a3-42a7-bba2-073c87af0748
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
Cargo.tomlcontract/proxy-curator-soroban/Cargo.tomlcontract/proxy-curator-soroban/src/contract.rscontract/proxy-curator-soroban/src/error.rscontract/proxy-curator-soroban/src/lib.rscontract/proxy-curator-soroban/src/tests.rscontract/proxy-curator-soroban/tests/integration_tests.rs
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
02ec0e8 to
8e4d21d
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
contract/proxy-curator-soroban/src/tests.rs (1)
422-433:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd an explicit length check before
get_uncheckedreads.Without a count assertion, Line 424/428/432 can panic before showing what failed when fixture behavior changes.
Suggested patch
let payloads = fixture.recorded_payloads(); + assert_eq!(payloads.len(), 3, "expected exactly 3 recorded vault commands"); assert_eq!( decode_command(&payloads.get_unchecked(0)), WireVaultCommand::ResyncIdleBalance🤖 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/proxy-curator-soroban/src/tests.rs` around lines 422 - 433, Add an explicit length/count assertion on the payloads returned by fixture.recorded_payloads() before calling payloads.get_unchecked(...) to avoid panics and improve diagnostics; for example, check payloads.len() or assert_eq!(payloads.len(), expected_count) prior to the three decode_command(payloads.get_unchecked(...)) calls (the assertions involving WireVaultCommand::ResyncIdleBalance, ::ExtendTtl and ::CancelMigration) so failures report a clear mismatch in number of payloads rather than an unchecked index panic.
🤖 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/proxy-curator-soroban/src/error.rs`:
- Around line 26-31: The match in from_vault_error_code currently uses
hard-coded numeric discriminants (3, 8); replace these magic numbers with named
constants or the VaultError enum discriminants imported from the vault crate (or
a shared module) so the mapping is type-safe and will break at compile time if
vault error values change; if a direct enum import is not available, introduce
shared consts (e.g., VAULT_ERR_INVALID_INPUT, VAULT_ERR_ALREADY_INITIALIZED) in
a common crate and use them here, and add a cross-contract regression test that
asserts the proxy mapping (from_vault_error_code) matches the vault's error
codes to catch drift.
In `@contract/proxy-curator-soroban/tests/integration_tests.rs`:
- Around line 16-98: The tests currently rely on
env.mock_all_auths_allowing_non_root_auth() (in setup_harness) which hides
authorization failures; add a new integration test that attempts to call the
privileged proxy method set_sentinel from an unauthorized address and asserts it
fails. Implement this by creating a test (e.g.,
proxy_set_sentinel_rejected_for_unauthorized) that builds a harness without
calling mock_all_auths_allowing_non_root_auth (or add a setup_harness_no_mock
variant), registers the same contracts (vault, governance, proxy), then call
invoke_contract on harness.proxy with Symbol::new(..., "set_sentinel") using a
non-admin caller and assert the invocation returns a failure/revert (or panics)
rather than producing a proposal id; reference
setup_harness/mock_all_auths_allowing_non_root_auth, set_sentinel,
invoke_contract, proxy, and governance to locate where to change/add the test.
---
Duplicate comments:
In `@contract/proxy-curator-soroban/src/tests.rs`:
- Around line 422-433: Add an explicit length/count assertion on the payloads
returned by fixture.recorded_payloads() before calling
payloads.get_unchecked(...) to avoid panics and improve diagnostics; for
example, check payloads.len() or assert_eq!(payloads.len(), expected_count)
prior to the three decode_command(payloads.get_unchecked(...)) calls (the
assertions involving WireVaultCommand::ResyncIdleBalance, ::ExtendTtl and
::CancelMigration) so failures report a clear mismatch in number of payloads
rather than an unchecked index panic.
🪄 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: 1712c293-c629-455e-8941-af4f286e7bdb
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
Cargo.tomlcontract/proxy-curator-soroban/Cargo.tomlcontract/proxy-curator-soroban/src/contract.rscontract/proxy-curator-soroban/src/error.rscontract/proxy-curator-soroban/src/lib.rscontract/proxy-curator-soroban/src/tests.rscontract/proxy-curator-soroban/tests/integration_tests.rs
e95be44 to
30a2246
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (4)
contract/proxy-curator-soroban/tests/integration_tests.rs (2)
16-98: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winGlobal auth mocking is hiding authorization regressions in privileged-path integration tests.
These tests only assert happy paths under
mock_all_auths_allowing_non_root_auth; they won't catch missing/changed auth checks on privileged proxy/governance calls.Add one negative integration case (unauthorized caller) for a privileged method like
set_sentinel, and assert it fails.As per coding guidelines: "Write comprehensive unit tests for new, non-trivial Rust logic; prefer
#[rstest]parameterization when it improves coverage and keeps cases readable".
50-98: 🧹 Nitpick | 🔵 Trivial | 🏗️ Heavy liftExpand test coverage to include error paths and governance lifecycle.
Current tests only verify happy-path flows (proposal submission, view queries). Consider adding:
- Error-path tests: Governance proposal rejection scenarios, vault command failures
- Lifecycle tests: Proposal accept/revoke/abdicate flows beyond just submission
- View aggregation edge cases: Empty/missing governance data
As per coding guidelines: "Write comprehensive unit tests for new, non-trivial Rust logic; prefer
#[rstest]parameterization when it improves coverage and keeps cases readable".🤖 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/proxy-curator-soroban/tests/integration_tests.rs` around lines 50 - 98, Add tests covering governance error paths and lifecycle events by extending the existing test module that contains proxy_submits_sentinel_change_through_real_governance_contract and proxy_exposes_governance_views: write new tests that call harness.env.invoke_contract against the proxy with failing proposal inputs (e.g., submit a proposal that should be rejected and assert it returns an error or non-accepted proposal_id), exercise accept/revoke/abdicate flows by invoking the governance entrypoints (use the same Symbol::new(&harness.env, "…") pattern for names like "set_sentinel", "accept_proposal", "revoke_proposal", "abdicate"), assert storage changes (VaultDataKey::Sentinel via harness.env.as_contract(&harness.vault, || { ... })) and view results (governance/admin) for both populated and empty/missing governance state; parameterize repetitive cases with #[rstest] to cover success/failure permutations and keep assertions against invoke_contract returns (Option/Result/Address/u64) consistent with existing tests.contract/proxy-curator-soroban/src/error.rs (1)
26-31:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAvoid hard-coded vault error discriminants in proxy mapping.
The match in
from_vault_error_codeuses raw codes3and8. If vault error numbering shifts, proxy classification silently degrades.As per coding guidelines: "Prefer stronger types over loosely constrained values in Rust."
🤖 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/proxy-curator-soroban/src/error.rs` around lines 26 - 31, The function from_vault_error_code should stop using raw literals 3 and 8 and instead map by converting the incoming code into the Vault error enum (or use VaultError's discriminant constants) so the mapping follows the VaultError type rather than magic numbers; update from_vault_error_code to attempt converting code -> VaultError (e.g., try_from/from_repr) and match on VaultError::InvalidInput and VaultError::AlreadyInitialized, falling back to Self::VaultError when conversion fails or for other variants, referencing the from_vault_error_code function and the VaultError enum in your changes.contract/proxy-curator-soroban/src/contract.rs (1)
183-202:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftLock down proxy initialization.
initializehas no auth check and no validation that stored addresses are contracts. Any account can front-run the deployer and permanently point the proxy at arbitrary addresses.As per coding guidelines: "
initialize()is highly sensitive because front-running it would seize governance/curator control."🤖 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/proxy-curator-soroban/src/contract.rs` around lines 183 - 202, initialize currently lacks authorization and address validation; update Proxy contract's initialize to (1) require it is called only by the deployer/owner (compare Env::invoker()/caller to the contract's deployer or a stored admin value) and return Unauthorized if not, (2) verify both vault_address and governance_address are contract addresses (use Address contract-type check / is_contract helper) before storing them, and (3) keep the existing is_initialized check and set ProxyDataKey::Initialized only after validations succeed; use the function name initialize and keys ProxyDataKey::VaultAddress, ProxyDataKey::GovernanceAddress, ProxyDataKey::Initialized to locate and modify the code.
🤖 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/proxy-curator-soroban/src/contract.rs`:
- Around line 183-202: initialize currently lacks authorization and address
validation; update Proxy contract's initialize to (1) require it is called only
by the deployer/owner (compare Env::invoker()/caller to the contract's deployer
or a stored admin value) and return Unauthorized if not, (2) verify both
vault_address and governance_address are contract addresses (use Address
contract-type check / is_contract helper) before storing them, and (3) keep the
existing is_initialized check and set ProxyDataKey::Initialized only after
validations succeed; use the function name initialize and keys
ProxyDataKey::VaultAddress, ProxyDataKey::GovernanceAddress,
ProxyDataKey::Initialized to locate and modify the code.
In `@contract/proxy-curator-soroban/src/error.rs`:
- Around line 26-31: The function from_vault_error_code should stop using raw
literals 3 and 8 and instead map by converting the incoming code into the Vault
error enum (or use VaultError's discriminant constants) so the mapping follows
the VaultError type rather than magic numbers; update from_vault_error_code to
attempt converting code -> VaultError (e.g., try_from/from_repr) and match on
VaultError::InvalidInput and VaultError::AlreadyInitialized, falling back to
Self::VaultError when conversion fails or for other variants, referencing the
from_vault_error_code function and the VaultError enum in your changes.
In `@contract/proxy-curator-soroban/tests/integration_tests.rs`:
- Around line 50-98: Add tests covering governance error paths and lifecycle
events by extending the existing test module that contains
proxy_submits_sentinel_change_through_real_governance_contract and
proxy_exposes_governance_views: write new tests that call
harness.env.invoke_contract against the proxy with failing proposal inputs
(e.g., submit a proposal that should be rejected and assert it returns an error
or non-accepted proposal_id), exercise accept/revoke/abdicate flows by invoking
the governance entrypoints (use the same Symbol::new(&harness.env, "…") pattern
for names like "set_sentinel", "accept_proposal", "revoke_proposal",
"abdicate"), assert storage changes (VaultDataKey::Sentinel via
harness.env.as_contract(&harness.vault, || { ... })) and view results
(governance/admin) for both populated and empty/missing governance state;
parameterize repetitive cases with #[rstest] to cover success/failure
permutations and keep assertions against invoke_contract returns
(Option/Result/Address/u64) consistent with existing tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2c6f8e17-8a9a-4cd2-94e9-bbe11b694d28
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
Cargo.tomlcontract/proxy-curator-soroban/Cargo.tomlcontract/proxy-curator-soroban/src/contract.rscontract/proxy-curator-soroban/src/error.rscontract/proxy-curator-soroban/src/lib.rscontract/proxy-curator-soroban/src/tests.rscontract/proxy-curator-soroban/tests/integration_tests.rscontract/vault/soroban/governance/src/lib.rs
30a2246 to
95e411b
Compare
There was a problem hiding this comment.
Warning
CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.
Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.
Actionable comments posted: 2
♻️ Duplicate comments (3)
contract/proxy-curator-soroban/tests/integration_tests.rs (1)
118-141: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winThis still doesn't exercise proxy-level auth enforcement.
mock_authsauthorizes the attacker for the fullset_sentinel -> submit_set_sentinelcall tree, so the only thing this test proves is that governance rejects a non-admin caller. If the proxy entrypoint dropped its owncaller.require_auth(), this test would still pass. Add a second case that invokesset_sentinelwithout mocking attacker auth and assert the proxy call itself is rejected. As per coding guidelines: "contract/vault/soroban/**/*.rs: Review auth on every privileged Soroban entrypoint. Do not rely on outer routing alone" and "**/tests/**/*.rs: Write comprehensive unit tests for new, non-trivial Rust logic".🤖 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/proxy-curator-soroban/tests/integration_tests.rs` around lines 118 - 141, The test only covers governance rejecting a non-admin because mock_auths authorizes the full set_sentinel -> submit_set_sentinel call tree; add a second assertion that invokes harness.proxy's set_sentinel via harness.env.try_invoke_contract without setting harness.env.mock_auths for the attacker (i.e., no MockAuth/MockAuthInvoke), and assert the call is rejected at the proxy level (verify the returned Err corresponds to an auth/unauthorized failure for set_sentinel), ensuring proxy-level caller.require_auth() is enforced.Source: Coding guidelines
contract/proxy-curator-soroban/src/contract.rs (2)
701-707:⚠️ Potential issue | 🟠 MajorUse aggregated timelock retrieval to avoid stitched governance snapshots.
Line 705 builds
timelocksvia many scalar cross-contract calls (timelocks_from_governance_scalars), which increases cost and can return an inconsistent mixed snapshot if state changes between calls.This can be replaced with a single governance
timelockscall.Also applies to: 900-948
🤖 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/proxy-curator-soroban/src/contract.rs` around lines 701 - 707, The governance_view function (and the other similar block around lines 900-948) currently builds timelocks by calling timelocks_from_governance_scalars which makes many scalar cross-contract reads and can produce an inconsistent stitched snapshot and higher costs; replace that logic with a single aggregated governance call that returns the full timelocks map (i.e., invoke the governance contract's timelocks getter instead of timelocks_from_governance_scalars), deserialize that single response into the GovernanceView.timelocks field, and remove the multiple scalar-read helper to ensure atomicity and lower gas usage (update call site in governance_view and the analogous function/range referenced in the diff).
184-202:⚠️ Potential issue | 🔴 CriticalLock down initialization authority and target validation.
Line 184 exposes a first-caller-wins initializer that can permanently set arbitrary
vault_address/governance_address, and Lines 193-201 persist them without contract-address validation.As per coding guidelines:
initialize()is highly sensitive because front-running it would seize governance/curator control; deployment/initialization assumptions must be explicit.🤖 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/proxy-curator-soroban/src/contract.rs` around lines 184 - 202, The initialize function currently allows any first caller to set vault/governance addresses; restrict this by requiring a specific authority (e.g., the deployment/owner) before writing storage: in initialize, verify the caller (via Env's invoker/caller check) equals the intended admin/deployer, and keep the is_initialized(&env) guard. Also validate the provided vault_address and governance_address (non-empty, not equal to each other and optionally check they are contract addresses or match expected patterns) before calling env.storage().instance().set for ProxyDataKey::VaultAddress, ProxyDataKey::GovernanceAddress and ProxyDataKey::Initialized to prevent front‑running and invalid targets.Source: Coding guidelines
🤖 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/proxy-curator-soroban/src/contract.rs`:
- Around line 340-343: The two public entrypoints set_guardian and
set_is_allocator currently always return ContractError::NotImplemented, so
either implement their governance logic or remove them from the public ABI to
avoid false affordances; specifically, replace the placeholder bodies in the
functions set_guardian and set_is_allocator with the actual guardian/allocation
governance operations (validate admin perms, update storage, emit events) OR
remove their public exports from the contract interface (and any contract
macro/ABI registration) so they are no longer callable, and ensure no
proxy/typed-proxy layers rely on these symbols.
In `@contract/proxy-curator-soroban/src/tests.rs`:
- Around line 651-772: Add a test that exercises the ambiguity path by creating
two pending proposals that both match the same selector and asserting the proxy
returns a selector/ambiguity error rather than succeeding; specifically, in a
new or existing test (e.g., next to
selector_accept_wrappers_resolve_exact_pending_proposal) add two PendingProposal
entries with identical selector keys (for example two
GovernanceAction::SetCap(...) with the same cap id or two RemoveMarket(...) with
the same market id), then call the corresponding proxy wrapper
(SorobanCuratorProxyContract::accept_cap or ::revoke_pending_market_removal) and
assert it returns the selector ambiguity error instead of Ok, and also verify
the governance mock was not invoked
(MockGovernanceContract::last_accept/last_revoke remains None or unchanged).
---
Duplicate comments:
In `@contract/proxy-curator-soroban/src/contract.rs`:
- Around line 701-707: The governance_view function (and the other similar block
around lines 900-948) currently builds timelocks by calling
timelocks_from_governance_scalars which makes many scalar cross-contract reads
and can produce an inconsistent stitched snapshot and higher costs; replace that
logic with a single aggregated governance call that returns the full timelocks
map (i.e., invoke the governance contract's timelocks getter instead of
timelocks_from_governance_scalars), deserialize that single response into the
GovernanceView.timelocks field, and remove the multiple scalar-read helper to
ensure atomicity and lower gas usage (update call site in governance_view and
the analogous function/range referenced in the diff).
- Around line 184-202: The initialize function currently allows any first caller
to set vault/governance addresses; restrict this by requiring a specific
authority (e.g., the deployment/owner) before writing storage: in initialize,
verify the caller (via Env's invoker/caller check) equals the intended
admin/deployer, and keep the is_initialized(&env) guard. Also validate the
provided vault_address and governance_address (non-empty, not equal to each
other and optionally check they are contract addresses or match expected
patterns) before calling env.storage().instance().set for
ProxyDataKey::VaultAddress, ProxyDataKey::GovernanceAddress and
ProxyDataKey::Initialized to prevent front‑running and invalid targets.
In `@contract/proxy-curator-soroban/tests/integration_tests.rs`:
- Around line 118-141: The test only covers governance rejecting a non-admin
because mock_auths authorizes the full set_sentinel -> submit_set_sentinel call
tree; add a second assertion that invokes harness.proxy's set_sentinel via
harness.env.try_invoke_contract without setting harness.env.mock_auths for the
attacker (i.e., no MockAuth/MockAuthInvoke), and assert the call is rejected at
the proxy level (verify the returned Err corresponds to an auth/unauthorized
failure for set_sentinel), ensuring proxy-level caller.require_auth() is
enforced.
🪄 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: b615aefa-3708-44a2-8f35-13e4a8343b75
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
Cargo.tomlcontract/proxy-curator-soroban/Cargo.tomlcontract/proxy-curator-soroban/src/contract.rscontract/proxy-curator-soroban/src/error.rscontract/proxy-curator-soroban/src/lib.rscontract/proxy-curator-soroban/src/tests.rscontract/proxy-curator-soroban/tests/integration_tests.rscontract/vault/soroban/governance/src/lib.rscontract/vault/soroban/shared-types/src/lib.rs
95e411b to
37bbf23
Compare
There was a problem hiding this comment.
Warning
CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.
Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.
Actionable comments posted: 2
🤖 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/proxy-curator-soroban/src/contract.rs`:
- Around line 362-370: Change submit_timelock to accept TimelockKind directly
(remove Option<TimelockKind>) so callers cannot pass None; update the signature
of submit_timelock to take kind: TimelockKind, remove the immediate None check
and the Err(ContractError::InvalidInput) branching, and pass the now-nonoptional
kind into invoke_governance and any other internal uses. Also update any call
sites (tests/clients/other contract methods) that currently pass an Option to
supply a concrete TimelockKind value. Ensure no remaining references to treating
kind as Option remain (e.g., .ok_or or pattern matches for None).
- Around line 95-102: CapGroupUpdate exposes variants (RemoveCap,
RemoveRelativeCap, RemoveMember) that are never supported and lead to
ContractError::NotImplemented at runtime; change the API to make these invalid
states unrepresentable by removing those variants from the CapGroupUpdate enum
and any public uses or constructors that produce them, and update all match arms
and callers (e.g., the code paths that match on CapGroupUpdate in the contract
execution logic) to only handle the supported variants (SetCap, SetRelativeCap,
SetMember) or to use a separate internal enum if needed; ensure compilation
fixes by adjusting function signatures and tests that expect the removed
variants.
🪄 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: 01902523-1319-4a37-975c-6d59f5c7b80e
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
Cargo.tomlcontract/proxy-curator-soroban/Cargo.tomlcontract/proxy-curator-soroban/src/contract.rscontract/proxy-curator-soroban/src/error.rscontract/proxy-curator-soroban/src/lib.rscontract/proxy-curator-soroban/src/tests.rscontract/proxy-curator-soroban/tests/integration_tests.rscontract/vault/soroban/governance/src/lib.rscontract/vault/soroban/shared-types/src/lib.rs
9a6d1f2 to
43da198
Compare
1 participant
Stack:
This change is