Security: close a critical injection and three broken invariants

An adversarial review found that three of eleven invariants did not hold as
written, and that in the generated workflow none of them were load-bearing at
all. Every case below was a working exploit before it was a test.

CRITICAL -- sync.mjs spliced the webhook's safeAddress raw into a JS source
string. A caller could close the string literal and append their own assemble()
declaration, which hoists over the real one and bypasses all eleven guards at
once, fully authoring the node's output. Verified against the real generated
workflow. Fixed twice over: the template is now substituted as a bare JSON
value rather than inside quotes, and a new gate-addr Condition rejects anything
that is not a 20-byte address before it reaches the code node at all.

I1 was broken. Nothing required the threshold to be >= 1, so threshold 0
returned executable:true with an empty signature blob -- and a NEGATIVE
threshold was worse, because slice(0, -5) counts from the end, so two real
signatures also became an empty blob. toBig accepted "-5" because BigInt does.
Counts now parse through toCount, which rejects negatives.

I7 was broken. It refused CONTRACT_SIGNATURE and APPROVED_HASH by reading
signatureType -- an optional field a hostile payload omits. Both schemes are
exactly 65 bytes, so they passed the shape check. v=0 is the dangerous one:
checkSignatures reads s as an offset and CALLS the address in r, an
attacker-controlled external call inside execTransaction. The guard now checks
the v byte, which cannot be omitted. The assertion already existed in
live-fixture.test.mjs; it just was not in the code.

I3 failed open. lower() maps undefined, null and "" to the same empty string,
so one stray "" in roster.json matched a missing safeAddress and opened the
gate for every run. Falsy entries are filtered and the address shape is checked
first.

drain.mjs had two gates that did not gate. Gate 2 printed "inner reverts" and
broadcast anyway -- real gas, nonce permanently consumed, nothing moved. Gate 3
asserted Response.ok, so a 2xx body carrying {success:false} passed what the
header calls a preflight. Both now stop. A refusal exits 3 so a wrapper can
tell it from an execution.

Also: the queue reads had no ordering, and the service defaults to newest-first
while the only executable entry is the LOWEST pending nonce -- so past 20
pending proposals gavel would have reported not-next-nonce forever, fail-closed
but silently wrong. The workflow's hydrate node was unauthenticated, which
would have turned a 401 into a plausible-looking empty queue. Malformed fields
now produce named refusals instead of throwing an undeclared tenth outcome.

Two tests were vacuous and are fixed. The case-insensitivity test compared an
all-1s address to itself and would have passed if matching were case-sensitive.
The test asserting the headline claim -- that the executor owns nothing --
compared the executor address against role handles like "O1", so it could never
fail; it now resolves roles to real addresses.

64 tests. The live blob still simulates SUCCESS against the real Safe.

Author: edycutjong

scripts/drain.mjs

@@ -84,7 +84,7 @@ console.log(`\n  ${chain.name} (${chainId}) — draining ${safeAddress}`);
 if (!chain.receiptsEligible) console.log(`  NOTE  receiptsEligible=false — nothing here may enter EVIDENCE.md.`);
 
 // ---- read: off-chain queue + on-chain state -------------------------------
-const res = await fetch(`${chain.txService}/safes/${safeAddress}/multisig-transactions/?executed=false&limit=20`,
+const res = await fetch(`${chain.txService}/safes/${safeAddress}/multisig-transactions/?executed=false&ordering=nonce&limit=20`,
   { headers: { Authorization: `Bearer ${safeKey}` } });
 if (!res.ok) { console.error(`  tx-service HTTP ${res.status} (addresses must be EIP-55 checksummed)`); process.exit(1); }
 const queue = await res.json();
@@ -99,7 +99,9 @@ const r = assemble({
 console.log(`  queue ${queue.count} · nonce ${nonce} · threshold ${threshold}`);
 console.log(`\n  [1/3] assemble  ${r.executable ? 'EXECUTABLE' : `REFUSED — ${r.reason}`}`);
 console.log(`        ${r.detail}`);
-if (!r.executable) process.exit(0);          // a named refusal is a success, not an error
+// A named refusal is correct behaviour, not an error -- but a wrapper must be
+// able to tell "refused" from "executed". Exit 3 is the refusal channel.
+if (!r.executable) process.exit(3);
 
 // ---- gate 2: local simulation ---------------------------------------------
 const executor = flags.executor && flags.executor !== true
@@ -113,7 +115,18 @@ try {
   const sim = await client.simulateContract({
     address: safeAddress, abi: EXEC_ABI, functionName: 'execTransaction', account: executor, args,
   });
-  console.log(`  [2/3] eth_call  ${sim.result === true ? 'SUCCEEDS' : 'outer ok, inner reverts (inner-call-failed)'}`);
+  if (sim.result !== true) {
+    // execTransaction returns success=false when the INNER call reverts. The outer
+    // call still succeeds whenever safeTxGas != 0 (Safe: require(success ||
+    // safeTxGas != 0 || gasPrice != 0)), so this does not throw -- it just moves
+    // nothing while permanently consuming the Safe nonce and burning real gas.
+    // Printing it and broadcasting anyway was the bug.
+    console.log(`  [2/3] eth_call  INNER CALL REVERTS -- would consume the nonce and move nothing`);
+    console.log(`        refusing to broadcast. This is the inner-call-failed / GS013 shape;`);
+    console.log(`        to produce it deliberately, use BENCH_GS013 rather than a live payout.`);
+    process.exit(1);
+  }
+  console.log(`  [2/3] eth_call  SUCCEEDS`);
 } catch (e) {
   const msg = String(e.shortMessage || e.message || e).split('\n')[0];
   console.log(`  [2/3] eth_call  REVERTS — ${msg.slice(0, 120)}`);
@@ -141,8 +154,16 @@ const call = (payload) => fetch('https://app.keeperhub.com/api/execute/contract-
 }).then(async (x) => ({ ok: x.ok, status: x.status, json: await x.json().catch(() => ({})) }));
 
 const pre = await call({ ...body, simulate: true });
-console.log(`  [3/3] KeeperHub simulate  HTTP ${pre.status} ${pre.ok ? 'OK' : 'FAILED'}`);
-if (!pre.ok) { console.log(`        ${JSON.stringify(pre.json).slice(0, 300)}`); process.exit(1); }
+// pre.ok is Response.ok -- transport success. A 2xx body carrying
+// {success:false, revertReason:"GS026"} would have sailed through the "third
+// gate", which made it a liveness check on the API rather than a preflight on
+// the transaction. Inspect the body.
+const preBad = pre.json?.success === false || pre.json?.error || pre.json?.revertReason;
+console.log(`  [3/3] KeeperHub simulate  HTTP ${pre.status} ${pre.ok && !preBad ? 'OK' : 'FAILED'}`);
+if (!pre.ok || preBad) {
+  console.log(`        ${JSON.stringify(pre.json).slice(0, 300)}`);
+  process.exit(1);
+}
 
 if (!flags.execute) {
   console.log(`\n  All three gates passed. Dry run — re-run with --execute to broadcast.\n`);

scripts/sync.mjs

@@ -74,9 +74,20 @@ const VIEW_ABI = (name, outType) => JSON.stringify([
  */
 function buildCodeBody() {
   const src = readFileSync(join(ROOT, 'src', 'assemble.mjs'), 'utf8');
-  if (/^\s*import\s/m.test(src)) {
-    console.error('assemble.mjs contains an import — it must stay dependency-free (invariant I10).');
-    process.exit(1);
+  // I10 must fail LOUDLY here, never silently in the sandbox. Static imports,
+  // dynamic import(), and the eval-family are all rejected.
+  for (const [re, what] of [
+    [/^\s*import\s/m, 'a static import'],
+    [/\bimport\s*\(/, 'a dynamic import()'],
+    [/\beval\s*\(/, 'eval()'],
+    [/\bnew\s+Function\s*\(/, 'new Function()'],
+    [/\bMath\.random\s*\(/, 'Math.random()'],
+    [/\bDate\.now\s*\(/, 'Date.now()'],
+  ]) {
+    if (re.test(src)) {
+      console.error(`assemble.mjs contains ${what} — it must stay pure and dependency-free (I10).`);
+      process.exit(1);
+    }
   }
   const stripped = src
     .replace(/^export default assemble;\s*$/m, '')
@@ -94,11 +105,24 @@ function buildCodeBody() {
 // caller-open door and the first check is bypassable through it.
 const ROSTER = ${roster};
 
+// The webhook value is substituted as a JSON VALUE, never inside a string
+// literal. Splicing it between quotes let a caller close the string and append
+// their own assemble() declaration, which hoists over the real one and
+// bypasses every guard at once. gate-addr below rejects non-addresses before
+// this node runs; this line makes the splice non-syntactic regardless.
+const __safeAddress = {{@trigger-1:Webhook.safeAddress}};
 const __hydrated = {{@hydrate-1:Hydrate Signatures.data}};
 const __transactions = (__hydrated && __hydrated.results) ? __hydrated.results : [];
 
+// Belt and braces: assemble refuses a malformed address anyway, but a value that
+// is not a string should never reach it from here.
+if (typeof __safeAddress !== "string") {
+  return { executable: false, reason: "malformed-payload",
+           detail: "safeAddress was not a string", signatures: "" };
+}
+
 return assemble({
-  safeAddress: "{{@trigger-1:Webhook.safeAddress}}",
+  safeAddress: __safeAddress,
   roster: ROSTER,
   queue: { transactions: __transactions, count: __transactions.length },
   onchainNonce: ${chainId === '8453'
@@ -160,17 +184,26 @@ if (!web3Integration || web3Integration === true) {
 const nodes = [
   node('trigger-1', 'Webhook', 'trigger', { triggerType: 'Webhook' }, 0, 0),
 
+  // The first thing that touches caller input. A webhook (or the public
+  // Marketplace listing) can send anything; nothing downstream should have to
+  // assume otherwise. Rejecting non-addresses here means the injection vector in
+  // the Code node is closed at the boundary as well as at the splice.
+  node('gate-addr', 'Valid Address', 'action', {
+    actionType: 'Condition',
+    condition: '{{@trigger-1:Webhook.safeAddress}} matchesRegex ^0x[0-9a-fA-F]{40}$',
+  }, 250, 0),
+
   node('queue-1', 'Safe Queue', 'action', {
     actionType: 'safe/get-pending-transactions', network: chainId,
     safeAddress: SAFE_ADDR, integrationId: safeIntegration,
-  }, 250, 0),
+  }, 500, 0),
 
   // Δ1 (RATIFIED, complexity.md §4a). Without it every idle sweep runs three
   // on-chain reads and the Code node, then posts "skipped: no work" — ~720 idle
   // messages a day across a 5-Safe roster, and the live feed stops being readable.
   node('gate-0', 'Has Work', 'action', {
     actionType: 'Condition', condition: '{{@queue-1:Safe Queue.count}} > 0',
-  }, 500, 0),
+  }, 750, 0),
 
   ...readNodes(),
 
@@ -179,8 +212,17 @@ const nodes = [
   // arguments, two of which are the hostile-refund vector (DX-1). Verified live.
   node('hydrate-1', 'Hydrate Signatures', 'action', {
     actionType: 'HTTP Request', httpMethod: 'GET',
-    endpoint: `${chain.txService}/safes/${SAFE_ADDR}/multisig-transactions/?executed=false&limit=20`,
-    timeout: 10, failOnError: false,
+    // ordering=nonce is load-bearing: the service defaults to -nonce (newest
+    // first), and the only executable entry is the LOWEST pending nonce. On a
+    // Safe with >20 pending proposals it would fall off page 1 and gavel would
+    // report not-next-nonce forever — fail-closed, but silently and wrongly.
+    endpoint: `${chain.txService}/safes/${SAFE_ADDR}/multisig-transactions/?executed=false&ordering=nonce&limit=20`,
+    httpHeaders: JSON.stringify({ Authorization: 'Bearer ${SAFE_TX_SERVICE_JWT}' }),
+    timeout: 10,
+    // failOnError:false keeps a transient outage from killing the run. The cost is
+    // that a 401 becomes an empty queue and a plausible-looking not-next-nonce, so
+    // the header above is what stops that being a silent permanent stall.
+    failOnError: false,
   }, 1500, 0),
 
   node('assemble-1', 'Assemble', 'action', {
@@ -204,7 +246,10 @@ const nodes = [
 ];
 
 const edges = [
-  { id: 'e-trigger-queue', source: 'trigger-1', target: 'queue-1' },
+  { id: 'e-trigger-gateaddr', source: 'trigger-1', target: 'gate-addr' },
+  // false branch is deliberately dangling: a malformed address ends the run
+  // silently rather than posting noise to the live feed.
+  { id: 'e-gateaddr-queue', source: 'gate-addr', target: 'queue-1', sourceHandle: 'true' },
   { id: 'e-queue-gate0', source: 'queue-1', target: 'gate-0' },
   { id: 'e-gate0-threshold', source: 'gate-0', target: 'threshold-1', sourceHandle: 'true' },
   { id: 'e-threshold-owners', source: 'threshold-1', target: 'owners-1' },

src/assemble.mjs

@@ -61,13 +61,33 @@ export const PRE_BROADCAST_REFUSALS = Object.freeze(REFUSALS.slice(0, 7));
  *                      would silently disarm the single most dangerous guard in the
  *                      design. Refuse instead.
  */
-export const SECURITY_REFUSALS = Object.freeze(['not-on-roster', 'incomplete-payload']);
+export const SECURITY_REFUSALS = Object.freeze([
+  'not-on-roster', 'incomplete-payload', 'malformed-payload',
+]);
 
 const ZERO_ADDRESS = '0x0000000000000000000000000000000000000000';
+const ADDRESS_RE = /^0x[0-9a-fA-F]{40}$/;
 
 /** Signature types we refuse outright (invariant I7). */
 const REFUSED_SIGNATURE_TYPES = Object.freeze(['CONTRACT_SIGNATURE', 'APPROVED_HASH']);
 
+/**
+ * The ONLY `v` values checkSignatures treats as an ECDSA signature.
+ *   27/28 — EIP-712 (eth_signTypedData)
+ *   31/32 — eth_sign, i.e. 27/28 + 4
+ *
+ * Everything else is a different scheme wearing 65 bytes:
+ *   v = 0  CONTRACT_SIGNATURE. `s` is an offset into the blob and `r` is a
+ *          contract address that checkSignatures CALLS (EIP-1271). That is an
+ *          attacker-controlled external call from inside execTransaction.
+ *   v = 1  APPROVED_HASH. `r` is an owner address and no signature is verified.
+ *
+ * I7 previously refused these by reading `signatureType`, a field the service
+ * supplies and an attacker-fed payload can simply omit. This set is checked on
+ * the BYTES, which cannot be omitted.
+ */
+const PASSTHROUGH_V = Object.freeze([27, 28, 31, 32]);
+
 /** The five fields the Safe plugin does not return; hydrate-1 must supply them. */
 const HYDRATED_FIELDS = Object.freeze([
   'safeTxGas', 'baseGas', 'gasPrice', 'gasToken', 'refundReceiver',
@@ -104,6 +124,29 @@ function isWellFormedSignature(sig) {
   return typeof sig === 'string' && /^0x[0-9a-fA-F]{130}$/.test(sig.trim());
 }
 
+/** The trailing byte of a 65-byte signature, as a number. */
+function signatureV(sig) {
+  return parseInt(String(sig).trim().slice(-2), 16);
+}
+
+/**
+ * A non-negative count, or null when the value is not one.
+ *
+ * `toBig` accepts "-5" happily, because BigInt does. A negative threshold made
+ * `signerCount < required` false and then `slice(0, -5)` counted from the END and
+ * returned an empty array — producing `executable: true` with an EMPTY signature
+ * blob. Counts get their own parser so that cannot recur.
+ */
+function toCount(value) {
+  try {
+    const n = toBig(value, 'count');
+    if (n < 0n || n > 1000n) return null;
+    return Number(n);
+  } catch {
+    return null;
+  }
+}
+
 function refuse(reason, detail, provenance = {}) {
   return {
     executable: false,
@@ -140,23 +183,49 @@ export function assemble(input) {
     onchainOwners = [],
   } = input ?? {};
 
-  const transactions = Array.isArray(queue.transactions) ? queue.transactions : [];
+  // ---- input validation ----------------------------------------------------
+  // Every guard below assumes well-formed inputs. Rather than trusting that, the
+  // malformed cases become a NAMED refusal here. Previously several of them threw
+  // out of the function instead, which in the workflow is an errored Code node —
+  // an undeclared tenth outcome whose effect on the downstream gate is untested.
+  const transactions = Array.isArray(queue?.transactions) ? queue.transactions : [];
   const queueDepth = transactions.length;
 
-  const nonceOnchain = toBig(onchainNonce, 'onchainNonce');
-  const thresholdOnchain = Number(toBig(onchainThreshold, 'onchainThreshold'));
-  const owners = onchainOwners.map(lower);
-
+  const nonceOnchain = toCount(onchainNonce);
+  const thresholdOnchain = toCount(onchainThreshold);
   const base = {
-    onchainNonce: nonceOnchain.toString(),
-    threshold: thresholdOnchain,
+    onchainNonce: nonceOnchain === null ? '' : String(nonceOnchain),
+    threshold: thresholdOnchain ?? 0,
     queueDepth,
   };
 
+  if (!ADDRESS_RE.test(String(safeAddress ?? ''))) {
+    return refuse('malformed-payload', `safeAddress ${safeAddress} is not a 20-byte address.`, base);
+  }
+  if (nonceOnchain === null) {
+    return refuse('malformed-payload', `onchainNonce ${onchainNonce} is not a valid count.`, base);
+  }
+  // A threshold of 0 is not a Safe. Treating it as one made an EMPTY signature
+  // blob executable, which is the failure this check exists for.
+  if (thresholdOnchain === null || thresholdOnchain < 1) {
+    return refuse('malformed-payload', `onchainThreshold ${onchainThreshold} is not >= 1.`, base);
+  }
+  if (!Array.isArray(onchainOwners) || onchainOwners.length === 0) {
+    return refuse('malformed-payload', 'onchainOwners is empty or not an array.', base);
+  }
+  const owners = onchainOwners.map(lower);
+
   // ---- I3 · roster ---------------------------------------------------------
   // Checked here and not only in roster-1, because the Marketplace listing is a
   // caller-open door and roster-1 is bypassable through it.
-  if (!roster.map(lower).includes(lower(safeAddress))) {
+  //
+  // Falsy entries are filtered BEFORE the membership test. lower() maps undefined,
+  // null and "" all to "", so a single stray "" in roster.json used to match a
+  // missing safeAddress and open the gate for every run.
+  const rosterSet = (Array.isArray(roster) ? roster : [])
+    .filter((a) => ADDRESS_RE.test(String(a ?? '')))
+    .map(lower);
+  if (!rosterSet.includes(lower(safeAddress))) {
     return refuse(
       'not-on-roster',
       `Safe ${safeAddress} is not on the opt-in roster; refusing to execute against it.`,
@@ -168,13 +237,7 @@ export function assemble(input) {
   // The service permits two DIFFERENT proposals at one nonce — that is how
   // "replace transaction" works. Choosing between them is a policy call gavel is
   // not entitled to make, so two candidates refuses just as firmly as zero.
-  const candidates = transactions.filter((tx) => {
-    try {
-      return toBig(tx?.nonce, 'tx.nonce') === nonceOnchain;
-    } catch {
-      return false;
-    }
-  });
+  const candidates = transactions.filter((tx) => toCount(tx?.nonce) === nonceOnchain);
   const candidateCount = candidates.length;
   const prov = { ...base, candidateCount };
 
@@ -196,7 +259,7 @@ export function assemble(input) {
   }
 
   const tx = candidates[0];
-  const withNonce = { ...prov, nonce: toBig(tx.nonce, 'tx.nonce').toString(), safeTxHash: String(tx.safeTxHash ?? '') };
+  const withNonce = { ...prov, nonce: String(toCount(tx.nonce)), safeTxHash: String(tx.safeTxHash ?? '') };
 
   // ---- payload completeness (precondition for I4) --------------------------
   const missing = HYDRATED_FIELDS.filter((f) => tx[f] === undefined || tx[f] === null || tx[f] === '');
@@ -224,7 +287,15 @@ export function assemble(input) {
   // The worst outcome in the design: execTransaction pays the refund from the
   // Safe to refundReceiver and CALLS gasToken, so a hostile token can re-enter.
   // The party this is aimed at is whoever executes — us.
-  const gasPrice = toBig(tx.gasPrice, 'tx.gasPrice');
+  // Parsed fail-closed: an unparseable gasPrice is refused rather than thrown.
+  // This is the guard against the drain vector, so "I could not read it" and
+  // "it was hostile" must reach the same outcome.
+  let gasPrice = null;
+  try { gasPrice = toBig(tx.gasPrice, 'tx.gasPrice'); } catch { gasPrice = null; }
+  if (gasPrice === null) {
+    return refuse('malformed-payload',
+      `gasPrice="${tx.gasPrice}" is not integer-like; refusing rather than assuming zero.`, withNonce);
+  }
   if (gasPrice !== 0n || !isZeroAddress(tx.gasToken) || !isZeroAddress(tx.refundReceiver)) {
     return refuse(
       'refund-requested',
@@ -253,6 +324,21 @@ export function assemble(input) {
     );
   }
 
+  // The check that actually holds. signatureType is an OPTIONAL off-chain label and
+  // a hostile payload simply omits it; v is in the bytes and cannot be. v=0 is an
+  // EIP-1271 contract signature, where checkSignatures CALLS the address in `r` --
+  // an attacker-controlled external call from inside execTransaction. v=1 is an
+  // approved-hash, where nothing is verified at all. Both are exactly 65 bytes.
+  const badV = confirmations.find((c) => !PASSTHROUGH_V.includes(signatureV(c.signature)));
+  if (badV) {
+    const v = signatureV(badV.signature);
+    return refuse(
+      'eip1271-unsupported',
+      `Confirmation from ${badV.owner} has v=${v}; only EOA/eth_sign (27, 28, 31, 32) are spliced.`,
+      withNonce,
+    );
+  }
+
   // ---- I6 · every confirming owner is STILL an owner ----------------------
   // The silent GS026: a signature collected legitimately last week from an owner
   // removed yesterday. Naive tooling broadcasts it.
@@ -277,8 +363,13 @@ export function assemble(input) {
   // ---- I1 · never execute below threshold ---------------------------------
   // Required is the MAX of the queue's cached value and the live on-chain read;
   // trusting the cached one alone is the silent failure this guard exists for.
-  const thresholdQueue = Number(toBig(tx.confirmationsRequired ?? thresholdOnchain, 'tx.confirmationsRequired'));
+  const thresholdQueue = toCount(tx.confirmationsRequired) ?? thresholdOnchain;
   const required = Math.max(thresholdQueue, thresholdOnchain);
+  // thresholdOnchain is already >= 1, so required is too. Asserted anyway: this is
+  // the line whose absence made an empty blob executable, and it costs nothing.
+  if (!Number.isInteger(required) || required < 1) {
+    return refuse('malformed-payload', `computed threshold ${required} is not >= 1.`, withSigners);
+  }
 
   if (signerCount < required) {
     // Distinguish a plain shortfall from the drift case, where the queue believed
@@ -293,6 +384,18 @@ export function assemble(input) {
   }
 
   // Exactly `required` signatures, ascending — checkSignatures needs no more.
+  if (!ADDRESS_RE.test(String(tx.to ?? ''))) {
+    return refuse('malformed-payload', `to ${tx.to} is not a 20-byte address.`, withSigners);
+  }
+  // Every remaining field is coerced below. Any that is not integer-like would
+  // throw out of the function and become an errored code node rather than a named
+  // outcome, so they are checked here while a refusal is still possible.
+  for (const f of ['value', 'safeTxGas', 'baseGas']) {
+    try { toBig(tx[f] ?? 0, f); } catch {
+      return refuse('malformed-payload', `${f}="${tx[f]}" is not integer-like.`, withSigners);
+    }
+  }
+
   const chosen = distinct.slice(0, required);
   const signatures = '0x' + chosen.map((c) => c.signature.trim().replace(/^0x/, '')).join('');