From 4b5f549fae0978dd688e21a4fa0fa6f5181377df Mon Sep 17 00:00:00 2001 From: Josh Hardy Date: Mon, 10 Aug 2026 15:33:16 +0000 Subject: [PATCH] fix(deploy): guard ST0X_SIGNER against the deploy key + correct deployer NatSpec MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adversarial-pass findings on the deploy path (mutation-test run @ c371a23). deploy C1 (MED) — deploySignedPriceStack guards ST0X_ADMIN and ST0X_ORACLE_ADMIN against equalling the hot CI deploy key, but not ST0X_SIGNER. updatePrice is permissionless and authorised solely by the signer's EIP-712 signature, so the signer controls every served price even more directly than the two guarded admin roles — a copy-pasted signer == deploy key would ship the feed under CI's control with no other guard catching it. Adds the matching require(signer != deployer). New assertion in testDeployEnvConfigDispatchAndKeySeparation; mutation-verified (removing the guard fails the test). deploy C2 (LOW/doc) — MorphoPairAdapterBeaconSetDeployer NatSpec claimed the central store is 'fixed for the beacon's whole life'. The beacon owner can upgrade to an implementation built with a different central; iCentral() then reports a stale value. NatSpec now frames iCentral() as 'the central at deploy time', a value an owner-authorised beacon upgrade can change — not a live invariant. deploy C3 (LOW/doc) — the Deployment event's NatSpec implied monitoring can trust it to identify the deployer, but minting is permissionless and config-salted (msg.sender excluded), so a front-runner can appear as caller. All three deployers' event docs now direct monitoring to key on the deterministic proxy address and note a front-run mint grants no authority. Comment-only except the one-line signer guard. 190 tests; fmt/slither clean. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_013hi8wq9YkjWPcGACXCEKeL --- script/Deploy.sol | 7 ++++++ .../DIAVaultOracleBeaconSetDeployer.sol | 10 +++++++- .../MorphoPairAdapterBeaconSetDeployer.sol | 24 +++++++++++++++---- .../ST0xPriceOracleBeaconSetDeployer.sol | 10 +++++++- test/src/script/Deploy.t.sol | 9 +++++++ 5 files changed, 53 insertions(+), 7 deletions(-) diff --git a/script/Deploy.sol b/script/Deploy.sol index 2f7fa0b..aff0bee 100644 --- a/script/Deploy.sol +++ b/script/Deploy.sol @@ -111,8 +111,15 @@ contract Deploy is Script { // therefore be governance, never the hot CI deploy key — the same // separation the beacon owner enforces. Fail the deploy loudly rather // than silently leaving the feed under the deploy key's control. + // + // ST0X_SIGNER gets the SAME guard: `updatePrice` is permissionless and + // authorised solely by this signer's EIP-712 signature, so the signer + // controls every served price even MORE directly than the two admin + // roles — a copy-pasted signer equal to the hot deploy key would ship + // the feed under CI's control with no other guard catching it. require(admin != deployer, "ST0X_ADMIN must not be the deploy key"); require(oracleAdmin != deployer, "ST0X_ORACLE_ADMIN must not be the deploy key"); + require(signer != deployer, "ST0X_SIGNER must not be the deploy key"); ST0xPriceOracle oracleImpl = new ST0xPriceOracle(); oracleBSD = new ST0xPriceOracleBeaconSetDeployer( diff --git a/src/concrete/deploy/DIAVaultOracleBeaconSetDeployer.sol b/src/concrete/deploy/DIAVaultOracleBeaconSetDeployer.sol index 0d4fc94..1219e3e 100644 --- a/src/concrete/deploy/DIAVaultOracleBeaconSetDeployer.sol +++ b/src/concrete/deploy/DIAVaultOracleBeaconSetDeployer.sol @@ -41,7 +41,15 @@ struct DIAVaultOracleBeaconSetDeployerConfig { contract DIAVaultOracleBeaconSetDeployer { /// @notice Emitted when a new DIAVaultOracle proxy is deployed. /// @param caller The direct on-chain caller of `newDIAVaultOracle`. - /// Indexed so monitoring can filter by deployer. + /// Indexed for filtering — but note minting is PERMISSIONLESS and the salt + /// is derived from the config alone (msg.sender excluded), so a third party + /// who sees the intended public config can front-run the mint and appear + /// here as `caller`. Monitoring that must identify a specific operator + /// should key on the deterministic proxy address (a commitment to the + /// config), not on this field. A front-run mint is config-identical, + /// `initializer`-guarded and sits behind the governance-owned beacon, so it + /// grants the front-runner no authority — it only reverts the operator's own + /// later mint on the CREATE2 collision. /// @param oracle The address of the new proxy. Indexed for filtering. event Deployment(address indexed caller, address indexed oracle); diff --git a/src/concrete/deploy/MorphoPairAdapterBeaconSetDeployer.sol b/src/concrete/deploy/MorphoPairAdapterBeaconSetDeployer.sol index 650d84e..a3c69eb 100644 --- a/src/concrete/deploy/MorphoPairAdapterBeaconSetDeployer.sol +++ b/src/concrete/deploy/MorphoPairAdapterBeaconSetDeployer.sol @@ -18,10 +18,16 @@ error ZeroBeaconOwner(); /// constructor. /// @param initialOwner The initial owner of the beacon (controls upgrades). /// @param central The central `ST0xPriceOracle` store every adapter deployed -/// through this beacon reads. Baked into the implementation as an immutable, so -/// it is fixed for the beacon's whole life (a fresh central needs a fresh -/// deployer). A zero central reverts `MorphoPairAdapter.ZeroCentral` in the -/// implementation constructor. +/// through this beacon reads. Baked into the FIRST implementation as an +/// immutable, and `iCentral` on this deployer records that value permanently. It +/// is NOT, however, structurally fixed for the beacon's whole life: the beacon +/// owner (governance) can upgrade the beacon to a `MorphoPairAdapter` built with +/// a different central, after which every live adapter reads the new central +/// while this deployer's `iCentral()` still reports the original. Treat +/// `iCentral()` as "the central at deploy time", not a live invariant — a +/// central change is an owner-authorised beacon upgrade, the same trust +/// boundary as any other implementation swap. A zero central reverts +/// `MorphoPairAdapter.ZeroCentral` in the implementation constructor. struct MorphoPairAdapterBeaconSetDeployerConfig { address initialOwner; ST0xPriceOracle central; @@ -41,7 +47,15 @@ struct MorphoPairAdapterBeaconSetDeployerConfig { contract MorphoPairAdapterBeaconSetDeployer { /// @notice Emitted when a new MorphoPairAdapter proxy is deployed. /// @param caller The direct on-chain caller of `newMorphoPairAdapter`. - /// Indexed so monitoring can filter by deployer. + /// Indexed for filtering — but note minting is PERMISSIONLESS and the salt + /// is derived from the config alone (msg.sender excluded), so a third party + /// who sees the intended public config can front-run the mint and appear + /// here as `caller`. Monitoring that must identify a specific operator + /// should key on the deterministic proxy address (a commitment to the + /// config), not on this field. A front-run mint is config-identical, + /// `initializer`-guarded and sits behind the governance-owned beacon, so it + /// grants the front-runner no authority — it only reverts the operator's own + /// later mint on the CREATE2 collision. /// @param oracle The address of the new proxy. Indexed for filtering. event Deployment(address indexed caller, address indexed oracle); diff --git a/src/concrete/deploy/ST0xPriceOracleBeaconSetDeployer.sol b/src/concrete/deploy/ST0xPriceOracleBeaconSetDeployer.sol index e7f2032..a495b54 100644 --- a/src/concrete/deploy/ST0xPriceOracleBeaconSetDeployer.sol +++ b/src/concrete/deploy/ST0xPriceOracleBeaconSetDeployer.sol @@ -41,7 +41,15 @@ struct ST0xPriceOracleBeaconSetDeployerConfig { contract ST0xPriceOracleBeaconSetDeployer { /// @notice Emitted when a new ST0xPriceOracle proxy is deployed. /// @param caller The direct on-chain caller of `newST0xPriceOracle`. - /// Indexed so monitoring can filter by deployer. + /// Indexed for filtering — but note minting is PERMISSIONLESS and the salt + /// is derived from the config alone (msg.sender excluded), so a third party + /// who sees the intended public config can front-run the mint and appear + /// here as `caller`. Monitoring that must identify a specific operator + /// should key on the deterministic proxy address (a commitment to the + /// config), not on this field. A front-run mint is config-identical, + /// `initializer`-guarded and sits behind the governance-owned beacon, so it + /// grants the front-runner no authority — it only reverts the operator's own + /// later mint on the CREATE2 collision. /// @param oracle The address of the new proxy. Indexed for filtering. event Deployment(address indexed caller, address indexed oracle); diff --git a/test/src/script/Deploy.t.sol b/test/src/script/Deploy.t.sol index 5e3970e..6ea7e5f 100644 --- a/test/src/script/Deploy.t.sol +++ b/test/src/script/Deploy.t.sol @@ -234,6 +234,15 @@ contract DeployTest is Test { vm.stopBroadcast(); vm.setEnv("ST0X_ORACLE_ADMIN", vm.toString(ST0X_ORACLE_ADMIN)); + // And for ST0X_SIGNER: `updatePrice` is permissionless and authorised + // solely by this signer, so a signer == deploy key ships the feed under + // CI's control. The guard must reject it by MESSAGE. + vm.setEnv("ST0X_SIGNER", vm.toString(deployer)); + vm.expectRevert("ST0X_SIGNER must not be the deploy key"); + deploy.run(); + vm.stopBroadcast(); + vm.setEnv("ST0X_SIGNER", vm.toString(ST0X_SIGNER)); + // ----- ST0X_TIMEOUT uint64 bound (#267) ----- // `ST0X_TIMEOUT` is read as a uint256 and narrowed to uint64. Solidity's // explicit downcast TRUNCATES silently, so a value of 2**64 + 3600 would