Skip to content

Commit

Permalink
Fix Ledger message signing on built-in non-ETH/MATIC networks
Browse files Browse the repository at this point in the history
Ledger was checking specifically for those two networks from a constant
that was never updated. We switch instead to check whether the network
in question shares a derivation path with the one used by the Eth app
(aka the Ethereum derivation path).

Custom networks added by users currently don't set a derivation path, so
they will still fail this test (as well as the guards on signing
messages and typed messages, which are already using derivation paths).
  • Loading branch information
Shadowfiend committed Jul 15, 2024
1 parent ecf7b27 commit 1c6ff61
Show file tree
Hide file tree
Showing 2 changed files with 8 additions and 7 deletions.
2 changes: 0 additions & 2 deletions background/constants/networks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -162,8 +162,6 @@ export const TEST_NETWORK_BY_CHAIN_ID = new Set(
[SEPOLIA, ARBITRUM_SEPOLIA].map((network) => network.chainID),
)

export const NETWORK_FOR_LEDGER_SIGNING = [ETHEREUM, POLYGON]

// Networks that are not added to this struct will
// not have an in-wallet Swap page
export const CHAIN_ID_TO_0X_API_BASE: {
Expand Down
13 changes: 8 additions & 5 deletions background/services/ledger/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ import {
import {
isEIP1559TransactionRequest,
isKnownTxType,
sameNetwork,
SignedTransaction,
TransactionRequestWithNonce,
} from "../../networks"
Expand All @@ -25,7 +24,7 @@ import { ServiceCreatorFunction, ServiceLifecycleEvents } from "../types"
import logger from "../../lib/logger"
import { getOrCreateDB, LedgerAccount, LedgerDatabase } from "./db"
import { ethersTransactionFromTransactionRequest } from "../chain/utils"
import { NETWORK_FOR_LEDGER_SIGNING } from "../../constants"
import { ETHEREUM } from "../../constants"
import { normalizeEVMAddress } from "../../lib/utils"
import { AddressOnNetwork } from "../../accounts"

Expand Down Expand Up @@ -542,10 +541,14 @@ export default class LedgerService extends BaseService<Events> {
{ address, network }: AddressOnNetwork,
hexDataToSign: HexString,
): Promise<string> {
// Currently the service assumes the Eth app, which requires a network that
// uses the same derivation path as Ethereum, or one that starts with the
// same components.
// FIXME This should take a `LedgerAccountSigner` and use `checkCanSign`
// FIXME like other signing methods.
if (
!NETWORK_FOR_LEDGER_SIGNING.find((supportedNetwork) =>
sameNetwork(network, supportedNetwork),
)
network.derivationPath !== ETHEREUM.derivationPath &&
!network.derivationPath?.startsWith(ETHEREUM.derivationPath ?? "")
) {
throw new Error("Unsupported network for Ledger signing")
}
Expand Down

1 comment on commit 1c6ff61

@Lydmilaa
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok

Please sign in to comment.