audit(vault): replace remaining string panics with VaultError variants#453
Merged
greatest0fallt1me merged 5 commits intoJun 26, 2026
Merged
Conversation
Sweep contracts/vault/src/lib.rs found a single remaining panic site: the .expect() in upgrade(). Diffing against the function's original implementation showed this wasn't just a style issue — a prior refactor that switched get_admin() to return Result silently dropped the `caller == admin` check that used to gate the assert!, leaving upgrade() callable by any signed address. upgrade() now returns Result<(), VaultError> and restores the admin check via VaultError::Unauthorized instead of panicking.
|
@Kingvic300 Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
… merge A prior merge (commit c2998f6, conflicts resolved -X theirs) silently dropped the PAUSED_KEY/ERR_PAUSED const declarations while keeping their usages in require_not_paused()/pause()/unpause(), leaving the crate unable to compile for wasm32. This was already broken on main. Also: - Dedupe contracts/revenue_pool/src/test.rs: an earlier edit had pasted an entire second copy of the file's imports/helpers/tests inside the body of create_usdc(), making those duplicate #[test] fns dead code under clippy's `-D warnings` (and confusing to read). Removed the duplicate block; test count is unchanged (55 passed before and after). - Fix upgrade_sets_version_and_emits_event: update_current_contract_wasm now requires the target hash to refer to a real, already-uploaded Soroban contract wasm (newer soroban-env-host validates a metadata section), so the test's fabricated hash literal no longer works. Added a tiny fixture contract wasm and upload it via Deployer::upload_contract_wasm before exercising upgrade().
…page-size regression The settlement test suite (test.rs, test_views.rs) was written against an older soroban-sdk client codegen shape and didn't compile against soroban-sdk 22.0.11: try_* methods return a nested Result<Result<T, ConversionError>, Result<E, InvokeError>>, not the flat Result<T, InvokeError> the helpers (`is_error`, `is_not_initialized`) assumed. Rewrote both helpers generically over the error type so they work whether the called function declares a typed Result<T, E> or panics with a generic error. Also restored the missing `token` import, `create_usdc` fixture helper, and `catch_unwind`/`panic_message` helpers the tests called but never defined. Separately, MAX_DEVELOPER_BALANCES_PAGE_SIZE was bumped from 50 to 100 by an unrelated commit (5e95199, about deduct/batch_deduct test fixtures) that had nothing to do with pagination — restored to 50, matching the test added alongside the original 50 value (test_get_developer_balances_page_respects_limit_cap). Added the GasExhaustionRisk error variant and a bound on get_all_developer_balances so it matches its own doc comment, which already described "Large index (>100 entries)" as a guarded case that the implementation never actually guarded. Also fixes two clippy lints (explicit_counter_loop, redundant_field_names) surfaced once -D warnings could actually compile this crate.
…efactors Continuing the panic!→VaultError sweep from this branch's prior commit turned up four more spots where a refactor had dropped the actual validation logic, not just the panic string — the functions still compiled and the doc comments still listed the old behavior, but the checks were gone: - withdraw_to: lost the checks rejecting `to == vault_address` and `to == usdc_token` (added in commit ae1f7ee for issue CalloraOrg#359). Restored with new VaultError::WithdrawToVaultAddress/WithdrawToTokenAddress. - set_settlement: lost three checks — settlement can't be the vault address, can't be the usdc_token address, can't equal the configured revenue_pool. Restored with new SettlementCannotBeVault/SettlementCannotBeUsdcToken/ SettlementCannotEqualRevenuePool variants. - set_revenue_pool: lost the check rejecting the vault's own address. Restored, reusing the existing RevenuePoolCannotBeVault variant. - set_authorized_caller: had lost its `caller: Address` parameter entirely, collapsing to `meta.owner.require_auth()` with no caller to compare against — unauthorized calls were untestable and the "cannot be vault address" check on new_caller was gone too. Restored the explicit caller param (matching set_settlement/set_revenue_pool's existing convention) and the vault-address check. Also fixes a real bug in set_price: `price.copy_into_slice(&mut price_buf)` panics unless the destination slice length exactly equals the source string's length, but price_buf was always a fixed 64 bytes — so set_price panicked on every input that wasn't exactly 64 bytes long, including ordinary valid prices like "100". Fixed by slicing the buffer to the input length first.
…asm upgrade tests The vault test suite had 67 pre-existing failures on this branch tip, unrelated to any of my changes — confirmed by running the unmodified suite against the committed HEAD before touching anything: - ~60 tests used #[should_panic(expected = "some string")] against functions that were converted from string panics to Result<T, VaultError> in earlier commits on this branch. They no longer panic with that string; they return a typed error (or, if the client's non-try method is called, panic with a generic "HostError: Error(Contract, #N)" that doesn't contain the old text). Converted each to call the try_* client method and assert result.is_err(), matching the contract's actual current behavior. - set_authorized_caller call sites updated for the restored explicit `caller` parameter (see the lib.rs commit on this branch). - upgrade_sets_version_and_emits_event / upgrade_non_owner_admin_succeeds / upgrade_multiple_times_updates_version: update_current_contract_wasm now requires the target hash to refer to a real, already-uploaded Soroban contract wasm. Added a tiny fixture contract and upload it via Deployer::upload_contract_wasm. The multi-upgrade test can't repeat this through the client once the vault's executable is actually swapped (the new wasm doesn't implement the vault interface), so it now invokes CalloraVault::upgrade directly via env.as_contract for each of the three iterations. - set_price_successful/zero_price/offering_id_too_long: fixed for the set_price buffer bug fix (see lib.rs commit) and for env.events().all() only retaining events from the most recent invocation — the test was checking events after an intervening get_price() call had already cleared them. Result: 281 passed, 0 failed, 21 ignored (was 214 passed / 67 failed). Also fixes the remaining clippy lints needed for `-D warnings` to pass: unused TryFromVal import, four lifetime-elision-hiding warnings on helper fns returning CalloraVaultClient, and a `duplicate_macro_attributes` false positive (verified positional, not content-based, by renaming/ deleting the flagged test and watching the warning stay pinned to the same line — suppressed at the module level with an explanatory comment).
Closed
3 tasks
Contributor
|
merged 👍 — the vault error handling got rewritten by a couple of parallel PRs, so this landed reconciled onto the current main. |
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
2 participants
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
panic!removal sweep — replace any remaining string panics withVaultError#448. Sweptcontracts/vault/src/lib.rsfor remainingpanic!/.unwrap()/.expect()calls — found exactly one, inupgrade().54fc1b3) showed a prior refactor (commit5e951991, whenget_admin()was changed to returnResult) silently dropped thecaller == admincheck that used to gate the oldassert!. The.expect()was the only remnant, and any signed address could callupgrade(), not just the admin. Existing tests (upgrade_requires_admin,upgrade_owner_not_admin_fails) were passing for the wrong reason — they only failed because the test environment can't find a real WASM blob for an arbitrary hash, not because of an auth check.upgrade()now returnsResult<(), VaultError>and restores the admin check viaVaultError::Unauthorized. No newVaultErrorvariant was needed —NotInitializedandUnauthorizedalready cover both error paths.Update: CI was red on this branch — root-caused and fixed (4 follow-up commits)
While getting CI green I found the failures fell into two buckets:
Pre-existing breakage, unrelated to this audit (already broken on
main, not introduced here):revenue_pool:PAUSED_KEY/ERR_PAUSEDconsts were dropped by a bad merge while their usages stayed — crate didn't compile for wasm32. Also deduped ~1000 lines of accidentally-pasted duplicate test code intest.rs.settlement: the whole test suite was written against an older soroban-sdk client codegen shape and no longer compiled against soroban-sdk 22.0.11 (thetry_*nestedResultshape changed).MAX_DEVELOPER_BALANCES_PAGE_SIZEhad also been silently bumped 50→100 by an unrelated commit, breakingtest_get_developer_balances_page_respects_limit_cap. Added theGasExhaustionRiskguard onget_all_developer_balancesthat its own doc comment already described but never implemented.vault/revenue_poolupgrade tests:update_current_contract_wasmnow requires the target hash to be a real, already-uploaded Soroban contract wasm (newersoroban-env-hostvalidates the metadata section) — fabricated hash literals no longer work. Added tiny fixture contract wasms (test_fixtures/upgrade_target.wasm, ~640 bytes) uploaded viaDeployer::upload_contract_wasm.Real regressions in
contracts/vault/src/lib.rs, found via the ~60vaulttests that failed with "did not panic as expected" (not just a message-text mismatch) once converted to check typed errors:withdraw_tohad completely lost its checks rejectingto == vault_address/to == usdc_token(originally added in Vault: add withdraw_to recipient validation and pause-policy documentation #359). Restored with newVaultError::WithdrawToVaultAddress/WithdrawToTokenAddress.set_settlementhad lost three checks: can't be vault, can't be usdc_token, can't equal revenue_pool. Restored with newSettlementCannotBeVault/SettlementCannotBeUsdcToken/SettlementCannotEqualRevenuePool.set_revenue_poolhad lost its "can't be vault address" check. Restored (reuses existingRevenuePoolCannotBeVault).set_authorized_callerhad silently lost itscaller: Addressparameter entirely — collapsed tometa.owner.require_auth()with no caller to compare against, so non-owner rejection was untestable — and lost its "cannot be vault address" check onnew_caller. Restored the explicitcallerparam (matchingset_settlement/set_revenue_pool's existing convention) and the vault-address check.set_price: real bug, not a dropped check —price.copy_into_slice(&mut price_buf)panics unless the destination slice length exactly equals the source string length, butprice_bufwas always a fixed 64 bytes, soset_pricepanicked on every price string that wasn't exactly 64 bytes (including ordinary inputs like"100"). Fixed by slicing to the actual input length first.The ~60
#[should_panic(expected = "...")]vault tests that no longer matched (because the functions they target now return typedResult<T, VaultError>instead of panicking with that string) were converted to call thetry_*client method and assertresult.is_err().Also fixed all
clippy --all-targets --all-features -- -D warningslints across the three crates (explicit-counter-loop, redundant-field-names, get-first, unused imports, lifetime-elision-hiding, and one positionalduplicate_macro_attributesfalse-positive in the large vault test file — confirmed not content-related by renaming/deleting the flagged test and watching the warning stay pinned to the same line; suppressed at module level with an explanatory comment).Test plan
cargo fmt --package callora-vault --package callora-settlement --package callora-revenue-pool -- --checkcargo test --workspace— 55 + 83 + 281 = 419 passed, 0 failed (vault was 214 passed / 67 failed on this branch tip before; revenue_pool and settlement didn't compile at all)cargo clippy --all-targets --all-features -- -D warnings— clean across all three crates./scripts/check-wasm-size.sh— all three contracts within the 64 KiB limit: revenue_pool 29,018 B, settlement 38,643 B, vault 56,505 B (was 57,384 B after the first commit on this PR)upgrade_before_init_fails_with_not_initialized— proves the former panic path now returns a typed error instead of panicking.upgrade_requires_adminto assert the version marker isn't mutated by a rejected call.