apollo_consensus_orchestrator,apollo_l1_gas_price: bound the per-block eth to fri rate change

crates/apollo_consensus_orchestrator/src/metrics.rs

@@ -18,6 +18,7 @@ define_metrics!(
         MetricGauge { CONSENSUS_L2_GAS_PRICE_AT_MINIMUM, "consensus_l2_gas_price_at_minimum", "1 when the accepted L2 gas price is clamped at the configured minimum (min_l2_gas_price_per_height, or the versioned-constants min_gas_price fallback), else 0" },
         MetricCounter { CONSENSUS_L1_GAS_PRICE_PROVIDER_ERROR, "consensus_l1_gas_price_provider_error", "Number of times the context got an error when querying the L1 gas price provider", init=0},
         MetricCounter { CONSENSUS_RETROSPECTIVE_BLOCK_HASH_MISMATCH, "consensus_retrospective_block_hash_mismatch", "Number of times the retrospective block hashes of the state sync and the batcher mismatched", init=0},
+        MetricCounter { CONSENSUS_ETH_TO_FRI_RATE_CLAMPED, "consensus_eth_to_fri_rate_clamped", "Number of times the eth to fri rate from the oracle was clamped to the per-block change bound around the rate implied by the previous block", init=0},
 
 
         // Cende metrics
@@ -101,6 +102,7 @@ pub(crate) fn register_metrics() {
     CONSENSUS_L2_GAS_PRICE_AT_MINIMUM.register();
     CONSENSUS_L1_GAS_PRICE_PROVIDER_ERROR.register();
     CONSENSUS_RETROSPECTIVE_BLOCK_HASH_MISMATCH.register();
+    CONSENSUS_ETH_TO_FRI_RATE_CLAMPED.register();
     CENDE_LAST_PREPARED_BLOB_BLOCK_NUMBER.register();
     CENDE_PREPARE_BLOB_FOR_NEXT_HEIGHT_LATENCY.register();
     CENDE_WRITE_PREV_HEIGHT_BLOB_LATENCY.register();

crates/apollo_consensus_orchestrator/src/utils.rs

@@ -16,6 +16,7 @@ use apollo_state_sync_types::errors::StateSyncError;
 use apollo_time::time::{Clock, DateTime};
 // TODO(Gilad): Define in consensus, either pass to blockifier as config or keep the dup.
 use blockifier::abi::constants::STORED_BLOCK_HASH_BUFFER;
+use ethnum::U256;
 use futures::channel::mpsc;
 use futures::SinkExt;
 use num_rational::Ratio;
@@ -35,10 +36,13 @@ use starknet_api::StarknetApiError;
 use tracing::{info, warn};
 
 use crate::metrics::{
+    CONSENSUS_ETH_TO_FRI_RATE_CLAMPED,
     CONSENSUS_L1_GAS_PRICE_PROVIDER_ERROR,
     CONSENSUS_RETROSPECTIVE_BLOCK_HASH_MISMATCH,
 };
 
+const PARTS_PER_THOUSAND: u128 = 1000;
+
 pub(crate) struct StreamSender {
     pub proposal_sender: mpsc::Sender<ProposalPart>,
 }
@@ -79,6 +83,7 @@ pub(crate) struct GasPriceParams {
     pub override_l1_gas_price_fri: Option<GasPrice>,
     pub override_l1_data_gas_price_fri: Option<GasPrice>,
     pub override_eth_to_fri_rate: Option<u128>,
+    pub max_eth_to_fri_rate_change_ppt: u128,
 }
 
 #[derive(Clone, Debug)]
@@ -161,6 +166,11 @@ pub(crate) async fn get_l1_prices_in_fri_and_wei_and_conversion_rate(
             "raw eth_to_fri_rate (from oracle): {eth_to_fri_rate}, raw l1 gas price wei (from \
              provider): {price_info:?}"
         );
+        let eth_to_fri_rate = bound_eth_to_fri_rate_change(
+            eth_to_fri_rate,
+            previous_proposal_init,
+            gas_price_params.max_eth_to_fri_rate_change_ppt,
+        );
         apply_fee_transformations(&mut price_info, gas_price_params);
         let prices_in_wei = L1PricesInWei {
             l1_gas_price: price_info.base_fee_per_gas,
@@ -483,7 +493,61 @@ pub(crate) fn make_gas_price_params(config: &ContextDynamicConfig) -> GasPricePa
         override_l1_gas_price_fri: config.override_l1_gas_price_fri.map(GasPrice),
         override_l1_data_gas_price_fri: config.override_l1_data_gas_price_fri.map(GasPrice),
         override_eth_to_fri_rate: config.override_eth_to_fri_rate,
+        max_eth_to_fri_rate_change_ppt: config.max_eth_to_fri_rate_change_ppt,
+    }
+}
+
+/// Clamp a freshly fetched eth to fri rate into a band of `max_change_ppt` parts per thousand
+/// around the rate implied by the previous block, and return the clamped value.
+///
+/// The band's center is recomputed from the previous block's recorded wei and fri prices, so it is
+/// a function of the block header and of config alone: every validator derives the same center and
+/// maps a given oracle reading to the same rate. Clamping rather than rejecting bounds the effect
+/// of a single manipulated reading, and forces an attacker to hold the feed across many blocks to
+/// move the rate far.
+fn bound_eth_to_fri_rate_change(
+    oracle_eth_to_fri_rate: u128,
+    previous_proposal_init: Option<&PreviousProposalInitInfo>,
+    max_change_ppt: u128,
+) -> u128 {
+    // With no previous block (startup, or the path that falls back to the minimal config values)
+    // there is no anchor to bound against, so the oracle rate is used as is.
+    let Some(previous_proposal_init) = previous_proposal_init else {
+        return oracle_eth_to_fri_rate;
+    };
+    let previous_eth_to_fri_rate = match calculate_eth_to_fri_rate(previous_proposal_init) {
+        Ok(previous_eth_to_fri_rate) => previous_eth_to_fri_rate,
+        Err(error) => {
+            warn!(
+                "Cannot bound the eth to fri rate change, the previous block info {:?} implies no \
+                 rate: {:?}",
+                previous_proposal_init, error
+            );
+            return oracle_eth_to_fri_rate;
+        }
+    };
+
+    // Multiply before dividing to avoid the precision loss of dividing first. U256 because the
+    // product of two u128 values overflows u128. The division truncates, which makes the band
+    // marginally tighter, never looser.
+    let previous_rate_u256 = U256::from(previous_eth_to_fri_rate);
+    let max_change =
+        (previous_rate_u256 * U256::from(max_change_ppt)) / U256::from(PARTS_PER_THOUSAND);
+    // The rate should not realistically approach u128::MAX, bound to avoid theoretical overflow.
+    let max_rate = u128::try_from(previous_rate_u256 + max_change).unwrap_or(u128::MAX);
+    let min_rate =
+        previous_eth_to_fri_rate.saturating_sub(u128::try_from(max_change).unwrap_or(u128::MAX));
+
+    let bounded_eth_to_fri_rate = oracle_eth_to_fri_rate.clamp(min_rate, max_rate);
+    if bounded_eth_to_fri_rate != oracle_eth_to_fri_rate {
+        warn!(
+            "Eth to fri rate {oracle_eth_to_fri_rate} from the oracle is more than \
+             {max_change_ppt} ppt away from the rate implied by the previous block \
+             ({previous_eth_to_fri_rate}), clamping it to {bounded_eth_to_fri_rate}."
+        );
+        CONSENSUS_ETH_TO_FRI_RATE_CLAMPED.increment(1);
     }
+    bounded_eth_to_fri_rate
 }
 
 fn calculate_eth_to_fri_rate(

crates/apollo_consensus_orchestrator/src/utils_test.rs

@@ -1,19 +1,37 @@
+use std::sync::Arc;
+
 use apollo_batcher_types::communication::BatcherClientError;
 use apollo_batcher_types::errors::BatcherError;
+use apollo_consensus_orchestrator_config::config::ContextDynamicConfig;
+use apollo_l1_gas_price_types::{MockL1GasPriceProviderClient, PriceInfo};
 use apollo_protobuf::consensus::ProposalInit;
 use apollo_state_sync_types::communication::StateSyncClientError;
 use apollo_state_sync_types::errors::StateSyncError;
 use assert_matches::assert_matches;
 use blockifier::abi::constants::STORED_BLOCK_HASH_BUFFER;
-use starknet_api::block::{BlockHash, BlockHashAndNumber, BlockNumber};
+use metrics_exporter_prometheus::PrometheusBuilder;
+use starknet_api::block::{
+    BlockHash,
+    BlockHashAndNumber,
+    BlockNumber,
+    GasPrice,
+    TEMP_ETH_BLOB_GAS_FEE_IN_WEI,
+    TEMP_ETH_GAS_FEE_IN_WEI,
+};
 use starknet_types_core::felt::Felt;
 
 use crate::build_proposal::ProposalBuildArguments;
+use crate::metrics::{register_metrics, CONSENSUS_ETH_TO_FRI_RATE_CLAMPED};
 use crate::test_utils::create_proposal_build_arguments;
 use crate::utils::{
     get_l1_prices_in_fri_and_wei,
+    get_l1_prices_in_fri_and_wei_and_conversion_rate,
+    make_gas_price_params,
     retrospective_block_hash,
     wait_for_retrospective_block_hash,
+    L1PricesInFri,
+    L1PricesInWei,
+    PreviousProposalInitInfo,
     RetrospectiveBlockHashError,
 };
 
@@ -22,6 +40,17 @@ const RETRO_BLOCK_NUMBER: BlockNumber = BlockNumber(0);
 const MUST_HAVE_BLOCK_HASH_FOR: BlockNumber = BlockNumber(1);
 const RETRO_BLOCK_HASH: BlockHash = BlockHash(Felt::from_hex_unchecked("0x1234567890abcdef"));
 
+const PROPOSAL_TIMESTAMP: u64 = 1_700_000_000;
+// A gas price of one gwei keeps the previous block's implied rate exactly equal to the rate it was
+// built with, for the rates used in these tests.
+const PREVIOUS_L1_GAS_PRICE_WEI: GasPrice = GasPrice(u128::pow(10, 9));
+const PREVIOUS_ETH_TO_FRI_RATE: u128 = 2 * u128::pow(10, 18);
+const MAX_ETH_TO_FRI_RATE_CHANGE_PPT: u128 = 50;
+const MAX_ETH_TO_FRI_RATE_CHANGE: u128 =
+    PREVIOUS_ETH_TO_FRI_RATE * MAX_ETH_TO_FRI_RATE_CHANGE_PPT / 1000;
+const ETH_TO_FRI_RATE_CEILING: u128 = PREVIOUS_ETH_TO_FRI_RATE + MAX_ETH_TO_FRI_RATE_CHANGE;
+const ETH_TO_FRI_RATE_FLOOR: u128 = PREVIOUS_ETH_TO_FRI_RATE - MAX_ETH_TO_FRI_RATE_CHANGE;
+
 async fn get_proposal_init(args: &ProposalBuildArguments) -> ProposalInit {
     let timestamp = args.deps.clock.unix_now();
     let (l1_prices_fri, l1_prices_wei) = get_l1_prices_in_fri_and_wei(
@@ -331,3 +360,133 @@ async fn wait_for_retrospective_block_hash_batcher_ready_after_a_while() {
         Some(BlockHashAndNumber { number: RETRO_BLOCK_NUMBER, hash: RETRO_BLOCK_HASH })
     );
 }
+
+/// Builds the previous block's recorded prices such that they imply `eth_to_fri_rate`.
+fn previous_proposal_init_with_rate(eth_to_fri_rate: u128) -> PreviousProposalInitInfo {
+    let l1_prices_wei = L1PricesInWei {
+        l1_gas_price: PREVIOUS_L1_GAS_PRICE_WEI,
+        l1_data_gas_price: PREVIOUS_L1_GAS_PRICE_WEI,
+    };
+    let l1_prices_fri = L1PricesInFri::convert_from_wei(&l1_prices_wei, eth_to_fri_rate)
+        .expect("Test prices should be convertible to fri.");
+    PreviousProposalInitInfo { timestamp: PROPOSAL_TIMESTAMP, l1_prices_wei, l1_prices_fri }
+}
+
+/// Fetches the eth to fri rate through the full oracle path, with a freshly built provider and a
+/// freshly built config, both independent of any other instance.
+async fn fetch_eth_to_fri_rate(
+    oracle_eth_to_fri_rate: u128,
+    previous_proposal_init: Option<&PreviousProposalInitInfo>,
+) -> u128 {
+    let mut l1_gas_price_provider = MockL1GasPriceProviderClient::new();
+    l1_gas_price_provider.expect_get_rate().return_const(Ok(oracle_eth_to_fri_rate));
+    l1_gas_price_provider.expect_get_price_info().return_const(Ok(PriceInfo {
+        base_fee_per_gas: GasPrice(TEMP_ETH_GAS_FEE_IN_WEI),
+        blob_fee: GasPrice(TEMP_ETH_BLOB_GAS_FEE_IN_WEI),
+    }));
+    let dynamic_config = ContextDynamicConfig {
+        max_eth_to_fri_rate_change_ppt: MAX_ETH_TO_FRI_RATE_CHANGE_PPT,
+        ..Default::default()
+    };
+
+    let (_l1_prices_fri, _l1_prices_wei, eth_to_fri_rate) =
+        get_l1_prices_in_fri_and_wei_and_conversion_rate(
+            Arc::new(l1_gas_price_provider),
+            PROPOSAL_TIMESTAMP,
+            previous_proposal_init,
+            &make_gas_price_params(&dynamic_config),
+        )
+        .await;
+    eth_to_fri_rate
+}
+
+#[tokio::test]
+async fn eth_to_fri_rate_inside_bound_is_not_clamped() {
+    let previous_proposal_init = previous_proposal_init_with_rate(PREVIOUS_ETH_TO_FRI_RATE);
+    let oracle_eth_to_fri_rate = PREVIOUS_ETH_TO_FRI_RATE + MAX_ETH_TO_FRI_RATE_CHANGE / 2;
+    assert_eq!(
+        fetch_eth_to_fri_rate(oracle_eth_to_fri_rate, Some(&previous_proposal_init)).await,
+        oracle_eth_to_fri_rate
+    );
+}
+
+#[tokio::test]
+async fn eth_to_fri_rate_above_bound_is_clamped_to_ceiling() {
+    let previous_proposal_init = previous_proposal_init_with_rate(PREVIOUS_ETH_TO_FRI_RATE);
+    assert_eq!(
+        fetch_eth_to_fri_rate(PREVIOUS_ETH_TO_FRI_RATE * 3, Some(&previous_proposal_init)).await,
+        ETH_TO_FRI_RATE_CEILING
+    );
+}
+
+#[tokio::test]
+async fn eth_to_fri_rate_below_bound_is_clamped_to_floor() {
+    let previous_proposal_init = previous_proposal_init_with_rate(PREVIOUS_ETH_TO_FRI_RATE);
+    assert_eq!(
+        fetch_eth_to_fri_rate(PREVIOUS_ETH_TO_FRI_RATE / 3, Some(&previous_proposal_init)).await,
+        ETH_TO_FRI_RATE_FLOOR
+    );
+}
+
+#[tokio::test]
+async fn eth_to_fri_rate_exactly_at_bound_is_not_clamped() {
+    let previous_proposal_init = previous_proposal_init_with_rate(PREVIOUS_ETH_TO_FRI_RATE);
+    assert_eq!(
+        fetch_eth_to_fri_rate(ETH_TO_FRI_RATE_CEILING, Some(&previous_proposal_init)).await,
+        ETH_TO_FRI_RATE_CEILING
+    );
+    assert_eq!(
+        fetch_eth_to_fri_rate(ETH_TO_FRI_RATE_FLOOR, Some(&previous_proposal_init)).await,
+        ETH_TO_FRI_RATE_FLOOR
+    );
+}
+
+#[tokio::test]
+async fn eth_to_fri_rate_is_not_clamped_without_a_previous_block() {
+    let oracle_eth_to_fri_rate = PREVIOUS_ETH_TO_FRI_RATE * 100;
+    assert_eq!(fetch_eth_to_fri_rate(oracle_eth_to_fri_rate, None).await, oracle_eth_to_fri_rate);
+}
+
+#[tokio::test]
+async fn eth_to_fri_rate_clamp_metric_increments_only_when_clamping() {
+    let recorder = PrometheusBuilder::new().build_recorder();
+    let _recorder_guard = metrics::set_default_local_recorder(&recorder);
+    register_metrics();
+
+    let previous_proposal_init = previous_proposal_init_with_rate(PREVIOUS_ETH_TO_FRI_RATE);
+    fetch_eth_to_fri_rate(PREVIOUS_ETH_TO_FRI_RATE, Some(&previous_proposal_init)).await;
+    CONSENSUS_ETH_TO_FRI_RATE_CLAMPED.assert_eq(&recorder.handle().render(), 0);
+
+    fetch_eth_to_fri_rate(PREVIOUS_ETH_TO_FRI_RATE * 3, Some(&previous_proposal_init)).await;
+    CONSENSUS_ETH_TO_FRI_RATE_CLAMPED.assert_eq(&recorder.handle().render(), 1);
+}
+
+/// Two instances built independently of each other, given the same previous block and the same
+/// oracle rate, must agree: the bound reads only the previous block and the config, so neither the
+/// instance nor the number of oracle calls it has made can change the outcome.
+#[tokio::test]
+async fn eth_to_fri_rate_bound_is_deterministic_across_instances() {
+    let oracle_eth_to_fri_rate = PREVIOUS_ETH_TO_FRI_RATE * 7;
+
+    let first_instance_rate = fetch_eth_to_fri_rate(
+        oracle_eth_to_fri_rate,
+        Some(&previous_proposal_init_with_rate(PREVIOUS_ETH_TO_FRI_RATE)),
+    )
+    .await;
+
+    // The second instance reaches the same previous block after a different number of oracle
+    // fetches.
+    fetch_eth_to_fri_rate(
+        PREVIOUS_ETH_TO_FRI_RATE / 5,
+        Some(&previous_proposal_init_with_rate(PREVIOUS_ETH_TO_FRI_RATE * 4)),
+    )
+    .await;
+    let second_instance_rate = fetch_eth_to_fri_rate(
+        oracle_eth_to_fri_rate,
+        Some(&previous_proposal_init_with_rate(PREVIOUS_ETH_TO_FRI_RATE)),
+    )
+    .await;
+
+    assert_eq!(first_instance_rate, second_instance_rate);
+    assert_eq!(first_instance_rate, ETH_TO_FRI_RATE_CEILING);
+}

crates/apollo_consensus_orchestrator_config/src/config.rs

@@ -85,6 +85,13 @@ const ETH_FACTOR: u128 = u128::pow(10, 18);
 // Default SNIP-35 target USD cost per L2 gas unit: $0.88 per 1e9 L2 gas = 880_000_000 atto-USD.
 pub const DEFAULT_SNIP35_TARGET_ATTO_USD_PER_L2_GAS: u128 = 880_000_000;
 
+// Default per-block bound on the Eth-to-Fri rate change: 5%. Wide enough that a single feed
+// update never trips it (the ETH and STRK feeds update on deviation thresholds of about 0.5% and
+// 1%, so their ratio steps by at most ~1.5% between blocks), tight enough that one manipulated
+// reading moves the rate by at most 5% and doubling it requires holding the feed for ~15
+// consecutive blocks.
+pub const DEFAULT_MAX_ETH_TO_FRI_RATE_CHANGE_PPT: u128 = 50;
+
 // This matches the min_gas_price in orchestrator_versioned_constants_0_14_1.json (0x1dcd65000).
 const MIN_ALLOWED_GAS_PRICE: u128 = 8_000_000_000;
 
@@ -290,6 +297,10 @@ pub struct ContextDynamicConfig {
     /// SNIP-35 target USD cost per L2 gas unit, in atto-USD ($0.88 per 1e9 L2 gas = 880_000_000
     /// atto-USD).
     pub snip35_target_atto_usd_per_l2_gas: u128,
+    /// Maximum change, in parts per thousand, of the Eth-to-Fri rate relative to the rate implied
+    /// by the previous block. A fresh oracle rate outside this band is clamped to the band's edge.
+    /// The bound does not apply when there is no previous block.
+    pub max_eth_to_fri_rate_change_ppt: u128,
     /// If given, will override the L2 gas price.
     pub override_l2_gas_price_fri: Option<u128>,
     /// If given, will override the L1 gas price in FRI.
@@ -363,6 +374,14 @@ impl SerializeConfig for ContextDynamicConfig {
                  880_000_000 atto-USD).",
                 ParamPrivacyInput::Public,
             ),
+            ser_param(
+                "max_eth_to_fri_rate_change_ppt",
+                &self.max_eth_to_fri_rate_change_ppt,
+                "Maximum change, in parts per thousand, of the Eth-to-Fri rate relative to the \
+                 rate implied by the previous block. A fresh oracle rate outside this band is \
+                 clamped to the band's edge.",
+                ParamPrivacyInput::Public,
+            ),
             ser_param(
                 "compare_retrospective_block_hash",
                 &self.compare_retrospective_block_hash,
@@ -427,6 +446,7 @@ impl Default for ContextDynamicConfig {
             l1_data_gas_price_multiplier_ppt: 135,
             l1_gas_tip_wei: GWEI_FACTOR,
             snip35_target_atto_usd_per_l2_gas: DEFAULT_SNIP35_TARGET_ATTO_USD_PER_L2_GAS,
+            max_eth_to_fri_rate_change_ppt: DEFAULT_MAX_ETH_TO_FRI_RATE_CHANGE_PPT,
             override_l2_gas_price_fri: None,
             override_l1_gas_price_fri: None,
             override_l1_data_gas_price_fri: None,

crates/apollo_deployments/resources/app_configs/consensus_manager_config.json

@@ -40,6 +40,7 @@
   "consensus_manager_config.context_config.static_config.l1_da_mode": true,
   "consensus_manager_config.context_config.dynamic_config.l1_data_gas_price_multiplier_ppt": 135,
   "consensus_manager_config.context_config.dynamic_config.l1_gas_tip_wei": 1000000000,
+  "consensus_manager_config.context_config.dynamic_config.max_eth_to_fri_rate_change_ppt": 50,
   "consensus_manager_config.context_config.dynamic_config.max_l1_data_gas_price_wei": 1000000000000,
   "consensus_manager_config.context_config.dynamic_config.max_l1_gas_price_wei": 1000000000000,
   "consensus_manager_config.context_config.dynamic_config.min_l1_data_gas_price_wei": 1,

crates/apollo_deployments/resources/app_configs/replacer_consensus_manager_config.json

@@ -36,6 +36,7 @@
   "consensus_manager_config.context_config.dynamic_config.compare_retrospective_block_hash": "$$$_CONSENSUS_MANAGER_CONFIG-CONTEXT_CONFIG-DYNAMIC_CONFIG-COMPARE_RETROSPECTIVE_BLOCK_HASH_$$$",
   "consensus_manager_config.context_config.dynamic_config.l1_data_gas_price_multiplier_ppt": 135,
   "consensus_manager_config.context_config.dynamic_config.l1_gas_tip_wei": 1000000000,
+  "consensus_manager_config.context_config.dynamic_config.max_eth_to_fri_rate_change_ppt": 50,
   "consensus_manager_config.context_config.dynamic_config.max_l1_data_gas_price_wei": 1000000000000,
   "consensus_manager_config.context_config.dynamic_config.max_l1_gas_price_wei": 1000000000000,
   "consensus_manager_config.context_config.dynamic_config.min_l1_data_gas_price_wei": 1,

crates/apollo_l1_gas_price/src/rate_bounds.rs

@@ -11,11 +11,12 @@ use crate::metrics::CHAINLINK_ORACLE_RATE_OUT_OF_BOUNDS_COUNT;
 #[path = "rate_bounds_test.rs"]
 mod rate_bounds_test;
 
-// TODO(Asaf): bound the rate's change against the previous block's implied rate. The absolute
-// bounds below are wide enough to pass a manipulated but plausible answer, the STRK/USD pair alone
-// accepting anything from $0.0001 to $10, which only a bound relative to the last accepted rate
-// catches. It must be anchored to the block header rather than to node-local history, so that every
-// validator accepts and rejects the same values.
+// The bounds below are absolute: they are wide enough to pass a manipulated but plausible answer,
+// the STRK/USD pair alone accepting anything from $0.0001 to $10, and only catch a wrong feed. The
+// bound relative to the previous block's implied rate lives in
+// `apollo_consensus_orchestrator::utils`, where the previous block is in hand, so that it is
+// anchored to the block header rather than to this client's own read history and every validator
+// accepts and rejects the same values.
 /// Absolute bounds are the only defense against a feed wired to the wrong asset or a
 /// plausible-but-poisoned answer: consensus checks that validators agree with each other, never
 /// that the agreed value is sane, and every node reads the same chain state.