Reject Swap approvals with foreign claim addresses (#821)

* Fix mock typing

* Assert valid swaps when authorizing a transaction

* Fix typings issues

* Rename helper

* Simply accept an Address

* Create new isControlledAddress helper

* Fix mock

* Use the new helper

* Remove claim address from SwapViewComponent

* Simplify syntax

* Rename helper

* Add back claim address to UI

packages/perspective/plan/get-address-view.test.ts

@@ -7,10 +7,10 @@ import {
 } from '@buf/penumbra-zone_penumbra.bufbuild_es/penumbra/core/keys/v1/keys_pb';
 import { bech32ToAddress } from '@penumbra-zone/bech32/src/address';
 
-const mockIsControlledAddress = vi.hoisted(() => vi.fn());
+const mockGetAddressIndexByAddress = vi.hoisted(() => vi.fn());
 
 vi.mock('@penumbra-zone/wasm/src/address', () => ({
-  isControlledAddress: mockIsControlledAddress,
+  getAddressIndexByAddress: mockGetAddressIndexByAddress,
 }));
 
 describe('getAddressView()', () => {
@@ -19,12 +19,12 @@ describe('getAddressView()', () => {
   const address = new Address({ inner: bech32ToAddress(addressAsBech32) });
 
   beforeEach(() => {
-    mockIsControlledAddress.mockReset();
+    mockGetAddressIndexByAddress.mockReset();
   });
 
   describe('when the address is controlled by the user represented by the full viewing key', () => {
     beforeEach(() => {
-      mockIsControlledAddress.mockImplementation(() => new AddressIndex({ account: 123 }));
+      mockGetAddressIndexByAddress.mockImplementation(() => new AddressIndex({ account: 123 }));
     });
 
     test('returns a visible `AddressView`', () => {
@@ -46,7 +46,7 @@ describe('getAddressView()', () => {
 
   describe('when the address is not controlled by the user represented by the full viewing key', () => {
     beforeEach(() => {
-      mockIsControlledAddress.mockImplementation(() => undefined);
+      mockGetAddressIndexByAddress.mockImplementation(() => undefined);
     });
 
     test('returns an opaque `AddressView`', () => {

packages/perspective/plan/get-address-view.ts

@@ -2,11 +2,10 @@ import {
   Address,
   AddressView,
 } from '@buf/penumbra-zone_penumbra.bufbuild_es/penumbra/core/keys/v1/keys_pb';
-import { isControlledAddress } from '@penumbra-zone/wasm/src/address';
-import { bech32Address } from '@penumbra-zone/bech32/src/address';
+import { getAddressIndexByAddress } from '@penumbra-zone/wasm/src/address';
 
 export const getAddressView = (address: Address, fullViewingKey: string): AddressView => {
-  const index = isControlledAddress(fullViewingKey, bech32Address(address));
+  const index = getAddressIndexByAddress(fullViewingKey, address);
 
   if (index) {
     return new AddressView({

packages/router/src/grpc/custody/authorize/assert-swap-claim-addresses-belong-to-current-user.test.ts

@@ -0,0 +1,123 @@
+import { describe, expect, it } from 'vitest';
+import { assertSwapClaimAddressesBelongToCurrentUser } from './assert-swap-claim-addresses-belong-to-current-user';
+import {
+  ActionPlan,
+  TransactionPlan,
+} from '@buf/penumbra-zone_penumbra.bufbuild_es/penumbra/core/transaction/v1/transaction_pb';
+import { Code, ConnectError } from '@connectrpc/connect';
+import { Address } from '@buf/penumbra-zone_penumbra.bufbuild_es/penumbra/core/keys/v1/keys_pb';
+
+const currentUserAddress1 = new Address({
+  inner: new Uint8Array([1, 2, 3]),
+});
+
+const currentUserAddress2 = new Address({
+  inner: new Uint8Array([4, 5, 6]),
+});
+
+const otherUserAddress = new Address({
+  inner: new Uint8Array([7, 8, 9]),
+});
+
+const swapWithCurrentUserAddress1 = new ActionPlan({
+  action: {
+    case: 'swap',
+    value: {
+      swapPlaintext: {
+        claimAddress: currentUserAddress1,
+      },
+    },
+  },
+});
+
+const swapWithCurrentUserAddress2 = new ActionPlan({
+  action: {
+    case: 'swap',
+    value: {
+      swapPlaintext: {
+        claimAddress: currentUserAddress2,
+      },
+    },
+  },
+});
+
+const swapWithOtherUserAddress = new ActionPlan({
+  action: {
+    case: 'swap',
+    value: {
+      swapPlaintext: {
+        claimAddress: otherUserAddress,
+      },
+    },
+  },
+});
+
+const swapWithUndefinedAddress = new ActionPlan({
+  action: {
+    case: 'swap',
+    value: {
+      swapPlaintext: {},
+    },
+  },
+});
+
+const mockIsControlledAddress = (address?: Address) =>
+  !!address && [currentUserAddress1, currentUserAddress2].includes(address);
+
+describe('assertSwapClaimAddressesBelongToCurrentUser()', () => {
+  describe('when the transaction plan has no swaps', () => {
+    it('does not throw', () => {
+      expect(() =>
+        assertSwapClaimAddressesBelongToCurrentUser(new TransactionPlan(), mockIsControlledAddress),
+      ).not.toThrow();
+    });
+  });
+
+  describe('when the transaction plan has swaps', () => {
+    describe("when all of the swaps' `claimAddress`es belong to the current user", () => {
+      it('does not throw', () => {
+        const plan = new TransactionPlan({
+          actions: [swapWithCurrentUserAddress1, swapWithCurrentUserAddress2],
+        });
+
+        expect(() =>
+          assertSwapClaimAddressesBelongToCurrentUser(plan, mockIsControlledAddress),
+        ).not.toThrow();
+      });
+    });
+
+    describe("when any of the swaps' `claimAddress`es do not belong to the current user", () => {
+      it('throws a `ConnectError` with the `PermissionDenied` code', () => {
+        const plan = new TransactionPlan({
+          actions: [swapWithCurrentUserAddress1, swapWithOtherUserAddress],
+        });
+
+        expect.assertions(2);
+
+        try {
+          assertSwapClaimAddressesBelongToCurrentUser(plan, mockIsControlledAddress);
+        } catch (error) {
+          expect(error).toBeInstanceOf(ConnectError);
+          expect((error as ConnectError).code).toBe(Code.PermissionDenied);
+        }
+      });
+    });
+
+    describe("when any of the swaps' `claimAddress`es are empty", () => {
+      it('throws a `ConnectError` with the `PermissionDenied` code', () => {
+        const plan = new TransactionPlan({
+          actions: [swapWithUndefinedAddress],
+        });
+
+        expect.assertions(2);
+
+        try {
+          assertSwapClaimAddressesBelongToCurrentUser(plan, mockIsControlledAddress);
+        } catch (error) {
+          expect(error).toBeInstanceOf(ConnectError);
+          expect((error as ConnectError).code).toBe(Code.PermissionDenied);
+        }
+      });
+    });
+  });
+});

packages/router/src/grpc/custody/authorize/assert-swap-claim-addresses-belong-to-current-user.ts

@@ -0,0 +1,19 @@
+import { Address } from '@buf/penumbra-zone_penumbra.bufbuild_es/penumbra/core/keys/v1/keys_pb';
+import { TransactionPlan } from '@buf/penumbra-zone_penumbra.bufbuild_es/penumbra/core/transaction/v1/transaction_pb';
+import { Code, ConnectError } from '@connectrpc/connect';
+
+export const assertSwapClaimAddressesBelongToCurrentUser = (
+  plan: TransactionPlan,
+  isControlledAddress: (address?: Address) => boolean,
+): void => {
+  plan.actions.forEach(action => {
+    if (action.action.case !== 'swap') return;
+
+    if (!isControlledAddress(action.action.value.swapPlaintext?.claimAddress)) {
+      throw new ConnectError(
+        "Tried to initiate a swap with a claim address belonging to a different user. This means that, when the swap is claimed, the funds would go to someone else's address, not yours. This should never happen. The website you are using may be trying to steal your funds!",
+        Code.PermissionDenied,
+      );
+    }
+  });
+};

packages/router/src/grpc/custody/authorize.test.ts → packages/router/src/grpc/custody/authorize/index.test.ts

@@ -1,9 +1,9 @@
 import { beforeEach, describe, expect, Mock, test, vi } from 'vitest';
 import { createContextValues, createHandlerContext, HandlerContext } from '@connectrpc/connect';
-import { approverCtx } from '../../ctx/approver';
-import { extLocalCtx, extSessionCtx, servicesCtx } from '../../ctx/prax';
-import { IndexedDbMock, MockExtLocalCtx, MockExtSessionCtx, MockServices } from '../test-utils';
-import { authorize } from './authorize';
+import { approverCtx } from '../../../ctx/approver';
+import { extLocalCtx, extSessionCtx, servicesCtx } from '../../../ctx/prax';
+import { IndexedDbMock, MockExtLocalCtx, MockExtSessionCtx, MockServices } from '../../test-utils';
+import { authorize } from '.';
 import { AuthorizeRequest } from '@buf/penumbra-zone_penumbra.bufbuild_es/penumbra/custody/v1/custody_pb';
 import { CustodyService } from '@buf/penumbra-zone_penumbra.connectrpc_es/penumbra/custody/v1/custody_connect';
 import {
@@ -36,7 +36,7 @@ describe('Authorize request handler', () => {
 
     mockServices = {
       getWalletServices: vi.fn(() =>
-        Promise.resolve({ indexedDb: mockIndexedDb }),
+        Promise.resolve({ indexedDb: mockIndexedDb, viewServer: { fullViewingKey: 'fvk' } }),
       ) as MockServices['getWalletServices'],
     };
 

packages/router/src/grpc/custody/authorize.ts → packages/router/src/grpc/custody/authorize/index.ts

@@ -1,19 +1,27 @@
-import type { Impl } from '.';
-import { extLocalCtx, extSessionCtx } from '../../ctx/prax';
-import { approverCtx } from '../../ctx/approver';
+import type { Impl } from '..';
+import { extLocalCtx, extSessionCtx, servicesCtx } from '../../../ctx/prax';
+import { approverCtx } from '../../../ctx/approver';
 import { generateSpendKey } from '@penumbra-zone/wasm/src/keys';
 import { authorizePlan } from '@penumbra-zone/wasm/src/build';
 import { Key } from '@penumbra-zone/crypto-web/src/encryption';
 import { Code, ConnectError } from '@connectrpc/connect';
 import { Box } from '@penumbra-zone/types/src/box';
 import { UserChoice } from '@penumbra-zone/types/src/user-choice';
+import { assertSwapClaimAddressesBelongToCurrentUser } from './assert-swap-claim-addresses-belong-to-current-user';
+import { isControlledAddress } from '@penumbra-zone/wasm/src/address';
 
 export const authorize: Impl['authorize'] = async (req, ctx) => {
   if (!req.plan) throw new ConnectError('No plan included in request', Code.InvalidArgument);
 
   const approveReq = ctx.values.get(approverCtx);
   const sess = ctx.values.get(extSessionCtx);
   const local = ctx.values.get(extLocalCtx);
+  const walletServices = await ctx.values.get(servicesCtx).getWalletServices();
+
+  const { fullViewingKey } = walletServices.viewServer;
+  assertSwapClaimAddressesBelongToCurrentUser(req.plan, address =>
+    isControlledAddress(fullViewingKey, address),
+  );
 
   if (!approveReq) throw new ConnectError('Approver not found', Code.Unavailable);
 

packages/router/src/grpc/test-utils.ts

@@ -36,7 +36,7 @@ export interface ShieldedPoolMock {
 }
 
 export interface ViewServerMock {
-  fullViewingKey?: Mock;
+  fullViewingKey?: string;
 }
 
 interface MockQuerier {

packages/router/src/grpc/view-protocol-server/index-by-address.ts

@@ -1,10 +1,9 @@
 import type { Impl } from '.';
 import { servicesCtx } from '../../ctx/prax';
 
-import { isControlledAddress } from '@penumbra-zone/wasm/src/address';
+import { getAddressIndexByAddress } from '@penumbra-zone/wasm/src/address';
 
 import { Code, ConnectError } from '@connectrpc/connect';
-import { bech32Address } from '@penumbra-zone/bech32/src/address';
 
 export const indexByAddress: Impl['indexByAddress'] = async (req, ctx) => {
   if (!req.address) throw new ConnectError('no address given in request', Code.InvalidArgument);
@@ -13,9 +12,7 @@ export const indexByAddress: Impl['indexByAddress'] = async (req, ctx) => {
     viewServer: { fullViewingKey },
   } = await services.getWalletServices();
 
-  const address = bech32Address(req.address);
-
-  const addressIndex = isControlledAddress(fullViewingKey, address);
+  const addressIndex = getAddressIndexByAddress(fullViewingKey, req.address);
 
   if (!addressIndex) return {};
 

packages/router/src/grpc/view-protocol-server/transaction-info-by-hash.test.ts

@@ -34,7 +34,8 @@ describe('TransactionInfoByHash request handler', () => {
       constants: vi.fn(),
     };
     mockViewServer = {
-      fullViewingKey: vi.fn(),
+      fullViewingKey:
+        'penumbrafullviewingkey1vzfytwlvq067g2kz095vn7sgcft47hga40atrg5zu2crskm6tyyjysm28qg5nth2fqmdf5n0q530jreumjlsrcxjwtfv6zdmfpe5kqsa5lg09',
     };
     mockTendermint = {
       getTransaction: vi.fn(),
@@ -59,9 +60,6 @@ describe('TransactionInfoByHash request handler', () => {
       url: '/mock',
       contextValues: createContextValues().set(servicesCtx, mockServices as unknown as Services),
     });
-    mockViewServer.fullViewingKey?.mockReturnValueOnce(
-      'penumbrafullviewingkey1vzfytwlvq067g2kz095vn7sgcft47hga40atrg5zu2crskm6tyyjysm28qg5nth2fqmdf5n0q530jreumjlsrcxjwtfv6zdmfpe5kqsa5lg09',
-    );
     mockTransactionInfo.mockReturnValueOnce({
       txp: transactionPerspective,
       txv: {},

packages/router/src/grpc/view-protocol-server/transaction-info.test.ts

@@ -40,7 +40,8 @@ describe('TransactionInfo request handler', () => {
     };
 
     mockViewServer = {
-      fullViewingKey: vi.fn(),
+      fullViewingKey:
+        'penumbrafullviewingkey1vzfytwlvq067g2kz095vn7sgcft47hga40atrg5zu2crskm6tyyjysm28qg5nth2fqmdf5n0q530jreumjlsrcxjwtfv6zdmfpe5kqsa5lg09',
     };
 
     mockServices = {
@@ -58,10 +59,6 @@ describe('TransactionInfo request handler', () => {
       contextValues: createContextValues().set(servicesCtx, mockServices as unknown as Services),
     });
 
-    mockViewServer.fullViewingKey?.mockReturnValueOnce(
-      'penumbrafullviewingkey1vzfytwlvq067g2kz095vn7sgcft47hga40atrg5zu2crskm6tyyjysm28qg5nth2fqmdf5n0q530jreumjlsrcxjwtfv6zdmfpe5kqsa5lg09',
-    );
-
     mockTransactionInfo.mockReturnValue({
       txp: {},
       txv: {},

packages/router/src/grpc/view-protocol-server/transaction-planner.test.ts

@@ -32,7 +32,8 @@ describe('TransactionPlanner request handler', () => {
       constants: vi.fn(),
     };
     mockViewServer = {
-      fullViewingKey: vi.fn(),
+      fullViewingKey:
+        'penumbrafullviewingkey1vzfytwlvq067g2kz095vn7sgcft47hga40atrg5zu2crskm6tyyjysm28qg5nth2fqmdf5n0q530jreumjlsrcxjwtfv6zdmfpe5kqsa5lg09',
     };
     mockServices = {
       getWalletServices: vi.fn(() =>
@@ -51,9 +52,6 @@ describe('TransactionPlanner request handler', () => {
       url: '/mock',
       contextValues: createContextValues().set(servicesCtx, mockServices as unknown as Services),
     });
-    mockViewServer.fullViewingKey?.mockReturnValueOnce(
-      'penumbrafullviewingkey1vzfytwlvq067g2kz095vn7sgcft47hga40atrg5zu2crskm6tyyjysm28qg5nth2fqmdf5n0q530jreumjlsrcxjwtfv6zdmfpe5kqsa5lg09',
-    );
 
     req = new TransactionPlannerRequest({});
   });