fix(tokens): confirm portfolio token balances with certified calls

CHANGELOG-Nns-Dapp-unreleased.md

@@ -38,6 +38,10 @@ proposal is successful, the changes it released will be moved from this file to
 
 #### Security
 
+- The Portfolio, Tokens and Staking pages now confirm every token balance with
+  a certified call. Before, they showed the answer of one replica and never
+  checked it.
+
 #### Not Published
 
 ### Operations

frontend/src/lib/services/accounts-balances.services.ts

@@ -1,5 +1,5 @@
-import { uncertifiedLoadSnsesAccountsBalances } from "$lib/services/sns-accounts-balance.services";
-import { uncertifiedLoadAccountsBalance } from "$lib/services/wallet-uncertified-accounts.services";
+import { syncIcrcAccountsBalances } from "$lib/services/icrc-accounts-balance.services";
+import { syncSnsAccountsBalances } from "$lib/services/sns-accounts-balance.services";
 import type { UniverseCanisterIdText } from "$lib/types/universe";
 import type { CanisterIdString } from "@icp-sdk/canisters/nns";
 import { Principal } from "@icp-sdk/core/principal";
@@ -23,7 +23,7 @@ export const loadSnsAccountsBalances = async (
 
   if (notLoadedIds.length === 0) return;
 
-  await uncertifiedLoadSnsesAccountsBalances({
+  await syncSnsAccountsBalances({
     rootCanisterIds: notLoadedIds.map((id) => Principal.fromText(id)),
   });
 };
@@ -35,7 +35,7 @@ export const loadAccountsBalances = async (
 
   if (notLoadedIds.length === 0) return;
 
-  await uncertifiedLoadAccountsBalance({
+  await syncIcrcAccountsBalances({
     universeIds: notLoadedIds,
   });
 };

frontend/src/lib/services/icrc-accounts-balance.services.ts

@@ -0,0 +1,33 @@
+import { syncAccounts } from "$lib/services/icrc-accounts.services";
+import { toastsError } from "$lib/stores/toasts.store";
+import type { UniverseCanisterIdText } from "$lib/types/universe";
+import { Principal } from "@icp-sdk/core/principal";
+
+/**
+ * Load Icrc accounts balances and token.
+ *
+ * The query answer shows first and the certified answer replaces it. If the
+ * certified answer arrives first, the query answer is skipped.
+ *
+ * @param {universeIds: UniverseCanisterIdText[]} params
+ * @param {UniverseCanisterIdText[]} params.universeIds The Icrc environment for which the balances should be loaded.
+ */
+export const syncIcrcAccountsBalances = async ({
+  universeIds,
+}: {
+  universeIds: UniverseCanisterIdText[];
+}): Promise<void> => {
+  const results = await Promise.allSettled(
+    universeIds.map((universeId) =>
+      syncAccounts({
+        ledgerCanisterId: Principal.fromText(universeId),
+      })
+    )
+  );
+
+  const error: boolean =
+    results.find(({ status }) => status === "rejected") !== undefined;
+  if (error) {
+    toastsError({ labelKey: "error.accounts_load" });
+  }
+};

frontend/src/lib/services/sns-accounts-balance.services.ts

@@ -5,12 +5,13 @@ import type { RootCanisterId } from "$lib/types/sns";
 /**
  * Load Sns projects accounts balances.
  *
- * ⚠️ WARNING: this feature only performs "query" calls. Effective "update" is performed when a Sns project is manually selected either through the token navigation switcher or accessed directly via the browser url.
+ * The query answer shows first and the certified answer replaces it. If the
+ * certified answer arrives first, the query answer is skipped.
  *
- * @param {rootCanisterIds: RootCanisterId[], excludeRootCanisterIds?: RootCanisterIdText[]} params
+ * @param {rootCanisterIds: RootCanisterId[]} params
  * @param {RootCanisterId[]} params.rootCanisterIds The list of root canister ids - Sns projects - for which the balance of the accounts should be fetched.
  */
-export const uncertifiedLoadSnsesAccountsBalances = async ({
+export const syncSnsAccountsBalances = async ({
   rootCanisterIds,
 }: {
   rootCanisterIds: RootCanisterId[];
@@ -19,7 +20,6 @@ export const uncertifiedLoadSnsesAccountsBalances = async ({
     rootCanisterIds.map((rootCanisterId) =>
       loadSnsAccounts({
         rootCanisterId,
-        strategy: "query",
       })
     )
   );

frontend/src/routes/(app)/(nns)/tokens/+page.svelte

@@ -26,8 +26,8 @@
   import { loadCkBTCTokens } from "$lib/services/ckbtc-tokens.services";
   import { loadIcpSwapTickers } from "$lib/services/icp-swap.services";
   import { removeImportedTokens } from "$lib/services/imported-tokens.services";
-  import { uncertifiedLoadSnsesAccountsBalances } from "$lib/services/sns-accounts-balance.services";
-  import { uncertifiedLoadAccountsBalance } from "$lib/services/wallet-uncertified-accounts.services";
+  import { syncSnsAccountsBalances } from "$lib/services/sns-accounts-balance.services";
+  import { syncIcrcAccountsBalances } from "$lib/services/icrc-accounts-balance.services";
   import { selectableUniversesStore } from "$lib/derived/selectable-universes.derived";
   import { importedTokensStore } from "$lib/stores/imported-tokens.store";
   import type { Account } from "$lib/types/account";
@@ -81,7 +81,7 @@
       return;
     }
 
-    await uncertifiedLoadSnsesAccountsBalances({
+    await syncSnsAccountsBalances({
       rootCanisterIds: notLoadedCanisterIds.map((id) => Principal.fromText(id)),
     });
   };
@@ -143,7 +143,7 @@
       return;
     }
 
-    await uncertifiedLoadAccountsBalance({
+    await syncIcrcAccountsBalances({
       universeIds,
     });
   };
@@ -153,7 +153,7 @@
       ({ rootCanisterId }) => rootCanisterId.toText() === universeId.toText()
     );
     if (isSnsProject) {
-      return uncertifiedLoadSnsesAccountsBalances({
+      return syncSnsAccountsBalances({
         rootCanisterIds: [universeId],
       });
     }

frontend/src/tests/e2e/portfolio-certified-balances.spec.ts

@@ -0,0 +1,90 @@
+import { AppPo } from "$tests/page-objects/App.page-object";
+import { PlaywrightPageObjectElement } from "$tests/page-objects/playwright.page-object";
+import { signInWithNewUser, step } from "$tests/utils/e2e.test-utils";
+import { expect, test, type Request } from "@playwright/test";
+
+// The Portfolio page must confirm every token balance with a certified update
+// call. Before the fix it loaded every SNS and ck token balance with a query
+// call only, so one malicious replica could show a forged balance.
+//
+// The IC HTTP interface uses one path per call type:
+//   /api/v2/canister/<canisterId>/query -> uncertified query call
+//   /api/v3/canister/<canisterId>/call  -> certified update call
+//
+// The CBOR request body carries the method name as a plain text string, so the
+// test selects the balance calls by searching the raw body for
+// "icrc1_balance_of". No CBOR parser is needed.
+
+const CALL_PATH_PATTERN = /^\/api\/v\d+\/canister\/([^/]+)\/(query|call)$/;
+
+const BALANCE_METHOD = "icrc1_balance_of";
+
+test("Portfolio balances are confirmed with certified update calls", async ({
+  page,
+  context,
+}) => {
+  const queriedLedgers = new Set<string>();
+  const updatedLedgers = new Set<string>();
+
+  page.on("request", (request: Request) => {
+    if (request.method() !== "POST") {
+      return;
+    }
+
+    const match = CALL_PATH_PATTERN.exec(new URL(request.url()).pathname);
+    if (match === null) {
+      return;
+    }
+
+    const body = request.postDataBuffer();
+    if (body === null) {
+      return;
+    }
+    if (!body.toString("latin1").includes(BALANCE_METHOD)) {
+      return;
+    }
+
+    const [, canisterId, callType] = match;
+    if (callType === "query") {
+      queriedLedgers.add(canisterId);
+    } else {
+      updatedLedgers.add(canisterId);
+    }
+  });
+
+  await page.goto("/");
+  await expect(page).toHaveTitle("Portfolio | Network Nervous System");
+
+  await signInWithNewUser({ page, context });
+
+  const pageElement = PlaywrightPageObjectElement.fromPage(page);
+  const appPo = new AppPo(pageElement);
+  const portfolioPagePo = appPo.getPortfolioPo().getPortfolioPagePo();
+
+  await step("Wait for the Portfolio page to load the balances");
+  await portfolioPagePo.getTotalAssetsCardPo().waitForLoaded();
+
+  await step("The Portfolio page reads at least one ledger balance");
+  await expect
+    .poll(() => queriedLedgers.size, { timeout: 60_000 })
+    .toBeGreaterThan(0);
+
+  await step(
+    "Every ledger that got a balance query also gets a balance update"
+  );
+  // On main this list keeps every queried ledger, because the Portfolio page
+  // sends no update call at all.
+  await expect
+    .poll(
+      () => [...queriedLedgers].filter((id) => !updatedLedgers.has(id)).sort(),
+      { timeout: 60_000 }
+    )
+    .toEqual([]);
+
+  await step("No ledger gets an update call without a query call");
+  // The query answer must still show first, so an update call alone would mean
+  // the page waits for the certified answer to render a balance.
+  expect(
+    [...updatedLedgers].filter((id) => !queriedLedgers.has(id)).sort()
+  ).toEqual([]);
+});

frontend/src/tests/lib/services/accounts-balances.services.spec.ts

@@ -3,15 +3,16 @@ import {
   loadSnsAccountsBalances,
   resetBalanceLoading,
 } from "$lib/services/accounts-balances.services";
+import * as walletServices from "$lib/services/icrc-accounts-balance.services";
 import * as snsBalanceServices from "$lib/services/sns-accounts-balance.services";
-import * as walletServices from "$lib/services/wallet-uncertified-accounts.services";
 import type { CanisterIdString } from "@icp-sdk/canisters/nns";
 import { Principal } from "@icp-sdk/core/principal";
 
 vi.mock("$lib/services/icrc-accounts.services", () => {
   return {
     loadAccounts: vi.fn(),
     loadIcrcToken: vi.fn(),
+    syncAccounts: vi.fn(),
   };
 });
 
@@ -28,15 +29,9 @@ describe("accounts-balances services", () => {
   beforeEach(() => {
     resetBalanceLoading();
 
-    accountsBalanceSpy = vi.spyOn(
-      walletServices,
-      "uncertifiedLoadAccountsBalance"
-    );
+    accountsBalanceSpy = vi.spyOn(walletServices, "syncIcrcAccountsBalances");
 
-    snsBalancesSpy = vi.spyOn(
-      snsBalanceServices,
-      "uncertifiedLoadSnsesAccountsBalances"
-    );
+    snsBalancesSpy = vi.spyOn(snsBalanceServices, "syncSnsAccountsBalances");
   });
 
   describe("loadSnsBalances", () => {