From 73433ad37eef5520ed229e4ba4016bbed0f299d8 Mon Sep 17 00:00:00 2001 From: Alex Buchkvoskyi Date: Mon, 24 Aug 2026 18:56:13 +0300 Subject: [PATCH 1/3] fix(soroban): stop declaring an error the registry cannot return (FIND-013) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RegistryError::InvalidSignature could never be returned. A failed ed25519 verification traps: the host builds a HostError (soroban-env-host crypto/mod.rs) and soroban-sdk discards it (`let _ = ...`), so a contract never sees the failure as a value. The declared error told integrators to write handling that could not fire, and validate_attestation's Result compounded it by implying an Ok(false) that never existed either. Removes the variant from RegistryError and from predicate-client's mirror of it, and narrows the return type to Result<(), RegistryError>. Discriminant 8 is left reserved rather than renumbered: renumbering would silently change the meaning of error codes already observed on-chain, which is a worse break than a gap. validate_attestation now documents both failure paths, since callers have to handle a typed error and an abort. The audit's fourth recommendation — map a failed verification onto InvalidSignature — is not available in soroban-sdk 23.5.3, which exposes no fallible ed25519 API. The only route would be a self-call through try_invoke_contract to catch the trap, paying a cross-contract invocation on every validation for an error code integrators cannot act on differently. Documented instead. test_invalid_signature_aborts_and_leaves_uuid_unspent pins both halves of the new documentation: try_validate_attestation returns Err(Err(InvokeError::Abort)) rather than a contract error, which is the mismatch itself, and the same uuid still validates afterwards with a correct signature. That second property comes from the host rolling back the invocation rather than from check ordering. Verified the test fails when the forged signature is made genuine. Co-Authored-By: Claude Opus 5 (1M context) --- soroban/predicate-client/src/lib.rs | 22 +++--- soroban/predicate-registry/src/lib.rs | 80 ++++++++++++++++++-- soroban/predicate-registry/src/types.rs | 8 +- soroban/predicate-registry/src/validation.rs | 18 ++++- 4 files changed, 108 insertions(+), 20 deletions(-) diff --git a/soroban/predicate-client/src/lib.rs b/soroban/predicate-client/src/lib.rs index bfa876ce..63839f6c 100644 --- a/soroban/predicate-client/src/lib.rs +++ b/soroban/predicate-client/src/lib.rs @@ -41,7 +41,7 @@ pub enum RegistryError { UuidAlreadyUsed = 5, UuidMismatch = 6, ExpirationMismatch = 7, - InvalidSignature = 8, + // 8 is reserved; see predicate-registry's RegistryError (FIND-013). NotInitialized = 9, AlreadyInitialized = 10, } @@ -56,11 +56,14 @@ pub enum RegistryError { /// constructed statement, mirroring the EVM pattern where these values originate /// from the attester's signed payload. /// -/// Returns `()` on success. On failure the registry returns an `Err`, and -/// `invoke_contract` propagates it as a trap carrying the registry's exact typed -/// error (e.g. `Error(Contract, #4)` for an expired attestation). The registry -/// never returns `Ok(false)`, so there is no boolean outcome for the caller to -/// branch on — a returning call means the transaction was authorized. +/// Returns `()` on success; a returning call means the transaction was authorized. +/// +/// Every failure aborts the invocation rather than returning something to branch +/// on. `invoke_contract` propagates the registry's typed errors as a trap +/// carrying the exact code (e.g. `Error(Contract, #4)` for an expired +/// attestation), and an invalid signature traps inside the registry itself as +/// `Error(Crypto, InvalidInput)` — see `validate_attestation` on why that one +/// cannot be a typed error. /// /// # Every argument must come from the live call /// @@ -122,10 +125,9 @@ pub fn authorize_transaction( target.clone().into_val(e), ]; - // The registry returns `Ok(true)` or traps with a typed `RegistryError`; the - // `true` carries no information, so we discard it and rely on trap propagation - // to surface the real error to the caller. - let _: bool = e.invoke_contract(registry, &Symbol::new(e, "validate_attestation"), args); + // The registry returns `Ok(())` or traps; we rely on trap propagation to + // surface the real error to the caller. + e.invoke_contract::<()>(registry, &Symbol::new(e, "validate_attestation"), args); } #[cfg(test)] diff --git a/soroban/predicate-registry/src/lib.rs b/soroban/predicate-registry/src/lib.rs index d8781e25..eadd8816 100644 --- a/soroban/predicate-registry/src/lib.rs +++ b/soroban/predicate-registry/src/lib.rs @@ -192,12 +192,31 @@ impl PredicateRegistryContract { /// authorization bypass in the integrating contract. Prefer /// `predicate_client::authorize_transaction`, which takes these as arguments /// and assembles the statement itself. See the crate-level trust boundary docs. + /// + /// # Failure modes + /// + /// Returns `Ok(())` on success — there is no boolean, because there was never + /// an `Ok(false)` to distinguish from `Ok(true)`. + /// + /// Failures arrive two different ways, and integrators have to handle both: + /// + /// * Checks on the attestation's shape — expiry, replay, uuid/expiration + /// agreement, attester registration — return a typed [`RegistryError`]. + /// * An **invalid signature traps** rather than returning an error, aborting + /// the invocation with `Error(Crypto, InvalidInput)`. The host escalates the + /// failure before a contract can see it, and soroban-sdk 23.5.3 exposes no + /// fallible ed25519 API, so this cannot be turned into a `RegistryError`. + /// `RegistryError` deliberately has no `InvalidSignature` variant as a + /// result (FIND-013). + /// + /// Either way the UUID is not marked spent, so a rejected attestation can be + /// retried once whatever was wrong with it is fixed. pub fn validate_attestation( e: &Env, statement: Statement, attestation: Attestation, caller: Address, - ) -> Result { + ) -> Result<(), RegistryError> { validation::validate(e, &statement, &attestation, &caller) } @@ -482,8 +501,7 @@ mod test { signature, }; - let result = client.validate_attestation(&statement, &attestation, &client.address); - assert!(result); + client.validate_attestation(&statement, &attestation, &client.address); } #[test] @@ -828,7 +846,7 @@ mod test { // `caller` is the statement's own target, so the digest verified here is // GV_DIGEST unchanged. let caller = Address::from_str(&e, GV_TARGET); - assert!(client.validate_attestation(&statement, &attestation, &caller)); + client.validate_attestation(&statement, &attestation, &caller); } #[test] @@ -864,8 +882,10 @@ mod test { assert!(client.is_attester_registered(&attester)); } + /// Pins the trap documented on `validate_attestation`: a bad signature aborts + /// the invocation with a host error, and never surfaces as a `RegistryError`. #[test] - #[should_panic(expected = "Error(Crypto, InvalidInput)")] // ed25519_verify panics on bad signature + #[should_panic(expected = "Error(Crypto, InvalidInput)")] fn test_validate_invalid_signature() { let e = Env::default(); e.mock_all_auths(); @@ -903,6 +923,56 @@ mod test { client.validate_attestation(&statement, &attestation, &client.address); } + /// The trap is only tolerable because it costs the caller nothing but the fee: + /// nothing is committed, so the uuid stays unspent and the same statement works + /// once a correct signature arrives. `try_validate_attestation` is what lets a + /// caller observe the abort without unwinding — and what it returns shows the + /// ABI mismatch FIND-013 is about, an invocation error rather than a + /// `RegistryError` the caller could match on. + #[test] + fn test_invalid_signature_aborts_and_leaves_uuid_unspent() { + let e = Env::default(); + e.mock_all_auths(); + let (owner, client) = setup(&e); + + let (sk, pub_key) = generate_ed25519_keypair(&e); + let (other_sk, _other_pk) = generate_ed25519_keypair(&e); + client.register_attester(&owner, &pub_key); + + let statement = Statement { + uuid: soroban_sdk::String::from_str(&e, "uuid-retry-after-bad-sig"), + msg_sender: Address::generate(&e), + target: client.address.clone(), + msg_value: 0, + encoded_sig_and_args: soroban_sdk::Bytes::from_slice(&e, &[0u8; 32]), + policy: soroban_sdk::String::from_str(&e, "x-test"), + expiration: e.ledger().timestamp() + 600, + }; + let hash = client.hash_statement(&statement); + + // Signed by a key the registry does not know: verification fails. + let forged = Attestation { + uuid: statement.uuid.clone(), + expiration: statement.expiration, + attester: pub_key.clone(), + signature: sign_hash(&e, &other_sk, &hash), + }; + let outcome = client.try_validate_attestation(&statement, &forged, &client.address); + // Err at the outer level is the invocation failing. The inner Err being an + // InvokeError rather than a RegistryError is the mismatch itself: there is + // no contract error code here for a caller to branch on. + assert_eq!(outcome, Err(Err(soroban_sdk::InvokeError::Abort))); + + // The failed attempt committed nothing, so the same uuid is still spendable. + let genuine = Attestation { + uuid: statement.uuid.clone(), + expiration: statement.expiration, + attester: pub_key, + signature: sign_hash(&e, &sk, &hash), + }; + client.validate_attestation(&statement, &genuine, &client.address); + } + #[test] fn test_uuid_marker_ttl_extended_to_max() { use soroban_sdk::testutils::storage::Persistent as _; diff --git a/soroban/predicate-registry/src/types.rs b/soroban/predicate-registry/src/types.rs index ff080f41..f94d0e2b 100644 --- a/soroban/predicate-registry/src/types.rs +++ b/soroban/predicate-registry/src/types.rs @@ -60,8 +60,12 @@ pub enum RegistryError { UuidMismatch = 6, /// Statement/Attestation expiration mismatch ExpirationMismatch = 7, - /// Ed25519 signature verification failed - InvalidSignature = 8, + // 8 was InvalidSignature. The host traps on a failed ed25519 verification + // instead of returning a value, so the registry could never produce this + // error; declaring it only misled integrators into writing handling that + // could not fire (FIND-013). The discriminant stays reserved — reusing 8 for + // something else would make existing integrator code read a new error as + // "invalid signature". /// Contract has not been initialized NotInitialized = 9, /// Contract has already been initialized diff --git a/soroban/predicate-registry/src/validation.rs b/soroban/predicate-registry/src/validation.rs index 4b0fb4bf..855ef6a2 100644 --- a/soroban/predicate-registry/src/validation.rs +++ b/soroban/predicate-registry/src/validation.rs @@ -48,12 +48,15 @@ pub fn compute_hash(e: &Env, statement: &Statement) -> BytesN<32> { /// 6. Ed25519 signature verification using caller-bound hash (hashStatementSafe) /// 7. Marks UUID as spent /// 8. Emits validation event +/// +/// Returns `Ok(())` when every check passes. Checks 1-5 return a typed +/// `RegistryError`; check 6 does not — see the note there. pub fn validate( e: &Env, statement: &Statement, attestation: &Attestation, caller: &Address, -) -> Result { +) -> Result<(), RegistryError> { // 0. Authenticate the caller — mirrors EVM's implicit msg.sender guarantee. // Without this, anyone could call validate_attestation with an arbitrary // caller address and burn valid UUIDs. @@ -99,7 +102,16 @@ pub fn validate( }; let hash = compute_hash(e, &safe_statement); let hash_bytes: Bytes = Bytes::from_slice(e, &hash.to_array()); - // NOTE: ed25519_verify panics on invalid signature + // A failed verification does NOT return Err — it traps, aborting the whole + // invocation with `Error(Crypto, InvalidInput)`. The host builds a HostError + // (soroban-env-host crypto/mod.rs) and the SDK discards it (`let _ = ...`), so + // a contract cannot observe the failure as a value; there is no fallible + // ed25519 API in soroban-sdk 23.5.3 to map onto a RegistryError. Callers must + // treat an invalid signature as an aborted invocation, not a returned error. + // + // This is why RegistryError has no InvalidSignature variant (FIND-013): an + // error the contract can never return is worse than none, because integrators + // write handling for it that cannot fire. e.crypto() .ed25519_verify(&attestation.attester, &hash_bytes, &attestation.signature); @@ -129,5 +141,5 @@ pub fn validate( ), ); - Ok(true) + Ok(()) } From 4d85a5d68eac972caa24d7e1f532cc1b026a5d26 Mon Sep 17 00:00:00 2001 From: Alex Buchkvoskyi Date: Mon, 24 Aug 2026 22:16:59 +0300 Subject: [PATCH 2/3] docs(soroban): write comments for a first deployment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Comments explained the code by contrast with earlier shapes of it — a boolean return that no longer exists, a network passphrase in the digest preimage, tokens deployed before the constructor registered their policy. Nothing integrates with this registry yet, so there is no history for a reader to need, and the audit ticket ids are no more use to them than the history is. Two consequences of there being no integrations, beyond wording: RegistryError keeps no reserved gap at 8. Nothing observes these codes on-chain yet, so nothing would misread a renumbered one, and NotInitialized simply takes 8. AlreadyInitialized is removed. It was never constructed — __constructor runs once at deploy, so it is unreachable — which is the same defect as declaring an InvalidSignature the host never lets the contract return. No test asserts codes 9 or 10, and 1-7 are unchanged. --- soroban/example-compliant-token/src/lib.rs | 7 +++---- soroban/predicate-client/src/lib.rs | 4 +--- soroban/predicate-registry/src/lib.rs | 14 ++++++-------- soroban/predicate-registry/src/types.rs | 10 +--------- soroban/predicate-registry/src/validation.rs | 19 +++++++++---------- 5 files changed, 20 insertions(+), 34 deletions(-) diff --git a/soroban/example-compliant-token/src/lib.rs b/soroban/example-compliant-token/src/lib.rs index 59bb65a6..1e971a0b 100644 --- a/soroban/example-compliant-token/src/lib.rs +++ b/soroban/example-compliant-token/src/lib.rs @@ -62,9 +62,8 @@ impl CompliantTokenContract { /// Register this contract's policy with the Predicate Registry. /// /// The constructor already does this, so a freshly deployed token needs no - /// follow-up call. Retained for tokens deployed before that was the case, - /// and to re-register if the registry entry is ever cleared. The admin - /// must authorize. + /// follow-up call. Kept so the entry can be re-registered if the registry's + /// mapping is ever cleared. The admin must authorize. pub fn register_policy(e: &Env) { let admin: Address = e.storage().instance().get(&ADMIN).unwrap(); admin.require_auth(); @@ -429,7 +428,7 @@ mod test { /// both `msg_value` and `encoded_sig_and_args` from its live `amount`, so an /// attestation approved for one amount cannot be spent at another — the case /// an integration would re-open by forwarding a user-supplied amount into the - /// statement instead of rebuilding it (audit FIND-002). + /// statement instead of rebuilding it. #[test] #[should_panic(expected = "Error(Crypto, InvalidInput)")] // signed digest bound 250, not 100 fn test_transfer_tampered_amount_rejected() { diff --git a/soroban/predicate-client/src/lib.rs b/soroban/predicate-client/src/lib.rs index 63839f6c..bcc1f444 100644 --- a/soroban/predicate-client/src/lib.rs +++ b/soroban/predicate-client/src/lib.rs @@ -41,9 +41,7 @@ pub enum RegistryError { UuidAlreadyUsed = 5, UuidMismatch = 6, ExpirationMismatch = 7, - // 8 is reserved; see predicate-registry's RegistryError (FIND-013). - NotInitialized = 9, - AlreadyInitialized = 10, + NotInitialized = 8, } // --- Client helper --- diff --git a/soroban/predicate-registry/src/lib.rs b/soroban/predicate-registry/src/lib.rs index eadd8816..e4657435 100644 --- a/soroban/predicate-registry/src/lib.rs +++ b/soroban/predicate-registry/src/lib.rs @@ -195,10 +195,8 @@ impl PredicateRegistryContract { /// /// # Failure modes /// - /// Returns `Ok(())` on success — there is no boolean, because there was never - /// an `Ok(false)` to distinguish from `Ok(true)`. - /// - /// Failures arrive two different ways, and integrators have to handle both: + /// Returns `Ok(())` when every check passes. Failures arrive two different + /// ways, and integrators have to handle both: /// /// * Checks on the attestation's shape — expiry, replay, uuid/expiration /// agreement, attester registration — return a typed [`RegistryError`]. @@ -207,7 +205,7 @@ impl PredicateRegistryContract { /// failure before a contract can see it, and soroban-sdk 23.5.3 exposes no /// fallible ed25519 API, so this cannot be turned into a `RegistryError`. /// `RegistryError` deliberately has no `InvalidSignature` variant as a - /// result (FIND-013). + /// result. /// /// Either way the UUID is not marked spent, so a rejected attestation can be /// retried once whatever was wrong with it is fixed. @@ -926,9 +924,9 @@ mod test { /// The trap is only tolerable because it costs the caller nothing but the fee: /// nothing is committed, so the uuid stays unspent and the same statement works /// once a correct signature arrives. `try_validate_attestation` is what lets a - /// caller observe the abort without unwinding — and what it returns shows the - /// ABI mismatch FIND-013 is about, an invocation error rather than a - /// `RegistryError` the caller could match on. + /// caller observe the abort without unwinding — and what it returns shows why + /// there is no `InvalidSignature` error to match on: an invocation error, not a + /// `RegistryError`. #[test] fn test_invalid_signature_aborts_and_leaves_uuid_unspent() { let e = Env::default(); diff --git a/soroban/predicate-registry/src/types.rs b/soroban/predicate-registry/src/types.rs index f94d0e2b..21f7e874 100644 --- a/soroban/predicate-registry/src/types.rs +++ b/soroban/predicate-registry/src/types.rs @@ -60,14 +60,6 @@ pub enum RegistryError { UuidMismatch = 6, /// Statement/Attestation expiration mismatch ExpirationMismatch = 7, - // 8 was InvalidSignature. The host traps on a failed ed25519 verification - // instead of returning a value, so the registry could never produce this - // error; declaring it only misled integrators into writing handling that - // could not fire (FIND-013). The discriminant stays reserved — reusing 8 for - // something else would make existing integrator code read a new error as - // "invalid signature". /// Contract has not been initialized - NotInitialized = 9, - /// Contract has already been initialized - AlreadyInitialized = 10, + NotInitialized = 8, } diff --git a/soroban/predicate-registry/src/validation.rs b/soroban/predicate-registry/src/validation.rs index 855ef6a2..75be6637 100644 --- a/soroban/predicate-registry/src/validation.rs +++ b/soroban/predicate-registry/src/validation.rs @@ -12,11 +12,10 @@ use crate::types::{Attestation, RegistryError, Statement}; /// `hashStatementWithExpiry`. /// /// There is no separate version tag. XDR is self-describing and length-prefixed, -/// so layouts cannot be confused for one another: this preimage opens with -/// `ScVal::Bytes`, where the previous one (a network passphrase) opened with -/// `ScVal::String`, and a statement is an `ScVal::Map` that no appended field -/// could impersonate. Changing the layout is therefore already a hard break, and -/// a tag would only restate that. +/// so one layout cannot be parsed as another: the network id is `ScVal::Bytes` +/// and the statement an `ScVal::Map`, and no appended field could impersonate +/// either. Any change to the layout is therefore already a hard break, which a +/// tag would only restate. /// /// The registry's own address is deliberately *not* in the preimage, matching /// EVM and Solana. Replay across registry instances is already constrained by @@ -109,17 +108,17 @@ pub fn validate( // ed25519 API in soroban-sdk 23.5.3 to map onto a RegistryError. Callers must // treat an invalid signature as an aborted invocation, not a returned error. // - // This is why RegistryError has no InvalidSignature variant (FIND-013): an + // This is why RegistryError has no InvalidSignature variant: an // error the contract can never return is worse than none, because integrators // write handling for it that cannot fire. e.crypto() .ed25519_verify(&attestation.attester, &hash_bytes, &attestation.signature); // 7. Mark UUID as spent. - // Extend the replay marker to the maximum possible TTL. A fixed short TTL - // (~30 days) could be archived/evicted while a longer-lived attestation is - // still valid, which would re-open replay. Tying the marker to the network - // max keeps the guard alive for as long as the ledger allows. + // Extend the replay marker to the maximum possible TTL. A short TTL could + // be archived while a longer-lived attestation is still valid, which would + // re-open replay; tying the marker to the network max keeps the guard alive + // for as long as the ledger allows. e.storage().persistent().set(&uuid_key, &true); e.storage() .persistent() From cc46c4497375a4233db88ad5987755572a2c0a75 Mon Sep 17 00:00:00 2001 From: Alex Buchkvoskyi Date: Wed, 26 Aug 2026 15:42:51 +0300 Subject: [PATCH 3/3] refactor(soroban): cut comments that restate the code The registry and client carried roughly 360 lines of comment for 400 lines of code: numbered step markers above each guard clause, section banners, and doc comments that repeated the function name back. Reading validate meant reading past a summary of itself. What remains states something the code cannot: units and encodings on Statement fields, why the digest binds the caller rather than the statement's target, that a failed ed25519 verification traps instead of returning, that the golden-vector constants must never be regenerated from hash_statement, and the trust boundary integrators have to respect. RegistryError keeps its per-variant lines because Soroban puts them in the contract spec. verify-golden-vector-detects-drift.sh anchored one of its mutations on a deleted comment. It failed loudly rather than reporting a mutation it had not applied; the anchor is now code-only. Co-Authored-By: Claude Opus 5 (1M context) --- soroban/predicate-client/src/lib.rs | 67 +----- soroban/predicate-registry/src/attesters.rs | 16 +- soroban/predicate-registry/src/lib.rs | 213 ++++-------------- soroban/predicate-registry/src/policy.rs | 3 +- soroban/predicate-registry/src/types.rs | 26 +-- soroban/predicate-registry/src/validation.rs | 82 ++----- .../verify-golden-vector-detects-drift.sh | 1 - 7 files changed, 72 insertions(+), 336 deletions(-) diff --git a/soroban/predicate-client/src/lib.rs b/soroban/predicate-client/src/lib.rs index bcc1f444..423db7ed 100644 --- a/soroban/predicate-client/src/lib.rs +++ b/soroban/predicate-client/src/lib.rs @@ -5,9 +5,8 @@ use soroban_sdk::{ Vec, }; -// --- Types (mirrored from predicate-registry to avoid linking the contract impl) --- +// Mirrored from predicate-registry so integrators need not link the contract impl. -/// Describes a transaction to be authorized. #[contracttype] #[derive(Clone, Debug, Eq, PartialEq)] pub struct Statement { @@ -20,7 +19,6 @@ pub struct Statement { pub expiration: u64, } -/// Ed25519-signed authorization from an attester. #[contracttype] #[derive(Clone, Debug, Eq, PartialEq)] pub struct Attestation { @@ -44,57 +42,17 @@ pub enum RegistryError { NotInitialized = 8, } -// --- Client helper --- - -/// Build a Statement and validate it against the Predicate Registry. -/// -/// This is the Soroban equivalent of the EVM PredicateClient._authorizeTransaction() pattern. -/// -/// The `uuid` and `expiration` fields are copied from the attestation into the -/// constructed statement, mirroring the EVM pattern where these values originate -/// from the attester's signed payload. -/// -/// Returns `()` on success; a returning call means the transaction was authorized. -/// -/// Every failure aborts the invocation rather than returning something to branch -/// on. `invoke_contract` propagates the registry's typed errors as a trap -/// carrying the exact code (e.g. `Error(Contract, #4)` for an expired -/// attestation), and an invalid signature traps inside the registry itself as -/// `Error(Crypto, InvalidInput)` — see `validate_attestation` on why that one -/// cannot be a typed error. -/// -/// # Every argument must come from the live call -/// -/// This helper exists to put the registry's trust boundary somewhere hard to get -/// wrong. The registry establishes only `target` (from the authenticated caller) -/// and the network (from the ledger) on its own; every other statement field is -/// taken on trust. So a returning call means "an attester signed the statement -/// built from these arguments" — which authorizes the action actually executing -/// only if the arguments describe it. -/// -/// Derive each one from the function you are protecting, never from a parameter an -/// end user can choose. Forwarding user-supplied values here validates one action -/// while executing another, with no signature forgery involved. +/// Assembles a Statement from the live call and has the registry validate it. /// -/// # Arguments -/// * `e` - Soroban environment -/// * `registry` - Address of the deployed PredicateRegistry contract -/// * `attestation` - The signed attestation from an authorized attester -/// * `encoded_sig_and_args` - Encoding of the *concrete* call: selector plus every -/// argument that matters for compliance. Omitting an argument leaves it free to -/// change between attestation and execution — see `encode_transfer_call` in -/// `example-compliant-token`, and the tampered-recipient/amount tests that pin it -/// * `msg_sender` - The live sender, already `require_auth()`ed by the caller -/// * `msg_value` - The live value (token amount, equivalent to EVM msg.value) -/// * `target` - The contract being called — callers should pass `e.current_contract_address()` -/// so that the registry's hashStatementSafe logic can bind the attestation to this contract -/// * `policy` - This contract's configured policy, read from its own storage +/// Returning means authorized; every failure aborts the invocation instead. /// -/// Domain separation (the network) is derived by the registry from the ledger, so -/// there is nothing for the integrator to configure or get wrong here. -// The argument list mirrors the Statement fields on purpose. Collapsing it into a -// params struct would make it natural to build one value and reuse it across -// calls, which is exactly what the section above rules out. +/// Each argument must describe the call being authorized — `target` is +/// `e.current_contract_address()`, `encoded_sig_and_args` covers every argument +/// that matters for compliance, `policy` comes from this contract's storage. +/// Forwarding a user-supplied value here authorizes one action while executing +/// another, with no signature forgery involved. +// Kept as positional arguments rather than a params struct: a struct invites +// building one value and reusing it, which is what the above rules out. #[allow(clippy::too_many_arguments)] pub fn authorize_transaction( e: &Env, @@ -123,8 +81,6 @@ pub fn authorize_transaction( target.clone().into_val(e), ]; - // The registry returns `Ok(())` or traps; we rely on trap propagation to - // surface the real error to the caller. e.invoke_contract::<()>(registry, &Symbol::new(e, "validate_attestation"), args); } @@ -173,7 +129,6 @@ mod test { let encoded = Bytes::from_slice(&e, &[0xBBu8; 16]); let msg_value: i128 = 1000; - // Build statement matching what authorize_transaction will build let statement = predicate_registry::Statement { uuid: String::from_str(&e, "uuid-client-test"), msg_sender: msg_sender.clone(), @@ -194,8 +149,6 @@ mod test { signature, }; - // A returning call means the transaction was authorized; a failed - // validation would trap and fail the test. authorize_transaction( &e, ®istry_addr, diff --git a/soroban/predicate-registry/src/attesters.rs b/soroban/predicate-registry/src/attesters.rs index 0a17a2fa..2b3bc3e6 100644 --- a/soroban/predicate-registry/src/attesters.rs +++ b/soroban/predicate-registry/src/attesters.rs @@ -1,17 +1,10 @@ -//! Attester registration and deregistration. -//! -//! The attester set is held as a single `Vec` in instance storage. The registry -//! is expected to hold a very small set (typically one attester, at most a -//! handful), so membership checks and removals scan the vector linearly rather -//! than maintaining auxiliary per-attester flag/index entries. Because the set -//! lives in instance storage, its lifetime is tied to the contract instance and -//! needs no per-entry TTL bookkeeping. +//! The set is a single `Vec` in instance storage, scanned linearly: it holds a +//! handful of keys at most, and instance storage needs no per-entry TTL upkeep. use soroban_sdk::{symbol_short, BytesN, Env, Vec}; use crate::types::RegistryError; -// Storage key const ATTESTERS_KEY: soroban_sdk::Symbol = symbol_short!("atts"); fn load(e: &Env) -> Vec> { @@ -25,9 +18,7 @@ fn store(e: &Env, attesters: &Vec>) { e.storage().instance().set(&ATTESTERS_KEY, attesters); } -/// Extend the contract instance TTL to the network maximum. The attester set -/// lives in instance storage, so refreshing the instance on every successful -/// validation keeps an actively-used registry from being archived. +/// Keeps an actively-used registry from being archived along with its attesters. pub fn refresh_ttl(e: &Env) { let max_ttl = e.storage().max_ttl(); e.storage().instance().extend_ttl(max_ttl, max_ttl); @@ -58,7 +49,6 @@ pub fn deregister(e: &Env, attester: &BytesN<32>) -> Result<(), RegistryError> { .first_index_of(attester) .ok_or(RegistryError::AttesterNotRegistered)?; - // Swap-and-pop: move the last element into the vacated slot, then truncate. let last_index = attesters.len() - 1; if index != last_index { let last_attester = attesters.get(last_index).unwrap(); diff --git a/soroban/predicate-registry/src/lib.rs b/soroban/predicate-registry/src/lib.rs index e4657435..972a7539 100644 --- a/soroban/predicate-registry/src/lib.rs +++ b/soroban/predicate-registry/src/lib.rs @@ -1,35 +1,11 @@ #![no_std] -//! Predicate Registry — verifies attester-signed [`Statement`]s for integrating -//! contracts. +//! Verifies attester-signed [`Statement`]s on behalf of integrating contracts. //! -//! # Trust boundary -//! -//! The registry verifies that a registered attester signed the statement it was -//! handed. It cannot see the call it is authorizing: the live function selector, -//! arguments, sender, and amount all belong to the integrating contract, which is -//! a separate invocation. Only two fields are established independently of what -//! the caller passes: -//! -//! * `target` — replaced with the authenticated `caller` before hashing, so an -//! attestation is bound to the contract presenting it (hashStatementSafe). -//! * the network — read from the ledger, never a parameter (see -//! `validation::compute_hash`). -//! -//! Every remaining field (`msg_sender`, `msg_value`, `encoded_sig_and_args`, -//! `policy`) is taken on trust. **Integrating contracts must derive them from the -//! call currently executing, never from values an end user supplies.** A contract -//! that forwards user-controlled statement fields will validate an attestation -//! for one action and then execute a different one — the signature check passes -//! because the attester really did sign the statement it was shown; it just is not -//! the statement describing what happens next. -//! -//! Use [`predicate_client::authorize_transaction`] rather than calling -//! [`PredicateRegistryContract::validate_attestation`] directly. It builds the -//! statement from its arguments, which keeps the live-data requirement at the -//! integrating function's signature where it is hard to get wrong. See -//! `example-compliant-token` for a worked integration. -//! -//! [`predicate_client::authorize_transaction`]: https://github.com/predicatelabs/predicate-contracts/blob/main/soroban/predicate-client/src/lib.rs +//! The registry cannot see the call it authorizes — that runs in the integrating +//! contract's own invocation. It establishes two fields itself, `target` and the +//! network, and trusts the rest as passed. Integrators must therefore build those +//! from the call being authorized; `predicate-client` exists to make that the +//! path of least resistance. See [`PredicateRegistryContract::validate_attestation`]. mod attesters; mod policy; @@ -42,7 +18,6 @@ use soroban_sdk::{ pub use types::{Attestation, RegistryError, Statement}; -// Storage keys const OWNER: Symbol = soroban_sdk::symbol_short!("owner"); const PENDING_OWNER: Symbol = soroban_sdk::symbol_short!("pnd_own"); @@ -51,26 +26,15 @@ pub struct PredicateRegistryContract; #[contractimpl] impl PredicateRegistryContract { - /// Initialize the registry with an owner address. - /// - /// # Arguments - /// - /// * `owner` - Address with administrative privileges. Can register and - /// deregister attesters, and propose a new owner via the two-step - /// `transfer_ownership` / `accept_ownership` flow. pub fn __constructor(e: &Env, owner: Address) { e.storage().instance().set(&OWNER, &owner); } - /// Return the contract owner. pub fn owner(e: &Env) -> Address { e.storage().instance().get(&OWNER).unwrap() } - /// Propose a new owner. Only the current owner may call this. - /// The new owner must call `accept_ownership` to finalize the transfer. - /// This two-step pattern mirrors EVM's Ownable2StepUpgradeable, preventing - /// accidental transfers to wrong addresses. + /// Proposes only; `new_owner` must call `accept_ownership` to take effect. pub fn transfer_ownership( e: &Env, current_owner: Address, @@ -86,7 +50,6 @@ impl PredicateRegistryContract { Ok(()) } - /// Accept a pending ownership transfer. Only the pending owner may call this. pub fn accept_ownership(e: &Env, new_owner: Address) -> Result<(), RegistryError> { let pending: Address = e .storage() @@ -109,12 +72,10 @@ impl PredicateRegistryContract { Ok(()) } - /// Return the pending owner, if any. pub fn pending_owner(e: &Env) -> Option
{ e.storage().instance().get(&PENDING_OWNER) } - /// Register a new attester. Only the contract owner may call this. pub fn register_attester( e: &Env, owner: Address, @@ -124,7 +85,6 @@ impl PredicateRegistryContract { attesters::register(e, &attester) } - /// Deregister an attester using swap-and-pop. Only the contract owner may call this. pub fn deregister_attester( e: &Env, owner: Address, @@ -134,81 +94,36 @@ impl PredicateRegistryContract { attesters::deregister(e, &attester) } - /// Check whether an attester is currently registered. pub fn is_attester_registered(e: &Env, attester: BytesN<32>) -> bool { attesters::is_registered(e, &attester) } - /// Return all registered attesters. pub fn get_registered_attesters(e: &Env) -> Vec> { attesters::get_all(e) } - /// Set the policy ID for the calling address. pub fn set_policy_id(e: &Env, caller: Address, policy_id: String) { policy::set(e, &caller, &policy_id); } - /// Get the policy ID for a client address. pub fn get_policy_id(e: &Env, client: Address) -> String { policy::get(e, &client) } - /// Compute SHA-256 hash of a statement for attester signing. - /// This is the "hashStatementWithExpiry" equivalent — attesters sign this hash. - /// - /// The digest is bound to the host network, read from the ledger rather than - /// supplied by the caller, so an attestation is only valid on the chain it was - /// signed for. + /// The digest an attester signs, bound to the host network so it is valid + /// only on the chain that produced it. pub fn hash_statement(e: &Env, statement: Statement) -> BytesN<32> { validation::compute_hash(e, &statement) } - /// Validate an attestation against a statement. - /// - /// The `caller` parameter implements the hashStatementSafe pattern: - /// it replaces `statement.target` with the actual caller address before - /// verifying the signature, preventing cross-contract replay attacks. - /// In Soroban, the calling contract should pass `e.current_contract_address()`. - /// `caller.require_auth()` makes that binding sound — a contract address - /// cannot be impersonated by whoever assembled the transaction. - /// - /// # The caller owns the statement's accuracy + /// `caller` should be `e.current_contract_address()`. It replaces + /// `statement.target` before hashing, so an attestation only works for the + /// contract presenting it; every other field is trusted exactly as passed. /// - /// `statement` is trusted input apart from `target`. A returning call means - /// "a registered attester signed *this* statement", not "the action you are - /// about to take is approved" — those coincide only when the caller built the - /// statement from the call it is executing: - /// - /// * `uuid` / `expiration` — copy from the attestation (both are cross-checked) - /// * `msg_sender`, `msg_value` — the live sender and amount, after - /// `require_auth()` on the sender - /// * `encoded_sig_and_args` — an encoding of the *concrete* call, covering every - /// argument that matters for compliance, so no argument can be swapped between - /// attestation and execution - /// * `policy` — the contract's own configured policy, from its storage - /// - /// Passing any of these straight through from a user-supplied parameter is an - /// authorization bypass in the integrating contract. Prefer - /// `predicate_client::authorize_transaction`, which takes these as arguments - /// and assembles the statement itself. See the crate-level trust boundary docs. - /// - /// # Failure modes - /// - /// Returns `Ok(())` when every check passes. Failures arrive two different - /// ways, and integrators have to handle both: - /// - /// * Checks on the attestation's shape — expiry, replay, uuid/expiration - /// agreement, attester registration — return a typed [`RegistryError`]. - /// * An **invalid signature traps** rather than returning an error, aborting - /// the invocation with `Error(Crypto, InvalidInput)`. The host escalates the - /// failure before a contract can see it, and soroban-sdk 23.5.3 exposes no - /// fallible ed25519 API, so this cannot be turned into a `RegistryError`. - /// `RegistryError` deliberately has no `InvalidSignature` variant as a - /// result. - /// - /// Either way the UUID is not marked spent, so a rejected attestation can be - /// retried once whatever was wrong with it is fixed. + /// Expiry, replay, uuid or expiration disagreement, and an unregistered + /// attester return a [`RegistryError`]. An invalid signature instead aborts + /// the invocation with `Error(Crypto, InvalidInput)`, which is why no + /// `InvalidSignature` variant exists. Neither outcome spends the uuid. pub fn validate_attestation( e: &Env, statement: Statement, @@ -218,12 +133,8 @@ impl PredicateRegistryContract { validation::validate(e, &statement, &attestation, &caller) } - /// Replace the registry's WASM bytecode in place. Only the owner may call this. - /// The contract address and all storage (owner, attesters, policies, spent UUIDs) - /// are preserved; only the executable code changes. - /// - /// `new_wasm_hash` is the SHA-256 hash of an already-uploaded contract WASM - /// (see `stellar contract upload`). + /// Swaps the bytecode in place: the address and all storage survive. + /// `new_wasm_hash` must already be uploaded — see `stellar contract upload`. pub fn upgrade( e: &Env, owner: Address, @@ -239,7 +150,6 @@ impl PredicateRegistryContract { } } -/// Internal helper: require that `caller` is the stored owner. pub(crate) fn require_owner(e: &Env, caller: &Address) -> Result<(), RegistryError> { let owner: Address = e .storage() @@ -262,10 +172,8 @@ mod test { use super::*; use crate::types::{Attestation, Statement}; - // Import the crate's own compiled WASM so the test can upload it and - // upgrade the registry to itself (proves the upgrade path + storage survival). - // Requires: stellar contract build --package predicate-registry - // (builds to wasm32v1-none, which the soroban host validator accepts) + // Requires `stellar contract build --package predicate-registry` first: the + // host validator only accepts the wasm32v1-none build. mod registry_wasm { soroban_sdk::contractimport!( file = "../target/wasm32v1-none/release/predicate_registry.wasm" @@ -407,8 +315,6 @@ mod test { assert_eq!(client.get_policy_id(&caller), p2); } - // --- Ownership transfer tests --- - #[test] fn test_two_step_ownership_transfer() { let e = Env::default(); @@ -416,12 +322,10 @@ mod test { let (owner, client) = setup(&e); let new_owner = Address::generate(&e); - // Step 1: propose client.transfer_ownership(&owner, &new_owner); assert_eq!(client.owner(), owner); // still the old owner assert_eq!(client.pending_owner(), Some(new_owner.clone())); - // Step 2: accept client.accept_ownership(&new_owner); assert_eq!(client.owner(), new_owner); assert_eq!(client.pending_owner(), None); @@ -452,9 +356,6 @@ mod test { client.accept_ownership(&attacker); // wrong address } - // --- Validation tests --- - - /// Helper: create an ed25519 signing key and return (signing_key, pub_key_bytes) fn generate_ed25519_keypair(e: &Env) -> (ed25519_dalek::SigningKey, BytesN<32>) { use ed25519_dalek::SigningKey; use rand::rngs::OsRng; @@ -463,7 +364,6 @@ mod test { (sk, BytesN::from_array(e, &pk_bytes)) } - /// Helper: sign a hash (BytesN<32>) with an ed25519 signing key, returning BytesN<64> fn sign_hash(e: &Env, sk: &ed25519_dalek::SigningKey, hash: &BytesN<32>) -> BytesN<64> { use ed25519_dalek::Signer; let sig = sk.sign(&hash.to_array()); @@ -568,9 +468,7 @@ mod test { signature, }; - // First call succeeds client.validate_attestation(&statement, &attestation, &client.address); - // Second call should fail with UuidAlreadyUsed client.validate_attestation(&statement, &attestation, &client.address); } @@ -673,9 +571,8 @@ mod test { client.validate_attestation(&statement, &attestation, &client.address); } - /// Build a statement whose `target` is already the caller, so `hash_statement` - /// returns exactly the digest `validate_attestation` recomputes. That isolates - /// the domain-separation checks below from the hashStatementSafe substitution. + /// `target` is the caller, so `hash_statement` returns exactly what + /// `validate_attestation` recomputes and the substitution is a no-op. fn caller_bound_statement(e: &Env, uuid: &str, caller: &Address) -> Statement { Statement { uuid: soroban_sdk::String::from_str(e, uuid), @@ -688,11 +585,8 @@ mod test { } } - /// The network is read from the ledger rather than supplied by the caller, so - /// the digest changes with the chain the registry is running on. Deliberately - /// *not* asserted here: that two registry instances on the same network hash - /// differently. The registry address is not part of the preimage — see the - /// rationale on `validation::compute_hash`. + /// Two instances on the same network hash identically; the registry address is + /// not in the preimage, only the network id. #[test] fn test_digest_is_bound_to_network_id() { let e = Env::default(); @@ -707,8 +601,7 @@ mod test { assert_ne!(hash, client.hash_statement(&statement)); } - /// An attestation signed on one chain cannot be presented on another, even to - /// the registry deployed at the same address. + /// An attestation signed on one chain cannot be presented on another. #[test] #[should_panic(expected = "Error(Crypto, InvalidInput)")] fn test_attestation_from_another_network_is_rejected() { @@ -733,24 +626,12 @@ mod test { client.validate_attestation(&statement, &attestation, &caller); } - // --- Golden vector --- - // - // Every other test here asks the contract for a digest and then signs it, so - // the contract is only ever checked against itself: swapping the order of the - // appends in `compute_hash`, or renaming a `Statement` field — `#[contracttype]` - // uses field names as ScMap keys — silently changes the wire format while every - // test still passes. A plain refactor can therefore break every attestation the - // API has already signed. - // - // The constants below are the fix. They come from `scripts/golden-vector.js`, a - // third implementation hand-rolled from the XDR spec that shares no code with - // this contract, so nothing but a byte-identical layout satisfies them. Pinning - // the same vector in the Go signer locks both sides to one value instead of each - // agreeing with itself. - // - // If a change here is deliberate, regenerate with that script and update both - // sides in the same rollout — the digest changing invalidates every attestation - // already issued. + // These constants come from `scripts/golden-vector.js`, an implementation that + // shares no code with this contract. Never regenerate them from `hash_statement` + // — a vector derived from the code under test cannot detect the code changing. + // Reordering the preimage or renaming a `Statement` field (`#[contracttype]` + // uses field names as ScMap keys) alters the wire format, and every other test + // here would still pass. The Go signer pins the same values. /// `sha256("Test SDF Network ; September 2015")` const GV_NETWORK_ID: &str = "cee0302d59844d32bdca915c8203dd44b33fbb7edc19051ea37abedf28ecd472"; @@ -791,9 +672,8 @@ mod test { out } - /// The statement the golden digest was computed over. `target` is the address - /// the test passes as `caller`, so `validate_attestation`'s hashStatementSafe - /// substitution is a no-op and it hashes exactly this. + /// `target` is the address the test passes as `caller`, so the substitution in + /// `validate_attestation` is a no-op and it hashes exactly this. fn golden_statement(e: &Env) -> Statement { Statement { uuid: soroban_sdk::String::from_str(e, GV_UUID), @@ -806,8 +686,6 @@ mod test { } } - /// The digest for a fixed statement on a fixed network must equal a value this - /// contract did not produce. #[test] fn test_golden_vector_digest() { let e = Env::default(); @@ -820,9 +698,8 @@ mod test { assert_eq!(to_hex(&digest.to_array()), GV_DIGEST); } - /// The same vector through the real verification path: an externally produced - /// ed25519 signature over `GV_DIGEST` must satisfy `validate_attestation`. This - /// covers the ed25519 call too, not just the hashing. + /// The same vector through the verification path, so the ed25519 call is + /// covered and not just the hashing. #[test] fn test_golden_vector_signature() { let e = Env::default(); @@ -841,8 +718,6 @@ mod test { signature: BytesN::from_array(&e, &unhex::<64>(GV_SIGNATURE)), }; - // `caller` is the statement's own target, so the digest verified here is - // GV_DIGEST unchanged. let caller = Address::from_str(&e, GV_TARGET); client.validate_attestation(&statement, &attestation, &caller); } @@ -866,22 +741,18 @@ mod test { e.mock_all_auths(); let (owner, client) = setup(&e); - // Seed storage before the upgrade. let attester = generate_attester_key(&e); client.register_attester(&owner, &attester); assert!(client.is_attester_registered(&attester)); - // Upload the crate's own WASM and upgrade to it. let wasm_hash = e.deployer().upload_contract_wasm(registry_wasm::WASM); client.upgrade(&owner, &wasm_hash); - // Same address, same storage after the bytecode swap. assert_eq!(client.owner(), owner); assert!(client.is_attester_registered(&attester)); } - /// Pins the trap documented on `validate_attestation`: a bad signature aborts - /// the invocation with a host error, and never surfaces as a `RegistryError`. + /// A bad signature aborts with a host error, never a `RegistryError`. #[test] #[should_panic(expected = "Error(Crypto, InvalidInput)")] fn test_validate_invalid_signature() { @@ -889,11 +760,10 @@ mod test { e.mock_all_auths(); let (owner, client) = setup(&e); - // Register attester A let (sk_a, pub_key_a) = generate_ed25519_keypair(&e); client.register_attester(&owner, &pub_key_a); - // Also register attester B (so it's registered) but sign with A's key + // B is registered too, so only the signature can be what fails. let (_sk_b, pub_key_b) = generate_ed25519_keypair(&e); client.register_attester(&owner, &pub_key_b); @@ -908,7 +778,6 @@ mod test { }; let hash = client.hash_statement(&statement); - // Sign with key A but claim attester is key B let signature = sign_hash(&e, &sk_a, &hash); let attestation = Attestation { @@ -921,12 +790,8 @@ mod test { client.validate_attestation(&statement, &attestation, &client.address); } - /// The trap is only tolerable because it costs the caller nothing but the fee: - /// nothing is committed, so the uuid stays unspent and the same statement works - /// once a correct signature arrives. `try_validate_attestation` is what lets a - /// caller observe the abort without unwinding — and what it returns shows why - /// there is no `InvalidSignature` error to match on: an invocation error, not a - /// `RegistryError`. + /// The abort commits nothing, so the uuid survives for a corrected retry. + /// `try_validate_attestation` observes it without unwinding. #[test] fn test_invalid_signature_aborts_and_leaves_uuid_unspent() { let e = Env::default(); diff --git a/soroban/predicate-registry/src/policy.rs b/soroban/predicate-registry/src/policy.rs index 6698c6f0..52e774a7 100644 --- a/soroban/predicate-registry/src/policy.rs +++ b/soroban/predicate-registry/src/policy.rs @@ -11,8 +11,7 @@ pub fn set(e: &Env, caller: &Address, policy_id: &String) { e.storage() .persistent() .set(&policy_storage_key(caller), policy_id); - // Extend to the network maximum: a fixed short TTL that is never refreshed - // could archive a client's policy binding while it is still in use. + // Max TTL: a binding archived while still in use silently unregisters a client. let max_ttl = e.storage().max_ttl(); e.storage() .persistent() diff --git a/soroban/predicate-registry/src/types.rs b/soroban/predicate-registry/src/types.rs index 21f7e874..f7eae7ce 100644 --- a/soroban/predicate-registry/src/types.rs +++ b/soroban/predicate-registry/src/types.rs @@ -1,46 +1,32 @@ use soroban_sdk::{contracterror, contracttype, Address, Bytes, BytesN, String}; -/// Mirrors the EVM Statement struct. -/// Describes a transaction to be authorized. +/// Describes a transaction to be authorized. Mirrors the EVM Statement struct. #[contracttype] #[derive(Clone, Debug, Eq, PartialEq)] pub struct Statement { - /// Unique identifier — replay protection key pub uuid: String, - /// Original transaction sender pub msg_sender: Address, - /// Target contract address pub target: Address, - /// Value sent with the transaction (token amount). - /// Equivalent to EVM's msg.value — included in signed digest so - /// attesters can constrain transaction value. pub msg_value: i128, - /// Encoded function signature and arguments — variable-length to match - /// the EVM `bytes encodedSigAndArgs` field. Callers may pass the raw - /// call data or a hash of it. + /// Raw call data or a hash of it; the attester signs whichever is supplied. pub encoded_sig_and_args: Bytes, - /// Policy identifier (e.g. "x-a1b2c3d4e5f6g7h8") + /// Policy identifier, e.g. "x-a1b2c3d4e5f6g7h8". pub policy: String, - /// Deadline ledger timestamp + /// Ledger timestamp, in seconds. pub expiration: u64, } -/// Ed25519-signed authorization from an attester. +/// Ed25519-signed authorization. `uuid` and `expiration` must match the statement's. #[contracttype] #[derive(Clone, Debug, Eq, PartialEq)] pub struct Attestation { - /// Must match Statement.uuid pub uuid: String, - /// Must match Statement.expiration pub expiration: u64, - /// Ed25519 public key of the attester (32 bytes) pub attester: BytesN<32>, - /// Ed25519 signature (64 bytes) pub signature: BytesN<64>, } -// TODO: Replace events().publish() with #[contractevent] when available in a future SDK version. -// soroban-sdk 23.5.3 does not support #[contractevent]. +// Switch events().publish() to #[contractevent] once the SDK supports it; 23.5.3 does not. #[contracterror] #[derive(Copy, Clone, Debug, Eq, PartialEq, PartialOrd, Ord)] diff --git a/soroban/predicate-registry/src/validation.rs b/soroban/predicate-registry/src/validation.rs index 75be6637..91c94b5d 100644 --- a/soroban/predicate-registry/src/validation.rs +++ b/soroban/predicate-registry/src/validation.rs @@ -3,133 +3,77 @@ use soroban_sdk::{symbol_short, Address, Bytes, BytesN, Env}; use crate::types::{Attestation, RegistryError, Statement}; -/// Compute SHA-256 hash of a statement for attester signing. +/// SHA-256 over the XDR of the host network id followed by the statement. /// -/// The preimage is the deterministic XDR serialization of the host network ID -/// followed by the statement. The network ID is read from the ledger rather than -/// taken as a parameter, so a caller cannot choose the chain it is validated -/// against — this mirrors `block.chainid` in the EVM registry's -/// `hashStatementWithExpiry`. -/// -/// There is no separate version tag. XDR is self-describing and length-prefixed, -/// so one layout cannot be parsed as another: the network id is `ScVal::Bytes` -/// and the statement an `ScVal::Map`, and no appended field could impersonate -/// either. Any change to the layout is therefore already a hard break, which a -/// tag would only restate. -/// -/// The registry's own address is deliberately *not* in the preimage, matching -/// EVM and Solana. Replay across registry instances is already constrained by -/// the caller binding below (see `validate`): it would need one integrating -/// contract wired to two registries running this same preimage layout. Adding -/// the address would instead require every attester to know which registry it is -/// signing for, and only one registry per network is deployed. If that ever -/// changes, append `e.current_contract_address()` here. +/// The network id comes from the ledger, never a parameter, so a caller cannot +/// choose the chain its signature is checked against. pub fn compute_hash(e: &Env, statement: &Statement) -> BytesN<32> { let mut payload = Bytes::new(e); - // Domain separator, read from the host — not caller-supplied. payload.append(&e.ledger().network_id().to_xdr(e)); - // Statement fields in deterministic order payload.append(&statement.clone().to_xdr(e)); e.crypto().sha256(&payload).to_bytes() } -/// Validate an attestation against a statement. -/// -/// Performs the following checks: -/// 0. Caller authentication (mirrors EVM's implicit msg.sender) -/// 1. Attestation not expired -/// 2. UUID not already spent (replay protection) -/// 3. UUID matches between statement and attestation -/// 4. Expiration matches between statement and attestation -/// 5. Attester is registered (cheap lookup before expensive crypto) -/// 6. Ed25519 signature verification using caller-bound hash (hashStatementSafe) -/// 7. Marks UUID as spent -/// 8. Emits validation event -/// -/// Returns `Ok(())` when every check passes. Checks 1-5 return a typed -/// `RegistryError`; check 6 does not — see the note there. +/// Verify that a registered attester signed `statement`, then spend its uuid. +/// An invalid signature traps instead of returning `Err`; see the note below. pub fn validate( e: &Env, statement: &Statement, attestation: &Attestation, caller: &Address, ) -> Result<(), RegistryError> { - // 0. Authenticate the caller — mirrors EVM's implicit msg.sender guarantee. - // Without this, anyone could call validate_attestation with an arbitrary - // caller address and burn valid UUIDs. + // Without this, anyone could pass another contract's address and spend uuids + // against it. caller.require_auth(); - // 1. Check expiration if e.ledger().timestamp() > attestation.expiration { return Err(RegistryError::AttestationExpired); } - // 2. Check UUID not already spent let uuid_key = (symbol_short!("uuid"), statement.uuid.clone()); let already_used: bool = e.storage().persistent().get(&uuid_key).unwrap_or(false); if already_used { return Err(RegistryError::UuidAlreadyUsed); } - // 3. UUID match if statement.uuid != attestation.uuid { return Err(RegistryError::UuidMismatch); } - // 4. Expiration match if statement.expiration != attestation.expiration { return Err(RegistryError::ExpirationMismatch); } - // 5. Check attester is registered (cheap lookup — do before expensive crypto) if !crate::attesters::is_registered(e, &attestation.attester) { return Err(RegistryError::AttesterNotRegistered); } - // Maximum TTL the network allows for a ledger entry — used below to keep the - // replay marker (and the attester's registration entries) alive for as long - // as possible instead of a fixed short window. let max_ttl = e.storage().max_ttl(); - // 6. Ed25519 signature verification — use caller-bound hash (hashStatementSafe) - // Replace statement.target with the actual caller to prevent cross-contract replay + // Binding the digest to the authenticated caller rather than the statement's + // own target is what stops one contract's attestation working on another. let safe_statement = Statement { target: caller.clone(), ..statement.clone() }; let hash = compute_hash(e, &safe_statement); let hash_bytes: Bytes = Bytes::from_slice(e, &hash.to_array()); - // A failed verification does NOT return Err — it traps, aborting the whole - // invocation with `Error(Crypto, InvalidInput)`. The host builds a HostError - // (soroban-env-host crypto/mod.rs) and the SDK discards it (`let _ = ...`), so - // a contract cannot observe the failure as a value; there is no fallible - // ed25519 API in soroban-sdk 23.5.3 to map onto a RegistryError. Callers must - // treat an invalid signature as an aborted invocation, not a returned error. - // - // This is why RegistryError has no InvalidSignature variant: an - // error the contract can never return is worse than none, because integrators - // write handling for it that cannot fire. + // Traps on failure rather than returning: soroban-sdk 23.5.3 exposes no + // fallible ed25519 API, so callers see an aborted invocation, not an Err. e.crypto() .ed25519_verify(&attestation.attester, &hash_bytes, &attestation.signature); - // 7. Mark UUID as spent. - // Extend the replay marker to the maximum possible TTL. A short TTL could - // be archived while a longer-lived attestation is still valid, which would - // re-open replay; tying the marker to the network max keeps the guard alive - // for as long as the ledger allows. + // Max TTL: a marker archived while its attestation is still valid re-opens + // replay. e.storage().persistent().set(&uuid_key, &true); e.storage() .persistent() .extend_ttl(&uuid_key, max_ttl, max_ttl); - // Refresh the contract instance TTL on every successful validation so that - // an actively-used registry (and its attester set, held in instance storage) - // is never archived out from under callers. crate::attesters::refresh_ttl(e); - // 8. Emit event (includes attester + caller for observability, mirroring EVM StatementValidated) #[allow(deprecated)] e.events().publish( (symbol_short!("validate"), symbol_short!("ok")), diff --git a/soroban/scripts/verify-golden-vector-detects-drift.sh b/soroban/scripts/verify-golden-vector-detects-drift.sh index 2de047dc..5abf31ef 100755 --- a/soroban/scripts/verify-golden-vector-detects-drift.sh +++ b/soroban/scripts/verify-golden-vector-detects-drift.sh @@ -90,7 +90,6 @@ echo echo "1. Swap the two appends in compute_hash (reorders the preimage)." apply "$VALIDATION" \ ' payload.append(&e.ledger().network_id().to_xdr(e)); - // Statement fields in deterministic order payload.append(&statement.clone().to_xdr(e));' \ ' payload.append(&statement.clone().to_xdr(e)); payload.append(&e.ledger().network_id().to_xdr(e));'