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 bfa876ce..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 { @@ -41,59 +39,20 @@ pub enum RegistryError { UuidAlreadyUsed = 5, UuidMismatch = 6, ExpirationMismatch = 7, - InvalidSignature = 8, - NotInitialized = 9, - AlreadyInitialized = 10, + 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. 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. -/// -/// # 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, @@ -122,10 +81,7 @@ 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); + e.invoke_contract::<()>(registry, &Symbol::new(e, "validate_attestation"), args); } #[cfg(test)] @@ -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 d8781e25..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,79 +94,47 @@ 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 - /// - /// `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 + /// `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. /// - /// 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. + /// 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, attestation: Attestation, caller: Address, - ) -> Result { + ) -> Result<(), RegistryError> { 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, @@ -222,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() @@ -245,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" @@ -390,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(); @@ -399,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); @@ -435,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; @@ -446,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()); @@ -482,8 +399,7 @@ mod test { signature, }; - let result = client.validate_attestation(&statement, &attestation, &client.address); - assert!(result); + client.validate_attestation(&statement, &attestation, &client.address); } #[test] @@ -552,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); } @@ -657,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), @@ -672,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(); @@ -691,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() { @@ -717,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"; @@ -775,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), @@ -790,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(); @@ -804,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(); @@ -825,10 +718,8 @@ 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); - assert!(client.validate_attestation(&statement, &attestation, &caller)); + client.validate_attestation(&statement, &attestation, &caller); } #[test] @@ -850,32 +741,29 @@ 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)); } + /// A bad signature aborts with a host error, never 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(); 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); @@ -890,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 { @@ -903,6 +790,52 @@ mod test { client.validate_attestation(&statement, &attestation, &client.address); } + /// 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(); + 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/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 ff080f41..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)] @@ -60,10 +46,6 @@ pub enum RegistryError { UuidMismatch = 6, /// Statement/Attestation expiration mismatch ExpirationMismatch = 7, - /// Ed25519 signature verification failed - InvalidSignature = 8, /// 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 4b0fb4bf..91c94b5d 100644 --- a/soroban/predicate-registry/src/validation.rs +++ b/soroban/predicate-registry/src/validation.rs @@ -3,122 +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 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. -/// -/// 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 +/// 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 { - // 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. +) -> Result<(), RegistryError> { + // 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()); - // NOTE: ed25519_verify panics on invalid signature + // 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 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. + // 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")), @@ -129,5 +84,5 @@ pub fn validate( ), ); - Ok(true) + 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));'