diff --git a/src/__tests__/tools-security.test.ts b/src/__tests__/tools-security.test.ts index 6a9eecd7..304caacb 100644 --- a/src/__tests__/tools-security.test.ts +++ b/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 ─────────────────────────────────────── diff --git a/src/agent/tools.ts b/src/agent/tools.ts index 0d2db864..8f3b254b 100644 --- a/src/agent/tools.ts +++ b/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 = Promise.resolve(); + +function withCreditTransferLock(fn: () => Promise): Promise { + 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({