Security review: operator RPC hardening findings (mirrors ust1-window sweep)

Brouie commented

2026-04-22 04:36:12 +00:00

(Migrated from gitlab.com)

@PlasticDigits parallel security check on operator following the ust1-window 40-category sweep and matching the canceler review in #114.

Same guidance from you: launch-blocking status none at medium severity. Filing as single summary issue.

Findings

EVM-01: Sequential RPC fallback, not multi-provider cross-check — Medium

operator/src/watchers/evm.rs:715 uses the same sequential fallback loop as canceler ("get_block_number succeeded on fallback RPC #{i+1}"). First responsive RPC wins, no consensus check.

Recommendation: mirror ust1-window 8c9fafb — parallel first two URLs + 0.01% tolerance + tiebreaker.

EVM-02: `finality_blocks` default is weak — Medium

operator already has the reorg-protection mechanic: operator/src/watchers/evm.rs:722 returns block.saturating_sub(self.finality_blocks). Good.

But: default_finality_blocks() = 1 in operator/src/config.rs:297. 1 block is insufficient for BSC, which has seen 6-12 block reorgs historically. opBNB similar.

Recommendation: raise default to something chain-appropriate (BSC ≥ 15, opBNB ≥ 12), or add per-chain defaults based on chain_id. Existing mechanic is correct — just needs tuning.

EVM-03: No canonical bridge address allowlist — Low-Med

Same as canceler: EVM_BRIDGE_ADDRESS accepted as any hex string, no canonical-address check at startup.

Recommendation: same as canceler — startup check against a compile-time constant or allowlist, scoped per deployment environment.

EVM-08: HTTPS validation warns but doesn't block — Low-Med

Operator uses multichain_rs::validate_rpc_url — shared helper that accepts http:// (tested to at multichain-rs/src/multi_evm.rs:632-633). Same weakness as canceler.

Recommendation: fix once in multichain-rs with DEV_ALLOW_HTTP=1 gate, both canceler + operator get the fix. Central crate already, so one change covers both.

EVM-10: `/health` returns unconditional `OK`, no staleness alert — Medium

operator/src/api.rs:123 routes /health → health_handler which returns literal "OK". /metrics endpoint exists and gathers Prometheus metrics, which is better than nothing, but no built-in staleness detection.

Recommendation: same as canceler — LivenessTracker with last_successful_broadcast_ts, return non-OK on extended silence. /metrics can stay exposed in parallel for Prometheus scrape.

EVM-14: No `eth_chainId` verification — Low-Med

Operator reads EVM_CHAIN_ID from env, never verifies against RPC's actual eth_chainId. get_chain_id_from_hash exists but that's a contract-level helper, not RPC verification.

Recommendation: startup verify_all_bsc_rpc_urls equivalent. Pattern from ust1-window 673ac74.

EVM-18: ✅ PASS (no finding)

EvmConfig, TerraConfig, SolanaConfig, DatabaseConfig all have manual fmt::Debug impls that redact secrets. Consistent pattern with canceler. Clean.

Shared fix opportunities

Several findings land in multichain-rs (shared crate used by both canceler + operator):

EVM-08 validate_rpc_url — one change, two components fixed
EVM-01 multi-provider cross-check helper — new function in multichain-rs, both callers adopt

Could knock out 2-3 findings per component with shared-crate work rather than duplicating.

Cross-references

Canceler counterpart: #114
ust1-window sweep: PlasticDigits/ust1-window#5

@PlasticDigits parallel security check on operator following the ust1-window 40-category sweep and matching the canceler review in #114. Same guidance from you: launch-blocking status **none** at medium severity. Filing as single summary issue. ## Findings ### EVM-01: Sequential RPC fallback, not multi-provider cross-check — Medium `operator/src/watchers/evm.rs:715` uses the same sequential fallback loop as canceler ("get_block_number succeeded on fallback RPC #{i+1}"). First responsive RPC wins, no consensus check. **Recommendation:** mirror ust1-window 8c9fafb — parallel first two URLs + 0.01% tolerance + tiebreaker. ### EVM-02: `finality_blocks` default is weak — Medium `operator` already has the reorg-protection mechanic: `operator/src/watchers/evm.rs:722` returns `block.saturating_sub(self.finality_blocks)`. Good. **But**: `default_finality_blocks() = 1` in `operator/src/config.rs:297`. 1 block is insufficient for BSC, which has seen 6-12 block reorgs historically. opBNB similar. **Recommendation:** raise default to something chain-appropriate (BSC ≥ 15, opBNB ≥ 12), or add per-chain defaults based on `chain_id`. Existing mechanic is correct — just needs tuning. ### EVM-03: No canonical bridge address allowlist — Low-Med Same as canceler: `EVM_BRIDGE_ADDRESS` accepted as any hex string, no canonical-address check at startup. **Recommendation:** same as canceler — startup check against a compile-time constant or allowlist, scoped per deployment environment. ### EVM-08: HTTPS validation warns but doesn't block — Low-Med Operator uses `multichain_rs::validate_rpc_url` — shared helper that accepts `http://` (tested to at `multichain-rs/src/multi_evm.rs:632-633`). Same weakness as canceler. **Recommendation:** fix once in `multichain-rs` with `DEV_ALLOW_HTTP=1` gate, both canceler + operator get the fix. Central crate already, so one change covers both. ### EVM-10: `/health` returns unconditional `OK`, no staleness alert — Medium `operator/src/api.rs:123` routes `/health` → `health_handler` which returns literal `"OK"`. `/metrics` endpoint exists and gathers Prometheus metrics, which is better than nothing, but no built-in staleness detection. **Recommendation:** same as canceler — `LivenessTracker` with `last_successful_broadcast_ts`, return non-OK on extended silence. `/metrics` can stay exposed in parallel for Prometheus scrape. ### EVM-14: No `eth_chainId` verification — Low-Med Operator reads `EVM_CHAIN_ID` from env, never verifies against RPC's actual `eth_chainId`. `get_chain_id_from_hash` exists but that's a contract-level helper, not RPC verification. **Recommendation:** startup `verify_all_bsc_rpc_urls` equivalent. Pattern from ust1-window 673ac74. ### EVM-18: ✅ PASS (no finding) `EvmConfig`, `TerraConfig`, `SolanaConfig`, `DatabaseConfig` all have manual `fmt::Debug` impls that redact secrets. Consistent pattern with canceler. Clean. ## Shared fix opportunities Several findings land in `multichain-rs` (shared crate used by both canceler + operator): - **EVM-08** `validate_rpc_url` — one change, two components fixed - **EVM-01** multi-provider cross-check helper — new function in multichain-rs, both callers adopt Could knock out 2-3 findings per component with shared-crate work rather than duplicating. ## Cross-references - Canceler counterpart: #114 - ust1-window sweep: `PlasticDigits/ust1-window#5`

PlasticDigits commented

2026-04-22 06:29:44 +00:00

(Migrated from gitlab.com)

Operator-side changes merged to main (8efb987).

EVM-01: EvmWatcher uses evm_consensus_latest_block + EvmRpcReadPolicy (same env vars as canceler). eth_getLogs uses the consensus endpoint first, then one pass over alternates if the consensus node returns a transport error.

EVM-02: FINALITY_BLOCKS default is chain-aware (BSC 15, opBNB 12, else 12).

EVM-03 / 08 / 14: EVM_CANONICAL_BRIDGE_ADDRESSES, stricter validate_rpc_url, startup eth_chainId verification for primary + EVM_CHAINS RPCs, plus Terra LCD/RPC URL validation on load.

EVM-10: packages/operator/src/liveness.rs — watchers call touch_activity(); /health returns 503 when idle exceeds HEALTH_MAX_IDLE_SECS (default 8h) after HEALTH_STARTUP_GRACE_SECS (default 5m).

E2E sets DEV_ALLOW_HTTP=1 for Anvil.

@Brouie please verify.

Operator-side changes merged to `main` (8efb987). **EVM-01:** `EvmWatcher` uses `evm_consensus_latest_block` + `EvmRpcReadPolicy` (same env vars as canceler). `eth_getLogs` uses the consensus endpoint first, then one pass over alternates if the consensus node returns a transport error. **EVM-02:** `FINALITY_BLOCKS` default is chain-aware (BSC 15, opBNB 12, else 12). **EVM-03 / 08 / 14:** `EVM_CANONICAL_BRIDGE_ADDRESSES`, stricter `validate_rpc_url`, startup `eth_chainId` verification for primary + `EVM_CHAINS` RPCs, plus Terra LCD/RPC URL validation on load. **EVM-10:** `packages/operator/src/liveness.rs` — watchers call `touch_activity()`; `/health` returns 503 when idle exceeds `HEALTH_MAX_IDLE_SECS` (default 8h) after `HEALTH_STARTUP_GRACE_SECS` (default 5m). E2E sets `DEV_ALLOW_HTTP=1` for Anvil. @Brouie please verify.

Brouie commented

2026-04-23 01:22:41 +00:00

(Migrated from gitlab.com)

@PlasticDigits verified on 8efb987.

Build + test baseline:

multichain-rs: 166 tests passing
operator: 95 tests passing (53 + 42, 3 ignored)
0 failures

Findings:

EVM-01 RPC quorum: inherits shared multichain-rs EvmRpcReadPolicy, consensus reads wired into EvmWatcher.
EVM-02 finality depth: chain-aware default_finality_for_native_chain (BSC=15, opBNB=12, default=12), unit test at config.rs:637 asserts each default.
EVM-03 bridge allowlist: EVM_CANONICAL_BRIDGE_ADDRESSES enforcement at load — same pattern as canceler. Validates primary + all multi-chain bridge addresses.
EVM-08 HTTPS gate: covered via shared validate_rpc_url. Terra and Solana URL validation also added (bonus coverage beyond my original scope).
EVM-10 /health staleness: new liveness.rs module. Atomic touch_activity() called in EVM, Terra, and Solana watchers on successful polls. /health returns 503 with diagnostic message when stale.
EVM-14 chainId verify: verify_evm_rpc_chain_ids at startup (multichain-rs), plus operator-level startup verify.
EVM-18 (no regression): manual impl fmt::Debug retained on DatabaseConfig, EvmConfig, TerraConfig, SolanaConfig.

Closing as verified.

@PlasticDigits verified on `8efb987`. Build + test baseline: - multichain-rs: 166 tests passing - operator: 95 tests passing (53 + 42, 3 ignored) - 0 failures Findings: - **EVM-01 RPC quorum**: inherits shared multichain-rs `EvmRpcReadPolicy`, consensus reads wired into `EvmWatcher`. - **EVM-02 finality depth**: chain-aware `default_finality_for_native_chain` (BSC=15, opBNB=12, default=12), unit test at config.rs:637 asserts each default. - **EVM-03 bridge allowlist**: `EVM_CANONICAL_BRIDGE_ADDRESSES` enforcement at load — same pattern as canceler. Validates primary + all multi-chain bridge addresses. - **EVM-08 HTTPS gate**: covered via shared `validate_rpc_url`. Terra and Solana URL validation also added (bonus coverage beyond my original scope). - **EVM-10 /health staleness**: new `liveness.rs` module. Atomic `touch_activity()` called in EVM, Terra, and Solana watchers on successful polls. `/health` returns 503 with diagnostic message when stale. - **EVM-14 chainId verify**: `verify_evm_rpc_chain_ids` at startup (multichain-rs), plus operator-level startup verify. - **EVM-18** (no regression): manual `impl fmt::Debug` retained on DatabaseConfig, EvmConfig, TerraConfig, SolanaConfig. Closing as verified.

Brouie (Migrated from gitlab.com) closed this issue

2026-04-23 01:22:54 +00:00

PlasticDigits commented

2026-08-31 04:29:43 +00:00

(Migrated from gitlab.com)

mentioned in issue #138