fix: apply a default organization spending cap instead of treating an absent cap as unlimited - #2031
Merged
Conversation
eskp
marked this pull request as draft
August 12, 2026 05:21
eskp
force-pushed
the
fix/spend-cap-fail-open
branch
from
August 24, 2026 09:24
2a35d16 to
10232a5
Compare
eskp
force-pushed
the
fix/spend-cap-fail-open
branch
from
August 25, 2026 00:13
0a084d0 to
7213ee0
Compare
joelorzet
requested changes
Aug 25, 2026
Contributor
There was a problem hiding this comment.
Closing the unlimited default and locking the cap row so orgs without one still serialize are both right.
Blocking: the Tempo path checks each call instead of the transaction, approve staying uncapped makes the rest of the ceiling advisory, and the three EXECUTE_DEFAULT_* overrides are not wired into either values.yaml.
Also worth folding in: the two cap denials on the same endpoint return different shapes, the two cores classify the same denial differently, and simulate skips the check. Details inline.
joelorzet
approved these changes
Aug 25, 2026
joelorzet
left a comment
joelorzet
left a comment
Contributor
There was a problem hiding this comment.
All addressed. Env vars are in both values files, and covering the executor block too is the part I missed: reserveOrgValue runs there for workflow steps, so app-only would have meant an operator widening a cap only widened it for direct executions.
created now comes from returning(), so the telemetry stops attributing a lost insert race to "no_cap_row".
The zero-value short circuit is a better catch than the one I raised. I flagged it as wasted lock contention; you found that total + reserved > dailyCap collapses to total > dailyCap at zero, so once an org went over for the day every later zero-value request was refused too, including off-chain node executions that cannot move anything. That was reachable only because absent stopped meaning unlimited, so it arrived with this change and leaves with it.
The stablecoin items moved to #2133 and are answered there. Since that branch contains this one and both target staging, this should merge first, then merge staging into #2133.
The daily value cap treated a missing organization_spend_caps row, or a null column for one chain family, as unlimited. Only the admin spend-cap route ever wrote that table and afterCreateOrganization seeded the subscription row alone, so every organization was uncapped until someone opted in and a leaked API key was bounded only by the wallet balance. Both reservation paths now fall back to a platform default instead: checkAndReserveExecution for the direct execution API, and reserveOrgValue / reserveOrgSolanaValue for workflow and protocol runs. Capping only the direct API would have left the workflow entrance to the same wallet wide open. The defaults live in lib/execute/spend-cap-defaults.ts, anchored on the 200 USD/day ceiling the agentic wallet already applies, with an env override per unit so ops can widen them without a deploy. Applying a default emits a spend_cap_default_applied security event carrying the org, chain family, reason and whether the request was denied, so the blast radius is measurable before it starts refusing traffic. Those defaults are enforced atomically for the organizations they apply to. SELECT ... FOR UPDATE locks nothing when the row is absent, which is every organization this change newly covers, so concurrent callers would each have read a zero day-total and each reserved the full default. lockOrgSpendCapRow creates the row on first use, with both cap columns left NULL so it is a lock anchor and not a frozen copy of the default, and every reservation path takes it. Token transfers reserved "0" because ERC-20 calldata carries no native value, so a leaked key could move unbounded USDC against a cap reading zero. Known stablecoins are now bounded per transaction with no oracle: supported_tokens records their decimals and the peg is ~1:1, so the amount rescales to micro-USD. The check sits in the shared cores -- transferTokenCore, writeContractCore and signTempoTx -- not in a route, so the transfer API, contract-call API, protocol actions, check-and-execute, node execution, the workflow steps and Tempo's TIP-20 sends all reach it; a ceiling on one route would only have said which door to use. transfer, transferFrom and their TIP-20 memo variants are refused above the ceiling; a large approve is reported but allowed, because refusing max-uint approvals would break ordinary protocol integrations. Unrecognised tokens still pass through; general ERC-20 pricing needs a price feed in the pre-broadcast path. The cap is now reported as well as enforced. GET /api/analytics/spend-cap returns the effective figure and a flag saying whether it is the platform default, so the dashboard gauge and the get_spending_limits MCP tool stop telling an agent there is no cap while requests are being refused. The admin UI copy, the schema comment and the public API docs say that clearing a cap hands the chain family back to the default rather than removing it. The two native defaults are denominated in ETH and SOL at a stated reference price, so their USD worth drifts with the market. A Chainlink feed does exist in this codebase, but the reservation runs inside a row lock and reserveOrgValue carries no chain id, so pricing there would mean holding the lock across an RPC call on a path with nothing to price.
The three EXECUTE_DEFAULT_* overrides introduced with the platform spend-cap defaults were read from process.env but never listed in .env.example, so the Environment Variable Sync check warned on them and an operator widening a cap mid-incident had no documented name to set.
Lowers the three defaults introduced with the fail-closed spend cap: daily EVM native 0.05 ETH -> 0.02 ETH daily Solana native 1 SOL -> 0.5 SOL per-tx stablecoin 200 USD -> 100 USD The reference ceiling is unchanged: the 200 USD/day figure the agentic wallet already applies. Each default now sits under it rather than at it. Two reasons. Every default is env-overridable without a deploy, so a default that binds too tightly costs a config change while one that binds too loosely is an unbounded outflow nobody notices; widening on evidence from spend_cap_default_applied is the recoverable direction. And the two native figures are denominated in the native asset, so their USD worth drifts upward with the market with nothing alerting -- they are now picked so drift cannot carry them past the anchor until ETH clears ~10,000 USD or SOL ~400 USD. The stablecoin ceiling goes to half the anchor because it is per transaction with no aggregate: the per-key rate limit is all that bounds the daily total, so the figure sizes the single worst transaction a leaked key can push through. Also regenerates specs/api-coverage.json, whose recorded line numbers shifted when this branch edited docs/api/direct-execution.md.
matchOutflowFunction compared the raw declared input types from a caller-supplied ABI against the literal string "uint256". Solidity treats uint as an alias of uint256 and ethers canonicalises before computing the selector, so an ABI declaring transfer(address,uint) encodes 0xa9059cbb -- byte-identical to a real ERC-20 transfer -- while the comparison failed, matchOutflowFunction returned null, and checkStablecoinContractCall short-circuited to allowed. loadStablecoin was never reached and no amount was ever compared, so one word in an attacker-supplied ABI defeated the whole ceiling. Declared types now go through ethers ParamType, which applies the same normalisation used to build the selector, so the two agree. A type ethers cannot parse passes through unchanged: it will not match a shape, but it also cannot be encoded into a transaction. Only checkStablecoinContractCall was affected. The decodeErc20Outflow path reads parsed.fragment.inputs, which ethers has already normalised. The regression test drives both spellings and asserts the token lookup actually ran, so it cannot pass by allowing for an unrelated reason; reverting the canonicalisation fails the uint case alone.
This branch conflated two things. Closing the fail-open default on the
organization spending cap is a small fix to an existing control: an absent
row meant unlimited, and it now resolves to a platform default. The
stablecoin per-transaction ceiling is a new control over a value class the
cap never covered at all, and it was 854 of the 1816 added lines.
They raise different questions. The native fix is bounded and its
behaviour is settled. The ceiling still needs decisions on whether approve
stays uncapped, whether Solana SPL transfers are in scope, and how the
per-transaction shape relates to a daily aggregate. Keeping them together
meant the settled half waited on the unsettled one.
Removed here and carried to fix/stablecoin-transfer-ceiling, unchanged
apart from the split itself:
lib/execute/stablecoin-cap.ts and its suite
the four call sites that invoke it (tempo-tx-core, transfer-token-core,
write-contract-core, and the transfer route comment)
the stablecoin default and its env key
the docs section describing the ceiling
What stays is the native cap: the default resolution, the cap-row lock
that serialises organizations which never configured one, the analytics
and UI surfaces that render an effective cap, and their tests.
Two problems introduced by making an absent cap resolve to a default. The comparison is `total + reserved > dailyCap`, which at reserved = 0 is `total > dailyCap`. Once an organization went over for the day, every later zero-value request was refused with it -- off-chain node executions and reads that cannot move anything. Before the default existed an unconfigured org returned before the comparison ran, so this only became reachable when absent stopped meaning unlimited. A request that moves no value cannot push a total over anything, so it now returns before the cap is consulted. withValueCap already does this on the ledger side. The same early return keeps the cap-row insert and its FOR UPDATE lock off the request path for traffic that is mostly zero-value. The row exists only as a lock anchor, and a request with nothing to reserve is not racing anyone for budget. Separately, lockOrgSpendCapRow reported created: true whenever it reached the insert, including when onConflictDoNothing did nothing because a concurrent transaction won the race. That attributed the race to "no_cap_row" in spend_cap_default_applied, which reads as "this org has never configured a cap" when the truth is that two requests arrived together. onConflictDoNothing returns a row only when the insert actually happened, so returning() distinguishes the two. Reverting either fix fails its own cases and nothing else.
The module claimed each default "can be overridden without a deploy", and .env.example and the public docs documented the keys, but EXECUTE_DEFAULT appeared in zero files under deploy/. Both values files enumerate their environment explicitly, so nothing set them and widening a cap that was binding a live integrator meant editing a constant and shipping code -- which is what the rollout in the description tells an operator not to have to do. Sets both native keys for the app and the executor in prod and staging. Both are needed: direct executions reserve in the app, workflow steps reserve in the executor through withValueCap, and the executor keeps its own env block rather than merging shared_env. The values match the compiled-in defaults, so this changes no behaviour on deploy. It only makes the escape hatch real. The claim itself is corrected rather than left overstated: type kv means a new figure ships with a helm upgrade, not instantly. parameterStore plus a pod restart is what "without a deploy" would actually take, and that is a choice worth making deliberately rather than implying. Also notes that AGENTIC_WALLET_DAILY_CAP_MICROS, cited in the original comment as the convention being followed, makes the same claim and is wired into neither environment.
eskp
force-pushed
the
fix/spend-cap-fail-open
branch
from
August 26, 2026 00:08
78be8bc to
e4449bc
Compare
Contributor
🧹 PR Environment Cleaned UpThe PR environment has been successfully deleted. Deleted Resources:
All resources have been cleaned up and will no longer incur costs. |
Contributor
ℹ️ No PR Environment to Clean UpNo PR environment was found for this PR. This is expected if:
|
This was referenced Aug 26, 2026
Merged
hicksonhaziel
pushed a commit
to hicksonhaziel/keeperhub
that referenced
this pull request
Aug 27, 2026
PR KeeperHub#2031 made an absent organization_spend_caps row resolve to a platform default of 0.02 ETH per day instead of unlimited. Every protocol-coverage suite executes as the one persistent org e2e-test-org, so the native value they move accumulates against a single daily ledger per shard. Shard 4 sends 0.04 ETH: 0.01 for wrapped/wrap, plus 0.01 for each of the three frax-ether-v2 mints. wrapped runs first, so frax/mint lands exactly on the 0.02 cap and passes, and mint-and-give and mint-and-stake are denied. Seed the org an explicit daily_value_cap_wei of 1 ETH. CI then keeps the same platform default as staging and prod, which both pin 0.02 ETH, and this org carries its own ceiling instead. The figure is 25 times the current worst case, so a new payable fixture does not break the suite again. The write is an upsert, not an insert-if-absent: the reservation path creates a row with both cap columns NULL on the org's first value-moving request, and such a row still resolves to the platform default.
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
3 participants
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 this changes
organization_spend_capsis read on every direct execution. When no row existed, or the column for that chain family was null, spending was unlimited. The only production writer was the admin settings route, andafterCreateOrganizationseeded only the subscription row, so an organization was unlimited until an admin opted in.Separately, the cap only counted native value.
app/api/execute/transfer/route.tsreserved"0"for token transfers because token value is not priced into the cap, and an ERC-20 contract call reservesvalue, which is zero for token calldata. Token movement was therefore invisible to the cap.This makes an absent cap resolve to a platform default rather than to unlimited, and adds a per-transaction stablecoin ceiling that applies on every write path.
Defaults introduced
EXECUTE_DEFAULT_DAILY_VALUE_CAP_WEIEXECUTE_DEFAULT_DAILY_SOLANA_VALUE_CAP_LAMPORTSEXECUTE_DEFAULT_STABLECOIN_CAP_MICRO_USDAll three are chosen against the one existing policy figure in the codebase,
DEFAULT_DAILY_CAP_MICROS = 200 USD/dayfromlib/agentic-wallet/daily-spend.ts, and each sits deliberately under it rather than at it. The two native figures are denominated in the native asset, so their USD worth drifts upward with the market with nothing alerting; they are picked so drift cannot carry them past the anchor until ETH clears roughly 10,000 USD or SOL roughly 400 USD. The stablecoin figure is half the anchor because it is per transaction with no aggregate behind it. These values need ratifying before merge.The override column is currently aspirational. None of the three keys appear in
deploy/keeperhub-stack/prod/values.yamlor the staging equivalent, both of which enumerate their environment explicitly. Until they are wired in, widening a cap requires a code change and a release, so the "merge, watch, then widen" sequence below is not executable as written.When the default is applied because no row existed, it emits
spend_cap_default_appliedwith organization, surface, chain family, reason, default and reserved amounts, so the blast radius is observable in Loki before it bites.Verification
pnpm check0 errors,pnpm type-checkcleanBlast radius
This is the widest change of the four. Read it carefully.
reserveOrgValue/reserveOrgSolanaValue, so scheduled workflows moving native value draw on the same daily budget./api/execute/transfer: contract-call, protocol actions, check-and-execute, node execution, the web3 workflow steps and Tempo TIP-20 sends. Solana SPL transfers are NOT covered:loadStablecoinmatches hex addresses only, so an SPL mint never resolves and the Solana daily cap counts native lamports alone.failedand an error string, the same shape as an invalid token address. Callers that special-case 403 for cap denials will not match it.Recommended sequence: merge, watch
spend_cap_default_appliedfor a day, count distinct orgs withexceeded=true, then widen the env defaults before enforcement bites anyone real.Open decisions
approveis deliberately not capped.approve(attacker, MAX_UINT)is a complete drain path for a leaked key, since the attacker then callstransferFromoff-platform where we cannot see it. It is left uncapped because max-uint approve before a swap is a legitimate and common pattern, and capping it would break real workflows. Worth a separate decision.