Skip to content

fix: add transfer mutex to prevent TOCTOU race in transfer_credits (CWE-367) #171

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
quangtran88 wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix: add transfer mutex to prevent TOCTOU race in transfer_credits (CWE-367) #171

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3721d66
fix: add transfer mutex to prevent TOCTOU race in transfer_credits (C…
quangtran88 Feb 22, 2026
ba94c3c
fix: also wrap fund_child in withTransferLock (addresses review feedb…
quangtran88 Feb 22, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 45 additions & 21 deletions src/agent/tools.ts
View file
Open in desktop
Copy link
Contributor

@devin-ai-integration devin-ai-integration bot Feb 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 fund_child bypasses transfer mutex, preserving the TOCTOU race the PR aims to fix

The fund_child tool at src/agent/tools.ts:1416-1425 performs the same balance-check-then-transfer pattern as transfer_credits, but does not use withTransferLock. This means concurrent transfer_credits and fund_child calls (or two fund_child calls) can still race, defeating the purpose of the mutex.

Root Cause

The PR adds withTransferLock to transfer_credits (line 884) to serialize the balance check (getCreditsBalance()) and the actual transfer (transferCredits()). However, fund_child at lines 1416-1425 performs the exact same TOCTOU-vulnerable sequence without the lock:

// fund_child — no lock!
const balance = await ctx.conway.getCreditsBalance();  // line 1416
if (amount > balance / 2) { ... }                      // line 1417
const transfer = await ctx.conway.transferCredits(...); // line 1421

If transfer_credits and fund_child are called concurrently (e.g., the agent issues both tool calls in a single turn), both can read the same balance, both pass the "half balance" guard, and both transfers execute — potentially draining more than intended.

Impact: The self-preservation guard ("don't transfer more than half your balance") can be bypassed via concurrent fund_child + transfer_credits calls, which is exactly the class of bug this PR is supposed to fix.

(Refers to lines 1416-1425)

Prompt for agents 
In src/agent/tools.ts, the fund_child tool (around lines 1395-1455) performs a balance check followed by a credit transfer without using the withTransferLock mutex. Wrap the balance-check-and-transfer section (lines 1416-1455, from `const balance = await ctx.conway.getCreditsBalance()` through the return statement) inside a `withTransferLock(async () => { ... })` call, similar to how transfer_credits does it at line 884. The early validation checks (child lookup, wallet validation, status check, amount validation) can remain outside the lock since they don't involve the balance race. The return value of the execute function should be `return withTransferLock(async () => { ... })` encompassing the balance check, transfer, transaction recording, funded amount update, lifecycle transition, and return message.
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Contributor Author

@quangtran88 quangtran88 Feb 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch — fund_child had the same TOCTOU-vulnerable pattern.

Fixed in ba94c3c: wrapped the balance-check-through-return sequence in fund_child with withTransferLock, same as transfer_credits. CI is passing (typecheck + tests on Node 20 & 22).

Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,28 @@ const logger = createLogger("tools");
// Tools whose results come from external sources and need sanitization
const EXTERNAL_SOURCE_TOOLS = new Set(["exec", "web_fetch", "check_social_inbox"]);

/**
* Mutex for serializing credit transfer operations.
* Prevents TOCTOU race between balance check and transfer execution.
*/
let transferMutexPromise: Promise<void> = Promise.resolve();

function withTransferLock<T>(fn: () => Promise<T>): Promise<T> {
let release: () => void;
const nextLock = new Promise<void>((resolve) => {
release = resolve;
});
const currentLock = transferMutexPromise;
transferMutexPromise = nextLock;
return currentLock.then(async () => {
try {
return await fn();
} finally {
release!();
}
});
}

// ─── Self-Preservation Guard ───────────────────────────────────
// Defense-in-depth: policy engine (command.forbidden_patterns rule) is the primary guard.
// This inline check is kept as a secondary safety net in case the policy engine is bypassed.
Expand Down Expand Up @@ -859,30 +881,32 @@ 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.`;
}
return withTransferLock(async () => {
// 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.`;
}

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,
);

const { ulid } = await import("ulid");
ctx.db.insertTransaction({
id: ulid(),
type: "transfer_out",
amountCents: amount,
balanceAfterCents:
transfer.balanceAfterCents ?? Math.max(balance - amount, 0),
description: `Transfer to ${args.to_address}: ${args.reason || ""}`,
timestamp: new Date().toISOString(),
});
const { ulid } = await import("ulid");
ctx.db.insertTransaction({
id: ulid(),
type: "transfer_out",
amountCents: amount,
balanceAfterCents:
transfer.balanceAfterCents ?? Math.max(balance - amount, 0),
description: `Transfer to ${args.to_address}: ${args.reason || ""}`,
timestamp: new Date().toISOString(),
});

return `Credit transfer submitted: $${(amount / 100).toFixed(2)} to ${transfer.toAddress} (status: ${transfer.status}, id: ${transfer.transferId || "n/a"})`;
return `Credit transfer submitted: $${(amount / 100).toFixed(2)} to ${transfer.toAddress} (status: ${transfer.status}, id: ${transfer.transferId || "n/a"})`;
});
},
},

Expand Down