fix(tickers): select the most liquid ICP Swap pool for a token price

CHANGELOG-Nns-Dapp-unreleased.md

@@ -38,6 +38,9 @@ proposal is successful, the changes it released will be moved from this file to
 
 #### Security
 
+- Use the most liquid ICPSwap pool that has a price for a token, so a new pool
+  with one tiny trade cannot change the USD values shown.
+
 #### Not Published
 
 ### Operations

frontend/src/lib/services/icp-swap.provider.ts

@@ -6,11 +6,55 @@ import { ProviderErrors, type TickersData } from "$lib/types/tickers";
 import { mapEntries } from "$lib/utils/utils";
 import { isNullish } from "@dfinity/utils";
 
+// The tickers come from ICP Swap, so a field can be missing or hold something
+// that is not a number. Such a field counts as 0.
+const toAmount = (value: string): number => {
+  const amount = Number(value);
+  return Number.isFinite(amount) ? amount : 0;
+};
+
+// A price is usable only when it is a finite number above 0. ICP Swap reports
+// a last_price of 0 for a pool that nobody has traded.
+const hasUsablePrice = ({ last_price }: IcpSwapTicker): boolean => {
+  const price = Number(last_price);
+  return price > 0 && Number.isFinite(price);
+};
+
+// Keep the pool with the most liquidity, because an attacker must lock more
+// value than the real pool to be selected. A tie goes to the higher 24h
+// volume, then to the first pool in the feed.
+// The caller must pass at least one ticker. reduce with no initial value
+// throws on an empty array.
+const selectMostLiquidTicker = (tickers: IcpSwapTicker[]): IcpSwapTicker =>
+  tickers.reduce((selected, ticker) => {
+    const liquidityDifference =
+      toAmount(ticker.liquidity_in_usd) - toAmount(selected.liquidity_in_usd);
+    if (liquidityDifference !== 0) {
+      return liquidityDifference > 0 ? ticker : selected;
+    }
+    const volumeDifference =
+      toAmount(ticker.volume_usd_24H) - toAmount(selected.volume_usd_24H);
+    return volumeDifference > 0 ? ticker : selected;
+  });
+
+// Anybody can create an ICP Swap pool, so a token can have several pools and
+// each pool reports its own price. Select among the pools that carry a usable
+// price, because a pool with no usable price gives no rate. Otherwise an
+// untraded pool with more liquidity wins and the pair loses its price.
+// When no pool of the pair carries a usable price, keep the most liquid pool.
+// The price checks below then drop the pair, as they did before.
+const selectTickerForPair = (
+  tickersForPair: IcpSwapTicker[]
+): IcpSwapTicker => {
+  const tickersWithPrice = tickersForPair.filter(hasUsablePrice);
+  return selectMostLiquidTicker(
+    tickersWithPrice.length > 0 ? tickersWithPrice : tickersForPair
+  );
+};
+
 const adapter = (tickers: IcpSwapTicker[]): TickersData => {
   if (isNullish(tickers)) throw new Error(ProviderErrors.NO_DATA);
 
-  // The contents of icpSwapTickersStore come from ICP Swap, so there's no
-  // guarantee that it's format is as expected.
   const icpLedgerCanisterId = LEDGER_CANISTER_ID.toText();
 
   // First, get all ICP-based tickers
@@ -30,44 +74,31 @@ const adapter = (tickers: IcpSwapTicker[]): TickersData => {
     {} as Record<string, IcpSwapTicker[]>
   );
 
-  // Apply volume filter only when there are multiple tickers for the same pair
-  const filteredTickers = Object.values(tickersByBaseId).flatMap(
-    (tickersForPair) => {
-      if (tickersForPair.length === 1) {
-        // Single ticker for this pair - keep it regardless of volume
-        return tickersForPair;
-      } else {
-        // Multiple tickers for this pair - filter by volume
-        return (
-          tickersForPair.find((ticker) => Number(ticker.volume_usd_24H) > 0) ??
-          []
-        );
-      }
-    }
-  );
-
-  const ledgerCanisterIdToTicker: Record<string, IcpSwapTicker> =
-    Object.fromEntries(
-      filteredTickers.map((ticker) => [ticker.base_id, ticker])
-    );
+  // Keep one ticker per pair.
+  const ledgerCanisterIdToTicker: Record<string, IcpSwapTicker> = mapEntries({
+    obj: tickersByBaseId,
+    mapFn: ([baseId, tickersForPair]) => [
+      baseId,
+      selectTickerForPair(tickersForPair),
+    ],
+  });
 
   const ckusdcTicker =
     ledgerCanisterIdToTicker[CKUSDC_LEDGER_CANISTER_ID.toText()];
   if (isNullish(ckusdcTicker)) {
     throw new Error(ProviderErrors.INVALID_CKUSDC_PRICE);
   }
 
-  const icpPriceInCkusdc = Number(ckusdcTicker?.last_price);
-
-  if (icpPriceInCkusdc === 0 || !Number.isFinite(icpPriceInCkusdc)) {
+  if (!hasUsablePrice(ckusdcTicker)) {
     throw new Error(ProviderErrors.INVALID_ICP_PRICE);
   }
 
+  const icpPriceInCkusdc = Number(ckusdcTicker.last_price);
+
   const ledgerCanisterIdToUsdPrice: Record<string, number> = mapEntries({
     obj: ledgerCanisterIdToTicker,
     mapFn: ([ledgerCanisterId, ticker]) => {
-      const lastPrice = Number(ticker.last_price);
-      if (lastPrice === 0 || !Number.isFinite(lastPrice)) {
+      if (!hasUsablePrice(ticker)) {
         return undefined;
       }
       return [ledgerCanisterId, icpPriceInCkusdc / Number(ticker.last_price)];

frontend/src/tests/e2e/icp-swap-pool-selection.spec.ts

@@ -0,0 +1,103 @@
+import type { IcpSwapTicker } from "$lib/types/icp-swap";
+import { AppPo } from "$tests/page-objects/App.page-object";
+import { PlaywrightPageObjectElement } from "$tests/page-objects/playwright.page-object";
+import {
+  dfxCanisterId,
+  disableCssAnimations,
+  signInWithNewUser,
+  step,
+} from "$tests/utils/e2e.test-utils";
+import { expect, test } from "@playwright/test";
+
+// The ICP price in USD that the banner shows is the `last_price` of the
+// ckUSDC ticker. The app formats it with `Intl.NumberFormat("fr-FR")` and two
+// fraction digits, so 4 becomes "4,00" and 400 becomes "400,00".
+const REAL_ICP_PRICE = "4";
+const ATTACKER_ICP_PRICE = "400";
+const REAL_ICP_PRICE_SHOWN = "4,00";
+
+const ticker = (overrides: Partial<IcpSwapTicker>): IcpSwapTicker => ({
+  ticker_id: "ne2vj-6yaaa-aaaag-qb3ia-cai",
+  ticker_name: "CKUSDC_ICP",
+  base_id: "2ouva-viaaa-aaaaq-aaamq-cai",
+  base_currency: "ckUSDC",
+  target_id: "ryjl3-tyaaa-aaaaa-aaaba-cai",
+  target_currency: "ICP",
+  last_price: "1",
+  base_volume: "0",
+  target_volume: "0",
+  base_volume_24H: "0",
+  target_volume_24H: "0",
+  total_volume_usd: "0",
+  volume_usd_24H: "0",
+  fee_usd: "0",
+  liquidity_in_usd: "0",
+  ...overrides,
+});
+
+test("The ICP price comes from the most liquid ICPSwap pool", async ({
+  page,
+  context,
+}) => {
+  const icpLedgerCanisterId = await dfxCanisterId("nns-ledger");
+  const ckusdcLedgerCanisterId = await dfxCanisterId("ckusdc_ledger");
+
+  // The real ckUSDC pool holds value but nobody traded it in the last 24
+  // hours. This is true of 296 of the 407 ICP pairs in the live feed.
+  const realCkusdcTicker = ticker({
+    base_id: ckusdcLedgerCanisterId,
+    target_id: icpLedgerCanisterId,
+    last_price: REAL_ICP_PRICE,
+    liquidity_in_usd: "617000",
+    volume_usd_24H: "0",
+  });
+
+  // Anybody can create a second ICPSwap pool for the same pair. This one holds
+  // almost nothing, carries one wash trade and reports an outlier price. The
+  // old code selected it, because it was the first ticker with a volume.
+  const attackerCkusdcTicker = ticker({
+    base_id: ckusdcLedgerCanisterId,
+    target_id: icpLedgerCanisterId,
+    last_price: ATTACKER_ICP_PRICE,
+    liquidity_in_usd: "5",
+    volume_usd_24H: "1",
+  });
+
+  let tickersRequested = false;
+  await page.route("**/tickers", async (route) => {
+    tickersRequested = true;
+    await route.fulfill({
+      // The attacker pool comes first, the order an attacker would want.
+      json: [attackerCkusdcTicker, realCkusdcTicker],
+    });
+  });
+
+  await page.goto("/accounts");
+  await disableCssAnimations(page);
+  await expect(page).toHaveTitle("Account | Network Nervous System");
+
+  await signInWithNewUser({ page, context });
+
+  const pageElement = PlaywrightPageObjectElement.fromPage(page);
+  const appPo = new AppPo(pageElement);
+  const usdValueBannerPo = appPo
+    .getAccountsPo()
+    .getNnsAccountsPo()
+    .getUsdValueBannerPo();
+  await usdValueBannerPo.waitFor();
+
+  // The app asks ICPSwap for the tickers only when the network configures an
+  // ICPSwap URL. `dfx.json` gives that URL to mainnet, app and beta, not to
+  // the local network, so the local app shows no price at all.
+  await page.waitForTimeout(5_000);
+  test.skip(
+    !tickersRequested,
+    "This network configures no ICPSwap URL, so the app requests no tickers."
+  );
+
+  step("The banner shows the price of the most liquid pool");
+
+  const icpPrice = await usdValueBannerPo.getIcpPrice();
+  expect(icpPrice).toBe(REAL_ICP_PRICE_SHOWN);
+  expect(await usdValueBannerPo.hasError()).toBe(false);
+});