[#3451] Harden Automation Registry diamond proxy

[aregng]

Summary

Addresses 3451, the EVM-readiness audit's diamond-proxy hardening pass on the Automation Registry (solidity/supra_contracts). Everything here is genesis-immutable once deployed, so it all lands as pre-genesis source changes:

• Ownership: LibDiamond.setContractOwner now rejects the zero address, closing the unguarded genesis-owner and transferOwnership paths. IERC173's docs no longer advertise renouncing ownership as supported.
• Init replay guard: DiamondInit now inherits OpenZeppelin's Initializable, so init() can only ever run once — including via an empty-cut diamondCut call that would otherwise re-run it over live registry state.
• Storage layout: AppStorage moves from implicit slot 0 to a namespaced ERC-7201-style slot (mirroring LibDiamond's own DIAMOND_STORAGE_POSITION pattern), and every facet/DiamondInit now fetches it via LibAppStorage.appStorage() instead of declaring it as a plain state variable — so its storage location no longer depends on what a future facet's inheritance chain happens to declare.
• Loupe routing: isInitialized() moves off Diamond.sol onto DiamondLoupeFacet as a normally-routed selector, via a new IRegistryStatus interface (kept separate from IDiamondLoupe so its well-known EIP-2535 interfaceId is unaffected). It previously bypassed the diamond's own selector-routing table entirely.
• ERC-165 completeness: DiamondInit now also registers support for the registry's own facet interfaces (ICoreFacet/IConfigFacet/IRegistryFacet/IRegistryStatus).
• Minor: diamondCut's DiamondCut event now fires after the initializer runs (was before); a stale comment on removeFunctions is fixed; CoreFacet's processTasks/monitorCycleEnd/removeRegisteredTask carry NatSpec noting they must never be removed via diamondCut (the node's VM-signer decoder hardcodes their selectors) — a documented governance constraint rather than an on-chain guard, by design.
• Tooling: adds script/check_facet_selectors.sh + CheckFacetSelectors.s.sol, cross-checking each facet's hand-maintained getSelectors() against its compiled ABI.
• Docs: regenerates the gas figures in AUTOMATION_REGISTRY_GAS_GUIDE.md and configs.rs's monitorCycleEnd table, both measurably shifted (a small, non-safety-affecting amount) by the storage-layout change above.

Test plan

• forge build — clean, no new warnings
• forge test — 467/467 passing, including new tests for each item (zero-address reverts, init-replay revert, isInitialized() routing, interfaceId regression guard) and the corrected MonitorCycleEndGas.t.sol slot arithmetic
• ./script/check_facet_selectors.sh — passes for all 6 facets; verified it actually catches a real omission (negative-control test) before trusting it
• cargo check -p revm-supra-extension — clean
• Two independent code reviews performed; one surfaced a real regression (adding isInitialized() directly to IDiamondLoupe silently changed type(IDiamondLoupe).interfaceId away from the well-known EIP-2535 value) which is fixed in this PR via the new IRegistryStatus interface
• Investigated whether these changes affect the node-runtime's isInitialized()-based automation-enable gate (in smr-moonshot) — no code change needed there; the existing gate is already maximally precise given Diamond's atomic-constructor guarantee, though a decode-type mismatch was flagged separately (not part of this PR, tracked for follow-up in that repo)

🤖 Generated with Claude Code

[isaacdoidge]

Reviewed on the PR head (68cf8530). Independently verified: forge build clean; forge test 457/457 green (description says 467 — nothing skipped, likely a forge-version count difference); boundary scan still 723/724; check_facet_selectors.sh passes for all six facets when run via bash; APP_STORAGE_POSITION matches its derivation comment; genesis compiles the contracts at build time and runs the Diamond constructor, so the storage move is safe; isInitializedCall comes from the hand-maintained SupraContractsBindings.sol, so the node's gate keeps working through the loupe.

Findings are inline. One has no diff line to anchor to:

Commit message (convention). The commit body describes the behaviour the code no longer has ("closing the unguarded … paths", "even via an empty-cut diamondCut call", "It previously bypassed … entirely") rather than the behaviour it implements. Per smr-moonshot CONTRIBUTING.md → "What Comments and Commit Messages May Say About a Vulnerability", which covers this fork explicitly, please reword to the enforced rules (owner must be non-zero; init runs at most once per Diamond; isInitialized is a routed loupe selector) and leave the rest to bluealloy#3451. This remote is public, so the message is permanent once merged. Pre-genesis, so nothing deployed is exposed — but the rule is categorical.

No import-alias findings (no Rust use changes).

[isaacdoidge]

Convention (vulnerability disclosure). This bullet narrates the pre-fix outcome and spells out the exact call shape that would reach it — see smr-moonshot CONTRIBUTING.md, "What Comments and Commit Messages May Say About a Vulnerability" ("Narration of the old behaviour" / "reusable exploitation facts that generalise beyond the fix"). The fork is public. Intended-behaviour form is enough: "init runs via delegatecall in the Diamond's own storage and is initializer-guarded, so it runs at most once per Diamond", plus a bare bluealloy#3451 reference.

It also contradicts the bullet two lines above ("not callable through the Diamond after deployment") — the two should agree on whether init is reachable via the Diamond.

Same applies to the @dev on testInitCannotBeReplayedViaDiamondCut in test/DiamondInit.t.sol ("which would otherwise re-run …").

[isaacdoidge]

Please check: this makes isInitialized() removable for the first time. On Diamond.sol it lived in the proxy's own bytecode and no cut could unroute it. As a loupe selector, an owner cut can Remove 0x392e53cd (or Replace the loupe with a facet lacking it); LibDiamond.removeFunction permits it since the facet is neither address(0) nor address(this).

The node side matters here: consensus/execution/src/automation/evm/data_store.rs:308-334 folds the resulting revert into Ok(false) at debug! level, and is_initialized() is only evaluated at node start. So already-running validators keep injecting processTasks/monitorCycleEnd while any validator that restarts boots with no viewer and injects nothing — a restart-dependent execution split.

Options: keep isInitialized on Diamond.sol (and register the selector in the loupe's routing table only as a secondary path), or add the selector to an on-chain protected set enforced in removeFunctions (see my comment on LibDiamond.sol).

[aregng]

The utilization of the isIntialized() is planned to be revisited on node-runtime side, most probably in scope of https://github.com/Entropy-Foundation/smr-moonshot/issues/3669 ticket. Double check and alignment with the existing move-automation will be conducted.

[isaacdoidge]

The description calls the comment-only approach "by design", but I think that decision rested on the node failing loudly, and it doesn't — which is the question bluealloy#3451 item 7 left unconfirmed. Tracing an owner cut that Removes one of these three: automation_types.rs:92-95 maps the revert to a RecoverableError, retries once, then parks in RegistryState::PausedReady; BlockMeta.blockPrologue swallows the monitorCycleEnd failure into a CallFailed event (BlockMeta.sol:169-175). No halt, no operator signal beyond a log line, and every block runs the dead call until a fix cut.

A bytes4 => bool protected-selector set (or constant list) checked in removeFunctions is one revert line and settles item 7 on-chain. LibDiamond already has an immutable-selector guard at ~L179 that is currently unreachable and could carry this.

Also, the NOTE reads as exhaustive but isn't: it omits isInitialized and the read selectors data_store.rs hardcodes (getCycleStateDetails, getTaskDetails, getTaskIdList, getActiveTaskIds, isAutomationEnabled).

[aregng]

The point 7 was not addressed as it was advised because the list of the protected functions might be updated in future in case of feature development, so the update interface also becomes a facet and its functions should also be listed as protected. So instead of hardcoding or adding an extra layer of protection and complicating update flow, The documenting the interface "immutability" was chosen as a solution. If extra layer of protection is preferable in anyway then it can be done.

[isaacdoidge]

Under set -euo pipefail, reported_selectors="$(grep … | …)" aborts the whole script when grep matches nothing — so a facet whose getSelectors() reports zero entries (or a FACETS name with no SELECTOR lines) exits 1 with no FAIL line and no diagnosis, which is the one case this checker exists to catch. Same mechanism at L26: if forge script fails, set -e exits before the "produced no SELECTOR lines" diagnostic at L28 ever runs. Appending || true inside the substitutions (or set +e around them) lets the comparison and FAIL reporting run.

Related: FACETS (L23) is now a third copy of the facet list (alongside Diamond.sol's constructor, LibDiamondUtils.deployFacets and CheckFacetSelectors.s.sol). A facet added to the .s.sol but not to FACETS is printed and then silently never checked — deriving FACETS from the SELECTOR lines would remove the copy.

[isaacdoidge]

Registering type(ICoreFacet/IConfigFacet/IRegistryFacet).interfaceId sits uneasily with the comment a few lines up that the mapping is never reconciled by later cuts. These ids are XORs of every function selector in the interface, so the first Replace that adds or changes a function on one of these facets leaves the diamond advertising an id it no longer implements (supportsInterface(oldId) → true, supportsInterface(newId) → false), and an integrator gating on the id picks the wrong ABI. The EIP-2535 quartet and IRegistryStatus (one fixed function) are stable and fine to register; for the three mutable facet interfaces either reconcile in add/replaceFunctions or leave them out.

[isaacdoidge]

Committed as mode 100644 — ./script/check_facet_selectors.sh (the invocation documented at L16 and in the test plan) fails with Permission denied on a fresh checkout of this head (reproduced). Every other .sh in the repo is 100755. git update-index --chmod=+x script/check_facet_selectors.sh.

[isaacdoidge]

Low, documentation: inheriting Initializable sets the Diamond's shared INITIALIZABLE_STORAGE version to 1 permanently, so a future upgrade initializer that copies this contract's plain initializer modifier (DiamondInitV2 is Initializable { function init(...) external initializer } — modelled on the only template in src/upgradeInitializers/) reverts with InvalidInitialization, and LibDiamond.initializeDiamondCut rethrows so the whole cut reverts. Not a state-corruption risk, but an operator debugging it may strip Initializable and undo this PR. One @dev sentence ("later initializers must use reinitializer(N)") plus a test that a reinitializer(2) initializer succeeds would close it.

[isaacdoidge]

This inverts what depends on AppStorage's field order: the base (APP_STORAGE_POSITION) is a fixed constant that a field reorder never moves, while the relative offset 7 is exactly what a reorder moves. A maintainer reading "only the base changes … not this relative math" leaves the +7 stale, _setOrderedTaskIdsDescending writes into an unrelated field, and only one of its two callers has the _assertStrictlyDescending liveness check. Swap the two clauses — or derive the slot from LibAppStorage.registryState()'s .slot via assembly and drop the literal 7 entirely.