feat(proxy-4626): add Soroban ERC-4626 proxy#415
Closed
carrion256 wants to merge 1 commit into
Closed
Conversation
Contributor
📝 WalkthroughSoroban ERC-4626 Proxy ImplementationOverviewAdds a new Soroban smart contract crate ( Key Changes
Critical Security Aspects1. Authorization & Allowance Pattern
2. Vault Command Serialization & Address Handling
3. Return Type Validation ✓
4. Slippage Protection ✓
5. Initialization & State Safety ✓
6. Token & Vault Contract Assumptions
Known Limitations
Recommendations for Reviewers
References
WalkthroughA new Soroban ERC-4626 proxy contract is introduced alongside its configuration, error handling, and comprehensive test suite. The contract routes deposit, mint, withdraw, and redeem operations through a configured vault abstraction while maintaining share token balances and enforcing authorization. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Proxy as Soroban4626Proxy
participant Vault as Vault Contract
participant Token as Share Token
Caller->>Proxy: deposit(assets, receiver)
activate Proxy
Proxy->>Proxy: authorize(caller)
Proxy->>Vault: proxy_view() [get preview]
Vault-->>Proxy: preview shares
Proxy->>Vault: execute(Deposit command)
activate Vault
Vault->>Vault: process deposit
Vault-->>Proxy: command result
deactivate Vault
Proxy->>Token: invoke share mint
Proxy->>Proxy: emit Deposit event
Proxy-->>Caller: return shares
deactivate Proxy
sequenceDiagram
participant Caller
participant Proxy as Soroban4626Proxy
participant Token as Share Token
participant Vault as Vault Contract
Caller->>Proxy: request_withdraw(shares, min_assets)
activate Proxy
Proxy->>Token: check allowance(contract)
Token-->>Proxy: allowance amount
Proxy->>Vault: execute(RequestWithdraw)
activate Vault
Vault->>Vault: queue withdrawal
Vault-->>Proxy: request_id
deactivate Vault
Proxy->>Proxy: emit Withdraw event
Proxy-->>Caller: return request_id
deactivate Proxy
Caller->>Proxy: execute_withdraw()
activate Proxy
Proxy->>Vault: execute(ExecuteWithdraw)
activate Vault
Vault->>Vault: process queued withdrawals
Vault-->>Proxy: success
deactivate Vault
Proxy-->>Caller: return unit
deactivate Proxy
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
Comment |
carrion256
force-pushed
the
refactor/extract-governance-proxy
branch
from
April 23, 2026 09:37
9ec3ebd to
1df2c0a
Compare
carrion256
force-pushed
the
refactor/extract-governance
branch
from
April 23, 2026 09:41
595c43a to
09b12ca
Compare
carrion256
force-pushed
the
refactor/extract-governance-proxy
branch
from
April 23, 2026 09:41
1df2c0a to
62a55fa
Compare
carrion256
force-pushed
the
refactor/extract-governance
branch
from
April 23, 2026 09:44
09b12ca to
ec781b5
Compare
carrion256
force-pushed
the
refactor/extract-governance-proxy
branch
from
April 23, 2026 09:44
62a55fa to
f9987f6
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: 5
🤖 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-4626-soroban/src/contract.rs`:
- Around line 281-304: The initialize function (initialize) currently lacks
access control so anyone can call it before initialization; add an authorization
check (e.g., require caller == deployer/admin) or accept an init-only-once
signer to prevent front-running: record or derive an admin/deployer address at
deployment (or add a parameter set by constructor) and check
env.invoker()/env.source() against it before setting ProxyDataKey::VaultAddress,
ProxyDataKey::AssetToken, ProxyDataKey::ShareToken and
ProxyDataKey::Initialized; alternatively document that initialize must be called
atomically in deployment scripts and add comments on is_initialized usage to
prevent misuse.
- Around line 472-492: The allowance check in require_auth_or_allowance is using
the proxy address as the spender, allowing any authenticated caller to act if
the proxy was approved; change the call_token_view_with_two_addresses invocation
so the allowance is fetched for (owner, caller) (i.e., use caller as the spender
argument instead of env.current_contract_address()/proxy) so the function
verifies owner approved this specific caller before returning Ok; keep the
existing caller.require_auth() and error handling for insufficient allowance.
In `@contract/proxy-4626-soroban/src/error.rs`:
- Around line 55-62: Replace the magic numbers in
ContractError::from_vault_error_code by defining named constants for the vault
error codes (e.g., VAULT_ERR_INVALID_INPUT and VAULT_ERR_ALREADY_INITIALIZED or
an enum/associated consts on ContractError or a VaultErrorCode type) and use
those constants in the match (instead of 3 and 8) so the mapping to
Self::InvalidInput and Self::AlreadyInitialized is explicit and maintainable;
update any related comments to reference the vault contract error meanings.
In `@contract/proxy-4626-soroban/src/lib.rs`:
- Around line 13-24: The tuple type aliases (ProxyCoreView, ProxyPolicyView,
ProxyPreviewView, ProxyViewResponse) are undocumented and hard to interpret; add
concise doc comments above each type alias describing the semantics of each
tuple position (e.g., for ProxyCoreView list what (0,0) through (0,3) represent,
what each element of the nested tuples means, and similarly enumerate
ProxyPolicyView and ProxyPreviewView positions), or add a single doc comment on
ProxyViewResponse that references the vault's proxy_view documentation and maps
tuple indices to named meanings so readers can quickly understand fields like
vault address, fee values, policy entries, and preview components.
In `@contract/proxy-4626-soroban/src/tests.rs`:
- Around line 489-524: Add a new unit test (e.g.,
test_redeem_fails_without_allowance) that mirrors
test_withdraw_fails_without_allowance but calls Soroban4626ProxyContract::redeem
via fixture.env.as_contract and verifies require_auth_or_allowance behavior:
create owner and receiver with Fixture and Address::generate, do not mock or
grant allowance for the caller, call redeem with some shares and preview config,
assert the call returns an Err indicating authorization/allowance failure
(matching the withdraw test's expected error), and assert no WireVaultCommand
payloads were recorded by fixture.recorded_payloads(); reference Fixture,
Address::generate, require_auth_or_allowance, Soroban4626ProxyContract::redeem,
and recorded_payloads to locate relevant helpers.
🪄 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: 5536db4d-610f-4434-81ce-329fcad74592
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
.gitignoreCargo.tomlcontract/proxy-4626-soroban/Cargo.tomlcontract/proxy-4626-soroban/src/contract.rscontract/proxy-4626-soroban/src/error.rscontract/proxy-4626-soroban/src/lib.rscontract/proxy-4626-soroban/src/tests.rscontract/proxy-4626-soroban/tests/integration_tests.rs
Comment on lines
+281
to
+304
| pub fn initialize( | ||
| env: Env, | ||
| vault_address: Address, | ||
| asset_token: Address, | ||
| share_token: Address, | ||
| ) -> Result<(), ContractError> { | ||
| if is_initialized(&env) { | ||
| return Err(ContractError::AlreadyInitialized); | ||
| } | ||
|
|
||
| env.storage() | ||
| .instance() | ||
| .set(&ProxyDataKey::VaultAddress, &vault_address); | ||
| env.storage() | ||
| .instance() | ||
| .set(&ProxyDataKey::AssetToken, &asset_token); | ||
| env.storage() | ||
| .instance() | ||
| .set(&ProxyDataKey::ShareToken, &share_token); | ||
| env.storage() | ||
| .instance() | ||
| .set(&ProxyDataKey::Initialized, &true); | ||
| Ok(()) | ||
| } |
Contributor
There was a problem hiding this comment.
Consider adding access control to initialize.
The initialize function can be called by anyone. While the AlreadyInitialized check prevents re-initialization, a malicious actor could front-run the deployment and initialize the proxy with arbitrary vault/token addresses, potentially redirecting funds.
Consider either:
- Adding an admin/deployer check
- Ensuring deployment and initialization are atomic (same transaction)
- Documenting that this is intentional and initialization should happen in the deployment script
Based on learnings: "Evaluate edge cases around... authorization... risks in smart contracts."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contract/proxy-4626-soroban/src/contract.rs` around lines 281 - 304, The
initialize function (initialize) currently lacks access control so anyone can
call it before initialization; add an authorization check (e.g., require caller
== deployer/admin) or accept an init-only-once signer to prevent front-running:
record or derive an admin/deployer address at deployment (or add a parameter set
by constructor) and check env.invoker()/env.source() against it before setting
ProxyDataKey::VaultAddress, ProxyDataKey::AssetToken, ProxyDataKey::ShareToken
and ProxyDataKey::Initialized; alternatively document that initialize must be
called atomically in deployment scripts and add comments on is_initialized usage
to prevent misuse.
Comment on lines
+472
to
+492
| pub(crate) fn require_auth_or_allowance( | ||
| env: &Env, | ||
| caller: &Address, | ||
| owner: &Address, | ||
| token: &Address, | ||
| amount: i128, | ||
| ) -> Result<(), ContractError> { | ||
| if caller == owner { | ||
| owner.require_auth(); | ||
| return Ok(()); | ||
| } | ||
|
|
||
| caller.require_auth(); | ||
| let proxy = env.current_contract_address(); | ||
| let allowance: i128 = | ||
| call_token_view_with_two_addresses(env, token, "allowance", owner, &proxy)?; | ||
|
|
||
| (allowance >= amount) | ||
| .then_some(()) | ||
| .ok_or(ContractError::InsufficientAllowance) | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check ERC-4626 spec for allowance semantics in withdraw/redeem
# Also check if there's a different authorization pattern intended
# Search for how allowance is used in similar vault/proxy contracts
rg -n "allowance" -C 5 --type rust contract/vault/soroban/
# Check if there are comments explaining the authorization model
rg -n "auth" -i -C 3 --type rust contract/proxy-4626-soroban/src/contract.rsRepository: Templar-Protocol/contracts
Length of output: 6203
🏁 Script executed:
#!/bin/bash
# Check the test to understand the expected authorization flow
rg -n "test_atomic_withdraw_requires_allowance_for_delegated_operator" -A 70 --type rust contract/vault/soroban/src/tests.rs | head -100
# Check if there's any recent context about allowance semantics
rg -n "require_auth_or_allowance\|allowance.*caller\|allowance.*proxy" -B 2 -A 2 --type rust contract/proxy-4626-soroban/Repository: Templar-Protocol/contracts
Length of output: 3121
🏁 Script executed:
#!/bin/bash
# Check if there are any comments or documentation about the authorization model
rg -n "withdraw|redeem" -B 5 -A 8 --type rust contract/proxy-4626-soroban/src/contract.rs | grep -A 8 "pub fn withdraw\|pub fn redeem"
# Check the function signature for withdraw/redeem to understand parameter order
ast-grep --pattern 'pub fn withdraw($$$) -> $_'Repository: Templar-Protocol/contracts
Length of output: 740
Critical authorization flaw: allowance check uses wrong spender address.
In require_auth_or_allowance, when caller != owner, the code checks allowance(owner, proxy) (line 487). This is incorrect. The allowance should verify that owner has approved caller (the actual operator), not the proxy contract.
Current behavior: Any authenticated caller can withdraw or redeem shares from owner as long as owner has previously approved the proxy for any reason, regardless of whether the caller is authorized.
Expected behavior: A caller can only withdraw or redeem from owner if owner has explicitly approved that specific caller.
This is demonstrated by the test test_atomic_withdraw_requires_allowance_for_delegated_operator (vault tests), which explicitly approves the operator (line 1245) and verifies allowance is checked between owner and operator (line 1258).
Proposed fix
pub(crate) fn require_auth_or_allowance(
env: &Env,
caller: &Address,
owner: &Address,
token: &Address,
amount: i128,
) -> Result<(), ContractError> {
if caller == owner {
owner.require_auth();
return Ok(());
}
caller.require_auth();
- let proxy = env.current_contract_address();
let allowance: i128 =
- call_token_view_with_two_addresses(env, token, "allowance", owner, &proxy)?;
+ call_token_view_with_two_addresses(env, token, "allowance", owner, caller)?;
(allowance >= amount)
.then_some(())
.ok_or(ContractError::InsufficientAllowance)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contract/proxy-4626-soroban/src/contract.rs` around lines 472 - 492, The
allowance check in require_auth_or_allowance is using the proxy address as the
spender, allowing any authenticated caller to act if the proxy was approved;
change the call_token_view_with_two_addresses invocation so the allowance is
fetched for (owner, caller) (i.e., use caller as the spender argument instead of
env.current_contract_address()/proxy) so the function verifies owner approved
this specific caller before returning Ok; keep the existing
caller.require_auth() and error handling for insufficient allowance.
Comment on lines
+55
to
+62
| impl ContractError { | ||
| pub(crate) const fn from_vault_error_code(code: u32) -> Self { | ||
| match code { | ||
| 3 => Self::InvalidInput, | ||
| 8 => Self::AlreadyInitialized, | ||
| _ => Self::VaultError, | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find vault error code definitions to verify the magic numbers match
# Expected: Error enum with InvalidInput=3 and AlreadyInitialized=8 (or similar)
rg -n "#\[contracterror\]" -A 20 --type rust contract/vault/soroban/Repository: Templar-Protocol/contracts
Length of output: 5112
🏁 Script executed:
#!/bin/bash
# Check proxy contract's Cargo.toml to see dependencies
echo "=== Proxy contract Cargo.toml ===" && \
cat contract/proxy-4626-soroban/Cargo.toml | head -50
echo -e "\n=== Check if vault error module is exported ===" && \
rg -n "pub use|pub mod" contract/vault/soroban/src/error/ -A 2
echo -e "\n=== Full error.rs context in proxy contract ===" && \
cat -n contract/proxy-4626-soroban/src/error.rsRepository: Templar-Protocol/contracts
Length of output: 3095
🏁 Script executed:
#!/bin/bash
# Check if vault error codes are publicly exported/accessible
echo "=== Vault soroban lib.rs to see public exports ===" && \
head -60 contract/vault/soroban/src/lib.rs
echo -e "\n=== Check vault soroban Cargo.toml ===" && \
cat contract/vault/soroban/Cargo.toml | head -60Repository: Templar-Protocol/contracts
Length of output: 3499
🏁 Script executed:
#!/bin/bash
# Verify that proxy contract cannot import from vault runtime in normal deps
echo "=== Proxy contract dependencies check ===" && \
grep -A 10 "\[dependencies\]" contract/proxy-4626-soroban/Cargo.toml
echo -e "\n=== Check if from_vault_error_code is used and how ===" && \
rg -n "from_vault_error_code" contract/proxy-4626-soroban/Repository: Templar-Protocol/contracts
Length of output: 1057
Define named constants for vault error codes.
The magic numbers in from_vault_error_code (3 → InvalidInput, 8 → AlreadyInitialized) match the vault contract's error codes and are used to translate cross-contract error responses. Extracting them as named constants improves readability and maintainability.
♻️ Suggested improvement
+// Vault error codes (from templar-soroban-runtime ContractError enum)
+const VAULT_ERROR_INVALID_INPUT: u32 = 3;
+const VAULT_ERROR_ALREADY_INITIALIZED: u32 = 8;
+
impl ContractError {
pub(crate) const fn from_vault_error_code(code: u32) -> Self {
match code {
- 3 => Self::InvalidInput,
- 8 => Self::AlreadyInitialized,
+ VAULT_ERROR_INVALID_INPUT => Self::InvalidInput,
+ VAULT_ERROR_ALREADY_INITIALIZED => Self::AlreadyInitialized,
_ => Self::VaultError,
}
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| impl ContractError { | |
| pub(crate) const fn from_vault_error_code(code: u32) -> Self { | |
| match code { | |
| 3 => Self::InvalidInput, | |
| 8 => Self::AlreadyInitialized, | |
| _ => Self::VaultError, | |
| } | |
| } | |
| // Vault error codes (from templar-soroban-runtime ContractError enum) | |
| const VAULT_ERROR_INVALID_INPUT: u32 = 3; | |
| const VAULT_ERROR_ALREADY_INITIALIZED: u32 = 8; | |
| impl ContractError { | |
| pub(crate) const fn from_vault_error_code(code: u32) -> Self { | |
| match code { | |
| VAULT_ERROR_INVALID_INPUT => Self::InvalidInput, | |
| VAULT_ERROR_ALREADY_INITIALIZED => Self::AlreadyInitialized, | |
| _ => Self::VaultError, | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contract/proxy-4626-soroban/src/error.rs` around lines 55 - 62, Replace the
magic numbers in ContractError::from_vault_error_code by defining named
constants for the vault error codes (e.g., VAULT_ERR_INVALID_INPUT and
VAULT_ERR_ALREADY_INITIALIZED or an enum/associated consts on ContractError or a
VaultErrorCode type) and use those constants in the match (instead of 3 and 8)
so the mapping to Self::InvalidInput and Self::AlreadyInitialized is explicit
and maintainable; update any related comments to reference the vault contract
error meanings.
Comment on lines
+13
to
+24
| pub(crate) type ProxyCoreView = ( | ||
| (Address, Address, Address, Address), | ||
| (i128, i128, bool), | ||
| (i128, i128, i128, i128), | ||
| (i128, u64, i128, i128), | ||
| ); | ||
| pub(crate) type ProxyPolicyView = ( | ||
| soroban_sdk::Vec<u32>, | ||
| soroban_sdk::Vec<(soroban_sdk::String, i128, i128)>, | ||
| ); | ||
| pub(crate) type ProxyPreviewView = (i128, i128, i128, i128, i128, i128, i128, i128); | ||
| pub(crate) type ProxyViewResponse = (ProxyCoreView, ProxyPolicyView, ProxyPreviewView); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider documenting the tuple field semantics.
The proxy view type aliases use large tuples that can be difficult to interpret without context. Consider adding doc comments explaining what each tuple position represents (e.g., ProxyCoreView.0.0 = vault address, etc.), or at minimum a reference to the vault's proxy_view documentation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contract/proxy-4626-soroban/src/lib.rs` around lines 13 - 24, The tuple type
aliases (ProxyCoreView, ProxyPolicyView, ProxyPreviewView, ProxyViewResponse)
are undocumented and hard to interpret; add concise doc comments above each type
alias describing the semantics of each tuple position (e.g., for ProxyCoreView
list what (0,0) through (0,3) represent, what each element of the nested tuples
means, and similarly enumerate ProxyPolicyView and ProxyPreviewView positions),
or add a single doc comment on ProxyViewResponse that references the vault's
proxy_view documentation and maps tuple indices to named meanings so readers can
quickly understand fields like vault address, fee values, policy entries, and
preview components.
Comment on lines
+489
to
+524
| #[test] | ||
| fn test_redeem_queued_flow() { | ||
| let fixture = Fixture::new(); | ||
| let owner = Address::generate(&fixture.env); | ||
| let receiver = Address::generate(&fixture.env); | ||
|
|
||
| fixture.env.mock_all_auths(); | ||
| fixture.initialize().expect("initialize succeeds"); | ||
| fixture.set_preview(MockPreviewConfig { | ||
| convert_to_assets: 88, | ||
| ..Default::default() | ||
| }); | ||
|
|
||
| let request_id = fixture.env.as_contract(&fixture.proxy, || { | ||
| Soroban4626ProxyContract::redeem( | ||
| fixture.env.clone(), | ||
| owner.clone(), | ||
| 55, | ||
| receiver.clone(), | ||
| owner.clone(), | ||
| ) | ||
| }); | ||
|
|
||
| assert_eq!(request_id, Ok(42)); | ||
| let payloads = fixture.recorded_payloads(); | ||
| assert_eq!(payloads.len(), 1); | ||
| assert_eq!( | ||
| decode_command(&payloads.get(0).expect("request payload exists")), | ||
| WireVaultCommand::RequestWithdraw { | ||
| owner: address_wire(&owner), | ||
| receiver: address_wire(&receiver), | ||
| shares: 55, | ||
| min_assets_out: 88, | ||
| } | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding a test for redeem with insufficient allowance.
The withdraw path has an allowance failure test (test_withdraw_fails_without_allowance), but redeem also performs allowance checks via require_auth_or_allowance. Consider adding a similar test for the redeem flow to ensure consistent coverage. Based on learnings: "Evaluate edge cases around refunds, withdrawals, account deletion, authorization, and replay or double-execution risks in smart contracts."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contract/proxy-4626-soroban/src/tests.rs` around lines 489 - 524, Add a new
unit test (e.g., test_redeem_fails_without_allowance) that mirrors
test_withdraw_fails_without_allowance but calls Soroban4626ProxyContract::redeem
via fixture.env.as_contract and verifies require_auth_or_allowance behavior:
create owner and receiver with Fixture and Address::generate, do not mock or
grant allowance for the caller, call redeem with some shares and preview config,
assert the call returns an Err indicating authorization/allowance failure
(matching the withdraw test's expected error), and assert no WireVaultCommand
payloads were recorded by fixture.recorded_payloads(); reference Fixture,
Address::generate, require_auth_or_allowance, Soroban4626ProxyContract::redeem,
and recorded_payloads to locate relevant helpers.
Collaborator
Author
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
Stack
Verification
This change is