MetaMask / MetaMask/core

fix: RpcBalanceFetcher reports a failed native balance read as success:true, value:0 (zeroes real balances)

Open
#10,043 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
TypeScript
Stars
413
Forks
308
Avg merge
1d 4h
Merged PRs (30d)
253

Description

What it says on the box: a failed native-balance read and a genuinely-zero native balance are indistinguishable in `RpcBalanceFetcher`, and the failed case wins, wiping the previously-known balance to `0x0`.

## Where

`packages/assets-controllers/src/rpc-service/rpc-balance-fetcher.ts:164-172` (native), `:204-212` (staked). Compare to the ERC-20 branch 12 lines below in the same function, which does it correctly:

```ts
// native, :164-172
allAddressesForNative.forEach((address) => {
const nativeBalance = tokenBalances[ZERO_ADDRESS]?.[address] || null;
chainResults.push({
success: true, // hard-coded, regardless of whether a read happened
value: nativeBalance || new BN('0'), // absent collapses into zero
...
});
});
...
// erc-20, :182 — same function
success: bn !== null,
```

## Why `success: true` is provably wrong here

An absent entry for `ZERO_ADDRESS` in `tokenBalances` can *only* mean the read failed, never a genuine zero:

- `processBalanceResults` (`multicall.ts:674-687`) writes `balanceMap[ZERO_ADDRESS][userAddress]` only inside `if (result.success)`. A genuine on-chain zero still comes back `success: true` from `aggregate3` (`allowFailure: true`) and gets written as `new BN(0)`. Only a failed subcall leaves the key absent.
- `getNativeBalancesFallback` (`multicall.ts:730-765`) records a result only when `status === 'fulfilled'`; a rejected `eth_getBalance` leaves no key.
- `getStakedBalancesForAddresses` (`:983-985`) catches any throw and returns `{}` — one RPC error zeroes every account on that chain.

So the biconditional holds: absent key ⟺ the underlying read failed. The native branch treats "absent" as "confirmed zero" instead.

## Write path (unconditional on `success`)

`TokenBalancesController.ts:1131` filters on `balance.success`, which is hard-coded `true` for native → `AccountTrackerController.updateNativeBalances` (`:996-1041`) sets `accountsByChainId[chainId][addr].balance = '0x0'` and persists it. `isDeprecated` defaults to `() => false` (`AccountTrackerController.ts:302`), so this lane is live by default, not behind a flag.

## Repro

Ported the four functions above verbatim; simulated a custom network → fallback lane where one address's `eth_getBalance` rejects (429):

```
BEFORE Alice balance: 0x4563918244f40000 = 5 ETH
fallback returned keys: [ '0xbbbb…' ] <- Alice absent (her read rejected, not zero)
NativeBalanceUpdate[] : [{"address":"0xaaaa…","chainId":"0x1f4","balance":"0x0"}]
AFTER Alice balance: 0x0 <- 5 ETH wiped, no error surfaced
```

## Reachability

Not exotic — any of these trigger the fallback/failure lane:
- Any user-added custom network with no `MULTICALL_CONTRACT_BY_CHAINID` entry routes every native read through the flaky per-address fallback.
- Any chain where `aggregate3` throws drops to the same fallback (`multicall.ts:1160-1180`).
- Mainnet/hoodi staked-balance reads: one throw in `getStakedBalancesForAddresses` zeroes every account on the chain in one shot.

## Anticipated objection, addressed in advance

`rpc-balance-fetcher.test.ts:910` pins *"should always include native token entry … even when balance is zero"*, so a zero-fill looks intentional at first read. But that premise doesn't apply to native: a *successful* read of a genuine zero already writes a key via the `aggregate3`/`new BN(0)` path shown above. The unconditional zero-fill has no legitimate case to serve on the native side — it only ever converts a failure into a fabricated zero. Precedent for the correct behavior already exists in the sibling fetcher: `api-balance-fetcher.ts:475-477`, *"without overwriting potentially stale balances with zero values"*.

The staked-balance face is genuinely different — `multicall.ts:940` filters `shares.gt(0)`, so an absent key legitimately does mean zero there — so I'm not proposing a change to that path here.

## Suggested direction

Mirror the ERC-20 branch on the native side: `success: nativeBalance !== null` instead of the hard-coded `true`. Filing as an issue rather than a PR first since the fix touches a path with an existing (arguably now-misleading) pinned test, and I'd rather get a maintainer read before reworking it.

Happy to open the PR once there's agreement on direction.

Contributor guide

No contributing guide indexed for this repository

Research direction

Start in packages/assets-controllers/src/rpc-service/rpc-balance-fetcher.ts at the native branch around lines 164-172, then compare its result handling with the ERC-20 branch and rpc-balance-fetcher.test.ts around line 910. Read the related paths in multicall.ts and the write flow through TokenBalancesController.ts and AccountTrackerController.ts. Done means failed native reads remain distinguishable from genuine zero balances and the relevant tests cover both cases.

Written by the indexing model from the issue text.

Assessment

Tech stack
typescript
Domain
api, backend
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
75/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.