fix: serialize balance-check-and-transfer to close TOCTOU race (CWE-367)#343
Open
Matthew-Selvam wants to merge 1 commit into
Open
fix: serialize balance-check-and-transfer to close TOCTOU race (CWE-367)#343Matthew-Selvam wants to merge 1 commit into
Matthew-Selvam wants to merge 1 commit into
Conversation
transfer_credits and fund_child both check getCreditsBalance() and then call transferCredits() as two separate awaits, with no atomicity between them. Two concurrent calls can each read the same starting balance, both pass the 'no more than half' guard, and together transfer more than the policy intends. Adds a per-process credit transfer mutex (a simple promise chain) that serializes the balance-check-and-transfer critical section across both tools, so a second concurrent call re-checks against the post-transfer balance rather than a stale one. A failed transfer doesn't wedge the lock for subsequent callers. Server-side atomic transfers (noted in the issue as the ideal long-term fix) are out of scope here since that lives in Conway's backend, not this client. Adds a regression test simulating two concurrent $40 transfers against a $100 balance: exactly one should succeed and the other should be blocked once the balance drops to $60 (where $40 exceeds the new half- balance limit of $30). Fixes Conway-Research#177
Author
|
This is ready for maintainer review whenever you get a chance — happy to make any changes requested. |
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
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.
Summary
Fixes #177.
transfer_credits(and the identical pattern infund_child) checkgetCreditsBalance()and calltransferCredits()as two separateawaits with no atomicity between them:Fix
Adds a per-agent-process credit transfer mutex (
withCreditTransferLock, a simple serialized promise chain) around the balance-check-and-transfer critical section in bothtransfer_creditsandfund_child, so a second concurrent call re-checks the guard against the post-transfer balance instead of a stale one. A failed transfer inside the lock doesn't wedge it for later callers.As the issue notes, a server-side atomic transfer would be the ideal long-term fix — that's out of scope here since it lives in Conway's backend, not this client. This closes the client-side race in the meantime.
Testing
tsc --noEmitpasses.tools-security.test.ts(72/72) andauthority-rules.test.ts(36/36, coversfund_child) pass.