Merge pull request #1155 from ayinde38/fix/issue-1111-oracle-access-control

fix(oracle): gate update_price on the registered oracle address

Author: yosemite01

backend/contracts/oracle/src/lib.rs

@@ -12,6 +12,9 @@ use soroban_sdk::{
     contract, contractimpl, contracttype, Address, Env, Symbol,
 };
 
+#[cfg(test)]
+mod test;
+
 // ---------------------------------------------------------------------------
 // Constants
 // ---------------------------------------------------------------------------
@@ -62,6 +65,8 @@ pub struct ValuationResult {
 
 #[contracttype]
 enum DataKey {
+    /// Contract administrator, set once by `initialize`.
+    Admin,
     /// Address of the registered oracle contract.
     OracleAddress,
     /// Last accepted price snapshot.
@@ -79,20 +84,68 @@ pub struct OracleContract;
 impl OracleContract {
     // ── Admin ────────────────────────────────────────────────────────────────
 
-    /// Register the address of the upstream oracle contract.
+    /// Set the contract administrator. Callable once.
+    ///
+    /// Without this, `set_oracle`'s `admin` parameter was self-asserted: the
+    /// caller passed whichever address they controlled, `require_auth()`
+    /// confirmed only that they controlled *that* address, and nothing tied it
+    /// to any privileged role. Anyone could re-point the oracle.
+    pub fn initialize(env: Env, admin: Address) {
+        assert!(
+            !env.storage().persistent().has(&DataKey::Admin),
+            "Contract already initialized"
+        );
+        admin.require_auth();
+        env.storage().persistent().set(&DataKey::Admin, &admin);
+    }
+
+    /// Register the address of the upstream oracle contract. Admin-only.
     pub fn set_oracle(env: Env, admin: Address, oracle: Address) {
         admin.require_auth();
+        Self::require_admin(&env, &admin);
         env.storage()
             .persistent()
             .set(&DataKey::OracleAddress, &oracle);
+
+        env.events().publish(
+            (Symbol::new(&env, "oracle"), Symbol::new(&env, "oracle_set")),
+            oracle,
+        );
+    }
+
+    /// Return the registered oracle address, if one has been set.
+    pub fn get_oracle(env: Env) -> Option<Address> {
+        env.storage().persistent().get(&DataKey::OracleAddress)
+    }
+
+    /// Return the administrator, if the contract has been initialized.
+    pub fn get_admin(env: Env) -> Option<Address> {
+        env.storage().persistent().get(&DataKey::Admin)
     }
 
     // ── Price feed ───────────────────────────────────────────────────────────
 
-    /// Push a new price observation (called by the oracle aggregator).
+    /// Push a new price observation. Callable only by the registered oracle.
     pub fn update_price(env: Env, caller: Address, price_data: PriceData) {
         caller.require_auth();
 
+        // The check this contract is named for, and previously did not perform.
+        //
+        // `require_auth()` alone proves only that `caller` authorised the call —
+        // it says nothing about *who* `caller` is. Any address could satisfy it
+        // with its own signature and write the authoritative price.
+        //
+        // Rejecting when no oracle is registered matters as much as the
+        // comparison: with an empty LastPrice there is nothing to deviate from,
+        // so the first writer could set any positive price at all, and every
+        // subsequent update would then be anchored to that value.
+        let registered: Address = env
+            .storage()
+            .persistent()
+            .get(&DataKey::OracleAddress)
+            .expect("No oracle registered; call set_oracle first");
+        assert!(caller == registered, "Caller is not the registered oracle");
+
         // Reject obviously anomalous prices (zero or negative).
         assert!(price_data.price_micro_usd > 0, "Price must be positive");
 
@@ -167,6 +220,22 @@ impl OracleContract {
             used_fallback,
         }
     }
+
+    // ── Internal ─────────────────────────────────────────────────────────────
+
+    /// Assert that `caller` is the stored administrator.
+    ///
+    /// Panics when the contract has not been initialized, rather than treating
+    /// an absent admin as "anyone may proceed" — an uninitialized contract must
+    /// be closed, not open.
+    fn require_admin(env: &Env, caller: &Address) {
+        let admin: Address = env
+            .storage()
+            .persistent()
+            .get(&DataKey::Admin)
+            .expect("Contract not initialized; call initialize first");
+        assert!(*caller == admin, "Caller is not the admin");
+    }
 }
 
 // ---------------------------------------------------------------------------

backend/contracts/oracle/src/test.rs

@@ -0,0 +1,168 @@
+#![cfg(test)]
+
+//! Access-control tests for the oracle price feed (issue #1111).
+//!
+//! The contract's purpose is to be a trustworthy price source for fiat-pegged
+//! bounties. Before this fix `update_price` checked only that the caller had
+//! signed for itself, never that it was the registered oracle — so these tests
+//! are mostly about proving the door is now shut, from each direction it was
+//! previously open.
+
+use super::*;
+use soroban_sdk::testutils::Address as _;
+
+fn setup() -> (Env, OracleContractClient<'static>, Address, Address) {
+    let env = Env::default();
+    env.mock_all_auths();
+    let id = env.register_contract(None, OracleContract);
+    let client = OracleContractClient::new(&env, &id);
+    let admin = Address::generate(&env);
+    let oracle = Address::generate(&env);
+    (env, client, admin, oracle)
+}
+
+fn price(env: &Env, micro_usd: i128) -> PriceData {
+    PriceData {
+        price_micro_usd: micro_usd,
+        timestamp: env.ledger().timestamp(),
+    }
+}
+
+// ── The hole itself ──────────────────────────────────────────────────────────
+
+#[test]
+#[should_panic(expected = "Caller is not the registered oracle")]
+fn arbitrary_address_cannot_push_a_price() {
+    let (env, client, admin, oracle) = setup();
+    client.initialize(&admin);
+    client.set_oracle(&admin, &oracle);
+
+    // Before the fix this succeeded: require_auth() proved only that the
+    // attacker controlled its own address, which it always does.
+    let attacker = Address::generate(&env);
+    client.update_price(&attacker, &price(&env, 999_999));
+}
+
+#[test]
+#[should_panic(expected = "No oracle registered")]
+fn price_cannot_be_set_before_an_oracle_is_registered() {
+    let (env, client, admin, _oracle) = setup();
+    client.initialize(&admin);
+
+    // The worst case previously: with no LastPrice there is nothing to deviate
+    // from, so the first writer could set *any* positive price, and every later
+    // update would be anchored to it — the deviation guard would then protect
+    // the attacker's number rather than the real one.
+    let attacker = Address::generate(&env);
+    client.update_price(&attacker, &price(&env, 1));
+}
+
+#[test]
+fn registered_oracle_can_push_a_price() {
+    let (env, client, admin, oracle) = setup();
+    client.initialize(&admin);
+    client.set_oracle(&admin, &oracle);
+
+    client.update_price(&oracle, &price(&env, 120_000));
+
+    assert_eq!(client.get_price().price_micro_usd, 120_000);
+}
+
+#[test]
+#[should_panic(expected = "Caller is not the registered oracle")]
+fn previous_oracle_cannot_push_after_being_replaced() {
+    let (env, client, admin, oracle) = setup();
+    client.initialize(&admin);
+    client.set_oracle(&admin, &oracle);
+    client.update_price(&oracle, &price(&env, 120_000));
+
+    // Rotating the oracle must actually revoke the old one.
+    let new_oracle = Address::generate(&env);
+    client.set_oracle(&admin, &new_oracle);
+    client.update_price(&oracle, &price(&env, 121_000));
+}
+
+// ── Admin gating ─────────────────────────────────────────────────────────────
+
+#[test]
+#[should_panic(expected = "Caller is not the admin")]
+fn non_admin_cannot_register_an_oracle() {
+    let (env, client, admin, oracle) = setup();
+    client.initialize(&admin);
+
+    // Previously set_oracle's `admin` parameter was self-asserted — the caller
+    // passed whichever address it controlled and require_auth() was satisfied.
+    let impostor = Address::generate(&env);
+    client.set_oracle(&impostor, &oracle);
+}
+
+#[test]
+#[should_panic(expected = "Contract not initialized")]
+fn set_oracle_fails_before_initialize() {
+    let (_env, client, admin, oracle) = setup();
+
+    // An uninitialized contract must be closed, not open — an absent admin is
+    // not "anyone may proceed".
+    client.set_oracle(&admin, &oracle);
+}
+
+#[test]
+#[should_panic(expected = "Contract already initialized")]
+fn initialize_is_once_only() {
+    let (env, client, admin, _oracle) = setup();
+    client.initialize(&admin);
+
+    // Otherwise anyone could re-initialize and take over as admin.
+    client.initialize(&Address::generate(&env));
+}
+
+#[test]
+fn admin_can_rotate_the_oracle() {
+    let (env, client, admin, oracle) = setup();
+    client.initialize(&admin);
+    client.set_oracle(&admin, &oracle);
+
+    let new_oracle = Address::generate(&env);
+    client.set_oracle(&admin, &new_oracle);
+
+    assert_eq!(client.get_oracle(), Some(new_oracle.clone()));
+    client.update_price(&new_oracle, &price(&env, 120_000));
+    assert_eq!(client.get_price().price_micro_usd, 120_000);
+}
+
+// ── Existing guards still apply to the registered oracle ─────────────────────
+
+#[test]
+#[should_panic(expected = "Price must be positive")]
+fn registered_oracle_still_cannot_push_a_non_positive_price() {
+    let (env, client, admin, oracle) = setup();
+    client.initialize(&admin);
+    client.set_oracle(&admin, &oracle);
+
+    client.update_price(&oracle, &price(&env, 0));
+}
+
+#[test]
+#[should_panic(expected = "Price deviation exceeds allowed threshold")]
+fn registered_oracle_still_cannot_exceed_the_deviation_bound() {
+    let (env, client, admin, oracle) = setup();
+    client.initialize(&admin);
+    client.set_oracle(&admin, &oracle);
+    client.update_price(&oracle, &price(&env, 100_000));
+
+    // +50%, well beyond MAX_PRICE_DEVIATION_BPS (10%). Authentication must not
+    // become a bypass for the sanity checks.
+    client.update_price(&oracle, &price(&env, 150_000));
+}
+
+#[test]
+fn deviation_within_bound_is_accepted() {
+    let (env, client, admin, oracle) = setup();
+    client.initialize(&admin);
+    client.set_oracle(&admin, &oracle);
+    client.update_price(&oracle, &price(&env, 100_000));
+
+    // +5%, inside the bound.
+    client.update_price(&oracle, &price(&env, 105_000));
+    assert_eq!(client.get_price().price_micro_usd, 105_000);
+}