From 5d46606c5f09285983c22c8722dad11e56db6df8 Mon Sep 17 00:00:00 2001 From: "Kappa.dev" Date: Fri, 21 Aug 2026 22:48:12 +0000 Subject: [PATCH] Seven `unwrap()`/`panic!` sites in contract source bypass the typed error enum and trap with opaque host errors #263 FIXED --- .github/workflows/ci.yml | 20 +++++ CHANGELOG.md | 11 +++ src/lib.rs | 76 ++++++++++--------- src/storage.rs | 51 ++++++++----- src/test.rs | 11 +++ .../test_admin_fails_before_initialize.1.json | 61 +++++++++++++++ 6 files changed, 178 insertions(+), 52 deletions(-) create mode 100644 test_snapshots/test/test_admin_fails_before_initialize.1.json diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index e966282..177f987 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -28,3 +28,23 @@ jobs: - name: Run tests run: cargo test + + - name: Reject unguarded unwrap/expect/panic in contract source + run: | + set -euo pipefail + # unwrap()/expect()/panic! are forbidden in contract source + # (src/test.rs is exempt) unless the line directly above carries an + # "INVARIANT:" comment naming the invariant that makes the panic + # unreachable. New untrusted failure paths must return typed errors + # from src/error.rs instead of trapping. + awk ' + FNR == 1 { prev = "" } + /\.(unwrap|expect)\(|panic!/ { + if (prev !~ /INVARIANT/) { + print FILENAME ":" FNR ": " $0 + bad = 1 + } + } + { prev = $0 } + END { exit bad } + ' src/lib.rs src/storage.rs src/events.rs src/types.rs src/error.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 78b6a23..0b288ee 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,17 @@ All notable changes to the AnchorNet contracts are documented here. ### Fixed +- **Errors:** the storage accessors `get_admin`, `get_pending_admin`, and + `get_operator` now return typed contract errors (`NotInitialized`, + `NoPendingAdmin`, `NoOperator`) when the corresponding entry is missing, + instead of trapping with an undecodable host panic. Public entrypoint + behavior is unchanged — the same errors, now enforced at the accessor. +- **Errors:** the four list-pagination `unwrap()`s in `list_anchors`, + `list_fee_waived_anchors`, `list_assets`, and `anchor_balances` were + audited and annotated as guarded invariants (in-bounds by the loop bound). +- **CI:** added a guard that rejects new unguarded `unwrap()`/`expect()`/ + `panic!` sites in contract source. + ### Security ## [0.9.0] diff --git a/src/lib.rs b/src/lib.rs index fdeb6b9..f2be09b 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -58,12 +58,10 @@ impl AnchornetContract { storage::has_admin(&env) } - /// Returns the current administrator address. + /// Returns the current administrator address, or + /// [`Error::NotInitialized`] if the contract has not been initialized. pub fn admin(env: Env) -> Result { - if !storage::has_admin(&env) { - return Err(Error::NotInitialized); - } - Ok(storage::get_admin(&env)) + storage::get_admin(&env) } /// Transfers administration to `new_admin`. Requires authorization from the @@ -107,7 +105,7 @@ impl AnchornetContract { /// Regression tests lock in all three phases (see `test.rs`, issue #130). pub fn propose_admin(env: Env, candidate: Address) -> Result<(), Error> { Self::require_admin(&env)?; - if candidate == storage::get_admin(&env) { + if candidate == storage::get_admin(&env)? { return Err(Error::InvalidAdminCandidate); } storage::set_pending_admin(&env, &candidate); @@ -115,23 +113,17 @@ impl AnchornetContract { Ok(()) } - /// Returns the address proposed to become the next administrator, if - /// any. + /// Returns the address proposed to become the next administrator, or + /// [`Error::NoPendingAdmin`] if no proposal is outstanding. pub fn pending_admin(env: Env) -> Result { - if !storage::has_pending_admin(&env) { - return Err(Error::NoPendingAdmin); - } - Ok(storage::get_pending_admin(&env)) + storage::get_pending_admin(&env) } /// Accepts a pending admin transfer proposed via /// [`propose_admin`](Self::propose_admin). Requires authorization from /// `candidate`, who must match the proposed address. pub fn accept_admin(env: Env, candidate: Address) -> Result<(), Error> { - if !storage::has_pending_admin(&env) { - return Err(Error::NoPendingAdmin); - } - if storage::get_pending_admin(&env) != candidate { + if storage::get_pending_admin(&env)? != candidate { return Err(Error::NotPendingAdmin); } candidate.require_auth(); @@ -176,10 +168,7 @@ impl AnchornetContract { /// [`clear_operator`](Self::clear_operator), so off-chain systems can /// distinguish the two removal paths. pub fn renounce_operator(env: Env, caller: Address) -> Result<(), Error> { - if !storage::has_operator(&env) { - return Err(Error::NoOperator); - } - if caller != storage::get_operator(&env) { + if caller != storage::get_operator(&env)? { return Err(Error::NotAuthorized); } caller.require_auth(); @@ -191,15 +180,16 @@ impl AnchornetContract { /// Returns the currently appointed operator, or [`Error::NoOperator`] if /// none has been appointed. pub fn operator(env: Env) -> Result { - if !storage::has_operator(&env) { - return Err(Error::NoOperator); - } - Ok(storage::get_operator(&env)) + storage::get_operator(&env) } /// Returns `true` if `address` is the currently appointed operator. pub fn is_operator(env: Env, address: Address) -> bool { - storage::has_operator(&env) && storage::get_operator(&env) == address + match storage::get_operator(&env) { + Ok(operator) => operator == address, + // No operator appointed — no address can be the operator. + Err(_) => false, + } } /// Pauses the contract, blocking liquidity and settlement mutations. @@ -367,6 +357,10 @@ impl AnchornetContract { let total = list.len(); let mut idx = start; while idx < total && (out.len() as u32) < limit { + // `idx` is strictly below `total`, which is this list's length + // captured before the loop, and the list is never mutated inside + // the loop, so `get(idx)` is always in bounds. + // INVARIANT: in-bounds `Vec::get` — `None` is unreachable here. let anchor = list.get(idx).unwrap(); if storage::is_anchor(&env, &anchor) && storage::is_fee_waived(&env, &anchor) { out.push_back(anchor); @@ -465,6 +459,10 @@ impl AnchornetContract { let total = list.len(); let mut idx = start; while idx < total && (out.len() as u32) < limit { + // `idx` is strictly below `total`, which is this list's length + // captured before the loop, and the list is never mutated inside + // the loop, so `get(idx)` is always in bounds. + // INVARIANT: in-bounds `Vec::get` — `None` is unreachable here. let anchor = list.get(idx).unwrap(); if storage::is_anchor(&env, &anchor) { out.push_back(anchor); @@ -968,6 +966,10 @@ impl AnchornetContract { let total = list.len(); let mut idx = start; while idx < total && (out.len() as u32) < limit { + // `idx` is strictly below `total`, which is this list's length + // captured before the loop, and the list is never mutated inside + // the loop, so `get(idx)` is always in bounds. + // INVARIANT: in-bounds `Vec::get` — `None` is unreachable here. out.push_back(list.get(idx).unwrap()); idx += 1; } @@ -1069,6 +1071,10 @@ impl AnchornetContract { let total = assets.len(); let mut idx = start; while idx < total && (out.len() as u32) < limit { + // `idx` is strictly below `total`, which is this list's length + // captured before the loop, and the list is never mutated inside + // the loop, so `get(idx)` is always in bounds. + // INVARIANT: in-bounds `Vec::get` — `None` is unreachable here. let asset = assets.get(idx).unwrap(); let balance = storage::get_balance(&env, &provider, &asset); if balance != 0 { @@ -1422,10 +1428,10 @@ impl AnchornetContract { impl AnchornetContract { /// Requires the call to be authorized by the current administrator. fn require_admin(env: &Env) -> Result<(), Error> { - if !storage::has_admin(env) { - return Err(Error::NotInitialized); - } - let admin = storage::get_admin(env); + // `get_admin` returns `Error::NotInitialized` when the contract has + // not been initialized, so authorization is only demanded once an + // administrator exists. + let admin = storage::get_admin(env)?; admin.require_auth(); Ok(()) } @@ -1444,11 +1450,13 @@ impl AnchornetContract { /// since Soroban contracts have no implicit "sender" and the two /// eligible identities must be told apart before demanding a signature. fn require_admin_or_operator(env: &Env, caller: &Address) -> Result<(), Error> { - if !storage::has_admin(env) { - return Err(Error::NotInitialized); - } - let is_admin = *caller == storage::get_admin(env); - let is_operator = storage::has_operator(env) && *caller == storage::get_operator(env); + // `get_admin` returns `Error::NotInitialized` when the contract has + // not been initialized, so that state is reported before any + // authorization is demanded. The operator side short-circuits on + // [`has_operator`], so `get_operator` can only fail when no operator + // exists, in which case `is_operator` is already `false`. + let is_admin = *caller == storage::get_admin(env)?; + let is_operator = storage::has_operator(env) && *caller == storage::get_operator(env)?; if !is_admin && !is_operator { return Err(Error::NotAuthorized); } diff --git a/src/storage.rs b/src/storage.rs index b3aab92..ff7ecb8 100644 --- a/src/storage.rs +++ b/src/storage.rs @@ -27,6 +27,7 @@ use soroban_sdk::{contracttype, Address, Env, Symbol, Vec}; +use crate::error::Error; use crate::types::{AnchorStatus, Pool, Settlement}; const DAY_IN_LEDGERS: u32 = 17_280; @@ -105,10 +106,18 @@ pub fn has_admin(env: &Env) -> bool { env.storage().instance().has(&DataKey::Admin) } -/// Reads the administrator address. Panics if uninitialized — callers should -/// guard with [`has_admin`] first. -pub fn get_admin(env: &Env) -> Address { - env.storage().instance().get(&DataKey::Admin).unwrap() +/// Reads the administrator address. +/// +/// Returns [`Error::NotInitialized`] when no administrator has been stored +/// yet, so callers can surface a typed, decodable error instead of trapping +/// on an unguarded `unwrap`. Every public entrypoint that needs the admin +/// either propagates this error with `?` (see `require_admin`) or treats it +/// as the contract's uninitialized state. +pub fn get_admin(env: &Env) -> Result { + env.storage() + .instance() + .get(&DataKey::Admin) + .ok_or(Error::NotInitialized) } /// Persists the administrator address in instance storage. @@ -116,19 +125,17 @@ pub fn set_admin(env: &Env, admin: &Address) { env.storage().instance().set(&DataKey::Admin, admin); } -/// Returns `true` if an admin transfer has been proposed and not yet -/// accepted or overwritten. -pub fn has_pending_admin(env: &Env) -> bool { - env.storage().instance().has(&DataKey::PendingAdmin) -} - -/// Reads the proposed next administrator. Panics if none is pending — -/// callers should guard with [`has_pending_admin`] first. -pub fn get_pending_admin(env: &Env) -> Address { +/// Reads the proposed next administrator. +/// +/// Returns [`Error::NoPendingAdmin`] when no transfer is pending, so callers +/// can surface a typed, decodable error instead of trapping on an unguarded +/// `unwrap`. Every public entrypoint that needs the pending admin propagates +/// this error with `?`. +pub fn get_pending_admin(env: &Env) -> Result { env.storage() .instance() .get(&DataKey::PendingAdmin) - .unwrap() + .ok_or(Error::NoPendingAdmin) } /// Persists the proposed next administrator. @@ -148,10 +155,18 @@ pub fn has_operator(env: &Env) -> bool { env.storage().instance().has(&DataKey::Operator) } -/// Reads the operator address. Panics if none is appointed — callers should -/// guard with [`has_operator`] first. -pub fn get_operator(env: &Env) -> Address { - env.storage().instance().get(&DataKey::Operator).unwrap() +/// Reads the operator address. +/// +/// Returns [`Error::NoOperator`] when no operator has been appointed, so +/// callers can surface a typed, decodable error instead of trapping on an +/// unguarded `unwrap`. Every public entrypoint that needs the operator +/// either propagates this error with `?` or guards with [`has_operator`] +/// first. +pub fn get_operator(env: &Env) -> Result { + env.storage() + .instance() + .get(&DataKey::Operator) + .ok_or(Error::NoOperator) } /// Persists the operator address in instance storage. diff --git a/src/test.rs b/src/test.rs index 503dd13..4abcf66 100644 --- a/src/test.rs +++ b/src/test.rs @@ -186,6 +186,17 @@ fn test_initialize_sets_admin() { assert_eq!(client.admin(), admin); } +#[test] +fn test_admin_fails_before_initialize() { + let env = Env::default(); + let (client, _admin) = setup(&env); + + // Reading the admin on an uninitialized contract must surface the typed + // `NotInitialized` error (via the storage accessor), not a host trap. + let err = client.try_admin().err().unwrap().unwrap(); + assert_eq!(err, Error::NotInitialized); +} + #[test] fn test_initialize_twice_fails() { let env = Env::default(); diff --git a/test_snapshots/test/test_admin_fails_before_initialize.1.json b/test_snapshots/test/test_admin_fails_before_initialize.1.json new file mode 100644 index 0000000..b6dae70 --- /dev/null +++ b/test_snapshots/test/test_admin_fails_before_initialize.1.json @@ -0,0 +1,61 @@ +{ + "generators": { + "address": 2, + "nonce": 0, + "mux_id": 0 + }, + "auth": [ + [], + [] + ], + "ledger": { + "protocol_version": 25, + "sequence_number": 0, + "timestamp": 0, + "network_id": "0000000000000000000000000000000000000000000000000000000000000000", + "base_reserve": 0, + "min_persistent_entry_ttl": 4096, + "min_temp_entry_ttl": 16, + "max_entry_ttl": 6312000, + "ledger_entries": [ + { + "entry": { + "last_modified_ledger_seq": 0, + "data": { + "contract_data": { + "ext": "v0", + "contract": "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAD2KM", + "key": "ledger_key_contract_instance", + "durability": "persistent", + "val": { + "contract_instance": { + "executable": { + "wasm": "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855" + }, + "storage": null + } + } + } + }, + "ext": "v0" + }, + "live_until": 4095 + }, + { + "entry": { + "last_modified_ledger_seq": 0, + "data": { + "contract_code": { + "ext": "v0", + "hash": "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855", + "code": "" + } + }, + "ext": "v0" + }, + "live_until": 4095 + } + ] + }, + "events": [] +} \ No newline at end of file