MetaMask / MetaMask/metamask-extension

Multichain: Audit MetaMaskController's `getApi` usages for chain ID

Open
#28,268 0 comments 0 reactions 0 assignees View on GitHub
INVALID-ISSUE-TEMPLATE multichain-final-boss
Dominant language
TypeScript
Stars
13.2k
Forks
5.6k
Avg merge
2d 5h
Merged PRs (30d)
451

Description

### What is this about?

Work was done months ago to bring Controllers to a multichain architecture. That said, there could be regression. All of the methods that `app/scripts/metamask-controller.js` 's `getApi` method should be audited to ensure they are passed a `chainId` and not assume the global chain Id.

Per Mark:

> Back to your question, the general area of concern I had was "every background operation that assumes it's for the globally selected chain ID". I had mentioned in a previous meeting something about auditing controller method calls or background API calls. It should be the case that all controller operations pertaining to a given chain accept a chainId parameter (apart from TokenBalancesController maybe, which someone is handling). But in many cases it's optional, and the globally selected chain is used as a default. Once we enable multichain, that would be a bug. The chain ID needs to be passed through everywhere.

> Some subset of them accept an optional chainId or networkClientId. We could read each one to come up with that smaller list, then look at the callsites to ensure that it's always passed in.

## WIP Audit

```
Legend:
* "-" have been studied and either have no reliance on chainId or properly accept a chainId argument
* "!" likely need controller work to not assume current globally selected chain ID
* "?" require more digging

CurrencyRateController
=================================
- setCurrentCurrency
- startPolling
- stopPollingByPollingToken

PreferencesController
=================================
- setUseBlockie
- setUseNonceField
- setUsePhishDetect
- setUseMultiAccountBalanceChecker
- setUseSafeChainsListValidation
- setUseTokenDetection
- setUseNftDetection
- setUse4ByteResolution
- setUseCurrencyRateCheck
- setOpenSeaEnabled
- getUseRequestQueue
- setSecurityAlertsEnabled
- setAddSnapAccountEnabled
- setWatchEthereumAccountEnabled
- setSolanaSupportEnabled
- setBitcoinSupportEnabled
- setBitcoinTestnetSupportEnabled
- setUseExternalNameSources
- setUseTransactionSimulations
- setIpfsGateway
- setIsIpfsGatewayEnabled
- setUseAddressBarEnsResolution
- setCurrentLocale
- setIncomingTransactionsPreferences
- setServiceWorkerKeepAlivePreference
- setFeatureFlag
- setPreference
- addKnownMethodData
- setDismissSeedBackUpReminder
- setAdvancedGasFee
- setTheme
- setSnapsAddSnapAccountModalDismissed
- setSelectedAddress
- setUseRequestQueue
- setPasswordForgotten
- toggleExternalServices

MetaMetricsController
=================================
`app/scripts/controllers/metametrics.ts` is affected heavily by https://github.com/MetaMask/metamask-extension/issues/28304

- setParticipateInMetaMetrics
- setDataCollectionForMarketing
- setMarketingCampaignCookieId
- metaMetricsController
- trackEvent
- trackPage
- createEventFragment
- updateEventFragment
- finalizeEventFragment

KeyringController
=================================
- submitQRCryptoHDKey
- submitQRCryptoAccount
- cancelQRSynchronization
- submitQRSignature
- cancelQRSignRequest
- setLocked
- createNewVaultAndKeychain
- createNewVaultAndRestore
- exportAccount

AccountsController
=================================
- getNextAvailableAccountName
- setSelectedInternalAccount
- setAccountName
- setAccountLabel

MetaMetricsDataDeletionController
=================================
- updateDataDeletionTaskStatus
- createMetaMetricsDataDeletionTask

MultichainBalancesController
=================================
- updateBalances
- updateBalance

NameController
=================================
- setName
- updateProposedNames

NotificationServicesController
=================================
? checkAccountsPresence
? createOnChainTriggers
? deleteOnChainTriggersByAccount
? updateOnChainTriggersByAccount
? fetchAndUpdateMetamaskNotifications
? markMetamaskNotificationsAsRead
? setFeatureAnnouncementsEnabled
? enablePushNotifications
? disablePushNotifications
? updateTriggerPushNotifications
? enableMetamaskNotifications
? disableMetamaskNotifications

UserStorageController
=================================
- enableProfileSyncing
- disableProfileSyncing
- setIsProfileSyncingEnabled
- syncInternalAccountsWithUserStorage
- performDeleteStorageAllFeatureEntries

AuthenticationController
=================================
- performSignIn
- performSignOut

AssetsContractController
=================================
! getBalancesInSingleCall - Calls `getCorrectChainId` which has a `NetworkController:getState` usage

NftDetectionController
=================================
! detectNfts - Uses `NetworkController:getNetworkClientById` to get the `chainId`

TransactionController
=================================
- updateTransaction
- approveTransactionsWithSameNonce
- retryTransaction
- stopTransaction
- createSpeedUpTransaction
- estimateGasFee
? getNonceLock - `networkClientId` is optional
! getTransactions - uses `getChainId`, `this.#getGlobalChainId()`, `this.#getGlobalNetworkClientId()`
- updateEditableParams
- updateTransactionGasFees
- updateTransactionSendFlowHistory
- updatePreviousGasParams
- abortTransactionSigning
? getLayer1GasFee - `chainId` is optional

DecryptMessageController
=================================
- decryptMessage
- decryptMessageInline
- cancelDecryptMessage

EncryptionPublicKeyController
=================================
- encryptionPublicKey
- cancelEncryptionPublicKey

OnboardingController
=================================
- setSeedPhraseBackedUp
- completeOnboarding
- setFirstTimeFlowType

AlertController
=================================
- setAlertEnabledness
- setUnconnectedAccountAlertShown
- setWeb3ShimUsageAlertDismissed

SmartTransactionsController
=================================
- setStatusRefreshInterval
! updateSmartTransaction - falls back to #this.chainId and uses `NetworkController:getNetworkClientById`
! fetchLiveness - uses `this.#getChainId({ networkClientId })`
! cancelSmartTransaction - uses `this.#getChainId({ networkClientId })`
! submitSignedTransactions - uses `this.#getChainId({ networkClientId })`
- clearFees
! getFees - uses `this.#getChainId({ networkClientId })`

TokensController
=================================
- ignoreTokens
! addImportedTokens - Calls 'NetworkController:getNetworkClientById'
! addDetectedTokens - Uses an initial chainId as fallback. Additionally has an `onNetworkDidChange` to store the fallback chainId
- addToken
? updateTokenType - `networkClientId` is an optional param

TokenRatesController
=================================
- startPolling
- stopPollingByPollingToken

ApprovalController
=================================
- accept
- reject
- addAndShowApprovalRequest

TokenDetectionController
=================================
? detectTokens - Accepts `networkClientId`

Backup
=================================
- backupUserData
- restoreUserData

AppStateController
=================================
- removePollingToken
- addPollingToken
- setLastActiveTime
- setCurrentExtensionPopupId
- setDefaultHomeActiveTabName
- setConnectedStatusPopoverHasBeenShown
- setRecoveryPhraseReminderHasBeenShown
- setRecoveryPhraseReminderLastShown
- setTermsOfUseLastAgreed
- setSurveyLinkLastClickedOrClosed
- setOnboardingDate
- setLastViewedUserSurvey
- setNewPrivacyPolicyToastClickedOrClosed
- setNewPrivacyPolicyToastShownDate
- setSnapsInstallPrivacyWarningShownStatus
- setOutdatedBrowserWarningLastShown
- setShowTestnetMessageInDropdown
- setShowBetaHeader
- setShowPermissionsTour
- setShowAccountBanner
- setShowNetworkBanner
- updateNftDropDownState
- setFirstTimeUsedNetwork
- setSwitchedNetworkDetails
- clearSwitchedNetworkDetails
- setSwitchedNetworkNeverShowMessage
- getLastInteractedConfirmationInfo
- setLastInteractedConfirmationInfo

GasFeeController
=================================
- getTimeEstimate
- stopPollingByPollingToken
- startPollingByNetworkClientId

AnnouncementController
=================================
- resetViewed
- updateViewed

PermissionController
=================================
- grantPermissionsIncremental
- grantPermissions
- revokePermissions
- acceptPermissionsRequest
- rejectPermissionsRequest
-

NotificationManager
=================================
- markAsAutomaticallyClosed

NetworkController
=================================
? setActiveNetwork - Will this go away?
? setActiveNetworkConfigurationId - Will this go away?
? rollbackToPreviousProvider
- addNetwork
- updateNetwork
- removeNetwork
- getCurrentNetworkEIP1559Compatibility
- getNetworkConfigurationByNetworkClientId

SelectedNetworkController
=================================
? setNetworkClientIdForDomain - Will this go away?

NftController
=================================
? addNft - `networkClientId` is not required
? addNftVerifyOwnership - `networkClientId` is not required
? removeAndIgnoreNft - `networkClientId` is not required
? removeNft - `networkClientId` is not required
? checkAndUpdateAllNftsOwnershipStatus - `networkClientId` is not required
? checkAndUpdateSingleNftOwnershipStatus - `networkClientId` is not required
- getNFTContractInfo
? isNftOwner - `networkClientId` is not required

AddressBookController
=================================
- set
- delete

ENSController
=================================
- reverseResolveAddress

MMIController
=================================
- connectCustodyAddresses
- getCustodianAccounts
- getCustodianTransactionDeepLink
- getCustodianConfirmDeepLink
- getCustodianSignMessageDeepLink
- getCustodianToken
- getCustodianJWTList
- getAllCustodianAccountsWithToken
- setCustodianNewRefreshToken

CustodyController
=================================
- setWaitForConfirmDeepLinkDialog
- getConfiguration
- removeAddTokenConnectRequest
- setConnectionRequest
- showInteractiveReplacementTokenBanner
- setCustodianDeepLink
- setNoteToTraderMessage
- logAndStoreApiRequest

SnapController
=================================
- disableSnap
- enableSnap
- install
- removeSnap
- handleSnapRequest
- revokeDynamicSnapPermissions
- dismissNotifications
- markNotificationsAsRead
- SnapController:disconnectOrigin

AccountOrderController
=================================
- updateNetworksList
- updateAccountsList
- updateHiddenAccountsList

PhishingController
=================================
- maybeUpdateState
-

Misc
=================================
- getRequestAccountTabIds
- openMetamaskTabsIDs
- addNewAccount
- getSeedPhrase
- submitPassword
- verifyPassword
- decodeTransactionData
- throwTestError
- trackInsightSnapView
- resetAccount
- removeAccount
- importAccountWithStrategy
- getAccountsBySnapId
- connectHardware
- forgetDevice
- checkHardwareStatus
- getDeviceNameForMetric
- unlockHardwareWalletAccount
- attemptLedgerTransportCreation
- getTokenStandardAndDetails
- getTokenSymbol
- estimateGas
? addTransaction - does `dappRequest?.networkClientId ?? this.networkController.state.selectedNetworkClientId,`
? getAddTransactionRequest - does `dappRequest?.networkClientId ?? this.networkController.state.selectedNetworkClientId,`
- createTransactionEventFragment
- getPermissionBackgroundApiMethods

// Left to audit:
getApi() {

return {
getProviderConfig: () =>
getProviderConfig({
metamask: this.networkController.state,
})

deleteInterface: this.controllerMessenger.call.bind(
this.controllerMessenger,
'SnapInterfaceController:deleteInterface',
),
updateInterfaceState: this.controllerMessenger.call.bind(
this.controllerMessenger,
'SnapInterfaceController:updateInterfaceState',
),

// swaps
fetchAndSetQuotes: this.controllerMessenger.call.bind(
this.controllerMessenger,
'SwapsController:fetchAndSetQuotes',
),
setSelectedQuoteAggId: this.controllerMessenger.call.bind(
this.controllerMessenger,
'SwapsController:setSelectedQuoteAggId',
),
resetSwapsState: this.controllerMessenger.call.bind(
this.controllerMessenger,
'SwapsController:resetSwapsState',
),
setSwapsTokens: this.controllerMessenger.call.bind(
this.controllerMessenger,
'SwapsController:setSwapsTokens',
),
clearSwapsQuotes: this.controllerMessenger.call.bind(
this.controllerMessenger,
'SwapsController:clearSwapsQuotes',
),
setApproveTxId: this.controllerMessenger.call.bind(
this.controllerMessenger,
'SwapsController:setApproveTxId',
),
setTradeTxId: this.controllerMessenger.call.bind(
this.controllerMessenger,
'SwapsController:setTradeTxId',
),
setSwapsTxGasPrice: this.controllerMessenger.call.bind(
this.controllerMessenger,
'SwapsController:setSwapsTxGasPrice',
),
setSwapsTxGasLimit: this.controllerMessenger.call.bind(
this.controllerMessenger,
'SwapsController:setSwapsTxGasLimit',
),
setSwapsTxMaxFeePerGas: this.controllerMessenger.call.bind(
this.controllerMessenger,
'SwapsController:setSwapsTxMaxFeePerGas',
),
setSwapsTxMaxFeePriorityPerGas: this.controllerMessenger.call.bind(
this.controllerMessenger,
'SwapsController:setSwapsTxMaxFeePriorityPerGas',
),
safeRefetchQuotes: this.controllerMessenger.call.bind(
this.controllerMessenger,
'SwapsController:safeRefetchQuotes',
),
stopPollingForQuotes: this.controllerMessenger.call.bind(
this.controllerMessenger,
'SwapsController:stopPollingForQuotes',
),
setBackgroundSwapRouteState: this.controllerMessenger.call.bind(
this.controllerMessenger,
'SwapsController:setBackgroundSwapRouteState',
),
resetPostFetchState: this.controllerMessenger.call.bind(
this.controllerMessenger,
'SwapsController:resetPostFetchState',
),
setSwapsErrorKey: this.controllerMessenger.call.bind(
this.controllerMessenger,
'SwapsController:setSwapsErrorKey',
),
setInitialGasEstimate: this.controllerMessenger.call.bind(
this.controllerMessenger,
'SwapsController:setInitialGasEstimate',
),
setCustomApproveTxData: this.controllerMessenger.call.bind(
this.controllerMessenger,
'SwapsController:setCustomApproveTxData',
),
setSwapsLiveness: this.controllerMessenger.call.bind(
this.controllerMessenger,
'SwapsController:setSwapsLiveness',
),
setSwapsFeatureFlags: this.controllerMessenger.call.bind(
this.controllerMessenger,
'SwapsController:setSwapsFeatureFlags',
),
setSwapsUserFeeLevel: this.controllerMessenger.call.bind(
this.controllerMessenger,
'SwapsController:setSwapsUserFeeLevel',
),
setSwapsQuotesPollingLimitEnabled: this.controllerMessenger.call.bind(
this.controllerMessenger,
'SwapsController:setSwapsQuotesPollingLimitEnabled',
),

// Bridge
[BridgeBackgroundAction.SET_FEATURE_FLAGS]:
this.controllerMessenger.call.bind(
this.controllerMessenger,
`${BRIDGE_CONTROLLER_NAME}:${BridgeBackgroundAction.SET_FEATURE_FLAGS}`,
),
[BridgeUserAction.SELECT_SRC_NETWORK]: this.controllerMessenger.call.bind(
this.controllerMessenger,
`${BRIDGE_CONTROLLER_NAME}:${BridgeUserAction.SELECT_SRC_NETWORK}`,
),
[BridgeUserAction.SELECT_DEST_NETWORK]:
this.controllerMessenger.call.bind(
this.controllerMessenger,
`${BRIDGE_CONTROLLER_NAME}:${BridgeUserAction.SELECT_DEST_NETWORK}`,
),
[BridgeUserAction.UPDATE_QUOTE_PARAMS]:
this.controllerMessenger.call.bind(
this.controllerMessenger,
`${BRIDGE_CONTROLLER_NAME}:${BridgeUserAction.UPDATE_QUOTE_PARAMS}`,
),
}
```

Contributor guide

Open the contributing guide

Research direction

Start in app/scripts/metamask-controller.js and review the listed getApi usages across the controllers, beginning with entries marked ! or ?. Compare each call with its chainId or networkClientId handling and trace the relevant callsites. Done means the audit is complete and operations for a given chain no longer rely on the globally selected chain ID.

Written by the indexing model from the issue text.

Assessment

Tech stack
typescript
Domain
api, backend
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.