fix: serialize balance-check-and-transfer to close TOCTOU race (CWE-367)

src/__tests__/tools-security.test.ts

@@ -566,6 +566,27 @@ describe("transfer_credits self-preservation", () => {
     expect(result).toContain("Blocked");
     expect(result).toContain("positive number");
   });
+
+  it("serializes concurrent transfers instead of racing on a stale balance (CWE-367)", async () => {
+    // $100 balance, two concurrent $40 transfers. The guard is "no more
+    // than half the CURRENT balance": once the first transfer succeeds,
+    // balance is $60, so the second $40 request (> $30, half of $60) must
+    // be blocked — not silently allowed against the stale $100 balance
+    // both requests would see without serialization.
+    const transferTool = tools.find((t) => t.name === "transfer_credits")!;
+    const [first, second] = await Promise.all([
+      transferTool.execute({ to_address: "0xa", amount_cents: 4000 }, ctx),
+      transferTool.execute({ to_address: "0xb", amount_cents: 4000 }, ctx),
+    ]);
+
+    const results = [first, second];
+    const succeeded = results.filter((r) => r.includes("transfer submitted"));
+    const blocked = results.filter((r) => r.includes("Blocked"));
+
+    expect(succeeded).toHaveLength(1);
+    expect(blocked).toHaveLength(1);
+    expect(conway.creditsCents).toBe(6000);
+  });
 });
 
 // ─── Tool Category Checks ───────────────────────────────────────

src/agent/tools.ts

@@ -25,6 +25,24 @@ import { createLogger } from "../observability/logger.js";
 
 const logger = createLogger("tools");
 
+// ─── Credit Transfer Mutex ─────────────────────────────────────
+// Serializes balance-check-and-transfer critical sections (transfer_credits,
+// fund_child) to close a TOCTOU race (CWE-367): without this, two concurrent
+// transfers can each read the same starting balance, both pass the "no more
+// than half" guard, and together exceed it.
+let creditTransferLock: Promise<unknown> = Promise.resolve();
+
+function withCreditTransferLock<T>(fn: () => Promise<T>): Promise<T> {
+  const run = creditTransferLock.then(fn);
+  // The shared chain always resolves, even if this call failed, so a
+  // failed transfer doesn't permanently wedge the lock for later callers.
+  creditTransferLock = run.then(
+    () => undefined,
+    () => undefined,
+  );
+  return run;
+}
+
 // ─── Path Confinement ─────────────────────────────────────────
 // write_file is restricted to the sandbox home directory tree.
 // The sandbox home is /root for both local and remote execution.
@@ -1015,17 +1033,30 @@ Model: ${ctx.inference.getDefaultModel()}
           return `Blocked: amount_cents must be a positive number, got ${amount}.`;
         }
 
-        // Guard: don't transfer more than half your balance
-        const balance = await ctx.conway.getCreditsBalance();
-        if (amount > balance / 2) {
-          return `Blocked: Cannot transfer more than half your balance ($${(balance / 100).toFixed(2)}). Self-preservation.`;
-        }
+        // Guard: don't transfer more than half your balance.
+        // The balance-check-and-transfer below must be atomic — otherwise
+        // two concurrent transfers can each pass the guard against the same
+        // starting balance and together exceed it (CWE-367, TOCTOU).
+        const outcome = await withCreditTransferLock(async () => {
+          const balance = await ctx.conway.getCreditsBalance();
+          if (amount > balance / 2) {
+            return {
+              blocked: `Blocked: Cannot transfer more than half your balance ($${(balance / 100).toFixed(2)}). Self-preservation.`,
+            } as const;
+          }
 
-        const transfer = await ctx.conway.transferCredits(
-          args.to_address as string,
-          amount,
-          args.reason as string | undefined,
-        );
+          const transfer = await ctx.conway.transferCredits(
+            args.to_address as string,
+            amount,
+            args.reason as string | undefined,
+          );
+          return { blocked: undefined, balance, transfer };
+        });
+
+        if (outcome.blocked) {
+          return outcome.blocked;
+        }
+        const { balance, transfer } = outcome;
 
         const { ulid } = await import("ulid");
         ctx.db.insertTransaction({
@@ -1754,16 +1785,28 @@ Model: ${ctx.inference.getDefaultModel()}
           return `Blocked: amount_cents must be a positive number, got ${amount}.`;
         }
 
-        const balance = await ctx.conway.getCreditsBalance();
-        if (amount > balance / 2) {
-          return `Blocked: Cannot transfer more than half your balance. Self-preservation.`;
-        }
+        // Balance-check-and-transfer must be atomic — see transfer_credits
+        // for the TOCTOU rationale (CWE-367).
+        const outcome = await withCreditTransferLock(async () => {
+          const balance = await ctx.conway.getCreditsBalance();
+          if (amount > balance / 2) {
+            return {
+              blocked: `Blocked: Cannot transfer more than half your balance. Self-preservation.`,
+            } as const;
+          }
 
-        const transfer = await ctx.conway.transferCredits(
-          child.address,
-          amount,
-          `fund child ${child.id}`,
-        );
+          const transfer = await ctx.conway.transferCredits(
+            child.address,
+            amount,
+            `fund child ${child.id}`,
+          );
+          return { blocked: undefined, balance, transfer };
+        });
+
+        if (outcome.blocked) {
+          return outcome.blocked;
+        }
+        const { balance, transfer } = outcome;
 
         const { ulid } = await import("ulid");
         ctx.db.insertTransaction({