From cbe5db2e5374dbdbdc110f49d0eba5fb8b4a3c5e Mon Sep 17 00:00:00 2001 From: Hanwen Cheng Date: Tue, 2 Jun 2026 01:56:45 +0800 Subject: [PATCH] fix(chain): require K11 self-attestation at first-master bootstrap (#165, anti-front-run) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit registerFirstMasterDevice was unauthenticated first-call-wins: an attacker could copy the victim's operatorOmni from the mempool and front-run with their own msg.sender, permanently locking the operator out (operatorMasterWallet[omni] = attacker). The 'attestation' bytes param was accepted but ignored. Fix (Approach A from #165 / the security review): require a K11 P-256 SELF-attestation verified against the to-be-registered pubkey (k11PubX/k11PubY) over a challenge that binds msg.sender + operator/actor omni + deviceKeyHash + k11Pub + chainid + contract. A captured assertion is non-transferable: a different sender → different challenge → the embedded clientDataJSON challenge no longer matches → verifyAssertion rejects; and an attacker cannot forge the operator's K11 signature. No chicken-and-egg — the attesting key IS the key being registered (a self-attestation), enrolled by bootstrap time (arch.md §9 stage 2). The binding is to msg.sender, so it survives the ERC-4337 migration unchanged (EOA today, smart-account address later). - SidecarRegistry.sol: add OP_REGISTER_1ST_MASTER; replace the ignored `bytes attestation` with a `K11Assertion` self-attestation; verify + store the read signCount. - AgentKeysV1.t.sol: +RejectsBogusSelfAttestation (unauthenticated path closed) and +RejectsFrontRunWithDifferentSender (captured assertion non-transferable; legit operator still bootstraps); update happy-path + helper to mock the verifier (real P-256 is covered in K11Verifier.t.sol / P256Verifier.t.sol). 41 forge tests pass. - heima-register-first-master.sh: header note — the live cast call is the old ABI; flip it to the new ABI + self-attestation IN THE SAME CHANGE that redeploys SidecarRegistry (coordinated; needs a live authenticator). Harness skips first-master. Activation = redeploy SidecarRegistry on Heima + update deployed-contracts.md + operator-workstation.env + verify-heima-contracts.sh (documented in #165). --- .../agentkeys-chain/src/SidecarRegistry.sol | 52 ++++++- crates/agentkeys-chain/test/AgentKeysV1.t.sol | 147 +++++++++++++++--- .../scripts/heima-register-first-master.sh | 17 ++ 3 files changed, 186 insertions(+), 30 deletions(-) diff --git a/crates/agentkeys-chain/src/SidecarRegistry.sol b/crates/agentkeys-chain/src/SidecarRegistry.sol index d890e498..5fbd5877 100644 --- a/crates/agentkeys-chain/src/SidecarRegistry.sol +++ b/crates/agentkeys-chain/src/SidecarRegistry.sol @@ -28,6 +28,7 @@ contract SidecarRegistry { uint8 public constant TIER_AGENT = 2; /// @notice Operation kind codes used in challenge-msg construction. + bytes32 public constant OP_REGISTER_1ST_MASTER = keccak256("agentkeys:v1:register-first-master"); bytes32 public constant OP_REGISTER_2ND_MASTER = keccak256("agentkeys:v1:register-master"); bytes32 public constant OP_REVOKE_MASTER = keccak256("agentkeys:v1:revoke-master"); bytes32 public constant OP_SET_THRESHOLD = keccak256("agentkeys:v1:set-recovery-threshold"); @@ -99,8 +100,17 @@ contract SidecarRegistry { // ─── Master device registration ────────────────────────────────────── /// @notice Register the FIRST master device for an operator. First call wins; /// subsequent master mutations need this sender. - /// @dev For initial bootstrap (no existing master), no K11 assertion is - /// required (chicken-and-egg — there's no prior K11 to attest with). + /// @dev Bootstrap requires a K11 **self-attestation**: the new device's own + /// platform authenticator signs a challenge that binds `msg.sender` + + /// the operator/actor omni + device-key-hash + the K11 pubkey being + /// registered + chainid + this contract. This defeats the mempool + /// front-run (issue #165): a captured attestation cannot be replayed by + /// a different sender — the contract rebinds the challenge to the new + /// `msg.sender`, so the embedded clientDataJSON challenge no longer + /// matches and `verifyAssertion` rejects — and an attacker cannot forge + /// the operator's K11 signature. There is no chicken-and-egg: the + /// attesting key IS the key being registered (a *self*-attestation), + /// which exists by bootstrap time (enrolled in stage 2 per arch.md §9). function registerFirstMasterDevice( bytes32 deviceKeyHash, bytes32 operatorOmni, @@ -109,8 +119,8 @@ contract SidecarRegistry { bytes32 k11RpIdHash, uint256 k11PubX, uint256 k11PubY, - bytes calldata attestation, - uint8 roles + uint8 roles, + K11Assertion calldata selfAttestation ) external { if (devices[deviceKeyHash].registeredAt != 0) { revert DeviceAlreadyRegistered(deviceKeyHash); @@ -120,6 +130,37 @@ contract SidecarRegistry { revert DeviceAlreadyRegistered(deviceKeyHash); } + // ── Anti-front-run (issue #165): K11 self-attestation bound to msg.sender ── + // Verified against the pubkey BEING registered (k11PubX/k11PubY) — there is + // no prior device to attest with at bootstrap. The challenge commits the + // sender so a captured assertion is non-transferable to another sender. + bytes32 expectedChallenge = keccak256( + abi.encode( + OP_REGISTER_1ST_MASTER, + operatorOmni, + actorOmni, + deviceKeyHash, + k11PubX, + k11PubY, + roles, + msg.sender, + block.chainid, + address(this) + ) + ); + bool ok = k11Verifier.verifyAssertion( + expectedChallenge, + k11RpIdHash, + selfAttestation.authenticatorData, + selfAttestation.clientDataJSON, + selfAttestation.challengeLocation, + selfAttestation.r, + selfAttestation.s, + k11PubX, + k11PubY + ); + if (!ok) revert K11VerificationFailed(); + operatorMasterWallet[operatorOmni] = msg.sender; recoveryThreshold[operatorOmni] = 1; emit OperatorBootstrapped(operatorOmni, msg.sender); @@ -134,13 +175,12 @@ contract SidecarRegistry { tier: TIER_MASTER, roles: roles, registeredAt: uint64(block.timestamp), - lastSignCount: 0, + lastSignCount: k11Verifier.readSignCount(selfAttestation.authenticatorData), revoked: false }); operatorDevices[operatorOmni].push(deviceKeyHash); emit DeviceRegistered(deviceKeyHash, operatorOmni, actorOmni, TIER_MASTER, roles, k11CredId); - attestation; // accepted but only emitted via event topics } /// @notice Register a 2nd+ master device. Existing master signs a K11 diff --git a/crates/agentkeys-chain/test/AgentKeysV1.t.sol b/crates/agentkeys-chain/test/AgentKeysV1.t.sol index 65bb784b..5f9f5a9a 100644 --- a/crates/agentkeys-chain/test/AgentKeysV1.t.sol +++ b/crates/agentkeys-chain/test/AgentKeysV1.t.sol @@ -74,21 +74,7 @@ contract AgentKeysV1Test is Test { // ─── SidecarRegistry: first-master bootstrap ───────────────────────── function test_RegisterFirstMasterDevice_BootstrapsOperator() public { - uint8 fullRoles = - registry.ROLE_CAP_MINT() | registry.ROLE_RECOVERY() | registry.ROLE_SCOPE_MGMT(); - - vm.prank(master); - registry.registerFirstMasterDevice( - deviceKeyHashMaster, - operatorOmni, - actorOmniMaster, - k11CredId, - k11RpIdHash, - k11PubX, - k11PubY, - hex"cafe", - fullRoles - ); + _registerFirstMaster(); assertEq(registry.operatorMasterWallet(operatorOmni), master); assertEq(uint256(registry.recoveryThreshold(operatorOmni)), 1); SidecarRegistry.DeviceEntry memory entry = registry.getDevice(deviceKeyHashMaster); @@ -100,12 +86,10 @@ contract AgentKeysV1Test is Test { } function test_RegisterFirstMaster_RejectsDuplicateBootstrap() public { - vm.prank(master); - registry.registerFirstMasterDevice( - deviceKeyHashMaster, operatorOmni, actorOmniMaster, k11CredId, k11RpIdHash, k11PubX, k11PubY, "", 7 - ); + _registerFirstMaster(); // Second bootstrap with a different device hash → rejected because - // operatorMasterWallet is now set. + // operatorMasterWallet is now set (checked before the K11 verify, so no + // mock needed here). vm.prank(master); vm.expectRevert( abi.encodeWithSelector( @@ -120,9 +104,109 @@ contract AgentKeysV1Test is Test { k11RpIdHash, k11PubX, k11PubY, - "", - 7 + 7, + _bogusAssertion(bytes32(0)) + ); + } + + /// @notice Issue #165: the bootstrap is no longer unauthenticated. Without a + /// valid K11 self-attestation the call reverts — closing the + /// first-call-wins front-run's enabler. + function test_RegisterFirstMaster_RejectsBogusSelfAttestation() public { + // No mock → the real K11Verifier runs against a bogus self-attestation and + // reverts (challenge/P-256 mismatch). + vm.prank(master); + vm.expectRevert(); + registry.registerFirstMasterDevice( + deviceKeyHashMaster, + operatorOmni, + actorOmniMaster, + k11CredId, + k11RpIdHash, + k11PubX, + k11PubY, + 7, + _bogusAssertion(bytes32(0)) ); + assertEq(registry.operatorMasterWallet(operatorOmni), address(0)); + } + + /// @notice Issue #165: a captured self-attestation is non-transferable to a + /// different sender. The challenge binds msg.sender, so a front-runner + /// replaying the victim's assertion with their own sender is rejected, + /// while the legitimate operator still bootstraps. + function test_RegisterFirstMaster_RejectsFrontRunWithDifferentSender() public { + uint8 roles = 7; + SidecarRegistry.K11Assertion memory att = _bogusAssertion(bytes32(0)); + // The verifier accepts ONLY the challenge that binds `master` as the sender. + bytes32 victimChallenge = keccak256( + abi.encode( + registry.OP_REGISTER_1ST_MASTER(), + operatorOmni, + actorOmniMaster, + deviceKeyHashMaster, + k11PubX, + k11PubY, + roles, + master, + block.chainid, + address(registry) + ) + ); + vm.mockCall( + address(k11), + abi.encodeWithSelector( + K11Verifier.verifyAssertion.selector, + victimChallenge, + k11RpIdHash, + att.authenticatorData, + att.clientDataJSON, + att.challengeLocation, + att.r, + att.s, + k11PubX, + k11PubY + ), + abi.encode(true) + ); + vm.mockCall( + address(k11), + abi.encodeWithSelector(K11Verifier.readSignCount.selector), + abi.encode(uint32(0)) + ); + + // Attacker front-runs with the victim's omni/pubkey/assertion but their own + // sender → contract recomputes the challenge with msg.sender = attacker → + // no mock match → real verifier → revert. Victim's omni stays unclaimed. + vm.prank(attacker); + vm.expectRevert(); + registry.registerFirstMasterDevice( + deviceKeyHashMaster, + operatorOmni, + actorOmniMaster, + k11CredId, + k11RpIdHash, + k11PubX, + k11PubY, + roles, + att + ); + assertEq(registry.operatorMasterWallet(operatorOmni), address(0)); + + // The legitimate operator bootstraps — its challenge matches the mock. + vm.prank(master); + registry.registerFirstMasterDevice( + deviceKeyHashMaster, + operatorOmni, + actorOmniMaster, + k11CredId, + k11RpIdHash, + k11PubX, + k11PubY, + roles, + att + ); + assertEq(registry.operatorMasterWallet(operatorOmni), master); } // ─── SidecarRegistry: 2nd master device requires K11 ──────────────── @@ -441,6 +525,20 @@ contract AgentKeysV1Test is Test { function _registerFirstMaster() internal { uint8 fullRoles = registry.ROLE_CAP_MINT() | registry.ROLE_RECOVERY() | registry.ROLE_SCOPE_MGMT(); + // Real P-256 verification is covered by K11Verifier.t.sol / P256Verifier.t.sol; + // here we mock the verifier so registry liveness tests don't need a real + // self-attestation. Mocks are cleared after bootstrap so later assertions + // (e.g. RejectsInvalidK11) exercise the real verifier. + vm.mockCall( + address(k11), + abi.encodeWithSelector(K11Verifier.verifyAssertion.selector), + abi.encode(true) + ); + vm.mockCall( + address(k11), + abi.encodeWithSelector(K11Verifier.readSignCount.selector), + abi.encode(uint32(0)) + ); vm.prank(master); registry.registerFirstMasterDevice( deviceKeyHashMaster, @@ -450,9 +548,10 @@ contract AgentKeysV1Test is Test { k11RpIdHash, k11PubX, k11PubY, - "", - fullRoles + fullRoles, + _bogusAssertion(bytes32(0)) ); + vm.clearMockedCalls(); } /// @dev Bogus assertion for SidecarRegistry — fails challenge or P-256 diff --git a/harness/scripts/heima-register-first-master.sh b/harness/scripts/heima-register-first-master.sh index 8a06804a..2724cda8 100755 --- a/harness/scripts/heima-register-first-master.sh +++ b/harness/scripts/heima-register-first-master.sh @@ -11,6 +11,23 @@ # # Reads primary master K11 pubkey + cred-id from # `~/.agentkeys/k11/.json` (must be `mode: "webauthn"`). +# +# ⚠️ ANTI-FRONT-RUN (issue #165) — REDEPLOY-COORDINATED CHANGE PENDING. +# The hardened SidecarRegistry now requires a K11 *self-attestation* at +# bootstrap, bound to msg.sender (defeats the mempool front-run). The new ABI is: +# registerFirstMasterDevice(bytes32,bytes32,bytes32,bytes32,bytes32,uint256, +# uint256,uint8,(bytes32,bytes,bytes,uint256,uint256,uint256)) +# where the trailing tuple is the K11Assertion (attestingDeviceKeyHash, +# authenticatorData, clientDataJSON, challengeLocation, r, s) signed over +# keccak256(abi.encode(OP_REGISTER_1ST_MASTER, operatorOmni, actorOmni, +# deviceKeyHash, k11PubX, k11PubY, roles, msg.sender, block.chainid, registry)) +# Generate the assertion exactly like scripts/heima-scope-set.sh --webauthn +# (cast abi-encode → cast keccak → `agentkeys k11 assert`). +# The `cast send` below still targets the OLD (pre-#165) deployed registry ABI. +# Flip it to the new ABI + self-attestation IN THE SAME CHANGE that REDEPLOYS +# SidecarRegistry (then update docs/spec/deployed-contracts.md + +# scripts/operator-workstation.env + re-run verify-heima-contracts.sh). Until +# that coordinated redeploy, this script bootstraps against the old contract. set -euo pipefail