diff --git a/CHANGELOG.md b/CHANGELOG.md index 22d20219..532639eb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -73,7 +73,9 @@ `AuthFlags` (`AuthFlag | (number & {})`), accepting any number for combined bitmask values. - `toXDRPrice` now rejects denominator equal to zero (`d <= 0` instead of - `d < 0`). + `d < 0`). Numeric price values of zero, negative, `NaN`, and `Infinity` are + now rejected with `"price must be positive"` before reaching `best_r()`, + instead of producing a confusing `"Couldn't find approximation"` error. - `checkUnsignedIntValue` string-to-number conversion changed from `parseFloat()` to `Number()`. Strings with trailing non-numeric characters (e.g., `"123abc"`) that previously silently succeeded will now be rejected. @@ -131,7 +133,9 @@ prototype chain from being used as type hints. - `nativeToScVal` now validates bounds for `u32`/`i32` types, throwing `TypeError` for out-of-range values that previously passed through to the XDR - layer. + layer. The string-to-`u32`/`i32` path now uses `BigInt()` instead of + `parseInt()`, rejecting strings with trailing junk (e.g., `"123abc"`) and + correctly parsing hex/octal/binary prefixes instead of silently returning `0`. - `best_r` (continued fraction approximation) no longer throws for small numbers whose reciprocal exceeds `MAX_INT`. It now computes a semi-convergent to recover a valid rational approximation. @@ -191,6 +195,18 @@ - `TransactionBuilder` constructor now validates `timebounds` and `ledgerbounds`: negative values and `min > max` now throw immediately instead of producing silently invalid transactions. +- `XdrLargeInt.toI128()` and `toI256()` now reject unsigned values that exceed + the signed range (e.g., `2^127` for i128, `2^255` for i256), throwing + `RangeError` instead of silently flipping the sign bit via `BigInt.asIntN`. +- `Memo.id()` now rejects non-plain-digit strings such as scientific notation + (`"1e18"`) and decimal strings (`"1.0"`) that previously passed `BigNumber` + validation but crashed in `BigInt()` during XDR serialization. +- `TransactionBuilder.build()` now throws when the total fee (`baseFee * + operations`, or `baseFee * operations + resourceFee` for Soroban) exceeds + the uint32 maximum (`4294967295`), instead of producing an invalid XDR value. +- `TransactionBuilder.cloneFrom()` now throws when the source transaction has + zero operations, instead of producing a builder with `baseFee` set to + `Infinity`. ## [`v14.1.0`](https://github.com/stellar/js-stellar-base/compare/v14.0.4...v14.1.0): diff --git a/src/keypair.ts b/src/keypair.ts index 5b38ef8b..828520a2 100644 --- a/src/keypair.ts +++ b/src/keypair.ts @@ -291,7 +291,7 @@ export class Keypair { let hint = Buffer.from(dataBuffer.subarray(-4)); if (hint.length < 4) { // append zeroes as needed - hint = Buffer.concat([hint, Buffer.alloc(4 - dataBuffer.length, 0)]); + hint = Buffer.concat([hint, Buffer.alloc(4 - hint.length, 0)]); } // XOR each byte of hint with corresponding byte of keyHint diff --git a/src/memo.ts b/src/memo.ts index f41afbbd..ed313df6 100644 --- a/src/memo.ts +++ b/src/memo.ts @@ -145,6 +145,13 @@ export class Memo { throw error; } + // Only plain decimal digit strings are accepted. Scientific notation + // ("1e18") and trailing-zero decimals ("1.0") pass BigNumber validation + // but crash in BigInt() during XDR serialization. + if (!/^[0-9]+$/.test(value)) { + throw error; + } + let number: BigNumber; try { number = new BigNumber(value); diff --git a/src/numbers/xdr_large_int.ts b/src/numbers/xdr_large_int.ts index 2866181d..59cd921c 100644 --- a/src/numbers/xdr_large_int.ts +++ b/src/numbers/xdr_large_int.ts @@ -165,6 +165,9 @@ export class XdrLargeInt { this._sizeCheck(128); const v = this.int.toBigInt(); + if (BigInt.asIntN(128, v) !== v) { + throw RangeError(`value too large for i128: ${v}`); + } const hi64 = BigInt.asIntN(64, v >> 64n); // encode top 64 w/ sign bit const lo64 = BigInt.asUintN(64, v); // grab btm 64, encode sign @@ -196,10 +199,13 @@ export class XdrLargeInt { /** * The integer encoded with `ScValType = I256` * - * Note: No size check needed - I256 is the largest signed type. + * @throws if the value cannot fit in a signed 256-bit integer */ toI256(): xdr.ScVal { const v = this.int.toBigInt(); + if (BigInt.asIntN(256, v) !== v) { + throw RangeError(`value too large for i256: ${v}`); + } const hiHi64 = BigInt.asIntN(64, v >> 192n); // keep sign bit const hiLo64 = BigInt.asUintN(64, v >> 128n); const loHi64 = BigInt.asUintN(64, v >> 64n); diff --git a/src/scval.ts b/src/scval.ts index f9d1c57c..b35c51cd 100644 --- a/src/scval.ts +++ b/src/scval.ts @@ -293,11 +293,29 @@ export function nativeToScVal( case "address": return new Address(val).toScVal(); - case "u32": - return xdr.ScVal.scvU32(parseInt(val, 10)); + case "u32": { + const bigintVal = BigInt(val); + if ( + bigintVal < BigInt(xdr.Uint32.MIN_VALUE) || + bigintVal > BigInt(xdr.Uint32.MAX_VALUE) + ) { + throw new TypeError(`invalid value (${val}) for type u32`); + } + return xdr.ScVal.scvU32(Number(bigintVal)); + } - case "i32": - return xdr.ScVal.scvI32(parseInt(val, 10)); + case "i32": { + const bigintVal = BigInt(val); + // TODO: Update this check once xdr.Int32.MIN_VALUE in XDR is properly + // set to negative. Check this globally. + if ( + bigintVal < -BigInt(xdr.Int32.MIN_VALUE) || + bigintVal > BigInt(xdr.Int32.MAX_VALUE) + ) { + throw new TypeError(`invalid value (${val}) for type i32`); + } + return xdr.ScVal.scvI32(Number(bigintVal)); + } default: if (XdrLargeInt.isType(optType)) { diff --git a/src/transaction_builder.ts b/src/transaction_builder.ts index 7c20bd6b..569d6bef 100644 --- a/src/transaction_builder.ts +++ b/src/transaction_builder.ts @@ -24,6 +24,7 @@ import { Address } from "./address.js"; import { Keypair } from "./keypair.js"; const HYPER_MAX_VALUE = Hyper.MAX_VALUE as unknown as bigint; +const UINT32_MAX = 4294967295; // 2^32 - 1 /** * Minimum base fee for transactions. If this fee is below the network @@ -297,6 +298,13 @@ export class TransactionBuilder { throw new TypeError(`unsupported tx source account: ${tx.source}`); } + if (tx.operations.length === 0) { + throw new Error( + "cannot clone a transaction with no operations: " + + "per-operation base fee cannot be determined", + ); + } + // the initial fee passed to the builder gets scaled up based on the number // of operations at the end, so we have to down-scale first const unscaledFee = Math.floor(parseInt(tx.fee, 10) / tx.operations.length); @@ -914,6 +922,14 @@ export class TransactionBuilder { const fee = new BigNumber(this.baseFee) .times(this.operations.length) .toNumber(); + + if (fee > UINT32_MAX) { + throw new Error( + `Total fee (baseFee * operations) exceeds the maximum uint32 value (${UINT32_MAX}). ` + + `Got ${fee} from baseFee=${this.baseFee} and ${this.operations.length} operation(s).`, + ); + } + const attrs: { fee: number; seqNum: xdr.SequenceNumber; @@ -1006,6 +1022,13 @@ export class TransactionBuilder { attrs.fee = new BigNumber(attrs.fee) .plus(this.sorobanData.resourceFee().toString()) .toNumber(); + + if (attrs.fee > UINT32_MAX) { + throw new Error( + `Total fee (baseFee * operations + resourceFee) exceeds the maximum uint32 value (${UINT32_MAX}). ` + + `Got ${attrs.fee}.`, + ); + } } else { attrs.ext = new xdr.TransactionExt(0); } diff --git a/src/util/operations.ts b/src/util/operations.ts index dfbadac1..843c4060 100644 --- a/src/util/operations.ts +++ b/src/util/operations.ts @@ -105,6 +105,10 @@ export function toXDRPrice( if (typeof price === "object" && "n" in price && "d" in price) { xdrObject = new xdr.Price(price); } else { + const priceBN = new BigNumber(price); + if (!priceBN.gt(0) || !priceBN.isFinite()) { + throw new Error("price must be positive"); + } const approx = best_r(price); xdrObject = new xdr.Price({ n: parseInt(String(approx[0]), 10), diff --git a/test/unit/memo.test.ts b/test/unit/memo.test.ts index b51b42bb..9cd7dec3 100644 --- a/test/unit/memo.test.ts +++ b/test/unit/memo.test.ts @@ -5,7 +5,7 @@ import { MemoID, MemoText, MemoHash, - MemoReturn + MemoReturn, } from "../../src/memo.js"; describe("Memo", () => { @@ -47,7 +47,7 @@ describe("Memo", () => { }); it("returns a value for a correct argument (utf8)", () => { - let memoText = new Memo(MemoText, Buffer.from([0xd1])) + const memoText = new Memo(MemoText, Buffer.from([0xd1])) .toXDRObject() .toXDR(); const expected = Buffer.from([ @@ -56,7 +56,7 @@ describe("Memo", () => { // length 0x00, 0x00, 0x00, 0x01, // value - 0xd1, 0x00, 0x00, 0x00 + 0xd1, 0x00, 0x00, 0x00, ]); expect(memoText.equals(expected)).toBe(true); }); @@ -97,32 +97,32 @@ describe("Memo", () => { it("throws an error when invalid argument was passed", () => { // @ts-expect-error testing missing arg expect(() => Memo.text()).toThrow( - /Expects string, array or buffer, max 28 bytes/ + /Expects string, array or buffer, max 28 bytes/, ); // @ts-expect-error testing invalid input expect(() => Memo.text({})).toThrow( - /Expects string, array or buffer, max 28 bytes/ + /Expects string, array or buffer, max 28 bytes/, ); // @ts-expect-error testing invalid input expect(() => Memo.text(10)).toThrow( - /Expects string, array or buffer, max 28 bytes/ + /Expects string, array or buffer, max 28 bytes/, ); // @ts-expect-error testing invalid input expect(() => Memo.text(Infinity)).toThrow( - /Expects string, array or buffer, max 28 bytes/ + /Expects string, array or buffer, max 28 bytes/, ); // @ts-expect-error testing invalid input expect(() => Memo.text(NaN)).toThrow( - /Expects string, array or buffer, max 28 bytes/ + /Expects string, array or buffer, max 28 bytes/, ); }); it("throws an error when string is longer than 28 bytes", () => { expect(() => Memo.text("12345678901234567890123456789")).toThrow( - /Expects string, array or buffer, max 28 bytes/ + /Expects string, array or buffer, max 28 bytes/, ); expect(() => Memo.text("三代之時三代之時三代之時")).toThrow( - /Expects string, array or buffer, max 28 bytes/ + /Expects string, array or buffer, max 28 bytes/, ); }); }); @@ -171,6 +171,26 @@ describe("Memo", () => { it("throws an error when value exceeds uint64 max", () => { expect(() => Memo.id("18446744073709551616")).toThrow(/Expects a uint64/); }); + + it("rejects scientific notation strings that BigInt cannot parse", () => { + // "1e18" passes BigNumber validation but BigInt("1e18") throws. + // Validation should reject it upfront instead of deferring the crash + // to toXDRObject(). + expect(() => Memo.id("1e18")).toThrow(/Expects a uint64/); + }); + + it("rejects trailing-zero decimal strings that BigInt cannot parse", () => { + // "1.0" passes BigNumber.isInteger() but BigInt("1.0") throws. + expect(() => Memo.id("1.0")).toThrow(/Expects a uint64/); + }); + + it("scientific notation equivalent works when written as plain digits", () => { + // The value itself is valid — it's the string format that's the problem. + expect(() => Memo.id("1000000000000000000")).not.toThrow(); + const memo = Memo.id("1000000000000000000"); + const xdrMemo = memo.toXDRObject(); + expect(xdrMemo.id().toString()).toBe("1000000000000000000"); + }); }); describe(".hash() & .return()", () => { @@ -191,7 +211,7 @@ describe("Memo", () => { expect(baseMemo.type).toBe(MemoHash); expect((baseMemo.value as Buffer).length).toBe(32); expect((baseMemo.value as Buffer).toString("hex")).toBe( - buffer.toString("hex") + buffer.toString("hex"), ); }); @@ -214,7 +234,7 @@ describe("Memo", () => { expect(Buffer.isBuffer(baseMemo.value)).toBe(true); expect((baseMemo.value as Buffer).length).toBe(32); expect((baseMemo.value as Buffer).toString("hex")).toBe( - buffer.toString("hex") + buffer.toString("hex"), ); }); @@ -224,8 +244,8 @@ describe("Memo", () => { expect(() => method(Buffer.alloc(32).toString("hex"))).not.toThrow(); expect(() => method( - "0000000000000000000000000000000000000000000000000000000000000000" - ) + "0000000000000000000000000000000000000000000000000000000000000000", + ), ).not.toThrow(); } }); @@ -251,20 +271,20 @@ describe("Memo", () => { expect(() => method("test")).toThrow(/Expects a 32 byte hash value/); // @ts-expect-error testing invalid input expect(() => method([0, 10, 20])).toThrow( - /Expects a 32 byte hash value/ + /Expects a 32 byte hash value/, ); expect(() => method(Buffer.alloc(33).toString("hex"))).toThrow( - /Expects a 32 byte hash value/ + /Expects a 32 byte hash value/, ); expect(() => method( - "00000000000000000000000000000000000000000000000000000000000000" - ) + "00000000000000000000000000000000000000000000000000000000000000", + ), ).toThrow(/Expects a 32 byte hash value/); expect(() => method( - "000000000000000000000000000000000000000000000000000000000000000000" - ) + "000000000000000000000000000000000000000000000000000000000000000000", + ), ).toThrow(/Expects a 32 byte hash value/); } }); @@ -291,20 +311,20 @@ describe("Memo", () => { const buffer = Buffer.alloc(32, 10); const memo = Memo.hash(buffer); - const value = memo.value as Buffer; + const value = memo.value; value[0] = 0xff; - expect((memo.value as Buffer)[0]).toBe(10); + expect(memo.value[0]).toBe(10); }); it("returns a copy for MemoReturn so mutations do not affect the original", () => { const buffer = Buffer.alloc(32, 20); const memo = Memo.return(buffer); - const value = memo.value as Buffer; + const value = memo.value; value[0] = 0xff; - expect((memo.value as Buffer)[0]).toBe(20); + expect(memo.value[0]).toBe(20); }); }); }); diff --git a/test/unit/numbers/sc_int.test.ts b/test/unit/numbers/sc_int.test.ts index 56e2ad8d..44311b0c 100644 --- a/test/unit/numbers/sc_int.test.ts +++ b/test/unit/numbers/sc_int.test.ts @@ -336,6 +336,9 @@ describe("ScInt", () => { expect(sci.toBigInt()).toBe(max); }); + // TODO: @stellar/js-xdr@4.0.0 now throws RangeError for oversized values + // instead of silently wrapping. Once the XDR upgrade is finalized, change + // this test to: expect(() => new ScInt(tooLarge, { type: "u64" })).toThrow(RangeError); it("wraps oversized values for specified type (overflow to zero)", () => { const tooLarge = 1n << 64n; const sci = new ScInt(tooLarge, { type: "u64" }); diff --git a/test/unit/numbers/xdr_large_int.test.ts b/test/unit/numbers/xdr_large_int.test.ts index e3ce2661..c1e8f0c9 100644 --- a/test/unit/numbers/xdr_large_int.test.ts +++ b/test/unit/numbers/xdr_large_int.test.ts @@ -394,6 +394,14 @@ describe("XdrLargeInt", () => { expect(() => tooLarge.toI128()).toThrow(/too large for 128 bits/); }); + it("throws RangeError for unsigned values exceeding signed i128 range", () => { + const u128AtSignedBoundary = new XdrLargeInt("u128", 1n << 127n); + expect(() => u128AtSignedBoundary.toI128()).toThrow(RangeError); + + const u128Max = new XdrLargeInt("u128", (1n << 128n) - 1n); + expect(() => u128Max.toI128()).toThrow(RangeError); + }); + it("handles boundary values", () => { const maxI128 = (1n << 127n) - 1n; const minI128 = -(1n << 127n); @@ -477,12 +485,20 @@ describe("XdrLargeInt", () => { expect(i256.hiHi().toBigInt()).toBe(-1n); }); - it("does not throw for large values (no size check)", () => { + it("does not throw for large values within signed range", () => { const huge = (1n << 200n) - 1n; const xdrInt = new XdrLargeInt("i256", huge); expect(() => xdrInt.toI256()).not.toThrow(); }); + it("throws RangeError for unsigned values exceeding signed i256 range", () => { + const u256AtSignedBoundary = new XdrLargeInt("u256", 1n << 255n); + expect(() => u256AtSignedBoundary.toI256()).toThrow(RangeError); + + const u256Max = new XdrLargeInt("u256", (1n << 256n) - 1n); + expect(() => u256Max.toI256()).toThrow(RangeError); + }); + it("handles large positive value", () => { const large = 1n << 200n; const xdrInt = new XdrLargeInt("i256", large); @@ -707,9 +723,13 @@ describe("XdrLargeInt", () => { expect(() => oversized64.toDuration()).toThrow(RangeError); }); - it("does not throw in toI256/toU256 for any size", () => { + it("does not throw in toI256 for values within signed range", () => { const huge = (1n << 250n) - 1n; expect(() => new XdrLargeInt("i256", huge).toI256()).not.toThrow(); + }); + + it("does not throw in toU256 for any size", () => { + const huge = (1n << 250n) - 1n; expect(() => new XdrLargeInt("u256", huge).toU256()).not.toThrow(); }); }); diff --git a/test/unit/operation.test.ts b/test/unit/operation.test.ts index b524f175..9ac3e7e0 100644 --- a/test/unit/operation.test.ts +++ b/test/unit/operation.test.ts @@ -159,4 +159,22 @@ describe("toXDRPrice()", () => { expect(() => toXDRPrice({ n: 1, d: 0 })).toThrow(/price must be positive/); expect(() => toXDRPrice({ n: 0, d: 0 })).toThrow(/price must be positive/); }); + + it("throws 'price must be positive' for zero numeric price", () => { + expect(() => toXDRPrice("0")).toThrow(/price must be positive/); + expect(() => toXDRPrice(0)).toThrow(/price must be positive/); + }); + + it("throws for non-numeric string inputs", () => { + expect(() => toXDRPrice("abc")).toThrow(/not a number/i); + expect(() => toXDRPrice("")).toThrow(/not a number/i); + }); + + it("throws 'price must be positive' for NaN input", () => { + expect(() => toXDRPrice(NaN)).toThrow(/price must be positive/); + }); + + it("throws 'price must be positive' for Infinity input", () => { + expect(() => toXDRPrice(Infinity)).toThrow(/price must be positive/); + }); }); diff --git a/test/unit/scval.test.ts b/test/unit/scval.test.ts index 6a6177d8..c96b6f59 100644 --- a/test/unit/scval.test.ts +++ b/test/unit/scval.test.ts @@ -1973,6 +1973,29 @@ describe("edge cases and stress tests", () => { expect(scv.value()).toBe(0); }); + it("rejects string with trailing non-numeric characters for u32", () => { + expect(() => nativeToScVal("123abc", { type: "u32" })).toThrow(SyntaxError); + }); + + it("rejects string with trailing non-numeric characters for i32", () => { + expect(() => nativeToScVal("42xyz", { type: "i32" })).toThrow(SyntaxError); + }); + + it("correctly parses hex-prefix string for u32", () => { + // BigInt("0x10") = 16n — hex is parsed correctly, unlike parseInt which returned 0 + const scv = nativeToScVal("0x10", { type: "u32" }); + expect(scv.switch().name).toBe("scvU32"); + expect(scv.value()).toBe(16); + }); + + it("rejects pure non-numeric string for u32", () => { + expect(() => nativeToScVal("hello", { type: "u32" })).toThrow(SyntaxError); + }); + + it("rejects scientific notation string for i32", () => { + expect(() => nativeToScVal("1e2", { type: "i32" })).toThrow(SyntaxError); + }); + it("nativeToScVal with no type hint auto-selects appropriate type", () => { // Positive -> unsigned expect(nativeToScVal(0).switch().name).not.toBe("scvI64"); diff --git a/test/unit/transaction_builder.test.ts b/test/unit/transaction_builder.test.ts index 06fe16b9..767d6bae 100644 --- a/test/unit/transaction_builder.test.ts +++ b/test/unit/transaction_builder.test.ts @@ -709,9 +709,11 @@ describe("TransactionBuilder", () => { }).toThrow(/resourceFee must be greater than 0/); }); - it("accepts resourceFee at i64 max", () => { + it("rejects resourceFee at i64 max (total fee overflows uint32)", () => { const asset = Asset.native(); + // Transaction.fee is Uint32 in XDR. An i64-max resourceFee added to + // any baseFee will always exceed uint32 max. expect(() => { new TransactionBuilder(source, { fee: "100", @@ -725,7 +727,7 @@ describe("TransactionBuilder", () => { }) .setTimeout(TimeoutInfinite) .build(); - }).not.toThrow(); + }).toThrow(/exceeds the maximum uint32 value/); }); }); }); @@ -1690,6 +1692,18 @@ describe("TransactionBuilder.cloneFrom", () => { expect(expectDefined(cloneTx).fee).toBe("999"); }); + it("throws when cloning a zero-operation transaction", () => { + const zeroOpTx = new TransactionBuilder(source, { + fee: "100", + timebounds: { minTime: 0, maxTime: 0 }, + networkPassphrase, + }).build(); + + expect(() => TransactionBuilder.cloneFrom(zeroOpTx)).toThrow( + /cannot clone a transaction with no operations/, + ); + }); + it("preserves extraSigners", () => { const extraSigner = "GA7QYNF7SOWQ3GLR2BGMZEHXAVIRZA4KVWLTJJFC7MGXUA74P7UJVSGZ"; @@ -2485,3 +2499,38 @@ describe("addSacTransferOperation with invalid destination", () => { }).not.toThrow(); }); }); + +describe("fee overflow protection", () => { + const source = new Account( + "GCEZWKCA5VLDNRLN3RPRJMRZOX3Z6G5CHCGSNFHEYVXM3XOJMDS674JZ", + "0", + ); + const networkPassphrase = Networks.TESTNET; + + it("throws when baseFee * operations exceeds uint32 max", () => { + // fee = 4294967295 * 2 = 8589934590 > uint32 max + expect(() => { + new TransactionBuilder(source, { + fee: "4294967295", + networkPassphrase, + timebounds: { minTime: 0, maxTime: 0 }, + }) + .addOperation(Operation.inflation({})) + .addOperation(Operation.inflation({})) + .build(); + }).toThrow(/fee/i); + }); + + it("allows fee exactly at uint32 max", () => { + // fee = 4294967295 * 1 = 4294967295 = uint32 max (valid) + expect(() => { + new TransactionBuilder(source, { + fee: "4294967295", + networkPassphrase, + timebounds: { minTime: 0, maxTime: 0 }, + }) + .addOperation(Operation.inflation({})) + .build(); + }).not.toThrow(); + }); +});