Priority: High
Description
engine-core is a single crate compiled to one WASM binary containing exactly two #[contract] structs: ControlPlane (core/control_plane.rs) and UpgradeableProxy (core/proxy.rs). Both share the same Soroban instance-storage namespace on deployment.
ControlPlane writes its admin to KEY_ADMIN = symbol_short!("ADMIN") (core/control_plane.rs:19). UpgradeableProxy independently defines ADMIN_KEY = symbol_short!("ADMIN") (core/proxy.rs:12) — the identical storage symbol. core/zk_hooks.rs:96-99 explicitly documents this collision is intentional ("Keep this in sync with control_plane::KEY_ADMIN ... Storage is shared because zk_hooks is invoked inside the same contract instance").
UpgradeableProxy::upgrade() (core/proxy.rs:47-61) checks only that the caller matches this shared ADMIN key and calls admin.require_auth() — no circuit-breaker check, no governance proposal, no multi-sig quorum, no timelock. This is a live, fully wired code path that sits alongside (and bypasses) the carefully governance-gated upgrade flow in upgrade.rs/governance.rs (multi-sig + TIMELOCK_LEDGERS delay).
Failure scenario
Once ControlPlane::initialize(admin) has run on a deployed instance, the same single admin address can call UpgradeableProxy::upgrade(new_wasm_hash) directly to replace the contract's code immediately — with none of the protections engineered into the governance-gated upgrade path. This makes the multi-sig + timelock upgrade design purely decorative, since a fully wired, single-key bypass exists in the same deployed binary. (Distinct from #178, which flags the governance upgrade path as unwired — this is a wired, insecure alternate path that defeats the intended control.)
Suggested fix
Either remove UpgradeableProxy::upgrade()'s direct admin path and route all upgrades through the governance-gated flow, or add the same circuit-breaker/multi-sig/timelock gating to it that upgrade.rs enforces.
Priority: High
Description
engine-coreis a single crate compiled to one WASM binary containing exactly two#[contract]structs:ControlPlane(core/control_plane.rs) andUpgradeableProxy(core/proxy.rs). Both share the same Soroban instance-storage namespace on deployment.ControlPlanewrites its admin toKEY_ADMIN = symbol_short!("ADMIN")(core/control_plane.rs:19).UpgradeableProxyindependently definesADMIN_KEY = symbol_short!("ADMIN")(core/proxy.rs:12) — the identical storage symbol.core/zk_hooks.rs:96-99explicitly documents this collision is intentional ("Keep this in sync with control_plane::KEY_ADMIN ... Storage is shared because zk_hooks is invoked inside the same contract instance").UpgradeableProxy::upgrade()(core/proxy.rs:47-61) checks only that the caller matches this sharedADMINkey and callsadmin.require_auth()— no circuit-breaker check, no governance proposal, no multi-sig quorum, no timelock. This is a live, fully wired code path that sits alongside (and bypasses) the carefully governance-gated upgrade flow inupgrade.rs/governance.rs(multi-sig +TIMELOCK_LEDGERSdelay).Failure scenario
Once
ControlPlane::initialize(admin)has run on a deployed instance, the same singleadminaddress can callUpgradeableProxy::upgrade(new_wasm_hash)directly to replace the contract's code immediately — with none of the protections engineered into the governance-gated upgrade path. This makes the multi-sig + timelock upgrade design purely decorative, since a fully wired, single-key bypass exists in the same deployed binary. (Distinct from #178, which flags the governance upgrade path as unwired — this is a wired, insecure alternate path that defeats the intended control.)Suggested fix
Either remove
UpgradeableProxy::upgrade()'s direct admin path and route all upgrades through the governance-gated flow, or add the same circuit-breaker/multi-sig/timelock gating to it thatupgrade.rsenforces.