From b3b6ef1243aecdb95255d5dab9a57aea933ccda7 Mon Sep 17 00:00:00 2001 From: woahwhattheheck Date: Wed, 7 Oct 2026 13:02:47 -0400 Subject: [PATCH 1/2] fix: reject invalid runtime split ratios --- src/payments/splitExecutor.ts | 70 +++++++++++++++++++++++++---------- 1 file changed, 51 insertions(+), 19 deletions(-) diff --git a/src/payments/splitExecutor.ts b/src/payments/splitExecutor.ts index e13c0ee..9752ab1 100644 --- a/src/payments/splitExecutor.ts +++ b/src/payments/splitExecutor.ts @@ -10,10 +10,10 @@ * entirely without altering any other pre-flight behaviour. * * Issue #778 — when recipients declare a `ratio`, the executor first verifies - * that the ratios sum to exactly 1.0 (within {@link SPLIT_RATIO_TOLERANCE}) - * and throws {@link SplitRatioSumError} otherwise, so floating-point rounding - * errors or user input mistakes can never silently over- or underpay a split. - * Splits without ratios behave exactly as before. + * that every declared ratio is a finite fraction in `[0, 1]` and that the + * ratios sum to exactly 1.0 (within {@link SPLIT_RATIO_TOLERANCE}), throwing + * {@link SplitRatioSumError} otherwise. Splits without ratios behave exactly + * as before. */ import { checkSubentryCapacity, SubentryCapacityGuardError } from "../account/subentryGuard.js"; @@ -35,12 +35,14 @@ export const SPLIT_RATIO_TOLERANCE = 1e-9; const SPLIT_RATIO_EXPECTED_SUM = 1; /** - * Thrown when the sum of all recipient ratios does not equal 1.0 within + * Thrown when a declared recipient ratio is not a finite fraction in `[0, 1]`, + * or when the sum of all recipient ratios does not equal 1.0 within * {@link SPLIT_RATIO_TOLERANCE}. The offending sum is exposed on - * {@link SplitRatioSumError.actualSum} so callers can report the exact drift. + * {@link SplitRatioSumError.actualSum}; malformed runtime ratio values that + * prevent a numeric total report `NaN`. */ export class SplitRatioSumError extends StellarSplitError { - /** The sum of all declared recipient ratios. */ + /** The sum of all declared recipient ratios, or NaN for malformed runtime values. */ readonly actualSum: number; /** The sum the ratios were expected to add up to (always `1`). */ readonly expectedSum: number; @@ -82,7 +84,8 @@ export function sumRecipientRatios(recipients: SplitRecipient[]): number { } /** - * Assert that the declared recipient ratios sum to 1.0 within `tolerance`. + * Assert that declared recipient ratios are finite fractions in `[0, 1]` and + * sum to 1.0 within `tolerance`. * * @param recipients - Recipient legs of the split; each should declare `ratio`. * @param tolerance - Absolute tolerance for the comparison. @@ -90,16 +93,44 @@ export function sumRecipientRatios(recipients: SplitRecipient[]): number { * * @returns The actual sum of the declared ratios (useful for logging/metrics). * - * @throws {SplitRatioSumError} When `|sum - 1| > tolerance`. + * @throws {SplitRatioSumError} When a declared ratio is non-numeric, + * non-finite, outside `[0, 1]`, or when the numeric sum is outside the + * allowed tolerance. */ export function validateSplitRatioSum( recipients: SplitRecipient[], tolerance: number = SPLIT_RATIO_TOLERANCE, ): number { - const actualSum = sumRecipientRatios(recipients); - if (Math.abs(actualSum - SPLIT_RATIO_EXPECTED_SUM) > tolerance) { + let actualSum = 0; + let hasInvalidRatio = false; + + for (const recipient of recipients) { + const ratio: unknown = recipient.ratio; + if (ratio === undefined) { + continue; + } + + if (typeof ratio !== "number" || !Number.isFinite(ratio)) { + hasInvalidRatio = true; + actualSum = Number.NaN; + break; + } + + actualSum += ratio; + if (ratio < 0 || ratio > 1) { + hasInvalidRatio = true; + } + } + + // Negating <= is deliberate: unlike a plain "> tolerance" comparison it + // fails closed if future arithmetic ever produces NaN. + if ( + hasInvalidRatio || + !(Math.abs(actualSum - SPLIT_RATIO_EXPECTED_SUM) <= tolerance) + ) { throw new SplitRatioSumError(actualSum, SPLIT_RATIO_EXPECTED_SUM, tolerance); } + return actualSum; } @@ -120,8 +151,8 @@ export interface SplitRecipient { */ requiredSlots?: number; /** - * The recipient's share of the split as a decimal ratio (a fraction of the - * total, e.g. `0.25` for 25 %). + * The recipient's share of the split as a finite decimal fraction in + * `[0, 1]` (e.g. `0.25` for 25 %). * * When at least one recipient declares a `ratio`, every recipient must do so * and the ratios must sum to 1.0 within {@link SPLIT_RATIO_TOLERANCE}; @@ -167,16 +198,17 @@ export interface SplitExecutionResult { * Executes a multi-recipient split payment after running subentry capacity * pre-flight checks for each recipient. * - * When any recipient declares a `ratio`, the ratios are validated to sum to - * 1.0 (within {@link SPLIT_RATIO_TOLERANCE}) before any transaction is built. + * When any recipient declares a `ratio`, every declared ratio is validated as + * a finite fraction in `[0, 1]` and the ratios are validated to sum to 1.0 + * (within {@link SPLIT_RATIO_TOLERANCE}) before any transaction is built. * * @param recipients - Array of recipient addresses, amounts, and required slots. * @param options - Execution options including the opt-out skip flag and Horizon URL. * * @returns {@link SplitExecutionResult} with capacity check outcomes. * - * @throws {SplitRatioSumError} When recipient ratios are declared but do not - * sum to 1.0 within {@link SPLIT_RATIO_TOLERANCE}. + * @throws {SplitRatioSumError} When recipient ratios are declared but any ratio + * is invalid or their sum is outside {@link SPLIT_RATIO_TOLERANCE}. * @throws {SubentryCapacityGuardError} When any recipient's account cannot * accommodate the required subentry slots and `skipCapacityCheck` is not set. * @@ -211,8 +243,8 @@ export async function splitExecutor( horizonUrl = "https://horizon.stellar.org", } = options; - // Issue #778 — fail fast on malformed ratios instead of processing (and - // paying out) a split whose shares silently over- or underpay the total. + // Issue #778 / #1012 — fail fast on malformed ratios instead of processing + // a split whose shares silently over- or underpay the total. if (recipients.some((recipient) => recipient.ratio !== undefined)) { validateSplitRatioSum(recipients); } From d6c8b9fa4e6b737b879c6140bcee7ccd9c8c2a5b Mon Sep 17 00:00:00 2001 From: woahwhattheheck Date: Wed, 7 Oct 2026 13:03:19 -0400 Subject: [PATCH 2/2] test: cover hostile split ratio values --- test/splitExecutor.invalid-ratios.test.ts | 121 ++++++++++++++++++++++ 1 file changed, 121 insertions(+) create mode 100644 test/splitExecutor.invalid-ratios.test.ts diff --git a/test/splitExecutor.invalid-ratios.test.ts b/test/splitExecutor.invalid-ratios.test.ts new file mode 100644 index 0000000..ebf0420 --- /dev/null +++ b/test/splitExecutor.invalid-ratios.test.ts @@ -0,0 +1,121 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; +import { checkSubentryCapacity } from "../src/account/subentryGuard.js"; +import { + splitExecutor, + SplitRatioSumError, + SPLIT_RATIO_TOLERANCE, + validateSplitRatioSum, + type SplitRecipient, +} from "../src/payments/splitExecutor.js"; + +vi.mock("../src/account/subentryGuard.js", () => ({ + checkSubentryCapacity: vi.fn().mockResolvedValue({ available: 1, limit: 1 }), + SubentryCapacityGuardError: class SubentryCapacityGuardError extends Error {}, +})); + +const A = "GABC00000000000000000000000000000000000000000000000000AA"; +const B = "GDEF00000000000000000000000000000000000000000000000000BB"; + +const r = (address: string, ratio?: unknown): SplitRecipient => ({ + address, + amount: 1_000_000n, + ...(ratio === undefined ? {} : { ratio: ratio as number }), +}); + +afterEach(() => { + vi.clearAllMocks(); +}); + +describe("validateSplitRatioSum hostile runtime inputs", () => { + it("rejects out-of-range ratios even when their aggregate equals 1", () => { + expect(() => validateSplitRatioSum([r(A, -1), r(B, 2)])).toThrow( + SplitRatioSumError, + ); + }); + + it.each([ + ["NaN", Number.NaN, 1], + ["positive infinity", Number.POSITIVE_INFINITY, 0], + ["opposing infinities", Number.POSITIVE_INFINITY, Number.NEGATIVE_INFINITY], + ])("rejects non-finite %s ratios", (_label, a, b) => { + expect(() => validateSplitRatioSum([r(A, a), r(B, b)])).toThrow( + SplitRatioSumError, + ); + }); + + it.each([ + ["explicit null", null, 1], + ["numeric string", "0.5", 0.5], + ["boolean", true, 0], + ])("rejects %s instead of treating it as an omitted numeric ratio", (_label, a, b) => { + expect(() => validateSplitRatioSum([r(A, a), r(B, b)])).toThrow( + SplitRatioSumError, + ); + }); + + it("accepts zero and one at the allowed boundaries", () => { + expect(validateSplitRatioSum([r(A, 0), r(B, 1)])).toBe(1); + }); + + it("preserves omitted-ratio contribution as zero once ratio mode is active", () => { + expect(validateSplitRatioSum([r(A, 1), r(B)])).toBe(1); + }); + + it("preserves the floating-point tolerance", () => { + const actual = validateSplitRatioSum([ + r(A, 0.7), + r(B, 0.2), + r("GC", 0.1), + ]); + + expect(Math.abs(actual - 1)).toBeLessThanOrEqual(SPLIT_RATIO_TOLERANCE); + }); + + it("preserves SplitRatioSumError fields for an under-allocated split", () => { + try { + validateSplitRatioSum([r(A, 0.5), r(B, 0.4)]); + throw new Error("expected ratio validation to fail"); + } catch (error) { + expect(error).toBeInstanceOf(SplitRatioSumError); + expect(error).toMatchObject({ + code: "SPLIT_RATIO_SUM_INVALID", + actualSum: 0.9, + expectedSum: 1, + tolerance: SPLIT_RATIO_TOLERANCE, + }); + } + }); + + it("reports NaN actualSum for a malformed runtime ratio that has no numeric total", () => { + try { + validateSplitRatioSum([r(A, null), r(B, 1)]); + throw new Error("expected runtime ratio validation to fail"); + } catch (error) { + expect(error).toBeInstanceOf(SplitRatioSumError); + expect(Number.isNaN((error as SplitRatioSumError).actualSum)).toBe(true); + } + }); +}); + +describe("splitExecutor ratio pre-flight", () => { + it.each([ + ["out-of-range aggregate", -1, 2], + ["explicit null", null, 1], + ["numeric string", "0.5", 0.5], + ["NaN", Number.NaN, 1], + ])("rejects %s before capacity work", async (_label, a, b) => { + await expect(splitExecutor([r(A, a), r(B, b)])).rejects.toBeInstanceOf( + SplitRatioSumError, + ); + expect(checkSubentryCapacity).not.toHaveBeenCalled(); + }); + + it("leaves amount-only splits unaffected", async () => { + const result = await splitExecutor([r(A), r(B)], { + skipCapacityCheck: true, + }); + + expect(result.success).toBe(true); + expect(checkSubentryCapacity).not.toHaveBeenCalled(); + }); +});