fix: harden keeper, product-name and confidence validation (#569 #570 #571 #572)

contracts/claims-processor/src/lib.rs

@@ -298,8 +298,17 @@ impl ClaimsProcessor {
 
     /// Admin-only: authorize `keeper` to call process_claim / auto_process /
     /// batch_auto_process. Without this, no address can settle claims.
+    ///
+    /// `keeper` must be a well-formed Stellar address (issue #572). A keeper
+    /// entry is a settlement authority: whatever holds it can move coverage
+    /// USDC out of the pool. Admitting an address whose StrKey encoding is
+    /// malformed writes a permanent registry entry that can never be
+    /// legitimately exercised and that off-chain indexers will silently fail
+    /// to resolve when reconciling `keeper_added` events, so the format is
+    /// checked up front and the write is refused.
     pub fn add_keeper(env: Env, admin: Address, keeper: Address) {
         Self::require_admin(&env, &admin);
+        Self::validate_stellar_address(&env, &keeper);
         env.storage().persistent().set(&StorageKey::Keeper(keeper.clone()), &true);
         env.storage().persistent().extend_ttl(&StorageKey::Keeper(keeper.clone()), TTL_THRESHOLD, TTL_EXTEND_TO);
         env.events().publish(
@@ -309,6 +318,12 @@ impl ClaimsProcessor {
     }
 
     /// Admin-only: revoke a keeper's settlement authority.
+    ///
+    /// Deliberately does *not* re-run the format check that `add_keeper`
+    /// applies. This is the remediation path for a bad registry entry: an
+    /// admin must be able to evict a keeper that was admitted before the
+    /// check existed, or a malformed entry would be unremovable. Removing an
+    /// unknown address is a no-op either way.
     pub fn remove_keeper(env: Env, admin: Address, keeper: Address) {
         Self::require_admin(&env, &admin);
         env.storage().persistent().remove(&StorageKey::Keeper(keeper.clone()));
@@ -2162,19 +2177,19 @@ impl ClaimsProcessor {
         id
     }
 
+    /// Panic unless `address` is a well-formed Stellar address.
+    ///
+    /// Delegates the actual format decision to [`is_well_formed_strkey`] so
+    /// the rule lives in one pure, directly testable function instead of
+    /// being spread across the call sites that need it (issue #572).
     fn validate_stellar_address(env: &Env, address: &Address) {
         let addr_str = address.to_string();
-
-        // Check length: Stellar public keys are exactly 56 characters
-        if addr_str.len() != 56 {
+        let mut buf = [0u8; STRKEY_LEN];
+        if addr_str.len() as usize != STRKEY_LEN {
             panic_with_error!(env, Error::InvalidAddress);
         }
-
-        let mut buf = [0u8; 56];
         addr_str.copy_into_slice(&mut buf);
-
-        // Check prefix: G (Stellar account) or C (Stellar contract)
-        if buf[0] != b'G' && buf[0] != b'C' {
+        if !is_well_formed_strkey(&buf) {
             panic_with_error!(env, Error::InvalidAddress);
         }
     }
@@ -2373,9 +2388,49 @@ fn map_comparison(
     }
 }
 
+/// Length of a Stellar `StrKey` (a `G…` account id or a `C…` contract id):
+/// one version character plus 55 base32 characters encoding the 33-byte
+/// (version + 32-byte payload) key, rounded up to a whole 5-bit group.
+const STRKEY_LEN: usize = 56;
+
+/// Whether `bytes` is a syntactically well-formed Stellar address string
+/// (issue #572).
+///
+/// A `StrKey` is 56 bytes long and encodes the 33-byte
+/// `version || payload` buffer in unpadded base32 (RFC 4648). Two version
+/// characters are meaningful on the network: `G` for a classic account and
+/// `C` for a contract. Everything after the version byte must therefore be a
+/// base32 symbol — `A`–`Z` or `2`–`7` — which is what makes this a real format
+/// check rather than the prefix test it replaced. Note that the StrKey
+/// checksum is intentionally *not* verified: the host already guarantees a
+/// canonical encoding for any `Address` it hands us, so a checksum failure is
+/// not reachable from here, and re-deriving one on-chain would cost a CRC16
+/// for no additional guarantee.
+///
+/// Takes bytes rather than `&str` because `soroban_sdk::String` (what
+/// `Address::to_string` returns) cannot be borrowed as `str`. Pure and
+/// infallible, so it can be exercised directly against byte strings that no
+/// `Address` could ever be constructed from.
+fn is_well_formed_strkey(bytes: &[u8]) -> bool {
+    if bytes.len() != STRKEY_LEN {
+        return false;
+    }
+    // Version byte: G (account) or C (contract).
+    if bytes[0] != b'G' && bytes[0] != b'C' {
+        return false;
+    }
+    bytes[1..]
+        .iter()
+        .all(|&b| (b >= b'A' && b <= b'Z') || (b >= b'2' && b <= b'7'))
+}
+
 #[cfg(test)]
 mod test;
 #[cfg(test)]
 mod test_integration;
 #[cfg(test)]
 mod test_advanced;
+#[cfg(test)]
+mod test_keeper_registry;
+#[cfg(test)]
+mod test_claim_uniqueness;