docs: audit BigNumber 15-sig-digit issue in @metamask/assets-controller#8806
Draft
Prithpal-Sooriya wants to merge 6 commits into
Draft
docs: audit BigNumber 15-sig-digit issue in @metamask/assets-controller#8806Prithpal-Sooriya wants to merge 6 commits into
@metamask/assets-controller#8806Prithpal-Sooriya wants to merge 6 commits into
Conversation
Co-authored-by: Prithpal Sooriya <prithpal.sooriya@gmail.com>
…ounded numbers Co-authored-by: Prithpal Sooriya <prithpal.sooriya@gmail.com>
Co-authored-by: Prithpal Sooriya <prithpal.sooriya@gmail.com>
…ults Co-authored-by: Prithpal Sooriya <prithpal.sooriya@gmail.com>
…sion + toFixed Co-authored-by: Prithpal Sooriya <prithpal.sooriya@gmail.com>
…tent 'e' edge Co-authored-by: Prithpal Sooriya <prithpal.sooriya@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Explanation
A separate audit on
metamask-extensionflagged thatbignumber.jsrejects (or, depending onBigNumber.DEBUG, silently truncates) JSnumberprimitives carrying more than 15 significant digits. The original report focused onCurrencyRateController.boundedPrecisionNumberin the old@metamask/assets-controllerspackage, which usestoFixed(9)(decimal places) rather thantoPrecision(15)(significant digits) and therefore stores values like40115252.21304121and1.0000000000000002in state. Downstream code that wraps those values innew BigNumber(...)crashes (underDEBUG=true) or silently loses precision (otherwise).This PR adds an audit report that answers the question "is the same class of bug present in the new
@metamask/assets-controllerpackage?" — short answer: yes, in three distinct call paths:PriceDataSourcewrites Price API numeric fields (price,usdPrice,marketCap,allTimeHigh,allTimeLow,circulatingSupply,totalVolume,pricePercentChange*) straight intoassetsPricestate with no precision bound.multipliedBy(jsNumber)sites (selectors/balance.tsline 471 andAssetsController.tsline 2368) feed those unbounded numbers back into BigNumber math;.multipliedBy(y)internally executesnew BigNumber(y), so the same constructor branch is hit.formatExchangeRatesForBridge/formatStateForTransactionPayre-emit unbounded numbers (includingusdPrice / nativeAssetUsdPrice, a classic float-division 17-sig-digit producer) into the legacyCurrencyRateController/TokenRatesControllershape that@metamask/bridge-controller's selectors consume by directly wrapping the values innew BigNumber(...)— i.e., the exact crash reproducer described in the upstream report, just routed through this package's bridge-compat shim.The audit also classifies every
BigNumber/BigNumberJSsite in this package (safe vs. at-risk) and recommends two layered fixes:boundedPrecisionNumberusingtoPrecision(15)(significant digits, nottoFixed(9)decimal places) applied at the API → state boundary inPriceDataSource, and insideformatExchangeRatesForBridgeforconversionRate,usdConversionRate, and the derivedpriceInNative = usdPrice / nativeAssetUsdPrice.AssetPrice.*numeric fields tostring(mirroringFungibleAssetBalance.amount). The audit explains why this is a viable internal change but does not remove the need to bound numbers at the bridge-compat shim layer (the legacy shapes this package emits to are owned by the older@metamask/assets-controllerspackage and typed asnumber, and the bridge selector wraps those exactnumberfields innew BigNumber(...)).This PR is documentation only — no source/behaviour change yet. It's intended as the analysis artefact for the team that owns the controller before any code fix lands.
References
CHANGELOG.mdentry forassets-controllersreferencing PR fix: add decimal bounds to currency rates #7324 ("Added decimal precision (default 9dp) forCurrencyRateControllerconversionRate... fixes any BigNumber conversion errors due to exceeding the 15 significant digit limit")boundedPrecisionNumberfix in the older@metamask/assets-controllersis deliberately out of scope here per the original request.Checklist