Skip to content

Commit e234704

Browse files
sreerajsbitgobot
authored andcommitted
fix(sdk-coin-sui): preserve custom transaction amount precision
Aggregate custom transaction recipient amounts with BigNumber instead of converting external u64 values to JavaScript numbers. Add a regression test and document the cross-coin audit findings because rounded Sui amounts can produce incorrect transaction explanations and input totals. Ticket: CSHLD-1018 Session-Id: c3c753a4-4f1a-4c00-a472-9e69452eb9da Task-Id: 9ebc0ba0-6ecb-47d2-854b-bad260bb0691
1 parent 0a41e1b commit e234704

3 files changed

Lines changed: 55 additions & 4 deletions

File tree

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
# JavaScript numeric precision audit
2+
3+
Ticket: CSHLD-1018
4+
5+
## Method
6+
7+
Audited TypeScript under `modules/sdk-coin-*`, `modules/abstract-*`, and
8+
shared transaction code for `Number`, `parseInt`, `parseFloat`, and big-number
9+
`toNumber()` conversions. Values were classified as metadata (indexes,
10+
versions, timestamps, opcodes) or blockchain quantities (amounts, balances,
11+
fees, and serialized integer fields).
12+
13+
## Findings and disposition
14+
15+
| Area | Risk | Disposition |
16+
| --- | --- | --- |
17+
| Sui custom transaction recipient aggregation | Recipient amounts are external decimal strings and can be u64 values. Converting each amount with `Number()` rounded values above `Number.MAX_SAFE_INTEGER` before summing and explaining a transaction. | Fixed in `sdk-coin-sui/src/lib/customTransaction.ts` by aggregating with `BigNumber` and returning a fixed-point decimal string. |
18+
| Sui transfer, token-transfer, staking, and Walrus paths | Existing code uses `BigNumber` for amount aggregation and `BigInt`/string-preserving paths for serialized u64 values. A few `Number()` conversions are limited to protocol metadata or SDK APIs that require JavaScript numbers; these require separate API-compatible changes before replacement. | Confirmed safe for blockchain quantities in the audited paths, except the custom transaction path fixed above. |
19+
| Solana SPL token transfer and mint/burn paths | Amounts are passed to SPL APIs as `BigInt` and existing large-amount tests cover values beyond the safe integer range. | Confirmed safe. Native System/Stake APIs still expose number-only parameters and need a dedicated compatibility change rather than an unsafe cast. |
20+
| EVM nonce/sequence, Cosmos/Substrate metadata, UTXO indexes, and timestamps | Conversions are bounded protocol metadata or array/index values rather than coin amounts. | Confirmed safe for this audit scope. |
21+
| Coin fee/balance explanation fields | Several coins intentionally expose legacy number fields while retaining string fields. These are compatibility surfaces and need coin-specific follow-up tickets where the underlying chain permits u64/u128 values. | Triaged for follow-up; no broad type change made in this ticket. |
22+
23+
## Follow-up triage
24+
25+
The remaining number-returning compatibility surfaces should be addressed per
26+
coin, with API changes that preserve existing string fields and add boundary
27+
tests before changing public types. In particular, review Solana native
28+
System/Stake instruction APIs and fee/balance explanation interfaces for coins
29+
that serialize u64/u128 quantities.

‎modules/sdk-coin-sui/src/lib/customTransaction.ts‎

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import { Transaction } from './transaction';
99
import { BaseCoin as CoinConfig } from '@bitgo/statics';
1010
import utils from './utils';
1111
import { BaseKey, InvalidTransactionError, Recipient, TransactionRecipient, TransactionType } from '@bitgo/sdk-core';
12+
import BigNumber from 'bignumber.js';
1213
import { UNAVAILABLE_TEXT } from './constants';
1314

1415
export class CustomTransaction extends Transaction<CustomProgrammableTransaction> {
@@ -77,7 +78,10 @@ export class CustomTransaction extends Transaction<CustomProgrammableTransaction
7778
value: recipient.amount,
7879
coin: this._coinConfig.name,
7980
}));
80-
const totalAmount = this._recipients.reduce((accumulator, current) => accumulator + Number(current.amount), 0);
81+
const totalAmount = this._recipients.reduce(
82+
(accumulator, current) => accumulator.plus(current.amount),
83+
new BigNumber(0)
84+
);
8185

8286
this._inputs = [
8387
{
@@ -169,11 +173,14 @@ export class CustomTransaction extends Transaction<CustomProgrammableTransaction
169173
private explainCustomTransaction(json: TxData, explanationResult: TransactionExplanation): TransactionExplanation {
170174
const recipients = utils.getRecipients(this.suiTransaction);
171175
const outputs: TransactionRecipient[] = recipients.map((recipient) => recipient);
172-
const outputAmount = recipients.reduce((accumulator, current) => accumulator + Number(current.amount), 0);
176+
const outputAmount = recipients.reduce(
177+
(accumulator, current) => accumulator.plus(current.amount),
178+
new BigNumber(0)
179+
);
173180
return {
174181
...explanationResult,
175182
outputs,
176-
outputAmount,
183+
outputAmount: outputAmount.toFixed(),
177184
};
178185
}
179186
}

‎modules/sdk-coin-sui/test/unit/transactionBuilder/customTransactionBuilder.ts‎

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -107,7 +107,22 @@ describe('Sui Custom Transaction Builder', () => {
107107
deserialized.toBroadcastFormat().should.equal(rawTx);
108108
});
109109

110-
it('should reject a custom tx with unsupported txn type', async function () {
110+
it('preserves precision when aggregating amounts above Number.MAX_SAFE_INTEGER', async function () {
111+
const tx = new CustomTransaction(coins.get('tsui'));
112+
tx.fromRawTransaction(CUSTOM_TX_PUBLIC_TRANSFER);
113+
114+
const largeAmount = '9007199254740993';
115+
(tx as unknown as { _recipients: { address: string; amount: string }[] })._recipients = [
116+
{ address: tx.recipients[0].address, amount: largeAmount },
117+
{ address: tx.recipients[1].address, amount: '1' },
118+
];
119+
tx.loadInputsAndOutputs();
120+
121+
should.equal(tx.inputs[0].value, '9007199254740994');
122+
should.equal(tx.explainTransaction().outputAmount, '9007199254740994');
123+
});
124+
125+
111126
should(() => factory.from(UNSUPPORTED_TX)).throwError(
112127
'unsupported target method 0000000000000000000000000000000000000000000000000000000000000003::staking_pool::split_staked_sui'
113128
);

0 commit comments

Comments
 (0)