diff --git a/SECURITY_REVIEW.md b/SECURITY_REVIEW.md index e61a6cb..c9446ab 100644 --- a/SECURITY_REVIEW.md +++ b/SECURITY_REVIEW.md @@ -43,7 +43,7 @@ Out of scope: | Metric | Current value | Notes | | --- | ---: | --- | | Contract implementation size | 1,304 LOC | `contracts/src/contract.rs` | -| Error enum size | 25 variants | `contracts/src/errors.rs` | +| Error enum size | 29 variants | `contracts/src/errors.rs` | | Type definitions size | 105 LOC | `contracts/src/types.rs` | | TypeScript bindings size | 765 LOC | `bindings/src/index.ts` | | Test modules | 13 | All modules included by `contracts/src/tests/mod.rs` | @@ -125,7 +125,8 @@ Soroban-specific risk considerations: - **Auth:** state-changing role/user operations call `require_auth()` on the expected `Address`; pause/unpause and windows are admin-gated, resolution is oracle-gated, user actions are user-gated. - **Storage:** persistent storage uses indexed per-user keys plus `RoundParticipants(round_id)` to avoid rewriting full maps on each bet, with legacy map fallbacks for migration. - **Arithmetic:** critical arithmetic uses checked operations. Payout paths increasingly route through `payout_add` / `payout_mul`, but one Precision indexed path still returns generic `Overflow` rather than `PayoutOverflow`. -- **Oracle inputs:** `OraclePayload` enforces non-zero price, timestamp not in the future, 300-second freshness, and round binding against `Round.start_ledger`. +- **Oracle inputs:** `OraclePayload` enforces non-zero price, timestamp not in the future, round binding against `Round.start_ledger`, and (optionally) an ed25519 attestation. When an attestation key is configured (`set_attestation_key`), every payload must carry a detached signature over a domain-separated message (`XELMA_ORACLE_ATTESTATION_V1` + XDR of network_id, contract_addr, round_id, price, timestamp, nonce); a missing/bad signature is rejected fail-closed — on-chain a failed `ed25519_verify` traps the transaction before any state change (`settlement.rs` attestation gate; see `contracts/src/tests/attestation.rs`). When no key is configured the gate is a no-op, so behaviour is identical to the pre-attestation ABI (Issue #263). +- **Attestation threat note:** the attestation (Issue #263) binds the payload to this network, contract, round, price, timestamp, and nonce, and is meant for deployments where the off-chain signer (e.g. HSM) differs from the Soroban account that submits `resolve_round`. It does **not** vouch for price *correctness* — a compromised or faulty signer can still attest wrong prices (see SR-2026-04-004). `confidence` is deliberately **excluded** from the signed message (advisory metadata only), so confidence changes cannot invalidate a valid attestation. The negative test suite in `contracts/src/tests/attestation.rs` asserts invalid/missing/malformed/wrong-key/tampered-field attestations are rejected and leave the round resolvable, and that disabled mode ignores an attacker-supplied signature field entirely. - **Resource limits:** resolution is O(n) over participants. Precision rounds include an admin-configurable participant cap; operators still need benchmark evidence before raising it near upper bounds. ## Findings diff --git a/contracts/src/errors.rs b/contracts/src/errors.rs index be0e894..8179286 100644 --- a/contracts/src/errors.rs +++ b/contracts/src/errors.rs @@ -105,4 +105,12 @@ pub enum ContractError { ClaimBatchTooLarge = 87, /// claim_many batch contains the same address more than once (Issue #277) DuplicateClaimAddress = 88, + /// Oracle heartbeat is unhealthy and blocks settlement in strict mode (Issue #264) + OracleHeartbeatUnhealthy = 89, + /// The caller lacks the required role for the requested action + AccessDenied = 90, + /// The dispute window for the round has expired + DisputeWindowExpired = 91, + /// Round settlement is locked against the attempted action + ClaimLocked = 92, } diff --git a/contracts/src/tests/attestation.rs b/contracts/src/tests/attestation.rs index 8debcce..4a6f329 100644 --- a/contracts/src/tests/attestation.rs +++ b/contracts/src/tests/attestation.rs @@ -4,7 +4,7 @@ use crate::contract::{VirtualTokenContract, VirtualTokenContractClient}; use crate::errors::ContractError; use crate::settlement::_build_attestation_message; -use crate::types::OraclePayload; +use crate::types::{DataKeyScoped, OraclePayload}; use ed25519_dalek::{Signer, SigningKey}; use rand::rngs::OsRng; use soroban_sdk::{ @@ -113,7 +113,6 @@ fn test_attestation_valid_signature_resolves_successfully() { } #[test] -#[should_panic] fn test_attestation_wrong_key_signature_rejected() { let env = Env::default(); let (client, contract_id, _admin, _oracle) = setup(&env); @@ -134,11 +133,15 @@ fn test_attestation_wrong_key_signature_rejected() { let signature = sign_payload(&env, &wrong_key, &payload); payload.attestation = Some(signature); - client.resolve_round(&payload); + // A bad signature surfaces as a host-level trap (`Err(Err(_))`), not a + // contract error, and the round must remain untouched for a legitimate + // oracle to settle later. + let result = client.try_resolve_round(&payload); + assert!(matches!(result, Err(Err(_)))); + assert!(client.get_active_round().is_some()); } #[test] -#[should_panic] fn test_attestation_tampered_price_after_signing_rejected() { let env = Env::default(); let (client, contract_id, _admin, _oracle) = setup(&env); @@ -159,7 +162,222 @@ fn test_attestation_tampered_price_after_signing_rejected() { payload.price = 2_000_0000; payload.attestation = Some(signature); + let result = client.try_resolve_round(&payload); + assert!(matches!(result, Err(Err(_)))); + assert!(client.get_active_round().is_some()); +} + +#[test] +fn test_attestation_malformed_signature_rejected() { + let env = Env::default(); + let (client, contract_id, _admin, _oracle) = setup(&env); + let (pubkey, _signing_key) = generate_keypair(&env); + + client.set_attestation_key(&Some(pubkey)); + client.create_round(&1_000_0000, &None); + + env.ledger().with_mut(|li| { + li.sequence_number = 12; + }); + + let mut payload = base_payload(&env, &contract_id, 0, 1, 1_000_0000); + // Well-formed length (64 bytes) but not a valid ed25519 signature. + payload.attestation = Some(BytesN::from_array(&env, &[0xABu8; 64])); + + let result = client.try_resolve_round(&payload); + assert!(matches!(result, Err(Err(_)))); + assert!(client.get_active_round().is_some()); +} + +#[test] +fn test_attestation_tampered_timestamp_after_signing_rejected() { + let env = Env::default(); + let (client, contract_id, _admin, _oracle) = setup(&env); + let (pubkey, signing_key) = generate_keypair(&env); + + client.set_attestation_key(&Some(pubkey)); + client.create_round(&1_000_0000, &None); + + env.ledger().with_mut(|li| { + li.sequence_number = 12; + }); + + let mut payload = base_payload(&env, &contract_id, 0, 1, 1_000_0000); + let signature = sign_payload(&env, &signing_key, &payload); + // Timestamp is part of the signed message, so flipping it invalidates + // the signature at the attestation gate (before the window check ever + // runs) even though a far-future timestamp would also be rejected later. + payload.timestamp = 60_000_000; + payload.attestation = Some(signature); + + let result = client.try_resolve_round(&payload); + assert!(matches!(result, Err(Err(_)))); + assert!(client.get_active_round().is_some()); +} + +#[test] +fn test_attestation_tampered_nonce_after_signing_rejected() { + let env = Env::default(); + let (client, contract_id, _admin, _oracle) = setup(&env); + let (pubkey, signing_key) = generate_keypair(&env); + + client.set_attestation_key(&Some(pubkey)); + client.create_round(&1_000_0000, &None); + + env.ledger().with_mut(|li| { + li.sequence_number = 12; + }); + + let mut payload = base_payload(&env, &contract_id, 0, 1, 1_000_0000); + let signature = sign_payload(&env, &signing_key, &payload); + // Nonce is part of the signed message; tampering with it means the + // signature no longer covers this payload. + payload.nonce = 99; + payload.attestation = Some(signature); + + let result = client.try_resolve_round(&payload); + assert!(matches!(result, Err(Err(_)))); + assert!(client.get_active_round().is_some()); +} + +#[test] +fn test_attestation_disabled_mode_ignores_invalid_signature() { + let env = Env::default(); + let (client, contract_id, _admin, _oracle) = setup(&env); + // No attestation key configured — attestation stays a no-op exactly as + // before Issue #263, and an attacker-supplied (garbage) signature field + // must not change behaviour. + assert_eq!(client.get_attestation_key(), None); + client.create_round(&1_000_0000, &None); + + env.ledger().with_mut(|li| { + li.sequence_number = 12; + }); + + let mut payload = base_payload(&env, &contract_id, 0, 1, 1_000_0000); + payload.attestation = Some(BytesN::from_array(&env, &[0x00u8; 64])); + client.resolve_round(&payload); + assert_eq!(client.get_active_round(), None); +} + +#[test] +fn test_attestation_valid_signature_does_not_bypass_nonce_replay_guard() { + let env = Env::default(); + let (client, contract_id, _admin, _oracle) = setup(&env); + let (pubkey, signing_key) = generate_keypair(&env); + + client.set_attestation_key(&Some(pubkey)); + client.create_round(&1_000_0000, &None); + + env.ledger().with_mut(|li| { + li.sequence_number = 12; + }); + + let mut payload = base_payload(&env, &contract_id, 0, 1, 1_000_0000); + let signature = sign_payload(&env, &signing_key, &payload); + payload.attestation = Some(signature); + + // Pre-consumed nonce: the first round gets the monotonic + // `round.round_id = 1` (distinct from `start_ledger = 0`). + env.as_contract(&contract_id, || { + env.storage() + .persistent() + .set(&DataKeyScoped::ConsumedOracleNonce(1, 1), &true); + }); + + // Even with a perfectly valid signature, an already-used nonce is + // rejected before settlement can proceed. + let result = client.try_resolve_round(&payload); + assert_eq!(result, Err(Ok(ContractError::OracleNonceReused))); + assert!(client.get_active_round().is_some()); + + // A fresh nonce with the same key and a valid signature still settles — + // proving the reject above was nonce-specific, not a round-wide lock. + let mut fresh = base_payload(&env, &contract_id, 0, 2, 1_000_0000); + let fresh_sig = sign_payload(&env, &signing_key, &fresh); + fresh.attestation = Some(fresh_sig); + + client.resolve_round(&fresh); + assert_eq!(client.get_active_round(), None); +} + +#[test] +fn test_attestation_confidence_changes_do_not_invalidate_signature() { + let env = Env::default(); + let (client, contract_id, _admin, _oracle) = setup(&env); + let (pubkey, signing_key) = generate_keypair(&env); + + client.set_attestation_key(&Some(pubkey)); + client.create_round(&1_000_0000, &None); + + env.ledger().with_mut(|li| { + li.sequence_number = 12; + }); + + // Sign the payload before the confidence value is attached. `confidence` + // is deliberately excluded from the signed message (advisory metadata), + // so attaching or updating it afterwards must not cause a rejection. + let mut payload = base_payload(&env, &contract_id, 0, 1, 1_000_0000); + let signature = sign_payload(&env, &signing_key, &payload); + payload.confidence = Some(5_000); + payload.attestation = Some(signature); + + client.resolve_round(&payload); + assert_eq!(client.get_active_round(), None); +} + +#[test] +fn test_attestation_message_is_domain_separated() { + let env = Env::default(); + let (_client, contract_id, _admin, _oracle) = setup(&env); + + let payload = base_payload(&env, &contract_id, 0, 1, 1_000_0000); + let message = _build_attestation_message(&env, &payload); + + // Messages carry a fixed domain-separation prefix so signatures can + // never be replayed against a different message type with identical + // XDR encoding. + let slice: std::vec::Vec = message.iter().collect(); + assert!(slice.starts_with(b"XELMA_ORACLE_ATTESTATION_V1")); + + // Same payload -> byte-for-byte identical message (deterministic signing + // target). + assert_eq!(message, _build_attestation_message(&env, &payload)); + + // Every bound field must alter the message; `confidence` and + // `attestation` must not. + let mut field_tamper = payload.clone(); + field_tamper.price += 1; + assert_ne!(message, _build_attestation_message(&env, &field_tamper)); + + let mut field_tamper = payload.clone(); + field_tamper.timestamp += 1; + assert_ne!(message, _build_attestation_message(&env, &field_tamper)); + + let mut field_tamper = payload.clone(); + field_tamper.round_id += 1; + assert_ne!(message, _build_attestation_message(&env, &field_tamper)); + + let mut field_tamper = payload.clone(); + field_tamper.nonce += 1; + assert_ne!(message, _build_attestation_message(&env, &field_tamper)); + + let mut field_tamper = payload.clone(); + field_tamper.network_id = BytesN::from_array(&env, &[0xFFu8; 32]); + assert_ne!(message, _build_attestation_message(&env, &field_tamper)); + + let mut field_tamper = payload.clone(); + field_tamper.contract_addr = Address::generate(&env); + assert_ne!(message, _build_attestation_message(&env, &field_tamper)); + + let mut unsigned_tamper = payload.clone(); + unsigned_tamper.confidence = Some(5_000); + assert_eq!(message, _build_attestation_message(&env, &unsigned_tamper)); + + let mut unsigned_tamper = payload.clone(); + unsigned_tamper.attestation = Some(BytesN::from_array(&env, &[0x01u8; 64])); + assert_eq!(message, _build_attestation_message(&env, &unsigned_tamper)); } #[test] diff --git a/docs/ORACLE_OPERATOR_RUNBOOK.md b/docs/ORACLE_OPERATOR_RUNBOOK.md index a97d8d9..4468906 100644 --- a/docs/ORACLE_OPERATOR_RUNBOOK.md +++ b/docs/ORACLE_OPERATOR_RUNBOOK.md @@ -53,6 +53,17 @@ field is validated in order; a failure at any step rejects the entire submission | `nonce` | `u64` | Yes | Per-round replay protection. Must be unique per round. | | `network_id` | `BytesN<32>`| Yes | SHA-256 hash of the network passphrase. Prevents cross-network replay. | | `contract_addr` | `Address` | Yes | The contract this payload targets. Prevents cross-contract replay. | +| `attestation` | `BytesN<64>` | Only when a key is configured | Detached ed25519 signature over the domain-separated message (see attestation threat note below). Rejected if missing or invalid when an attestation key is set. | +| `confidence` | `Option` | No | Advisory belief strength in basis points. **Not** covered by the attestation signature. | + +> **Attestation threat note (Issue #263):** when `set_attestation_key` is configured, the +> oracle signs a domain-separated message (`XELMA_ORACLE_ATTESTATION_V1` + the XDR encoding +> of network_id, contract_addr, round_id, price, timestamp, nonce) with a key that may differ +> from the submitting Soroban account. Verification is fail-closed: an invalid signature +> aborts the transaction before any state change, and the round is left resolvable. When no +> key is configured this gate is a no-op, preserving pre-#263 behaviour. The signature vouches +> for *binding* (correct network/contract/round/price/time/nonce), not for the correctness of +> the price itself — the oracle remains a trusted signer for price accuracy (§1). ### Validation order (`settlement.rs`)