fix(execution): refuse to submit any action whose signer is not its owner - #71
Merged
Merged
Conversation
…wner
On-chain, ActionVerifier._verifySignerPermission allows signer != owner when
sessionKeys[signer][owner] >= block.timestamp. That registry is live on the
deployed Matching -- registerSessionKey/deregisterSessionKey/sessionKeys are
all in its runtime bytecode, and the on-chain ACTION_TYPEHASH matches the
source string including `address signer` -- and NO session key has ever been
registered: SessionKeyRegistered has zero logs across the contract's history.
So this is a no-op today on a stronger basis than "we do not use session keys":
with an empty registry the chain itself rejects a non-owner signer, so no
action this executor has ever submitted successfully could have had one. The
guard refuses earlier, for free, instead of paying gas to learn it.
It exists because that is configuration, not a property of this service. The
moment any owner registers a session key, actions signed by it become
chain-valid through EVERY module this executor can reach -- deposit, transfer,
withdrawal, liquidate, rfq, trade -- and nothing here would have objected:
- assertWithdrawalPolicy covers withdrawals only.
- assertPayloadConsistency checks signer only when EXPECTED_ACTION_SIGNER is
configured, and it is set NOWHERE in infra/ (nor is EXPECTED_ACTION_OWNER).
So the settlement path has never checked this at all.
There were two call paths to Matching.verifyAndMatch, execute() and withdraw(),
sharing only enqueueSend -- a serial queue that takes an opaque closure and
never sees an action. Both now route through one private submitVerifyAndMatch,
so the check is at the boundary and a third caller inherits it rather than
having to remember it. The ABI and the meaning of a failed simulation are
passed in, so behaviour on both paths is unchanged. AtomicSigningExecutor is
authorised on Matching but this service never calls it.
Deliberately absolute: no allowlist, no per-module exception, no config escape.
Relaxing it for a specific delegated signer is a separate reviewable change --
the point is that the relaxation has to be deliberate rather than discovered.
This binds the service, not the key: anything else holding kms:Sign on the
executor key can call Matching directly and never pass through here.
Tests assert the guard fires rather than that it exists. The boundary tests
point at a closed port and distinguish outcomes by WHICH error arrives: a
refusal returns SignerNotOwnerError in ~1ms having touched no socket, while
owner-signed traffic takes ~1s and fails with "HTTP request failed", proving
it got to simulateContract. An earlier version of those tests asserted only
"not SignerNotOwnerError" and passed while the request was dying in ABI
encoding, never reaching the RPC -- green for a reason unrelated to the guard.
They now pin the transport error so that cannot recur.
On the withdrawal path the guard is a backstop that never fires today, because
assertWithdrawalPolicy rejects signer != owner first; a test documents that
overlap rather than hiding it. withdrawal.ts and the EXPECTED_ACTION_* code are
untouched.
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
robertleifke
added a commit
that referenced
this pull request
Sep 20, 2026
…leton services (#73) execution-service and market-maker-spot deployed with the ECS defaults, 100% minimum healthy and 200% maximum, so every rolling deploy ran two tasks at once. Both are singletons, and the matcher already sets 0/100 for exactly this reason. This applies the same setting to the two that were missed. execution-service: two tasks sign with the SAME KMS key, and nothing coordinates the executor EOA's nonce across processes. serial-queue.ts serialises within one process and states the hazard plainly -- "viem reads the next nonce from the RPC at send time, so two sends in flight at once can take the same nonce and one replaces or rejects the other". There is no load balancer to drain: Cloud Map returns an A record for BOTH tasks (MULTIVALUE, TTL 10s), so a settlement lands on whichever the matcher resolves. The deployment circuit breaker is disabled, so nothing would have caught it either. market-maker-spot: two market makers quote the same book. Already observed -- a draining task kept placing orders for seconds after its successor had finished its startup reconciliation, leaving quotes resting under the previous configuration that nothing re-examined. markets-service is deliberately left rolling. It is an HTTP API with no singleton constraint, so rolling avoids downtime and costs nothing. The cost is a gap of roughly a minute or two per deploy on those two services. For settlement that is lag, not loss: the matcher classifies an unrecognised executor error as transient (internal/matching/revert.go, revertUnknown is the zero value and documented as "treated like a transient failure") and retries on a doubling backoff -- 2s to a 5m cap, then every 30m after twelve failures -- that never cancels an order. A connection failure carries no revert selector, so it cannot be classified permanent and cannot park a pair. Applying this changes deploymentConfiguration only. It registers no task definition and forces no new deployment, so nothing restarts. Found while preparing the execution-service deploy of #71/#69: the rollout plan was a rolling one, and the nonce sequence is shared. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Every action this executor submits must be signed by the account's own owner. One guard, at the single point where actions reach
Matching.verifyAndMatch.Call paths — there were two
You asked me to check before writing code. There were two paths, sharing no chokepoint that sees an action:
execute()assertPayloadConsistencywithdraw()assertWithdrawalPolicyThe only thing they shared was
enqueueSend, a serial queue taking an opaque closure — a concurrency primitive, not a policy boundary. Both now route through one privatesubmitVerifyAndMatch, which is why this is a small refactor rather than an inserted line. The ABI and the meaning of a failed simulation are passed in, so behaviour on both paths is unchanged.AtomicSigningExecutoris authorised on Matching but this service never calls it — zero references.Why it is a no-op
Stronger than "we don't use session keys."
ActionVerifier._verifySignerPermissionallowssigner != owneronly whensessionKeys[signer][owner] >= block.timestamp, and no session key has ever been registered —SessionKeyRegisteredhas zero logs across the contract's entire history. With an empty registry the chain rejects a non-owner signer itself, so no action ever submitted successfully could have had one. This refuses earlier, for free, rather than paying gas to find out.The registry is verified live, not assumed:
registerSessionKey/deregisterSessionKey/sessionKeysselectors are all in the deployed runtime bytecode, and on-chainACTION_TYPEHASH()equalskeccak256of the source string includingaddress signer.Why it exists anyway
That's configuration, not a property of this service. The moment any owner registers a key, actions signed by it are chain-valid through every module this executor can reach — deposit, transfer, withdrawal, liquidate, rfq, trade — and nothing here would have objected:
assertWithdrawalPolicycovers withdrawals only.assertPayloadConsistencychecks the signer only whenEXPECTED_ACTION_SIGNERis configured — and it is set nowhere ininfra/, nor isEXPECTED_ACTION_OWNER. So the settlement path has never checked this at all. Left untouched here, as asked, but worth its own decision.Deliberately absolute: no allowlist, no per-module exception, no config escape. Relaxing it for a delegated signer should be a separate, reviewable change — the point is that the relaxation is deliberate rather than discovered.
It binds the service, not the key. Anything else holding
kms:Signon the executor key can call Matching directly and never pass through here.Tests
11 new, asserting the guard fires, not that it exists.
The boundary tests point at a closed port and distinguish outcomes by which error arrives: a refusal returns
SignerNotOwnerErrorin ~1ms having touched no socket; owner-signed traffic takes ~1s and fails withHTTP request failed, proving it reachedsimulateContract.Worth flagging: an earlier version of those tests asserted only "not
SignerNotOwnerError" and passed while the request was dying in ABI encoding, never reaching the RPC — green for a reason unrelated to the guard. I found it by probing the actual error rather than trusting the green. They now pin the transport error so it can't recur, and the unnamed-tuple ABI that caused it is fixed with a comment explaining why the components must be named.On the withdrawal path the guard is a backstop that never fires today, because
assertWithdrawalPolicyrejectssigner != ownerfirst. A test documents that overlap rather than hiding it../scripts/verify.sh node→ all checks passed. 80 tests, 0 failures (69 before, +7 guard, +4 boundary).What this does not buy
It does not make session keys safe. It makes their absence enforced. Untouched: the 600-second floor on revocation (
deregisterSessionKeysets expiry tonow + 600, so it extends a key with less than 10 minutes left), and the fact thatassertWithdrawalPolicyis stateless so any "per-cycle" cap would really be per-request.🤖 Generated with Claude Code