Repository navigation
Conversation
…in `amadeus-typescript-sdk` ### Description This pull request addresses Critical, High, and Medium severity findings from the workspace security audit targeting the `amadeus-typescript-sdk` repository[cite: 36]. Previously, amount conversions suffered from severe floating-point precision loss and accepted malformed inputs, cryptographic nonces collided within the same millisecond, BLS scalars were improperly sized, and vault encryption payloads lacked KDF versioning[cite: 36]. This PR systematically remediates these issues across transaction building, conversion, and signing modules[cite: 36]. ### Key Changes & Remediations #### 1. Numeric Precision & Amount Conversion (Critical) * **Exact BigInt Arithmetic:** `toAtomicAma` and `toAtomicAmaString` in `src/conversion.ts` replace binary floating-point multiplication with exact BigInt parsing over decimal strings[cite: 36]. Inputs are strictly validated against `/^[0-9]+(\.[0-9]+)?$/`, truncating decimals past 9 places without rounding up and rejecting negative, empty, or exponential strings[cite: 36]. * **Safe Integer Ceiling:** Amounts exceeding `Number.MAX_SAFE_INTEGER` (~9,007,199 AMA) now throw an explicit error in `toAtomicAma`, directing callers to `toAtomicAmaString` for lossless string representations[cite: 36]. * **Strict Atomic Units:** `fromAtomicAma` rejects non-integer strings and inputs smaller than 1 atomic unit or greater than `MAX_SAFE_INTEGER`[cite: 36]. #### 2. Monotonic Nonce Generation (High) * **Sub-Millisecond Collision Defense:** `generateNonce` in `src/signing.ts` replaces raw `Date.now() * 1_000_000n` with a strictly monotonic counter[cite: 36]. Two transactions generated in the same millisecond receive incrementing nonces, preventing hash collisions and silent chain drops[cite: 36]. #### 3. BLS Scalar Normalization (High) * **Scalar Reduction:** `normalizeSignerSk` in `src/signing.ts` normalizes 64-byte private key seeds down to 32-byte BLS12-381 secret scalars via `reduce512To256LE`[cite: 36]. It now accepts both Base58 strings and raw 64-byte `Uint8Array` seeds identically[cite: 36]. #### 4. Vault Encryption & KDF Metadata (Medium) * **KDF Versioning:** `encryptWithPassword` in `src/encryption.ts` now records the `kdf` algorithm (`PBKDF2-SHA256`) and `iterations` count inside the `EncryptedPayload`[cite: 36]. * **Backward Compatibility:** `decryptWithPassword` honors recorded iteration counts, accepts `RECOMMENDED_PBKDF2_ITERATIONS` (600,000), falls back to `DEFAULT_PBKDF2_ITERATIONS` (100,000) for legacy payloads, and explicitly rejects unsupported KDF schemes[cite: 36]. ### How to Review 1. **Conversion Logic & Unit Tests:** Inspect `src/conversion.ts` and verify all 24 test cases pass in `src/tests/conversion.test.ts`[cite: 36]. 2. **Nonce & Signing:** Check the monotonic counter and key normalization in `src/signing.ts`, supported by `src/tests/signing.test.ts`[cite: 36]. 3. **Encryption & KDF Support:** Review metadata persistence in `src/encryption.ts` and test coverage in `src/tests/encryption.test.ts`[cite: 36]. 4. **API Documentation:** Ensure `docs/API.md` accurately documents the new conversion behavior and KDF parameters[cite: 36].
TypeError86
left a comment
TypeError86
left a comment
There was a problem hiding this comment.
Thanks for this, it's solid work and the diagnoses hold up. I checked the nonce one against our node: the txpool drops any tx where nonce <= chain_nonce, so two txs built in the same millisecond (same Date.now()*1e6 value) really do collide and the second gets dropped. Good catch.
A few things to sort before this can go in. The main one is that the new lossless path isn't actually used anywhere yet.
-
toAtomicAmaString has no caller outside the tests. The transfer and staking builders still call toAtomicAma(...).toString(), which now throws above ~9,007,199 AMA, so large transfers break instead of going lossless. Can you switch these over:
- src/contracts/coin.ts:50
- src/transaction-builder.ts:378 and :427
(They're not in this PR's diff, that's why I'm flagging them here and not inline.)
-
The read side has the same ceiling and no string version. fromAtomicAma still throws above ~9M AMA, and both vault parsers catch that and return null (src/contracts/lockup/parsers.ts:49, src/contracts/lockup-prime/parsers.ts:55), so a vault bigger than ~9M AMA just vanishes instead of erroring. A fromAtomicAmaString (or bigint) counterpart used in those parsers would cover it. Amounts on our node are arbitrary-precision ints and genesis vaults are already 1M AMA, so balances over 9M are realistic.
-
Versioning and docs. The PR is on 1.2.0 but that's already published (npm is on 1.3.0), so it needs a version bump plus a CHANGELOG entry. README.md:494 and docs/API.md also still show the old toAtomicAma(number): number signature and don't mention toAtomicAmaString.
-
On the vault KDF: recording kdf/iterations is the right call, but hold the default at 100k in this PR. Our mobile wallet clamps PBKDF2 to 10k, so we need to line up a single iteration count across all the wallets (plus a re-encrypt on unlock) before bumping it, otherwise vaults stop opening across platforms. Better as its own change once we've sorted that out.
Appreciate you digging into this. If you want to take the wiring in 1 and 2 that'd be great, and I can put a PR on your branch for the mechanical bits if that's easier.
| function generateNonce(): bigint { | ||
| return BigInt(Date.now()) * 1_000_000n | ||
| const candidate = BigInt(Date.now()) * 1_000_000n | ||
| lastNonce = candidate > lastNonce ? candidate : lastNonce + 1n |
There was a problem hiding this comment.
Checked this against the node and it's a real one: the txpool drops nonce <= chain_nonce, so the old same-millisecond collision did drop the second tx. Per-process scope is fine, and the node caps nonce at 2^64-1 so the +1 fallback has plenty of room.
| const value = Number(atomic) | ||
| if (!Number.isSafeInteger(value)) { | ||
| throw new Error( | ||
| `Amount ${JSON.stringify(ama)} is ${atomic} atomic units, which exceeds the maximum safe integer. Use toAtomicAmaString for exact large amounts.` |
There was a problem hiding this comment.
The cap itself is right, but this is the function the builders actually call (coin.ts, transaction-builder.ts), so as it stands it blocks any transfer over ~9M AMA instead of sending it through the string path. See point 1 in the summary.
| * toAtomicAmaString('1.0000000005') // '1000000000' — truncated, never rounded up | ||
| * ``` | ||
| */ | ||
| export function toAtomicAmaString(ama: number | string): string { |
There was a problem hiding this comment.
Good addition. Only thing is nothing outside the tests calls it yet, so wiring it into the builders is what actually gets you the lossless behaviour.
| const key = await deriveKey(password, saltBytes) | ||
| // A payload that records its iteration count is derived with that count; one | ||
| // that does not predates the field and used the legacy value. | ||
| const iterations = payload.iterations ?? LEGACY_PBKDF2_ITERATIONS |
There was a problem hiding this comment.
payload.iterations comes straight from the stored payload (so it's attacker-controlled, and it sits outside the GCM tag), and deriveKey only checks the lower bound. A tampered payload with something like iterations: 2_000_000_000 would make decrypt grind through that many PBKDF2 rounds, which locks up the main thread in a browser. Worth capping it here before deriveKey, e.g. reject anything over ~10,000,000.
| * raise this default once every reader honours the `iterations` field recorded in | ||
| * the payload. | ||
| */ | ||
| export const DEFAULT_PBKDF2_ITERATIONS = 100_000 |
There was a problem hiding this comment.
Recording the count above is what lets us raise this later, but can you keep the default at 100k for now. Mobile clamps PBKDF2 to 10k, so bumping this before we coordinate would make new vaults unreadable on the other wallets. Same as point 4 in the summary.
|
Thanks for the thorough review and for verifying the nonce collision behavior against the node! I've addressed all the feedback in the latest commit:
All unit tests and lint checks are green across both Node 20.x and 22.x! |
Updated amount conversion to use toAtomicAmaString() for lossless handling.
Refactor conversion functions to improve precision handling and error messages.
Updated PBKDF2 iteration constants and validation logic. Introduced MIN_PBKDF2_ITERATIONS and MAX_PBKDF2_ITERATIONS for better control over iteration counts.
Updated amount conversion to use fromAtomicAmaString for better handling of large vaults.
Added tests for fromAtomicAmaString function and updated existing tests for toAtomicAma to ensure correct behavior.
Update version to 1.3.1 with new features and fixes.
Added citations to various sections and improved formatting for clarity.
2 participants
Description
This pull request addresses Critical, High, and Medium severity findings from the workspace security audit targeting the
amadeus-typescript-sdkrepository[cite: 36]. Previously, amount conversions suffered from severe floating-point precision loss and accepted malformed inputs, cryptographic nonces collided within the same millisecond, BLS scalars were improperly sized, and vault encryption payloads lacked KDF versioning[cite: 36]. This PR systematically remediates these issues across transaction building, conversion, and signing modules[cite: 36].Key Changes & Remediations
1. Numeric Precision & Amount Conversion (Critical)
toAtomicAmaandtoAtomicAmaStringinsrc/conversion.tsreplace binary floating-point multiplication with exact BigInt parsing over decimal strings[cite: 36]. Inputs are strictly validated against/^[0-9]+(\.[0-9]+)?$/, truncating decimals past 9 places without rounding up and rejecting negative, empty, or exponential strings[cite: 36].Number.MAX_SAFE_INTEGER(~9,007,199 AMA) now throw an explicit error intoAtomicAma, directing callers totoAtomicAmaStringfor lossless string representations[cite: 36].fromAtomicAmarejects non-integer strings and inputs smaller than 1 atomic unit or greater thanMAX_SAFE_INTEGER[cite: 36].2. Monotonic Nonce Generation (High)
generateNonceinsrc/signing.tsreplaces rawDate.now() * 1_000_000nwith a strictly monotonic counter[cite: 36]. Two transactions generated in the same millisecond receive incrementing nonces, preventing hash collisions and silent chain drops[cite: 36].3. BLS Scalar Normalization (High)
normalizeSignerSkinsrc/signing.tsnormalizes 64-byte private key seeds down to 32-byte BLS12-381 secret scalars viareduce512To256LE[cite: 36]. It now accepts both Base58 strings and raw 64-byteUint8Arrayseeds identically[cite: 36].4. Vault Encryption & KDF Metadata (Medium)
encryptWithPasswordinsrc/encryption.tsnow records thekdfalgorithm (PBKDF2-SHA256) anditerationscount inside theEncryptedPayload[cite: 36].decryptWithPasswordhonors recorded iteration counts, acceptsRECOMMENDED_PBKDF2_ITERATIONS(600,000), falls back toDEFAULT_PBKDF2_ITERATIONS(100,000) for legacy payloads, and explicitly rejects unsupported KDF schemes[cite: 36].How to Review
src/conversion.tsand verify all 24 test cases pass insrc/tests/conversion.test.ts[cite: 36].src/signing.ts, supported bysrc/tests/signing.test.ts[cite: 36].src/encryption.tsand test coverage insrc/tests/encryption.test.ts[cite: 36].docs/API.mdaccurately documents the new conversion behavior and KDF parameters[cite: 36].