diff --git a/packages/transaction-controller/CHANGELOG.md b/packages/transaction-controller/CHANGELOG.md index 29d1afc6755..f857beac36a 100644 --- a/packages/transaction-controller/CHANGELOG.md +++ b/packages/transaction-controller/CHANGELOG.md @@ -7,6 +7,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +- Fix `LineaGasFeeFlow` silently under-computing fee estimates for the `medium` and `high` levels ([#10045](https://github.com/MetaMask/core/pull/10045)) + - `BN.muln()` was called with the fractional multipliers (`1.35`, `1.7`, `1.05`, `1.1`) used to derive Linea gas fee levels; `muln` truncates non-integer input per 26-bit word instead of throwing, so the previous implementation silently returned an under-computed value (e.g. `1 gwei * 1.05` returned `1003023795` instead of `1050000000`). + ### Changed - Bump `@metamask/remote-feature-flag-controller` from `^6.0.0` to `^6.1.0` ([#9980](https://github.com/MetaMask/core/pull/9980)) diff --git a/packages/transaction-controller/src/gas-flows/LineaGasFeeFlow.test.ts b/packages/transaction-controller/src/gas-flows/LineaGasFeeFlow.test.ts index 7e35e21412a..618b286b63f 100644 --- a/packages/transaction-controller/src/gas-flows/LineaGasFeeFlow.test.ts +++ b/packages/transaction-controller/src/gas-flows/LineaGasFeeFlow.test.ts @@ -110,8 +110,8 @@ describe('LineaGasFeeFlow', () => { expect(priorityFees).toStrictEqual([ LINEA_RESPONSE_MOCK.priorityFeePerGas, - '0x23a3d70a3', - '0x25658bf25', + '0x23d70a3d6', + '0x258bf258b', ]); expect(rpcRequestMock).toHaveBeenCalledTimes(1); @@ -138,11 +138,41 @@ describe('LineaGasFeeFlow', () => { expect(maxFees).toStrictEqual([ '0x333333333', - '0x3a7ae1479', - '0x42428f5c1', + '0x3ae147ae0', + '0x428f5c28e', ]); }); + it('applies fractional multipliers without integer truncation', async () => { + // Values chosen so a naive `BN.muln(1.35)` truncates to the wrong + // integer per 26-bit word instead of throwing - proving the + // implementation multiplies via an arbitrary-precision path. + const basePerGas = 30_000_000_000n; // 30 gwei + const priorityPerGas = 1_000_000_000n; // 1 gwei + + rpcRequestMock.mockResolvedValue({ + baseFeePerGas: `0x${basePerGas.toString(16)}`, + priorityFeePerGas: `0x${priorityPerGas.toString(16)}`, + }); + + const flow = new LineaGasFeeFlow(); + const response = await flow.getGasFees(request); + const estimates = response.estimates as FeeMarketGasFeeEstimates; + + expect(BigInt(estimates.medium.maxPriorityFeePerGas)).toBe( + (priorityPerGas * 105n) / 100n, + ); + expect(BigInt(estimates.high.maxPriorityFeePerGas)).toBe( + (priorityPerGas * 110n) / 100n, + ); + expect(BigInt(estimates.medium.maxFeePerGas)).toBe( + (basePerGas * 135n) / 100n + (priorityPerGas * 105n) / 100n, + ); + expect(BigInt(estimates.high.maxFeePerGas)).toBe( + (basePerGas * 170n) / 100n + (priorityPerGas * 110n) / 100n, + ); + }); + it('uses default flow if error', async () => { jest .spyOn(DefaultGasFeeFlow.prototype, 'getGasFees') diff --git a/packages/transaction-controller/src/gas-flows/LineaGasFeeFlow.ts b/packages/transaction-controller/src/gas-flows/LineaGasFeeFlow.ts index f43e0eb8e77..4ad154365e9 100644 --- a/packages/transaction-controller/src/gas-flows/LineaGasFeeFlow.ts +++ b/packages/transaction-controller/src/gas-flows/LineaGasFeeFlow.ts @@ -1,4 +1,9 @@ -import { ChainId, hexToBN, toHex } from '@metamask/controller-utils'; +import { + ChainId, + fractionBN, + hexToBN, + toHex, +} from '@metamask/controller-utils'; import type { NetworkClientId } from '@metamask/network-controller'; import { createModuleLogger } from '@metamask/utils'; import type { Hex } from '@metamask/utils'; @@ -28,6 +33,16 @@ type FeesByLevel = { const log = createModuleLogger(projectLogger, 'linea-gas-fee-flow'); +/** + * Precision used to convert a decimal multiplier (e.g. 1.35) into an integer + * numerator/denominator pair for `fractionBN`, which does the multiplication + * as an arbitrary-precision BN operation. `BN.muln` cannot be used directly + * with a fractional multiplier: it silently truncates to the integer part on + * a per-26-bit-word basis instead of throwing, so `base.muln(1.35)` returns a + * wrong, under-computed result rather than failing loudly. + */ +const MULTIPLIER_PRECISION = 100; + const LINEA_CHAIN_IDS: Hex[] = [ ChainId['linea-mainnet'], ChainId['linea-goerli'], @@ -150,9 +165,9 @@ export class LineaGasFeeFlow implements GasFeeFlow { multipliers: { low: number; medium: number; high: number }, ): FeesByLevel { const base = hexToBN(value); - const low = base.muln(multipliers.low); - const medium = base.muln(multipliers.medium); - const high = base.muln(multipliers.high); + const low = this.#applyMultiplier(base, multipliers.low); + const medium = this.#applyMultiplier(base, multipliers.medium); + const high = this.#applyMultiplier(base, multipliers.high); return { low, @@ -161,6 +176,25 @@ export class LineaGasFeeFlow implements GasFeeFlow { }; } + /** + * Multiplies a BN value by a decimal multiplier, quantized to + * `MULTIPLIER_PRECISION` decimal places, without `BN.muln`'s word-boundary + * truncation. All multipliers this flow uses have at most two decimal + * places, so this is exact for them; it is not a general-purpose + * arbitrary-precision decimal multiply. + * + * @param value - The value to multiply. + * @param multiplier - The decimal multiplier (e.g. 1.35). + * @returns The multiplied value. + */ + #applyMultiplier(value: BN, multiplier: number): BN { + return fractionBN( + value, + Math.round(multiplier * MULTIPLIER_PRECISION), + MULTIPLIER_PRECISION, + ); + } + #getMaxFees( baseFees: Record, priorityFees: Record, diff --git a/packages/user-operation-controller/CHANGELOG.md b/packages/user-operation-controller/CHANGELOG.md index 3115f045791..9e29c391fca 100644 --- a/packages/user-operation-controller/CHANGELOG.md +++ b/packages/user-operation-controller/CHANGELOG.md @@ -7,6 +7,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +- Fix `normalizeGasEstimate` silently under-computing the gas buffer for estimates that need more than one 26-bit `BN` word (values at or above ~67.1M) ([#10045](https://github.com/MetaMask/core/pull/10045)) + - Same root cause as the `LineaGasFeeFlow` fix in `@metamask/transaction-controller`: `BN.muln(1.5)` truncates instead of throwing once the value crosses the 26-bit-word boundary (~67.1M), e.g. `100000000 * 1.5` previously returned `116445568` instead of `150000000`. + ## [41.2.9] ### Changed diff --git a/packages/user-operation-controller/src/utils/gas.test.ts b/packages/user-operation-controller/src/utils/gas.test.ts index 04bbdaf9529..b7a0794ce99 100644 --- a/packages/user-operation-controller/src/utils/gas.test.ts +++ b/packages/user-operation-controller/src/utils/gas.test.ts @@ -123,6 +123,26 @@ describe('gas', () => { ); }); + it('applies the gas buffer multiplier without integer truncation for large estimates', async () => { + // 100_000_000 is above the 2^26 threshold at which `BN.muln` with a + // fractional multiplier silently truncates instead of throwing - + // `muln(1.5)` on this value previously returned 116445568 instead of + // the correct 150000000. + bundlerMock.estimateUserOperationGas.mockResolvedValue({ + callGasLimit: 100_000_000, + preVerificationGas: 456, + verificationGasLimit: 789, + }); + + await callUpdateGas(); + + expect(metadata.userOperation).toStrictEqual( + expect.objectContaining({ + callGasLimit: '0x8f0d180', + }), + ); + }); + it('if estimates are hexadecimal strings', async () => { bundlerMock.estimateUserOperationGas.mockResolvedValue( ESTIMATE_RESPONSE_HEX_MOCK, diff --git a/packages/user-operation-controller/src/utils/gas.ts b/packages/user-operation-controller/src/utils/gas.ts index ebc26d74402..6d4ef044c42 100644 --- a/packages/user-operation-controller/src/utils/gas.ts +++ b/packages/user-operation-controller/src/utils/gas.ts @@ -1,4 +1,4 @@ -import { hexToBN } from '@metamask/controller-utils'; +import { fractionBN, hexToBN } from '@metamask/controller-utils'; import { add0x } from '@metamask/utils'; import BN from 'bn.js'; @@ -17,6 +17,18 @@ const log = createModuleLogger(projectLogger, 'gas'); */ const GAS_ESTIMATE_MULTIPLIER = 1.5; +/** + * Precision used to convert `GAS_ESTIMATE_MULTIPLIER` into an integer + * numerator/denominator pair for `fractionBN`. `BN.muln` cannot be used + * directly with a fractional multiplier: it silently truncates to the + * integer part on a per-26-bit-word basis instead of throwing, so + * `value.muln(1.5)` can return a wrong, under-computed result once the gas + * estimate needs more than one 26-bit word (values at or above ~67.1M, i.e. + * 2^26, are susceptible, though not every value above that boundary is + * actually affected). + */ +const GAS_ESTIMATE_MULTIPLIER_PRECISION = 100; + /** * Populates the gas properties for a user operation. * @@ -87,7 +99,11 @@ function normalizeGasEstimate(rawValue: string | number): string { const value = typeof rawValue === 'string' ? hexToBN(rawValue) : new BN(rawValue); - const bufferedValue = value.muln(GAS_ESTIMATE_MULTIPLIER); + const bufferedValue = fractionBN( + value, + Math.round(GAS_ESTIMATE_MULTIPLIER * GAS_ESTIMATE_MULTIPLIER_PRECISION), + GAS_ESTIMATE_MULTIPLIER_PRECISION, + ); return add0x(bufferedValue.toString(16)); }