Seven unwrap()/panic! sites in contract source bypass the typed e…

.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

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]

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<Address, Error> {
-        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,31 +105,25 @@ 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);
         events::admin_proposed(&env, &candidate);
         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<Address, Error> {
-        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<Address, Error> {
-        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);
         }

src/storage.rs

@@ -40,6 +40,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;
@@ -136,11 +137,26 @@ pub fn has_admin(env: &Env) -> bool {
     env.storage().instance().has(&DataKey::Admin)
 }
 
+ Seven-`unwrap()`/`panic!`-sites-in-contract-source-bypass-the-typed-error-enum-and-trap-with-opaque-host-errors-#263
+/// 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<Address, Error> {
+    env.storage()
+        .instance()
+        .get(&DataKey::Admin)
+        .ok_or(Error::NotInitialized)
+
 /// Reads the administrator address. Panics if uninitialized — callers should
 /// guard with [`has_admin`] first.
 pub fn get_admin(env: &Env) -> Address {
     bump_instance(env);
     env.storage().instance().get(&DataKey::Admin).unwrap()
+
 }
 
 /// Persists the administrator address in instance storage.
@@ -149,6 +165,15 @@ pub fn set_admin(env: &Env, admin: &Address) {
     bump_instance(env);
 }
 
+ Seven-`unwrap()`/`panic!`-sites-in-contract-source-bypass-the-typed-error-enum-and-trap-with-opaque-host-errors-#263
+/// 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<Address, Error> {
+
 /// Returns `true` if an admin transfer has been proposed and not yet
 /// accepted or overwritten.
 pub fn has_pending_admin(env: &Env) -> bool {
@@ -160,10 +185,11 @@ pub fn has_pending_admin(env: &Env) -> bool {
 /// callers should guard with [`has_pending_admin`] first.
 pub fn get_pending_admin(env: &Env) -> Address {
     bump_instance(env);
+
     env.storage()
         .instance()
         .get(&DataKey::PendingAdmin)
-        .unwrap()
+        .ok_or(Error::NoPendingAdmin)
 }
 
 /// Persists the proposed next administrator.
@@ -186,11 +212,26 @@ pub fn has_operator(env: &Env) -> bool {
     env.storage().instance().has(&DataKey::Operator)
 }
 
+ Seven-`unwrap()`/`panic!`-sites-in-contract-source-bypass-the-typed-error-enum-and-trap-with-opaque-host-errors-#263
+/// 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<Address, Error> {
+    env.storage()
+        .instance()
+        .get(&DataKey::Operator)
+        .ok_or(Error::NoOperator)
+
 /// Reads the operator address. Panics if none is appointed — callers should
 /// guard with [`has_operator`] first.
 pub fn get_operator(env: &Env) -> Address {
     bump_instance(env);
     env.storage().instance().get(&DataKey::Operator).unwrap()
+
 }
 
 /// Persists the operator address in instance storage.

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();

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": []
+}