contracts: removeDevice is inert while allowAnyDevice is true, and both the mapping and the audit event report a revocation that changed no decision

[kvinwang]

Label: DESIGN. No test fails against the current contracts; the tests below pass and pin present behaviour.

What the design currently is

DstackApp.isAppAllowed composes the device flag and the device list as a short-circuit:

// contracts/DstackApp.sol:209
if (!allowAnyDevice && !allowedDeviceIds[bootInfo.deviceId]) {
    return (false, "Device not allowed");
}

So whenever allowAnyDevice == true, the contents of allowedDeviceIds do not participate in any authorization decision. addDevice and removeDevice (DstackApp.sol:175, DstackApp.sol:182) still write the mapping and still emit DeviceAdded / DeviceRemoved plus PolicyChanged(actor, keccak("device"), deviceId, enabled) unconditionally.

Call sequence an operator actually performs — the one docs/auth-simple-operations.md:160 recommends ("Use allowAnyDevice: true initially, then restrict to specific devices after capturing IDs from logs"), and the one the shipped script/Manage.s.sol:159 default (ALLOW_ANY_DEVICE defaults to true) puts you in by default:

appOwner : app.setAllowAnyDevice(true)     // to bring up / debug a new host
...
appOwner : app.removeDevice(DEVICE_A)      // "host A is compromised, revoke it"

Resulting state: allowedDeviceIds[DEVICE_A] == false, one DeviceRemoved, one PolicyChanged(..., false). isAppAllowed with deviceId = DEVICE_A still returns (true, "").

The mirror case: setAllowAnyDevice(false) re-arms every device ever added, and the single PolicyChanged("allow-any-device", bytes32(0), false) it emits names none of them.

Both are pinned in the new test/ScenarioWalk.t.sol:

[PASS] test_S4_RemoveDeviceIsANoOpUnderAllowAnyDevice_ButStillLogsARevocation() (gas: 307587)
       assertEq(_auditCount(logs), 1, "the audit log records a revocation");
       assertFalse(app.allowedDeviceIds(DEVICE_A), "and the mapping agrees");
       assertTrue(ok, why);   // <-- the revoked device still boots

[PASS] test_S4_SetAllowAnyDeviceFalse_SilentlyResurrectsAStaleDeviceList() (gas: 341290)
       assertEq(_auditCount(logs), 1, "one flag event -- it names no device");
       assertTrue(a && b, "both historical devices are live again");

Steelman

This is the cheapest correct composition. An allowAnyDevice escape hatch is genuinely useful during bring-up, and making the setters conditional on the flag would either cost an extra SLOAD on every write or make the setters revert in a state an operator legitimately wants to prepare (stage the device list before flipping the flag off — which the current design supports and a revert would break). Emitting the event unconditionally is also the simpler invariant for a log indexer: one event per state transition of the mapping, with no flag-dependent branch to replay.

test/EventAudit.t.sol:82-96 already walks this exact sequence and asserts the event and the mapping — so the current behaviour is deliberate at the level of the mapping. The gap is that the mapping is not the policy.

What it costs

The operator scenario it breaks is emergency device revocation, and it breaks it in the quietest possible way: the transaction succeeds, the mapping reads false, and the permanent audit log records a revocation. Every signal an operator or an auditor would check agrees that the device was revoked. Only isAppAllowed disagrees.

docs/onchain-governance.md:171 documents removeDevice as "Remove a device from whitelist" with no mention of the flag, and PolicyChanged is documented in-contract as the "additive audit event for reconstructing authorization policy" — which it is, for the mapping, and is not, for the decision.

Reachability: who — the app owner; credential — the app owner key; frequency — operator-paced. This is not an attacker-triggered defect. It is a control that reports success without taking effect.

Improvement direction

Redeployment status: none of these needs a new proxy. DstackApp is UUPS with __gap[49]; every code option below is an implementation upgrade behind existing proxy addresses with no storage change. The caveat that decides priority is different: DstackApp proxies are owned by each app's owner, so an implementation fix reaches an app only when that owner upgrades, and never for an app that has called disableUpgrades(). The documentation half of the fix therefore has to stand on its own.

Options, cheapest first:

1. Docs only, no contract change. State in docs/onchain-governance.md and the removeDevice natspec that the device list is inert while allowAnyDevice is true, and that revoking a device requires setAllowAnyDevice(false) first. Reaches every deployed app immediately, including frozen ones. Does not fix the misleading audit event.
2. Make the write reflect the policy (impl upgrade, no storage). require(!allowAnyDevice, "device list is inert while allowAnyDevice") in addDevice/removeDevice. Loudest and simplest, but breaks the legitimate stage-then-flip workflow, so it is probably the wrong trade.
3. Make the event tell the truth (impl upgrade, no storage). Keep the setters permissive, but widen the audit event so a log replayer can compute the decision — e.g. emit PolicyChanged("device", deviceId, enabled && !allowAnyDevice), or add a distinct DevicePolicyInert(deviceId) marker. Preserves the workflow; the log becomes replayable to the decision rather than to the mapping. Cost: a log indexer keyed on the current semantics needs updating.
4. Emit the affected set on the flag transition (impl upgrade, needs an enumerable device set — new storage past __gap). Highest review cost, and the unbounded-iteration gas profile is a real objection. Probably not worth it.

(3) plus (1) looks like the right combination: it keeps the flag's usefulness, keeps the staging workflow, and closes the gap between the audit story and the decision.

Found during a scenario-driven review of the authorization contracts; full walk in .agent/CONTRACT-SCENARIOS.md (scenario 4), tests in dstack/kms/auth-eth/test/ScenarioWalk.t.sol.