fix(soroban): make share-token vault immutable#429
Conversation
|
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:
📝 WalkthroughWalkthroughEnforces immutability for vault and metadata, adds VaultImmutable and MetadataImmutable errors, emits a supplemental burn_from event with spender/from topics, switches contract detection to AddressPayload, enables soroban-sdk hazmat-address, expands tests for these behaviors, and removes the Justfile set-metadata recipe/help entry. ChangesShare Token Immutability and Event Emission
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
carrion256
force-pushed
the
audit/share-token-admin-immutable
branch
from
33819e4 to
5c302e8
Compare
carrion256
force-pushed
the
audit/share-token-admin-immutable
branch
from
3c5d098 to
f8dedf6
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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 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/share-token/src/tests.rs`:
- Around line 452-468: Update the test
admin_cannot_change_metadata_after_deployment to not just #[should_panic] but to
explicitly catch the error from invoking set_metadata, assert the error is the
exact metadata-immutability error string/variant expected, and then read the
token's metadata (use the contract's read/getters such as
get_name/get_symbol/get_decimals or the equivalent query methods on the token
instance) to assert that name, symbol and decimals remain unchanged; locate the
invocation of set_metadata and replace the blanket panic expectation with a
try/catch (or Result) assert on the error plus explicit state reads on token to
confirm no partial write occurred.
- Around line 109-120: Replace the broad #[should_panic] in the test
set_vault_rejects_account_address with an explicit assertion that invoking
set_vault with an account-style address returns the specific InvalidInput
contract error: call env.invoke_contract for the token's "set_vault" with
account_address(&env) and assert the result is an Err containing
ContractError::InvalidInput (or map the returned error code to that variant);
keep the test name and use the symbols set_vault, account_address, and
ContractError::InvalidInput to locate and update the behavior check.
🪄 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: 115236c2-27e5-4119-84b0-e7034165c408
📒 Files selected for processing (5)
contract/vault/soroban/justfilecontract/vault/soroban/share-token/Cargo.tomlcontract/vault/soroban/share-token/src/lib.rscontract/vault/soroban/share-token/src/tests.rscontract/vault/soroban/share-token/src/types.rs
💤 Files with no reviewable changes (1)
- contract/vault/soroban/justfile
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
contract/vault/soroban/share-token/src/tests.rs (1)
89-107:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReplace broad
#[should_panic]with exact contract-error assertions in negative-path tests.These tests currently pass on any panic, including unrelated regressions. Switch to
try_invoke_contract::<(), ShareTokenError>(...)and assert specific variants for each path to lock behavior.Also applies to: 249-280, 282-378
🤖 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/share-token/src/tests.rs` around lines 89 - 107, The test constructor_rejects_account_vault_address (and other negative-path tests in this file) currently use #[should_panic]; replace each with a call to try_invoke_contract::<(), ShareTokenError>(...) targeting the SorobanShareTokenContract invocation and assert the returned Err matches the specific ShareTokenError variant expected (e.g., InvalidVaultAddress or similar) instead of relying on any panic; locate the contract call registration code and swap the env.register invocation wrapped by try_invoke_contract, then use pattern matching or assert_eq! on the Err to ensure the exact ShareTokenError is produced.
🤖 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/share-token/src/tests.rs`:
- Around line 163-215: The test relies on calling env.events().all() as if it
clears events; instead record the current event count into a baseline before
calling VaultCaller::burn_from and then assert that the new events length equals
baseline + 2; locate the setup where VaultCaller::mint and VaultCaller::approve
are invoked and replace the unused env.events().all() call with code that
captures let baseline =
env.events().all().filter_by_contract(&token).events().len() (or equivalent),
then after VaultCaller::burn_from compute let events =
env.events().all().filter_by_contract(&token) and
assert_eq!(events.events().len(), baseline + 2) while keeping the same checks
against the burn_from event body.
---
Duplicate comments:
In `@contract/vault/soroban/share-token/src/tests.rs`:
- Around line 89-107: The test constructor_rejects_account_vault_address (and
other negative-path tests in this file) currently use #[should_panic]; replace
each with a call to try_invoke_contract::<(), ShareTokenError>(...) targeting
the SorobanShareTokenContract invocation and assert the returned Err matches the
specific ShareTokenError variant expected (e.g., InvalidVaultAddress or similar)
instead of relying on any panic; locate the contract call registration code and
swap the env.register invocation wrapped by try_invoke_contract, then use
pattern matching or assert_eq! on the Err to ensure the exact ShareTokenError is
produced.
🪄 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: fda34278-3319-4fc3-8965-45a2dd13b6c2
📒 Files selected for processing (1)
contract/vault/soroban/share-token/src/tests.rs
| #[test] | ||
| fn burn_from_emits_supplemental_spender_event() { | ||
| let (env, _admin, vault, token) = setup(); | ||
| let from = Address::generate(&env); | ||
| let spender = Address::generate(&env); | ||
|
|
||
| env.as_contract(&vault, || { | ||
| VaultCaller::mint(env.clone(), token.clone(), from.clone(), 1000); | ||
| }); | ||
| env.as_contract(&vault, || { | ||
| VaultCaller::approve( | ||
| env.clone(), | ||
| token.clone(), | ||
| from.clone(), | ||
| spender.clone(), | ||
| 400, | ||
| 300, | ||
| ); | ||
| }); | ||
| env.events().all(); | ||
|
|
||
| env.as_contract(&vault, || { | ||
| VaultCaller::burn_from( | ||
| env.clone(), | ||
| token.clone(), | ||
| spender.clone(), | ||
| from.clone(), | ||
| 250, | ||
| ); | ||
| }); | ||
|
|
||
| let events = env.events().all().filter_by_contract(&token); | ||
| assert_eq!(events.events().len(), 2); | ||
| let burn_from_event = &events.events()[1]; | ||
| let ContractEventBody::V0(body) = &burn_from_event.body; | ||
| assert_eq!(body.topics.len(), 3); | ||
| assert_eq!( | ||
| body.topics[0], | ||
| ScVal::try_from_val(&env, &symbol_short!("burn_from")).unwrap() | ||
| ); | ||
| let spender_val: Val = spender.clone().into_val(&env); | ||
| let from_val: Val = from.clone().into_val(&env); | ||
| let amount_val: Val = 250i128.into_val(&env); | ||
| assert_eq!( | ||
| body.topics[1], | ||
| ScVal::try_from_val(&env, &spender_val).unwrap() | ||
| ); | ||
| assert_eq!( | ||
| body.topics[2], | ||
| ScVal::try_from_val(&env, &from_val).unwrap() | ||
| ); | ||
| assert_eq!(body.data, ScVal::try_from_val(&env, &amount_val).unwrap()); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In soroban-sdk testutils, does calling env.events().all() clear/reset accumulated events, or is it purely a read operation?
💡 Result:
In the soroban-sdk testutils, calling env.events.all is purely a read operation [1][2]. It retrieves the events that have been accumulated in the environment up to that point and does not clear or reset them [1][3]. The events remain in the environment's state, allowing you to call env.events.all multiple times within a test to inspect the accumulated event history [1][3]. If you need to clear events between specific test steps, you would typically need to create a new environment (Env::default) or manage the event state manually, as the SDK does not provide a built-in method to clear the event buffer on an existing environment instance [1][4].
Citations:
- 1: https://mintlify.com/stellar/rs-soroban-sdk/api/modules/testutils
- 2: https://docs.rs/soroban-sdk/latest/soroban_sdk/testutils/struct.ContractEvents.html
- 3: Support comparing SDK objects across Env's stellar/rs-soroban-sdk#1360
- 4: https://docs.rs/soroban-sdk/latest/soroban_sdk/struct.Env.html
🏁 Script executed:
# Find the token contract implementation to understand what events are emitted
fd -e rs . contract/vault/soroban/share-token/src/ | grep -v test | head -20Repository: Templar-Protocol/contracts
Length of output: 164
🏁 Script executed:
# Look at the token implementation to see what events are emitted by mint/approve/burn_from
cat contract/vault/soroban/share-token/src/lib.rs | head -100Repository: Templar-Protocol/contracts
Length of output: 2778
🏁 Script executed:
# Search for event emissions in the token code
rg "emit_" contract/vault/soroban/share-token/src/lib.rs -A 3 -B 1Repository: Templar-Protocol/contracts
Length of output: 565
🏁 Script executed:
# Search for stellar_tokens references or vendored code in the repo
find . -name "stellar_tokens*" -o -name "*stellar*tokens*" 2>/dev/null | head -20Repository: Templar-Protocol/contracts
Length of output: 52
🏁 Script executed:
# Check Cargo.toml to see how stellar_tokens is included
cat contract/vault/soroban/share-token/Cargo.tomlRepository: Templar-Protocol/contracts
Length of output: 691
🏁 Script executed:
# Search for any test utilities or mocking around mint/approve events
rg "VaultCaller::mint|VaultCaller::approve" contract/vault/soroban/share-token/src/tests.rs -B 2 -A 5 | head -60Repository: Templar-Protocol/contracts
Length of output: 1601
🏁 Script executed:
# Find the VaultCaller implementation
rg "impl VaultCaller|fn mint|fn approve" contract/vault/soroban/share-token/src/tests.rs -B 2 -A 10 | head -80Repository: Templar-Protocol/contracts
Length of output: 755
🏁 Script executed:
# Also search for any event assertions in the test file to understand the pattern
rg "assert_eq.*len\(\)" contract/vault/soroban/share-token/src/tests.rs -B 3 -A 1Repository: Templar-Protocol/contracts
Length of output: 347
🏁 Script executed:
# Search for all event assertions in the test file to understand the pattern
rg "events\(\)\.all\(\)" contract/vault/soroban/share-token/src/tests.rs -B 5 -A 5Repository: Templar-Protocol/contracts
Length of output: 628
🏁 Script executed:
# Check if there are other tests that verify event counts and how they handle baseline
rg "#\[test\]" contract/vault/soroban/share-token/src/tests.rs -A 30 | grep -A 30 "burn_from_emits"Repository: Templar-Protocol/contracts
Length of output: 827
Event baseline is not isolated; test relies on fragile implicit assumption about mint/approve event emission.
The test calls env.events().all() on line 182 without assigning the result, suggesting an intent to clear events. However, this is purely a read operation and does not clear accumulated events in the Soroban SDK. The subsequent assertion assert_eq!(events.events().len(), 2) implicitly assumes that VaultCaller::mint() and VaultCaller::approve() do not emit token contract events—this assumption works today but is fragile and undocumented. If stellar_tokens::Base behavior changes to emit events, the test will break unexpectedly.
Explicitly baseline the event count before burn_from() to make the invariant visible and robust:
Suggested fix
- env.events().all();
+ let before = env.events().all().filter_by_contract(&token).events().len();
env.as_contract(&vault, || {
VaultCaller::burn_from(
env.clone(),
token.clone(),
spender.clone(),
from.clone(),
250,
);
});
let events = env.events().all().filter_by_contract(&token);
- assert_eq!(events.events().len(), 2);
- let burn_from_event = &events.events()[1];
+ let new_events = &events.events()[before..];
+ assert_eq!(new_events.len(), 2);
+ let burn_from_event = &new_events[1];🤖 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/share-token/src/tests.rs` around lines 163 - 215, The
test relies on calling env.events().all() as if it clears events; instead record
the current event count into a baseline before calling VaultCaller::burn_from
and then assert that the new events length equals baseline + 2; locate the setup
where VaultCaller::mint and VaultCaller::approve are invoked and replace the
unused env.events().all() call with code that captures let baseline =
env.events().all().filter_by_contract(&token).events().len() (or equivalent),
then after VaultCaller::burn_from compute let events =
env.events().all().filter_by_contract(&token) and
assert_eq!(events.events().len(), baseline + 2) while keeping the same checks
against the burn_from event body.
…4fcd-a45f-1ea4f695f5af) Trace: 5c302e8
…6a-455f-9b9c-3aa8227c255a) Trace: 0aad51e
…Nexus ca6dd926-fd39-405d-b710-af4fe8744727 Nexus 5af46c5f-dbd8-46e6-93fb-0e21dd4aaf69 Nexus 12319f5b-fa52-46b1-8b49-d91fbceeca7d) Trace: 3c5d098
…3 Nexus 37a5412f-fb6a-455f-9b9c-3aa8227c255a Nexus ca6dd926-fd39-405d-b710-af4fe8744727)
carrion256
force-pushed
the
audit/share-token-admin-immutable
branch
from
467fdf8 to
8470b7f
Compare
…xus 37a5412f-fb6a-455f-9b9c-3aa8227c255a Nexus ca6dd926-fd39-405d-b710-af4fe8744727)
1 participant
Summary
Audit Context
share-token-adminspr/refactor/vault-ergonomics/4f330057ata657f92contracts-audit-clusters/share-token-admin.audit.mdVerification
cargo fmt --allcargo +1.86.0 test -p templar-soroban-share-token -- --nocapture— blocked before package tests by existing workspace dependency compile failure instellar-contract-utils 0.7.1(use of unstable library feature unsigned_is_multiple_ofinsrc/crypto/merkle.rs:102)RUSTFLAGS='-Zcrate-attr=feature(unsigned_is_multiple_of)' cargo +nightly test -p templar-soroban-share-token -- --nocapture— 9 passedsize-budget-checkpassed at93961bytesHalborn Finding IDs
Included for Halborn SSC GitHub remediation detection:
6157e4aa-116a-4fcd-a45f-1ea4f695f5af— Share-token admin can redirect vault-mint privilege without timelock or event37a5412f-fb6a-455f-9b9c-3aa8227c255a— Share-token metadata can be changed after deployment without timelock or eventca6dd926-fd39-405d-b710-af4fe8744727— Share-token contract-address validation relies on StrKey prefix inspection5af46c5f-dbd8-46e6-93fb-0e21dd4aaf69— Share-token burn and burn_from emit indistinguishable burn events12319f5b-fa52-46b1-8b49-d91fbceeca7d— Share-token tests omit admin rotation and allowance security coverageFolded Share-Token Findings
This PR is the consolidated share-token cluster PR. It now also folds the formerly separate A-050/A-093/A-094/A-097 work; PR #440 and PR #443 are superseded by this PR.
37a5412f-fb6a-455f-9b9c-3aa8227c255aset_metadatafail after admin auth withShareTokenError::MetadataImmutable.set-metadatajustfile recipe/help.ca6dd926-fd39-405d-b710-af4fe8744727soroban_sdk::address_payload::AddressPayloaddiscrimination.set_vault.5af46c5f-dbd8-46e6-93fb-0e21dd4aaf69burn_fromevent keyed by spender and owner, so delegated burns are distinguishable from direct burns.12319f5b-fa52-46b1-8b49-d91fbceeca7dFold Verification
RUSTUP_TOOLCHAIN=1.89.0 cargo fmt --all --checkgit diff --checkRUSTUP_TOOLCHAIN=1.89.0 cargo test -p templar-soroban-share-token -- --nocapture— 20 passedjust -f contract/vault/soroban/justfile buildjust -f contract/vault/soroban/justfile size-budget-check— 93,961 bytes <= 131,072 bytesThis change is