feat(proxy-4626): add Soroban ERC-4626 proxy#418
Conversation
📝 WalkthroughSoroban ERC-4626 Proxy Contract Addition - Security & Correctness ReviewOverviewThis PR adds Critical Security & Correctness Points for Human Review🔴 HIGH PRIORITY
🟡 MEDIUM PRIORITY
🟢 LOWER PRIORITY
Positive Security Observations✅ Proper Authorization: All mutating methods call Files Requiring Detailed Line-by-Line Review
Behavioral Incompatibilities to Document in User Guides
Integration Test Coverage Assessment✅ Tests verify successful initialization, double-init rejection, input validation, vault command encoding, event emission WalkthroughThis PR adds a new Soroban ERC-4626 proxy crate that delegates previews and executions to a configured vault, introduces error types and shared proxy view schemas, implements full proxy entrypoints (deposit/mint/withdraw/redeem/queued flows), and includes unit and integration tests plus a small build recipe update. ChangesSoroban ERC-4626 Proxy Contract
Permissionless TTL extension & adapter/governance updates
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested Reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 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 |
|
@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. Now we have some codegen room the goal for this proxy contract is to have a cleaner interface into the vaults for depositors, rather than the obtuse, opaque interface. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 135-145: The Withdraw event is being emitted when only a
withdrawal request is enqueued (via invoke_vault_execute with
VaultCommand::RequestWithdraw), which is incorrect; remove the
emit_withdraw_event call from the request paths (the call in the block using
expect_u64_result/invoke_vault_execute and the similar call at lines ~159-169)
and instead invoke emit_withdraw_event inside execute_withdraw immediately after
the actual asset transfer completes successfully, keeping the same payload
(caller, receiver, owner, assets, shares) and updating any imports/signatures as
necessary so the event payload still compiles and reflects the real transfer.
- Around line 281-303: The initialize function currently allows any caller to
set vault/asset/share addresses; require authorization by verifying the
transaction signer before writing state: update initialize (and add
ProxyDataKey::Admin) to accept or derive an expected admin and assert that
env.invoker() (or env.signer() per your runtime API) equals that admin,
otherwise return ContractError::Unauthorized; store the admin via
env.storage().instance().set(&ProxyDataKey::Admin, &admin) the first time and on
subsequent calls use is_initialized(&env) combined with an authorization check
(use initialize, is_initialized, ProxyDataKey::Admin,
ProxyDataKey::VaultAddress, ProxyDataKey::AssetToken, ProxyDataKey::ShareToken,
ContractError::Unauthorized to locate code).
- Around line 82-189: Public amount parameters can be negative and must be
rejected at the proxy boundary; add non-negative guards at the start of deposit,
mint, withdraw, redeem, and request_withdraw to validate assets, shares, and
min_assets_out (as applicable) are >= 0, and return a ContractError (e.g., an
InvalidAmount/BadInput variant) when violated. Locate the checks in the
functions deposit (assets), mint (shares), withdraw (assets and derived shares),
redeem (shares and derived assets), and request_withdraw (shares and
min_assets_out) and return the error before any require_auth/allowance or
invoke_vault_execute calls so invalid values never reach invoke_vault_execute,
call_proxy_view, or require_auth_or_allowance.
- Around line 353-367: The proxy is letting the vault's numeric error codes 1–6
be decoded into the local ContractError enum via try_invoke_contract::<Bytes,
ContractError>, bypassing from_vault_error_code; change the invocation to use a
generic raw error type (e.g., try_invoke_contract::<Bytes, u32> or the raw
InvokeError) instead of ContractError, then update the match arms around result
(the block handling Ok(Ok(bytes)), Ok(Err(_)), Err(Ok(...)),
Err(Err(invoke_error))) so that any numeric vault code is received as raw data
and you explicitly pass it through ContractError::from_vault_error_code (or map
InvokeError -> from_vault_error_code) before returning, ensuring all vault codes
are mapped via from_vault_error_code rather than being auto-decoded into the
local enum.
🪄 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: bff2656d-2d68-4c38-b31a-78613d5da1d6
⛔ 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
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me explore the codebase to give you a thorough description and design critique. 🏁 Script executed: Length of output: 1354 🏁 Script executed: Length of output: 19756 🏁 Script executed: Length of output: 30283 🏁 Script executed: Length of output: 22205 🏁 Script executed: Length of output: 8714 Now I have enough to give a thorough description and design critique. Let me also search the ERC-4626 standard to confirm key interface deviations.
PR
|
4f964ca to
71ecf80
Compare
9b352cf to
57e1f69
Compare
43a9e56 to
559c13f
Compare
91fcfc6 to
6db2fc4
Compare
6db2fc4 to
3f4224c
Compare
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
contract/proxy-4626-soroban/src/contract.rs (1)
97-105: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winDeposit has no slippage protection.
min_shares_out: 0accepts any share amount, exposing depositors to sandwich attacks. Unlikemint()which uses the requested shares as a minimum,deposit()offers no guarantee on the exchange rate.Consider adding a
min_shares_outparameter or computing a floor from preview (e.g.,preview_shares * 99 / 100).Option A: Add parameter
pub fn deposit( env: Env, caller: Address, assets: i128, + min_shares_out: i128, receiver: Address, ) -> Result<i128, ContractError> { require_non_negative(assets)?; + require_non_negative(min_shares_out)?; caller.require_auth(); let shares = expect_i128_result(invoke_vault_execute( &env, VaultCommand::DepositWithMin { owner: caller.clone(), receiver: receiver.clone(), assets, - min_shares_out: 0, + min_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/proxy-4626-soroban/src/contract.rs` around lines 97 - 105, The deposit call currently passes min_shares_out: 0 to VaultCommand::DepositWithMin (via invoke_vault_execute and expect_i128_result), allowing unlimited slippage; update the deposit logic to require a min_shares_out and use it instead of 0 (add a min_shares_out: i128 parameter to the deposit function and pass it into VaultCommand::DepositWithMin), or if you prefer a default safety floor compute preview_shares first (call the vault preview endpoint with invoke_vault_execute to get preview_shares) and set min_shares_out = preview_shares * 99 / 100 (or similar) before calling VaultCommand::DepositWithMin so deposits are protected from sandwich/slippage attacks.
🤖 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.
Outside diff comments:
In `@contract/proxy-4626-soroban/src/contract.rs`:
- Around line 97-105: The deposit call currently passes min_shares_out: 0 to
VaultCommand::DepositWithMin (via invoke_vault_execute and expect_i128_result),
allowing unlimited slippage; update the deposit logic to require a
min_shares_out and use it instead of 0 (add a min_shares_out: i128 parameter to
the deposit function and pass it into VaultCommand::DepositWithMin), or if you
prefer a default safety floor compute preview_shares first (call the vault
preview endpoint with invoke_vault_execute to get preview_shares) and set
min_shares_out = preview_shares * 99 / 100 (or similar) before calling
VaultCommand::DepositWithMin so deposits are protected from sandwich/slippage
attacks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 748bdd6c-8415-491e-b13f-b56b36cf6c95
📒 Files selected for processing (1)
contract/proxy-4626-soroban/src/contract.rs
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/proxy-4626-soroban/src/contract.rs (1)
275-300:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReject negative inputs on the preview/view entrypoints too.
convert_to_shares,convert_to_assets,preview_mint, andpreview_withdrawstill forward signed negatives into the vault view API. That leaves impossible ERC-4626 states reachable on the read path and makes the proxy return vault-specific behavior instead of a consistentInvalidInput.Suggested fix
pub fn convert_to_shares(env: Env, assets: i128) -> Result<i128, ContractError> { + require_non_negative(assets)?; let preview = call_proxy_view(&env, &env.current_contract_address(), assets, 0)?; Ok(preview.convert_to_shares) } pub fn convert_to_assets(env: Env, shares: i128) -> Result<i128, ContractError> { + require_non_negative(shares)?; let preview = call_proxy_view(&env, &env.current_contract_address(), 0, shares)?; Ok(preview.convert_to_assets) } @@ pub fn preview_mint(env: Env, shares: i128) -> Result<i128, ContractError> { + require_non_negative(shares)?; let preview = call_proxy_view(&env, &env.current_contract_address(), 0, shares)?; Ok(preview.preview_mint_assets) } @@ pub fn preview_withdraw(env: Env, assets: i128) -> Result<i128, ContractError> { + require_non_negative(assets)?; let preview = call_proxy_view(&env, &env.current_contract_address(), assets, 0)?; Ok(preview.preview_withdraw_shares) }As per coding guidelines, "
**/*.rs: Prefer parsing over validation in Rust; express invariants in types wherever practical, and make invalid states unrepresentable."🤖 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-4626-soroban/src/contract.rs` around lines 275 - 300, The preview/view entrypoints (convert_to_shares, convert_to_assets, preview_mint, preview_withdraw, and the delegating preview_deposit/preview_redeem paths) currently forward negative i128 inputs into call_proxy_view; add explicit validation at the start of each function to reject negative inputs and return Err(ContractError::InvalidInput) (or the contract's equivalent) before calling call_proxy_view, referencing the functions convert_to_shares, convert_to_assets, preview_mint, preview_withdraw and the helper call_proxy_view so invalid signed values are caught early and impossible states are made unrepresentable.
🤖 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-4626-soroban/src/contract.rs`:
- Around line 129-139: The current path calls
invoke_vault_execute(VaultCommand::DepositWithMin...) and discards the actual
minted shares returned by expect_i128_result, then emits and returns the
requested shares variable; capture the actual minted share amount from
expect_i128_result(invoke_vault_execute(...)) into a local (e.g. minted_shares)
and use that value when calling emit_deposit_event(&env, &operator, &receiver,
assets, minted_shares) and when returning Ok(...) so the event and return
reflect the real fill instead of the requested `shares`.
- Line 174: The RedeemRequest event call is passing the payout recipient
(`receiver`) as the indexed controller topic; change the first argument of
`emit_redeem_request_event` to the actual controller (`operator`) so the
signature matches `(controller, owner, request_id, sender, shares)` — update the
call at the shown site (and the analogous calls around lines 210 and 239) to
pass `operator` (not `receiver`) along with `owner`, `request_id`, `sender`, and
`shares` so indexers see the correct controller.
---
Outside diff comments:
In `@contract/proxy-4626-soroban/src/contract.rs`:
- Around line 275-300: The preview/view entrypoints (convert_to_shares,
convert_to_assets, preview_mint, preview_withdraw, and the delegating
preview_deposit/preview_redeem paths) currently forward negative i128 inputs
into call_proxy_view; add explicit validation at the start of each function to
reject negative inputs and return Err(ContractError::InvalidInput) (or the
contract's equivalent) before calling call_proxy_view, referencing the functions
convert_to_shares, convert_to_assets, preview_mint, preview_withdraw and the
helper call_proxy_view so invalid signed values are caught early and impossible
states are made unrepresentable.
🪄 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: fbaa71ca-9591-4a9e-8e8a-93f2098e07e6
📒 Files selected for processing (2)
contract/proxy-4626-soroban/src/contract.rscontract/proxy-4626-soroban/src/tests.rs
|
commit-id:4641dafc
960a594 to
6cd183a
Compare
|
@coderabbitai incremental review (resolved your comments) |
|
Kicking off a fresh incremental review now. ✏️ Learnings added
🧠 Learnings used✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
contract/vault/soroban/governance/src/lib.rs (1)
676-680: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider simplifying the return type.
Both
extend_instance_ttl(&env)andextend_pending_queue_ttl(&env)are infallible, so this method can no longer fail. TheResult<(), GovernanceError>return type is now unnecessary and could be simplified to().♻️ Simplified signature
- pub fn extend_ttl(env: Env) -> Result<(), GovernanceError> { + pub fn extend_ttl(env: Env) { extend_instance_ttl(&env); extend_pending_queue_ttl(&env); - Ok(()) }🤖 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/governance/src/lib.rs` around lines 676 - 680, The function extend_ttl currently returns Result<(), GovernanceError> but both called helpers extend_instance_ttl and extend_pending_queue_ttl are infallible, so change extend_ttl's signature to return () (remove Result and GovernanceError), remove the trailing Ok(()), and update any call sites that expect a Result to use the new void return; keep the same body calls to extend_instance_ttl(&env) and extend_pending_queue_ttl(&env).
♻️ Duplicate comments (1)
contract/proxy-4626-soroban/src/contract.rs (1)
194-194:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPass the request owner as controller, not the payout receiver.
All three
emit_redeem_request_eventcalls pass&receiveras thecontrollerparameter, but the controller should be the address that owns and controls the request (&ownerin all three cases). Passing the receiver misattributes the request to the asset payout destination and will break off-chain indexers, wallet UIs, and any system querying "requests by controller."ERC-7540 semantics:
controlleris the address that controls the request lifecycle, not the address receiving withdrawn assets.🔧 Proposed fix
// In withdraw (line 194) - emit_redeem_request_event(&env, &receiver, &owner, request_id, &operator, shares); + emit_redeem_request_event(&env, &owner, &owner, request_id, &operator, shares); // In redeem (line 230) - emit_redeem_request_event(&env, &receiver, &owner, request_id, &operator, shares); + emit_redeem_request_event(&env, &owner, &owner, request_id, &operator, shares); // In request_withdraw (line 259) - emit_redeem_request_event(&env, &receiver, &owner, request_id, &owner, shares); + emit_redeem_request_event(&env, &owner, &owner, request_id, &owner, shares);Also applies to: 230-230, 259-259
🤖 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-4626-soroban/src/contract.rs` at line 194, The three calls to emit_redeem_request_event are incorrectly passing &receiver as the controller; update each call to pass &owner (the request owner/controller) instead of &receiver so the event's controller field reflects the address that controls the request lifecycle—locate the calls to emit_redeem_request_event (the instances using variables receiver, owner, request_id, operator, shares) and replace the controller argument to use owner in all occurrences.
🤖 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-oracle/soroban/sep40-adapter-contract/src/lib.rs`:
- Around line 216-219: Removing the unintended side-effect of reads extending
instance TTL: delete the call to extend_instance_ttl(env) from load_config and
instead invoke extend_instance_ttl only in the contract's mutating/maintenance
entrypoints (e.g., the keeper/update functions and any setters) so pure read
methods (the SEP-40 trait methods base, assets, decimals, resolution, price,
prices, lastprice) call load_config without changing TTL; if you do want
permissionless keepers to extend TTL, add an explicit public keeper method that
calls extend_instance_ttl and is invoked by off-chain actors rather than
embedding it in load_config.
---
Outside diff comments:
In `@contract/vault/soroban/governance/src/lib.rs`:
- Around line 676-680: The function extend_ttl currently returns Result<(),
GovernanceError> but both called helpers extend_instance_ttl and
extend_pending_queue_ttl are infallible, so change extend_ttl's signature to
return () (remove Result and GovernanceError), remove the trailing Ok(()), and
update any call sites that expect a Result to use the new void return; keep the
same body calls to extend_instance_ttl(&env) and extend_pending_queue_ttl(&env).
---
Duplicate comments:
In `@contract/proxy-4626-soroban/src/contract.rs`:
- Line 194: The three calls to emit_redeem_request_event are incorrectly passing
&receiver as the controller; update each call to pass &owner (the request
owner/controller) instead of &receiver so the event's controller field reflects
the address that controls the request lifecycle—locate the calls to
emit_redeem_request_event (the instances using variables receiver, owner,
request_id, operator, shares) and replace the controller argument to use owner
in all occurrences.
🪄 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: ce3ef2b8-6a5b-40d6-849e-ee38c2d41336
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (18)
Cargo.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.rscontract/proxy-oracle/soroban/README.mdcontract/proxy-oracle/soroban/governance-contract/src/lib.rscontract/proxy-oracle/soroban/governance-contract/src/storage.rscontract/proxy-oracle/soroban/governance-contract/src/tests.rscontract/proxy-oracle/soroban/integration-tests/tests/ttl_extend.rscontract/proxy-oracle/soroban/sep40-adapter-contract/src/lib.rscontract/proxy-oracle/soroban/sep40-adapter-contract/src/tests.rscontract/vault/soroban/README.mdcontract/vault/soroban/governance/src/lib.rscontract/vault/soroban/justfilecontract/vault/soroban/shared-types/src/lib.rs
1 participant
Stack:
This change is