Skip to content

feat: AppAuthority + owner-gated governance (v1.5.0)#20

Open
crypt0fairy wants to merge 18 commits into
masterfrom
taras/app-governance
Open

feat: AppAuthority + owner-gated governance (v1.5.0)#20
crypt0fairy wants to merge 18 commits into
masterfrom
taras/app-governance

Conversation

@crypt0fairy

@crypt0fairy crypt0fairy commented Apr 23, 2026

Copy link
Copy Markdown

AppController v1.5.0 — AppAuthority + owner-gated governance

Rebuilds AppController's auth model. Extracts per-app roles and ownership into a dedicated AppAuthority contract, gates all critical ops on the current owner, and introduces first-class support for Safe- and Timelock-owned apps — without AppController needing to classify them.

Replaces the original soli/gov branch. Ships against master (v1.4.0-isolated-billing) as a clean, append-only v1.5.0 release.

1. What we had before

Auth model (v1.4.0): every app-scoped function gated on checkCanCall(address(app)) via EigenLayer's PermissionController. The admin set that made those gates pass lived in PermissionController, a shared cross-AVS contract outside eigenx's trust boundary.

Who could do what:

Actor Could do
An app's PermissionController admins (any of them) upgradeApp, transferOwnership, terminateApp, updateAppMetadataURI, startApp, stopApp — every app-level op
UAM admins of AppController itself setMaxActiveAppsPerUser, setMaxGlobalActiveApps, terminateAppByAdmin, suspend
Anyone createApp, createAppWithIsolatedBilling, view reads

Consequences of this model:

  • The admin set's mutation surface lived outside AppController. No contract-level guarantee that critical ops were bound to any particular signer or governance mechanism — adding/removing admins happened in PermissionController, which AppController did not and could not control.
  • Every admin of an app held full critical-op power. "Add a co-admin" meant "hand over the app."
  • No way to delegate operational powers (pause / edit metadata) without also handing over upgrade and transfer rights.
  • Safe- or Timelock-owned apps had no special handling. A Timelock could own an app, but any co-admin could call upgradeApp directly and bypass the Timelock's delay.

2. New role hierarchy

Roles live in a new AppAuthority contract. AppController delegates all auth checks there.

Role Who is it? Scope
Platform admin UAM admins of AppController itself (via EigenLayer PermissionController) Protocol-wide
Scope owner The app's owner address (EOA, Safe, Timelock, or any contract) Per-app
ADMIN Scope owner + anyone the owner has granted Per-app
DEVELOPER Granted by any ADMIN Per-app
PAUSER Granted by any ADMIN Per-app

Key invariants enforced in AppAuthority:

  • The scope owner is always ADMIN on their scope.
  • The owner cannot renounce or self-revoke ADMIN.
  • transferScopeOwnership is the only path that rotates the owner; it adds the new owner to ADMIN and removes the previous owner atomically.
  • Only the owner may grant / revoke ADMIN. Any existing ADMIN may grant / revoke PAUSER / DEVELOPER.

3. App is a scope

Each IApp is a scope in AppAuthority. The contract is generic over scope identity (mapping(IApp => ...)), but AppController is the sole authorized "consumer" — only AppController can initialize a scope (createApp), transfer scope ownership, or seed ADMIN via migration. Role grants/revokes/renounces are called directly on AppAuthority by users.

Scope state per app:

  • scopeOwner[app] — the current owner address.
  • _roles[app][ADMIN | PAUSER | DEVELOPER] — role membership sets.

4. Critical operations

Critical = destructive or reassigns control. These go through onlyCreator on AppController, which delegates to AppAuthority.isScopeOwner:

Function Gate
upgradeApp msg.sender == scopeOwner
transferOwnership (step 1 of 2) msg.sender == scopeOwner — records pending proposal, no state rotation
acceptOwnership (step 2 of 2) msg.sender == pendingOwner — rotates scope + ADMIN, migrates counter
cancelOwnershipTransfer msg.sender == scopeOwner — rescinds pending proposal
terminateApp msg.sender == scopeOwner
grantRole(app, ADMIN, x) msg.sender == scopeOwner
revokeRole(app, ADMIN, x) msg.sender == scopeOwner

Only the current owner of the app can perform these. ADMIN membership is not sufficient — a co-ADMIN who was granted the role cannot upgrade, transfer, or terminate the app. The owner-gate is unconditional.

terminateAppByAdmin is also critical but is a platform-level lever: gated on UAM permissions for AppController. Platform admin can terminate any app uniformly, no owner-type carve-out. This is protocol policy that sits above app-level governance.

See §4a for the full two-step transfer semantics, quota enforcement, and rationale.

4a. Two-step ownership transfer

Ownership transfer is a two-step flow (propose + accept) rather than a single atomic call. The receiver must explicitly opt in by calling acceptOwnership from their own address.

Why: an active DEFAULT-billed app binds its billing account (who pays for compute) and its quota cost (who consumes maxActiveApps capacity) to the owner. A one-step transfer would let any current owner push those liabilities onto an unwilling receiver — consuming their quota, charging their billing account — without the receiver ever signing a transaction. Two-step requires the receiver's signature, turning "I'm giving you my app" from unilateral to consensual.

Flow:

  1. Current owner: transferOwnership(app, newOwner) → records pendingOwner. No AppAuthority rotation, no counter migration, no ADMIN change. Emits OwnershipTransferProposed. If a prior proposal existed, it's silently superseded (OwnershipTransferCancelled is emitted for the previous target).
  2. Pending owner: acceptOwnership(app) → rotates scope ownership + ADMIN in AppAuthority, mirrors creator, migrates the active-app counter for DEFAULT-billed active apps. Emits AppOwnershipTransferred.

Optional: current owner can cancelOwnershipTransfer(app) to rescind a pending proposal. The view getPendingOwner(app) surfaces the current pending target (zero if none).

Quota enforcement: on acceptOwnership, the receiver must satisfy activeAppCount < maxActiveApps — the same strict less-than check createApp uses. Prevents an owner from pushing an app onto an account with zero quota or already at its cap. Applies only to DEFAULT-billed active apps (ISOLATED billing accounts are the app itself, so receiver quota is irrelevant).

During the pending window: the current owner retains all authority. Proposed transfer is a non-blocking offer — the owner can still upgradeApp, terminateApp, grant/revoke roles, or propose a different receiver. If the app is terminated while a proposal is pending, the proposal becomes degenerate (acceptOwnership still works but the counter migration is skipped because !_isActive).

ABI surface:

  • New: acceptOwnership(IApp), cancelOwnershipTransfer(IApp), getPendingOwner(IApp) returns (address).
  • New events: OwnershipTransferProposed(app, currentOwner, proposedOwner), OwnershipTransferCancelled(app, currentOwner, cancelledOwner).
  • New error: NotPendingOwner().
  • Changed semantics: transferOwnership(IApp, address) is no longer atomic. Integrators must implement the two-step flow — AppOwnershipTransferred fires only on accept, not on propose.

5. Remaining (operational) operations

Operational = bounded, reversible, delegable:

Function Gate
startApp onlyAdmin — scope owner OR any granted ADMIN
stopApp onlyRoleOrAdmin(PAUSER) — PAUSER or ADMIN
updateAppMetadataURI onlyRoleOrAdmin(DEVELOPER) — DEVELOPER or ADMIN
grantRole(app, PAUSER | DEVELOPER, x) onlyAdmin (any ADMIN)
revokeRole(app, PAUSER | DEVELOPER, x) onlyAdmin
renounceRole(app, PAUSER | DEVELOPER) caller revokes themselves

Platform-admin operational ops:

Function Gate
setMaxActiveAppsPerUser UAM admin of AppController
setMaxGlobalActiveApps UAM admin of AppController
suspend(account, apps[]) account owner OR UAM admin
migrateAppsToAppAuthority UAM admin (one-shot, idempotent)

Anyone can call createApp, createAppWithIsolatedBilling (subject to quota), and view functions.

6. New owner identities: Safe and Timelock

AppController treats the owner as an opaque address. The owner's contract type determines how critical ops are authenticated:

Owner type What "msg.sender == scopeOwner" requires
EOA A signed tx from that EOA
Gnosis Safe A Safe transaction meeting the multisig threshold
Timelock A schedule → wait minDelay → execute flow
Any other contract Whatever that contract's internal authorization logic enforces

Two supporting contracts are introduced for convenient governance deployment:

  • SafeTimelockFactory — deploys canonical Gnosis Safes (via SafeProxyFactory) and TimelockControllerImpl clones on demand. Maintains registries (isSafe, isTimelock, getSafesByDeployer, getTimelocksByDeployer) so off-chain tooling can verify provenance. AppController does not depend on this factory — users may deploy their own Safes/Timelocks elsewhere and AppController will still treat them correctly.
  • TimelockControllerImpl — extends OZ's TimelockControllerUpgradeable with on-chain pending-op enumeration (capped at 32 entries) and a schedule-time ICallValidator probe on targets (ERC-165 + bounded gas + bounded returndata).

7. Critical ops from a Timelock owner's perspective

When the owner is a TimelockControllerImpl:

  1. A proposer calls Timelock.schedule(appController, upgradeApp.selector + args, predecessor, salt, delay).
  2. Schedule-time, TimelockControllerImpl._validateTarget staticcalls AppController.canCall(timelock, data) to reject doomed ops (wrong caller, wrong selector). Non-owner callers are rejected at schedule time — the delay window is not burned on impossible ops.
  3. The op waits minDelay (set by the user when deploying the Timelock — enforced by the Timelock, not AppController).
  4. An executor calls Timelock.execute(...). The Timelock contract, with msg.sender = Timelock, calls AppController.upgradeApp.
  5. AppController's onlyCreator gate sees msg.sender == scopeOwner == Timelock → passes.

The Timelock owner cannot bypass schedule → execute. There is no code path where a signature from a Timelock proposer / executor / admin produces msg.sender == Timelock at AppController without first going through the Timelock's own execute, which enforces the delay. The only way the delay can be shorter than the user intended is if the user chose a shorter minDelay at Timelock deployment time — AppController does not enforce a floor.

Same structure applies to Safe-owned apps: critical ops require a Safe transaction, which requires the Safe's multisig threshold. Co-ADMINs cannot bypass the multisig because they don't satisfy msg.sender == scopeOwner.

8. Migration

The upgrade is append-only vs v1.4.0. No storage slot shifts.

Storage compatibility:

  • AppConfigStorage byte layout 0–29 is byte-for-byte identical to v1.4.0. Bytes 30–31 are unused (zero on all v1.4.0 state; zero on all newly created v1.5.0 state).
  • The v1.4.0 __gap[45] remains __gap[45] — a new IAppAuthority public immutable appAuthority was added, but as an immutable it consumes no storage slot.

Deploy sequence (zeus v1.5.0-governance):

  1. EOA phase deploys:
    • TimelockControllerImpl (clone master, no proxy)
    • SafeTimelockFactory impl + TransparentUpgradeableProxy
    • AppAuthority impl (consumer-bound to the existing AppController proxy address) + TransparentUpgradeableProxy
    • New AppController impl wired to the AppAuthority proxy
  2. Multisig phase: ProxyAdmin.upgrade(AppController proxy, new impl) via computeOpsMultisig. After this, the upgraded AppController delegates auth to AppAuthority.
  3. Per-app migration (platform-admin-gated, idempotent): call AppController.migrateAppsToAppAuthority([app1, app2, ...]) for every existing v1.4.0 app. For each app:
    • If AppAuthority has no owner recorded, initialize the scope with the app's current creator field.
    • Seed AppAuthority's ADMIN role with the app's PermissionController admins. ADMIN is an operational-only role in the new model, so migrated admins carry no critical-op exposure; the owner remains the only critical-op authority.
    • Safe to re-run — initializeScope is one-shot per scope and grantRole is set-semantics.

Existing apps keep working unchanged. Users who want delayed governance transfer ownership to a TimelockControllerImpl (via SafeTimelockFactory.deployTimelock or deployed independently): transferOwnership(app, timelock) records the proposal, then the Timelock calls acceptOwnership(app) through a scheduled op — the receiver's explicit acceptance fits naturally into the Timelock's schedule → execute flow. Users who want multisig governance transfer ownership to a Safe, and the Safe signs an acceptOwnership tx to complete. AppController treats both transparently via the owner-gate.

What is intentionally not in this PR

  • Changes to PermissionController itself (cross-AVS, not ours to modify). Platform-admin functions on AppController still gate on checkCanCall(address(this)) — that's the correct layer.
  • App beacon ownership / ProxyAdmin centralization. Those are operational decisions (transfer ownership to a Timelock), not code changes.
  • Factory-level floors on Safe threshold or Timelock minDelay. Users choose their own governance parameters; AppController does not enforce a minimum.
  • Per-proposer quota on the Timelock's pending-op cap, strict 32-byte returndata copy in _validateTarget, Timelock op expiry, and several other hardening items — tracked for a follow-up v1.5.1 pass.

Testing

179 / 179 non-release tests pass. Highlights:

  • AppController.t.sol: end-to-end RBAC, owner-gate, migration, billing, quota, suspend flow.
  • AppAuthority.t.sol: 24 direct unit tests on the authority contract — scope init, ownership transfer, role lifecycle, owner-can't-renounce invariant, scope isolation.
  • TimelockControllerImplValidation.t.sol: 9 tests on _validateTarget behavior (EOA, no-ERC165, says-no, allow, reject, revert-fails-closed, OOG, returndata-bomb) plus the pending-ops cap.

Test plan

  • forge test (non-release) passes: 179 / 179.
  • forge fmt --check clean.
  • forge build --sizes does not regress AppController further (currently 29,058 bytes).
  • On a local Anvil fork: deploy factory + AppAuthority + upgrade AppController via the v1.5.0 release scripts; verify migrateAppsToAppAuthority seeds scopes + admins for an existing app; verify the two-step flow — transferOwnership then acceptOwnership from the receiver — rotates ownership to a Safe/Timelock; critical op from the new owner works end-to-end.
  • On Sepolia-dev staging (after merge): set zeus env safeSingleton / safeProxyFactory / safeFallbackHandler, run phases 1–2 via zeus, call migrateAppsToAppAuthority over the existing app set, confirm no regressions in existing app flows.

Purely additive: no existing file modified.

- SafeTimelockFactory deploys canonical Gnosis Safes (via upstream
  SafeProxyFactory) and TimelockControllerImpl clones (EIP-1167), and
  maintains a registry of (isSafe / isTimelock / getSafesByDeployer /
  getTimelocksByDeployer) used later by AppController to detect
  governance-owned apps.
- TimelockControllerImpl extends OZ TimelockControllerUpgradeable with
  on-chain pending-op enumeration (getPendingOperationIds /
  getPendingOperations) and a schedule-time validator hook that calls
  ICallValidator.canCall(address(this), data) on the target, failing
  closed if the target explicitly returns false. Targets that don't
  implement the interface are allowed through for backwards compat.
- ICallValidator is a one-method interface targets may implement to
  reject doomed operations at schedule time instead of waiting for
  the delay to pass.
- ISafe / ISafeProxyFactory / ISafeTimelockFactory interfaces.

AppController integration comes in a follow-up commit.
- AppControllerStorage gains an immutable safeTimelockFactory reference
  (constructor arg). No storage slot consumed; the field is used to
  detect governance-owned apps without adding a separate registry
  lookup.
- AppConfigStorage adds `bool timelocked` at byte 30 of the existing
  slot (immediately after BillingType at byte 29). This position was
  zero on every pre-v1.5.0 app, so the upgrade is byte-safe. An explicit
  byte-layout comment documents the invariant so a future reorder
  doesn't silently collide with isolated-billing's existing byte 29.
- AppController constructor takes the factory arg and forwards it.
- All historical release deploy scripts (v1.0.4, v1.1.1, v1.4.0) pass
  ISafeTimelockFactory(address(0)) so the repo still compiles; they
  are already applied on-chain and never run again.
- script/Deploy.s.sol (fresh-deploy fast path, used in tests) carries a
  TODO to deploy the factory before AppController when the v1.5.0
  governance release lands.

No governance behavior is enabled by this commit. The flag stays false
and none of the sensitive-op runtime gates exist yet. Follow-up commits
add canCall, team roles, transferOwnership semantics, and the release
script.
When the app's creator is a factory-deployed Timelock the sensitive
operations must be routed through Timelock.schedule → execute. Until
now those calls were gated only by PermissionControllerMixin, so any
admin the Timelock had granted via UAM could trigger an instant
upgrade or termination and bypass the delay entirely.

- _deployApp sets _appConfigs[app].timelocked = isTimelock(msg.sender)
  at creation. Previously the flag was unset on this path, so a
  Timelock calling createApp for itself skipped all of the new gates.
  Defensive zero-address check for historical deployments that have no
  factory wired in.
- upgradeApp requires msg.sender == creator when timelocked.
- terminateApp requires msg.sender == creator when timelocked.
- terminateAppByAdmin refuses to run against timelocked apps entirely —
  protocol admin must go through the Timelock, not around it.
- New view getAppTimelocked for off-chain tooling.

transferOwnership isn't on this branch yet; that semantic comes with
the team-RBAC overhaul in a later commit. For now the flag is only
written at creation.
- Deploy.s.sol now deploys TimelockControllerImpl and
  SafeTimelockFactory (impl + proxy) before the AppController and
  threads the factory proxy into the AppController constructor. Safe
  infrastructure addresses are left as zero — tests don't use
  deploySafe, and production releases will wire them via a dedicated
  release script.

- Five regression tests covering the timelocked gate on createApp,
  upgradeApp, terminateApp, and terminateAppByAdmin. Each tests
  mocks isTimelock(developer)=true, grants a PermissionController
  admin to a coAdmin, then confirms the coAdmin is blocked from the
  sensitive path while the Timelock-as-creator is not. Closes the
  bypass path that the audit flagged as V-1 / G-1 / A-1.
When an app's owner is handed off to a new address, the timelocked flag
must update atomically. Before this commit, apps could only acquire the
flag via the createApp path, so there was no way to opt an
already-running app into delayed governance — and no way back out.

- New transferOwnership(IApp, address). Gated by checkCanCall like
  other sensitive ops. When the app is already timelocked the caller
  must be the Timelock itself (msg.sender == creator), otherwise any
  admin granted via UAM could move the app out of governance without
  going through schedule → execute.
- New owner that is a factory-registered Timelock sets timelocked=true;
  any other address (EOA, externally deployed Safe, arbitrary contract)
  clears it.
- Active-app counter migration only happens for BillingType.DEFAULT
  apps. ISOLATED apps bill the app address, which doesn't change on
  transfer, so the counter must NOT move or a future terminate would
  double-decrement.
- New AppOwnershipTransferred event.

Six regression tests covering both directions of the flag flip, the
timelocked-gate, zero-address rejection, and the two billing accounting
branches.
- ICallValidator now inherits IERC165. Targets MUST advertise the
  interface via supportsInterface for their canCall to be consulted;
  a target that reverts canCall without advertising is treated as
  non-implementing (backwards compatible). A target that advertises
  and reverts is treated as a definitive "no" and the schedule is
  rejected — closing the fail-open bypass the audit called out
  (G-3 / V-4 / D-2 / A-7 / P-9).

- AppController implements ICallValidator + supportsInterface. canCall
  rejects at schedule time any operation that deterministically fails
  the runtime gate: non-owner upgradeApp/terminateApp/transferOwnership
  on a timelocked app, or terminateAppByAdmin against any timelocked
  app. All other calls pass through to runtime (PermissionController
  stays authoritative for non-governance auth).

- TimelockControllerImpl._validateTarget now:
  1. Short-circuits for EOAs / zero-code addresses.
  2. Probes ERC-165 supportsInterface with a 30k gas cap.
  3. Only calls canCall if the target claims support, capped at 200k
     gas with bounded returndata (32 bytes — ignores returndata bombs).
  4. Treats canCall revert as rejection (fail closed) rather than
     silent pass-through.

Tests:
- 5 new AppController.t.sol cases for canCall + supportsInterface.
- 8 new TimelockControllerImplValidation.t.sol cases with mock
  targets: no-ERC165, ERC165-says-no, allow, reject, revert, OOG,
  returndata-bomb, EOA fast path.

Full suite: 141 tests pass.
Unbounded growth of _pendingIds lets a misbehaving or compromised
proposer brick getPendingOperations() / getPendingOperationIds() for
off-chain tooling by queuing arbitrarily many ops with large calldata
blobs. Audit D-1 — High.

Introduce MAX_PENDING_OPS = 128 and a new TooManyPendingOperations
error. _addPending reverts when the cap is hit; executing or
cancelling an op frees a slot. 128 keeps getPendingOperations well
under the block gas limit even with multi-hundred-byte calldata per
op, and is an order of magnitude above any realistic governance
queue depth.

Test covers filling the cap, rejecting the next schedule, and being
able to schedule again after cancelling.
Two zeus phases:

1. EOA (1-deployGovernanceContracts.s.sol) — deploys in order:
   - TimelockControllerImpl (direct, no proxy; immutable master for
     the minimal-proxy Timelock clones created by the factory)
   - SafeTimelockFactory impl, referencing the canonical Gnosis Safe
     infrastructure (singleton / proxy factory / fallback handler)
     pulled from zeus env
   - SafeTimelockFactory TransparentUpgradeableProxy, owned by the
     existing protocol ProxyAdmin
   - New AppController implementation, wired to the factory proxy

2. Multisig (2-upgradeAppController.s.sol) — Env.proxyAdmin().upgrade
   points the AppController proxy at the new impl. No initializer call;
   the storage layout is append-only (new `bool timelocked` lands at
   byte 30, previously zero on every app).

Env.sol additions:
- Env.proxy.safeTimelockFactory(), Env.impl.safeTimelockFactory()
- Env.timelockControllerImpl()
- Env.safeSingleton() / safeProxyFactory() / safeFallbackHandler()

Zeus must supply the Safe infrastructure addresses per chain via env
keys safeSingleton / safeProxyFactory / safeFallbackHandler.
Smaller cap is more aggressive about preventing on-chain enumeration
bloat. 32 is still well above any realistic governance queue depth —
a team with that many concurrent pending ops has bigger problems than
list enumeration cost.
Replaces the PermissionController-based app-level auth with an
AppController-owned role set. The Timelock guarantees only bind when
the admin set the gates trust is a set this contract controls — which
was the underlying reason the previous gate (checkCanCall) couldn't
actually protect operational-layer control of a Timelocked app.

Roles, stored as mapping(IApp => mapping(TeamRole => AddressSet)):

  - ADMIN     → full app-level authority. Required for upgradeApp,
                transferOwnership, terminateApp, and for managing
                team membership. When the app is timelocked, ADMIN is
                further narrowed to msg.sender == creator for the
                three critical ops.
  - PAUSER    → operational. Can call stopApp.
  - DEVELOPER → operational. Can call updateAppMetadataURI.

Auditor findings A-2 and A-3 addressed by design:

  - grantTeamRole(ADMIN, _) on a timelocked app requires msg.sender == creator.
    Grants of PAUSER/DEVELOPER do NOT — those are operational powers
    the ADMIN can revoke at any time.
  - revokeTeamRole(ADMIN, _) on a timelocked app requires msg.sender == creator,
    closing the one-tx Timelock-stripping path.
  - renounceTeamRole refuses to drop the last ADMIN via CannotRevokeLastAdmin.

migrateAdmins(IApp[]) seeds ADMIN from the existing PermissionController
admin set per app — intended to be run once per existing app after the
v1.5.0 upgrade. Platform-admin gated (checkCanCall on AppController).

Changes:
  - Added TeamRole enum, events, errors, and function signatures to
    IAppController.
  - AppControllerStorage adds _teamRoles mapping; __gap shrunk by 1
    slot to keep the overall storage footprint stable.
  - Swapped checkCanCall(address(app)) on upgradeApp, transferOwnership,
    terminateApp, updateAppMetadataURI, startApp, stopApp for role-based
    gates (onlyAdmin or onlyRoleOrAdmin).
  - _deployApp now seeds the creator as the initial ADMIN and emits
    TeamRoleGranted.
  - transferOwnership auto-grants ADMIN to the new owner so the new
    team is not stranded without governance rights.

Tests:
  - Updated 7 existing tests where the expected revert shape shifted
    (InvalidPermissions → InvalidTeamRole when the role gate fires
    first, and so on). The co-admin-in-timelocked tests were rewritten
    to grant ADMIN via the new RBAC path instead of via UAM.
  - Added 15 new tests covering grant/revoke/renounce semantics, the
    A-2/A-3 timelocked ADMIN-grant gate, PAUSER/DEVELOPER behavior
    on stopApp and updateAppMetadataURI, transferOwnership auto-ADMIN,
    migrateAdmins seeding, and the platform-admin gate on migrateAdmins.

Full suite: 158 tests pass (from 142 pre-RBAC).
Move ownership and role state out of AppController into a dedicated
AppAuthority contract. AppController delegates auth checks to
AppAuthority; role management (grant/revoke/renounce/hasRole) is
called directly on AppAuthority by clients. Critical ops remain on
AppController and are owner-gated via AppAuthority.isScopeOwner.

Fixes the Option-2 invariants at the type level:
- The owner is always ADMIN on their scope; rotation only through
  transferScopeOwnership (add-before-remove preserves last-admin).
- ADMIN mutations (grant/revoke) are owner-only unconditionally.
- The owner cannot renounce or self-revoke ADMIN.

Storage layout: _teamRoles removed from AppController; __gap
restored to 45 slots. AppAuthority is a new upgradeable contract
behind its own proxy. AppController gets an IAppAuthority immutable.
Cached `creator` stays on AppController for billing / App.initialize
/ event stability; it mirrors AppAuthority.scopeOwner on transfer.

migrateAdmins becomes migrateAppsToAppAuthority: initializes each
legacy app's scope in AppAuthority from the cached creator, then
seeds ADMIN from the app's PermissionController admins (operational-
only under this model; no critical-op exposure).

AppController runtime size: 33,131 -> 30,288 bytes (-2,843).

Tests: 185/185 passing (was 161; +24 AppAuthority unit tests).
Remove the `timelocked` boolean from AppConfigStorage and every path
that wrote or read it. Under the owner-gated (Option-2) model the
flag is vestigial: critical ops are gated on `msg.sender == creator`
unconditionally, so a Timelock owner naturally forces schedule →
execute without a separate classification on AppController.

Behavior changes:

- terminateAppByAdmin no longer refuses Timelock-owned apps. Protocol
  admin (UAM-gated) can terminate any app uniformly. If the protocol
  wants delay-gated terminations, that's an operational decision
  about the UAM multisig, not an AppController concern.
- AppController no longer imports ISafeTimelockFactory. The factory
  is still deployed and usable for attested Safe/Timelock deployments,
  but AppController's correctness no longer depends on it.
- getAppTimelocked view removed.

Storage: AppConfigStorage byte 30 returns to unused (zero on all
existing chain state). SafeTimelockFactory immutable dropped from
AppControllerStorage; constructor goes 8 args → 7.

ISafe / ISafeProxyFactory interface files gain comments documenting
why they're hand-rolled (minimal surface, Safe v1.3.0–v1.4.1 stable
signatures) and when to reconsider.

Also closes audit findings:
- V-1 / A-7 (factory floors): factory boolean is no longer a security
  claim AppController relies on; user picks their own config.
- V-6 (Timelock → EOA drops protection): no protection to drop.
- A-4 (suspend not timelock-gated): no asymmetry to fix.

AppController runtime size: 30,288 → 29,058 (−1,230). Total shrinkage
vs pre-refactor: 33,131 → 29,058 (−4,073 / −12%).

Tests: 179/179 passing (was 185; 6 dropped tests were about the
flag-flipping behavior and the terminateAppByAdmin carve-out — both
gone now).
"Option-2" was a label from a design discussion, not anything defined
in the codebase. Reviewers coming to these files cold had no way to
know what it referred to. Swap it for descriptions of the actual
invariants: owner-gated critical ops, operational-only ADMIN, owner
cannot renounce/self-revoke, transferScopeOwnership is the sole owner
rotation path.
@crypt0fairy crypt0fairy changed the title feat: app governance (Safe/Timelock) + per-app team RBAC feat: AppAuthority + owner-gated governance (v1.5.0) Apr 25, 2026
@crypt0fairy crypt0fairy marked this pull request as ready for review April 25, 2026 00:32
Four docstring/comment cleanups, no code changes:

- IAppController.AppConfigStorage: remove the slot-layout block that
  documented a retired draft (byte 30 timelocked flag) and the
  inter-file nav comment about AppAuthority.Role. Struct declaration
  speaks for itself.
- Deploy.s.sol: rewrite the SafeTimelockFactory comment so it
  describes the code's behavior for any reader (zero Safe infra
  addresses means deploySafe unavailable; deployTimelock works),
  instead of a branch-relative "no longer depends" framing.
- v1.5.0-governance/1-deployGovernanceContracts.s.sol: expand the
  "phase 1" docstring to say what release it's a step of and what it
  actually deploys (now 6 items — AppAuthority impl + proxy were
  missing from the list). Drops the stale "timelocked-flag detection"
  reference.
- v1.5.0-governance/2-upgradeAppController.s.sol: same shape — drops
  "phase 2" in favor of "Second step of the v1.5.0 release" and stale
  "new bool timelocked" language.
Merges master (v1.4.1 two-phase upgrade + ContainerPolicy) into this
branch. No conflicts in substance — master's changes are about release
state plumbing and coordinator migration races, while this branch's
changes are about auth (AppAuthority + owner-gated critical ops). The
two work streams don't overlap semantically; conflicts were positional.

From master:
- ContainerPolicy + EnvVar structs; Release struct gains
  containerPolicy field (KMS-006).
- pendingReleaseBlockNumber field on AppConfig / AppConfigStorage.
- confirmUpgrade(IApp) — UAM-gated; promotes pending release to
  confirmed. Prevents the coordinator /secrets race during GCP
  instance migration (KMS-009).
- _upgradeApp writes to pending; _deployApp auto-promotes initial
  release; getAppPendingReleaseBlockNumber view.
- 5 new two-phase upgrade tests.

Preserved from this branch:
- AppAuthority delegation (scope ownership + roles)
- Owner-gated critical ops (onlyCreator)
- transferOwnership, migrateAppsToAppAuthority
- NotCreator / InvalidTeamRole errors
- Governance (Timelock/Safe) infrastructure

Tests: 184/184 passing (was 179; +5 from master's two-phase tests).

Note: master added pendingReleaseBlockNumber (uint32) to
AppConfigStorage, which expands the struct past a single slot. This
is inherited from master, not introduced by this PR — the append-only
storage claim in the PR description should be updated to reflect that
status and billingType now live on slot 2 post-merge.
Comment on lines +96 to +101
if (_roles[scope][Role.ADMIN].add(newOwner)) {
emit RoleGranted(scope, Role.ADMIN, newOwner);
}
if (_roles[scope][Role.ADMIN].remove(previousOwner)) {
emit RoleRevoked(scope, Role.ADMIN, previousOwner);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should probably revert on self-transfer. currently there's a bug where the owner would lose their ADMIN role if doing so

* roles (PAUSER, DEVELOPER) can be granted via AppAuthority. A
* follow-up call to `migrateAppsToAppAuthority` seeds AppAuthority
* state for existing apps — without it, auth checks on pre-upgrade
* apps revert because AppAuthority has no owner recorded.

@solimander solimander Apr 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a script for this? who calls it? important as all owner-gated calls for pre-existing apps will revert until completed

Comment thread src/AppController.sol
import {ICallValidator} from "./interfaces/ICallValidator.sol";
import {IERC165} from "@openzeppelin/contracts/utils/introspection/IERC165.sol";

contract AppController is

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_isDeveloper still uses the permission controller auth

Address three review comments from @solimander on PR #20:

1. AppAuthority.transferScopeOwnership: self-transfer (newOwner ==
   previousOwner) would collapse the ADMIN add+remove pair into a net
   remove, stranding the owner without ADMIN. Add explicit guard +
   SameOwnerTransfer error. Test verifies the owner's ADMIN role
   survives the rejected call.

2. AppController._isDeveloper: was still reading from
   permissionController.isAdmin(app, developer) — leftover from the
   pre-v1.5.0 auth model. Switch to appAuthority.hasRole(app, ADMIN,
   developer). Update the pre-existing getAppsByDeveloper test to
   reflect the new semantic (creator is auto-seeded as ADMIN on every
   app, so getAppsByDeveloper returns all apps the address has ADMIN
   on; transferOwnership removes them atomically).

3. v1.5.0-governance/2-upgradeAppController: the migration from
   legacy PC admins to AppAuthority was documented as a follow-up
   call, but there was no script for it — owner-gated ops on
   pre-existing apps would revert in the gap between the proxy
   upgrade and the (absent) migration call. Fold the migration into
   the phase-2 multisig tx: after ProxyAdmin.upgrade, paginate
   through getApps() and call migrateAppsToAppAuthority in batches
   of 50. Atomic — no deployment window where owner-gated ops are
   broken.

Tests: 184 → 185 passing (+1 self-transfer test; 1 pre-existing test
renamed and rewritten for the new _isDeveloper semantic).
@crypt0fairy crypt0fairy requested a review from solimander April 30, 2026 23:13
Address two review comments on AppController.transferOwnership:

1. Consent — transferring a DEFAULT-billed active app was
   unilateral. The receiver became the billing account (paying for
   compute, consuming their maxActiveApps capacity) without signing
   anything. Split into propose + accept:
   - transferOwnership(app, newOwner) records pendingOwner only. No
     AppAuthority rotation, no counter migration. Emits
     OwnershipTransferProposed. If an earlier proposal exists, emits
     OwnershipTransferCancelled for it first.
   - acceptOwnership(app) — pending-owner-only. Rotates scope
     ownership + ADMIN in AppAuthority, mirrors creator, migrates
     the active-app counter. Emits AppOwnershipTransferred.
   - cancelOwnershipTransfer(app) — owner-only rescind.
   - getPendingOwner(app) — view.

2. Quota — transferOwnership bumped newOwner.activeAppCount without
   checking their maxActiveApps, so an owner could push an app to a
   zero-quota or at-cap account. acceptOwnership now enforces the
   same `activeAppCount < maxActiveApps` check createApp uses,
   before the counter migrates. DEFAULT-billed active apps only;
   ISOLATED bills the app address and doesn't consume user quota.

Storage: AppConfigStorage gains `address pendingOwner` (20 bytes)
in slot 2, alongside the master-introduced pendingReleaseBlockNumber.
No shift to prior fields; __gap unchanged.

canCall selector list extended: transferOwnership and
cancelOwnershipTransfer stay owner-gated at schedule time.
acceptOwnership is NOT in the list — it's gated on the per-app
pending-owner field, which is dynamic state not worth duplicating
in schedule-time logic.

Also closes audit items this refactor touches:
- G-1 (Low): transferOwnership bypassed per-user quota. Fixed by
  acceptOwnership's MaxActiveAppsExceeded check.
- A-6 (Medium): mistyped owner address was irreversible. Fixed by
  cancelOwnershipTransfer + the inherent two-step nature — the
  wrong receiver simply never accepts.

Tests: 185 → 192. Added propose/accept flow, non-pending rejection,
no-proposal rejection, receiver-quota enforcement, cancellation,
re-propose supersession, owner-retains-authority-during-pending,
plus the existing transfer tests rewritten for the two-step semantic.

ABI is a breaking semantic change on transferOwnership (no longer
atomic). CLI and SDK must implement the accept step. New entry
points: acceptOwnership, cancelOwnershipTransfer, getPendingOwner.
New events: OwnershipTransferProposed, OwnershipTransferCancelled.
New error: NotPendingOwner.
Picks up the ABI additions from c70f43e (two-step transferOwnership)
and 8f6e0e4 (migrateAppsToAppAuthority):

- acceptOwnership(IApp)
- cancelOwnershipTransfer(IApp)
- getPendingOwner(IApp) -> address
- migrateAppsToAppAuthority(IApp[])
- OwnershipTransferProposed(IApp, address, address) event
- OwnershipTransferCancelled(IApp, address, address) event
- AppConfigStorage.pendingOwner field

Go consumers (sidecar, ecloud-billing-api, coordinator tooling) must
update to the new surface. transferOwnership semantics change: no
longer atomic — receiver must call acceptOwnership in a second tx.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants