fix: reject negative escrow amount; clean up dead multi-same-out check#1031
Open
ohager wants to merge 3 commits into
Open
fix: reject negative escrow amount; clean up dead multi-same-out check#1031ohager wants to merge 3 commits into
ohager wants to merge 3 commits into
Conversation
Add an explicit non-negative guard for the escrow creation amount in ESCROW_CREATION.validateAttachment, consistent with the negative-value guards added for commitment and distribute-to-holders in #1028. A negative escrow amount nets to zero at payout (not a mint) but is an illegal value that should be rejected at validation on every ingress path, including raw byte broadcast. Also remove the dead/misleading recipient-count check in MULTI_SAME_OUT.validateAttachment (recipient bounds are already enforced when the attachment is parsed) and document why divisibility is intentionally NOT enforced: the per-recipient share is floor(amount/n) so any remainder is burned (never minted), and tightening this consensus rule would reject historically-accepted transactions on resync. Tests: - TransactionTypeEscrowTest: negative amount rejected, non-negative passes. - TransactionTypeMultiSameOutTest: characterizes that a non-divisible amount stays valid, preventing a future accidental consensus break. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
PR Size Check: PassedFiles: 3/10, Lines added: 151/1000 Protocol-Frozen Files ModifiedThe following consensus-critical files were changed. These require explicit maintainer approval and are only permitted for verified security fixes:
|
PR Size Check: PassedFiles: 3/10, Lines added: 151/1000 Protocol-Frozen Files ModifiedThe following consensus-critical files were changed. These require explicit maintainer approval and are only permitted for verified security fixes:
|
PR Size Check: PassedFiles: 4/10, Lines added: 156/1000 Protocol-Frozen Files ModifiedThe following consensus-critical files were changed. These require explicit maintainer approval and are only permitted for verified security fixes:
|
ohager
added
claude-review
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
1 participant
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
Two value-validation hardening changes found during a focused security review of integer-overflow / negative-value / value-minting risks. Both are in
TransactionType.javaand are consistent with the negative-value guards added in #1028.1. Reject negative escrow amount (defense-in-depth)
ESCROW_CREATION.validateAttachmentonly checked the aggregateamount + fee + signers·ONE_SIGNAagainst[0, MAX_BALANCE], so a small negativeamountNqt(read raw viabuffer.getLong()) could pass. It is not a mint — the negative nets to zero when the escrow is settled at its mandatory deadline action — but it is an illegal value that should be rejected at validation on every ingress path (API, raw broadcast, peer relay, block acceptance). Now guarded explicitly, mirroring the commitment / distribute-to-holders guards from #1028.2. Remove dead/misleading
MULTI_SAME_OUTcheckThe existing
if (size < 2 && amount % size == 0)check is dead (recipient count is already bounded to2..MAXat parse time) and misleadingly looks like a divisibility check. Enforcing divisibility was rejected as unsafe:validateAttachmentruns during block acceptance, and non-divisible multi-same-out transactions have been accepted on mainnet historically, so rejecting them would break resync (chain split) with no security benefit (the per-recipient share isfloor(amount/n), so the remainder is burned, never minted). The dead check is removed and the intentional behavior is documented.Tests
TransactionTypeEscrowTest— negative amount rejected, non-negative passes (written test-first, watched fail before the fix).TransactionTypeMultiSameOutTest— characterization test locking in that a non-divisible amount stays valid, to prevent a future accidental consensus break.Full unit +
chain.*integration suite passes locally.Reviewer note (consensus)
Change #1 tightens a consensus validation. As with the #1028 guards, it assumes no historical mainnet escrow ever carried a negative
amountNqt. Escrow is an old feature, so a quick chain scan to confirm this is worthwhile before merge.🤖 Generated with Claude Code