From 10469a2710921068f0271f7b479c1a754653ad3c Mon Sep 17 00:00:00 2001 From: Omar Khan Date: Fri, 13 May 2022 15:58:25 -0400 Subject: [PATCH] fix verification bug in Wallet.sign() when signing a non-XRP Payment amount with trailing zeros (#2000) * fix serialize/deserialize verficiation bug in Wallet.sign() when signing a non-XRP Payment transaction with an amount that contains trailing insignificant zeros --- packages/xrpl/HISTORY.md | 1 + packages/xrpl/src/Wallet/index.ts | 33 ++++++++++++++++++-- packages/xrpl/test/wallet/index.ts | 50 ++++++++++++++++++++++++++++++ 3 files changed, 81 insertions(+), 3 deletions(-) diff --git a/packages/xrpl/HISTORY.md b/packages/xrpl/HISTORY.md index 5a3ff7d7..d9a27f95 100644 --- a/packages/xrpl/HISTORY.md +++ b/packages/xrpl/HISTORY.md @@ -8,6 +8,7 @@ Subscribe to [the **xrpl-announce** mailing list](https://groups.google.com/g/xr * Updated dependencies which had a warning when running `npm audit` * Infinite error/reconnect in browser if an exception was raised during the initial websocket connection event. * Errors during reliable submission with no error message now properly show the preliminary result instead of a type error +* Fixed serialize/deserialize verification bug in `Wallet.sign()` when signing a non-XRP Payment with an amount that contains trailing insignificant zeros ## 2.2.3 (2022-05-04) ### Fixed diff --git a/packages/xrpl/src/Wallet/index.ts b/packages/xrpl/src/Wallet/index.ts index ca719fac..189aeaad 100644 --- a/packages/xrpl/src/Wallet/index.ts +++ b/packages/xrpl/src/Wallet/index.ts @@ -1,4 +1,5 @@ /* eslint-disable max-lines -- There are lots of equivalent constructors which make sense to have here. */ +import BigNumber from 'bignumber.js' import { fromSeed } from 'bip32' import { mnemonicToSeedSync } from 'bip39' import _ from 'lodash' @@ -318,13 +319,17 @@ class Wallet { multisignAddress = this.classicAddress } - if (transaction.TxnSignature || transaction.Signers) { + const tx = { ...transaction } + + if (tx.TxnSignature || tx.Signers) { throw new ValidationError( 'txJSON must not contain "TxnSignature" or "Signers" properties', ) } - const txToSignAndEncode = { ...transaction } + removeTrailingZeros(tx) + + const txToSignAndEncode = { ...tx } txToSignAndEncode.SigningPubKey = multisignAddress ? '' : this.publicKey @@ -346,7 +351,7 @@ class Wallet { ) } const serialized = encode(txToSignAndEncode) - this.checkTxSerialization(serialized, transaction) + this.checkTxSerialization(serialized, tx) return { tx_blob: serialized, hash: hashSignedTx(serialized), @@ -473,4 +478,26 @@ function computeSignature( return sign(encodeForSigning(tx), privateKey) } +/** + * Remove trailing insignificant zeros for non-XRP Payment amount. + * This resolves the serialization mismatch bug when encoding/decoding a non-XRP Payment transaction + * with an amount that contains trailing insignificant zeros; for example, '123.4000' would serialize + * to '123.4' and cause a mismatch. + * + * @param tx - The transaction prior to signing. + */ +function removeTrailingZeros(tx: Transaction): void { + if ( + tx.TransactionType === 'Payment' && + typeof tx.Amount !== 'string' && + tx.Amount.value.includes('.') && + tx.Amount.value.endsWith('0') + ) { + // eslint-disable-next-line no-param-reassign -- Required to update Transaction.Amount.value + tx.Amount = { ...tx.Amount } + // eslint-disable-next-line no-param-reassign -- Required to update Transaction.Amount.value + tx.Amount.value = new BigNumber(tx.Amount.value).toString() + } +} + export default Wallet diff --git a/packages/xrpl/test/wallet/index.ts b/packages/xrpl/test/wallet/index.ts index 1366087f..4767e0e4 100644 --- a/packages/xrpl/test/wallet/index.ts +++ b/packages/xrpl/test/wallet/index.ts @@ -589,6 +589,56 @@ describe('Wallet', function () { wallet.sign(payment) }, /^1.1234567 is an illegal amount/u) }) + + it('sign handles non-XRP amount with a trailing zero', async function () { + const payment: Transaction = { + TransactionType: 'Payment', + Account: 'r9cZA1mLK5R5Am25ArfXFmqgNwjZgnfk59', + Destination: 'rQ3PTWGLCbPz8ZCicV5tCX3xuymojTng5r', + Amount: { + currency: 'FOO', + issuer: 'rnURbz5HLbvqEq69b1B4TX6cUTNMmcrBqi', + value: '123.40', + }, + Flags: 2147483648, + Sequence: 23, + LastLedgerSequence: 8819954, + Fee: '12', + } + const result = wallet.sign(payment) + const expectedResult = { + tx_blob: + '12000022800000002400000017201B008694F261D504625103A72000000000000000000000000000464F4F00000000002E099DD75FDD96EB4A603037844F964832FED86B68400000000000000C732102A8A44DB3D4C73EEEE11DFE54D2029103B776AA8A8D293A91D645977C9DF5F5447446304402206EBFC9B8061C3F82D521506CE62B6BBA99995B2175BFE0E1BC516775932AECEB0220172B9CE9C0EFB3F4870E19B79B4E817DD376E5785F034AB792708F92282C65F781145E7B112523F68D2F5E879DB4EAC51C6698A693048314FDB08D07AAA0EB711793A3027304D688E10C3648', + hash: '6235E5A3CC14DB97F75CAE2A4B5AA9B4134B7AD48D7DD8C15473D81631435FE4', + } + + assert.deepEqual(result, expectedResult) + }) + + it('sign handles non-XRP amount with trailing zeros', async function () { + const payment: Transaction = { + TransactionType: 'Payment', + Account: 'r9cZA1mLK5R5Am25ArfXFmqgNwjZgnfk59', + Destination: 'rQ3PTWGLCbPz8ZCicV5tCX3xuymojTng5r', + Amount: { + currency: 'FOO', + issuer: 'rnURbz5HLbvqEq69b1B4TX6cUTNMmcrBqi', + value: '123.000', + }, + Flags: 2147483648, + Sequence: 23, + LastLedgerSequence: 8819954, + Fee: '12', + } + const result = wallet.sign(payment) + const expectedResult = { + tx_blob: + '12000022800000002400000017201B008694F261D5045EADB112E000000000000000000000000000464F4F00000000002E099DD75FDD96EB4A603037844F964832FED86B68400000000000000C732102A8A44DB3D4C73EEEE11DFE54D2029103B776AA8A8D293A91D645977C9DF5F54474473045022100C0C77D7D6D6453F0C5EDFF61DE60B5D3D6952C8F30D51543560936D72FA103B00220258CBFCEAC4D2DB5CC2B9417EB46225943E9F4B92944B303ADB810002530BFFB81145E7B112523F68D2F5E879DB4EAC51C6698A693048314FDB08D07AAA0EB711793A3027304D688E10C3648', + hash: 'FADCD5EE33C01103AA129FCF0923637D543DB56250CD57A1A308EC386A211CBB', + } + + assert.deepEqual(result, expectedResult) + }) }) describe('verifyTransaction', function () {