Reject `Swap` approvals with foreign claim addresses #821

jessepinho · 2024-03-23T01:49:39Z

From #548:

The claim address is specified in the SwapPlan. It should always be an address the user controls.

The extension's approval code should check that any submitted SwapPlan has a claim address controlled by the user, and automatically reject it if not.

Adding this check means that we don't need to display the claim address to the user in the approval view.

In this PR

In the CustodyService's authorize implementation, assert that transaction swaps (if there are any) contain claim addresses controlled by the current user.
- To keep things clean, and because we may want to add more assertions as we think of other cases like this, I put the assertion code and its tests in a separate file from the authorize implementation.
Rename the poorly named isControlledAddress helper to getAddressIndexByAddress to better represent what it does.
Create a new isControlledAddress helper that actually returns a boolean.
Remove the claim address from <SwapViewComponent />.

Manual testing

The code:

diff --git a/apps/minifront/src/state/swap.ts b/apps/minifront/src/state/swap.ts
index 49ca99fe..52a867f4 100644
--- a/apps/minifront/src/state/swap.ts
+++ b/apps/minifront/src/state/swap.ts
@@ -24,6 +24,7 @@ import { getAssetId } from '@penumbra-zone/getters/src/metadata';
 import { getSwapCommitmentFromTx } from '@penumbra-zone/getters/src/transaction';
 import { getAddressIndex } from '@penumbra-zone/getters/src/address-view';
 import { toBaseUnit } from '@penumbra-zone/types/src/lo-hi';
+import { bech32ToAddress } from '@penumbra-zone/bech32/src/address';

 export interface SimulateSwapResult {
   output: ValueView;
@@ -163,7 +164,13 @@ const assembleSwapRequest = async ({ assetIn, amount, assetOut }: SwapSlice) =>
           ),
           assetId: getAssetIdFromValueView(assetIn.balanceView),
         },
-        claimAddress: await getAddressByIndex(addressIndex.account),
+        // claimAddress: await getAddressByIndex(addressIndex.account),
+        claimAddress: {
+          inner: bech32ToAddress(
+            'penumbra1tj9s0dh8ymphnw2qgrva57wpcmpr38e9a8w05t5c8dyynr7dt58hgnare7tunqyuryc7yag084rqxrsrj9yx8uk9yhs8lmg64hl993wn3rphlvan3paxdd6suyhwnpkd6cwzes',
+          ),
+        },
+
         // TODO: Calculate this properly in subsequent PR
         //       Asset Id should almost certainly be upenumbra,
         //       may need to indicate native denom in registry

The result:

Screen.Recording.2024-03-22.at.7.19.09.PM.mov

Note that it just says "Transaction canceled," rather than showing the error message about the website stealing your funds. That's because it throws a ConnectError with Code.PermissionDenied, which minifront catches and displays as "Transaction canceled."

Closes #548

jessepinho · 2024-03-23T02:26:16Z

packages/ui/components/ui/tx/view/swap.tsx

@@ -3,17 +3,10 @@ import { SwapView } from '@buf/penumbra-zone_penumbra.bufbuild_es/penumbra/core/
 import { fromBaseUnitAmount, joinLoHiAmount } from '@penumbra-zone/types/src/amount';
 import { uint8ArrayToBase64 } from '@penumbra-zone/types/src/base64';
 import { ActionDetails } from './action-details';
-import { AddressView } from '@buf/penumbra-zone_penumbra.bufbuild_es/penumbra/core/keys/v1/keys_pb';
-import { AddressViewComponent } from './address-view';

 export const SwapViewComponent = ({ value }: { value: SwapView }) => {


We no longer need to show the claim address. Here's what that looks like in the approval dialog (it's the same in the Swap transaction view of a successful transaction, too):

Hmm. I'm not sure we should hide it. The claiming part of a swap seems so integral to penumbra. I don't think it hurts to show it.

Hm OK. I was basing it on Henry's comment in #548:

Adding this check means that we don't need to display the claim address to the user in the approval view.

That said, I think the main point of that ticket was to ensure the claim address belonged to the current user, so I'll add it back.

Then up to you! 🤷

Yeah, I'd prefer to hide it. The user shouldn't ever have to know about the claim address, it should just do the right thing automatically.

As a follow-up to this PR, we should also change the swap rendering to show formatted values, so that as a user I see something like "Swap 1234 UM => TestUsd"

jessepinho · 2024-03-23T02:26:34Z

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

@@ -35,7 +35,7 @@ export interface ShieldedPoolMock {
 }

 export interface ViewServerMock {
-  fullViewingKey?: Mock;
+  fullViewingKey?: string;


This should have always been a string

packages/wasm/src/address.ts

TalDerei · 2024-03-25T03:59:41Z

minor nit; to maintain consistency, can we deactivate the swap button until the user enters a non-zero amount, similar to how we handle send transactions?

packages/router/src/grpc/custody/authorize/assert-valid-swaps.ts

TalDerei · 2024-03-25T04:47:48Z

That's because it throws a ConnectError with Code.PermissionDenied, which minifront catches and displays as "Transaction canceled."

we might want to consider displaying a more stateful error message describing why the swap transaction was canceled down the line.

TalDerei · 2024-03-25T05:04:12Z

@jessepinho this is out of scope for this PR, but maybe we should separately consider adding a minor delay between swap and swap claim toasts, or slight offset in the UI? I'm not sure this is the most intuitive UX, because the view changes so quickly between transactions that I find myself missing the display of the first transaction. Curious what you think.

grod220 · 2024-03-25T12:00:41Z

packages/router/src/grpc/custody/authorize/assert-valid-swaps.ts

+      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,
+      );


This seems like quite a helpful error message. Can we show this in our tooltip instead of it being swallowed?

This is related to @TalDerei 's comment above:

we might want to consider displaying a more stateful error message describing why the swap transaction was canceled down the line.

Here's the issue: minifront will never try to steal the user's funds, so we never need to show this message to minifront users.

We should show this message to users on other sites who try to steal the user's funds, but obviously, a malicious website isn't going to proactively show this error to people. But if they're trying to steal people's funds, they'll presumably test the stealing-people's-funds functionality first, and they'll realize that this error shows up, at which point they'll either A) give up, or B) at least suppress this error.

So I'm not actually sure how useful this error is?

The only way it could be useful is if we could show this error in the transaction approval popup, which the malicious website has no control over. Not sure if this is possible, though, since this error is thrown inside an RPC method implementation. What if we merge this as-is for now, and then revisit this question in a separate ticket?

created separate ticket to limit scope

packages/router/src/grpc/custody/authorize/assert-valid-swaps.ts

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

grod220 · 2024-03-25T12:08:39Z

packages/ui/components/ui/tx/view/swap.tsx

@@ -3,17 +3,10 @@ import { SwapView } from '@buf/penumbra-zone_penumbra.bufbuild_es/penumbra/core/
 import { fromBaseUnitAmount, joinLoHiAmount } from '@penumbra-zone/types/src/amount';
 import { uint8ArrayToBase64 } from '@penumbra-zone/types/src/base64';
 import { ActionDetails } from './action-details';
-import { AddressView } from '@buf/penumbra-zone_penumbra.bufbuild_es/penumbra/core/keys/v1/keys_pb';
-import { AddressViewComponent } from './address-view';

 export const SwapViewComponent = ({ value }: { value: SwapView }) => {


Hmm. I'm not sure we should hide it. The claiming part of a swap seems so integral to penumbra. I don't think it hurts to show it.

packages/wasm/src/address.ts

grod220 · 2024-03-25T12:14:34Z

packages/wasm/src/address.ts

 ): AddressIndex | undefined => {
-  const res = is_controlled_address(fullViewingKey, address) as JsonValue;
+  const res = is_controlled_address(fullViewingKey, bech32Address(address)) as JsonValue;


Think we should consider changing the rust api as well.

is_controlled_address() should return a boolean

get_address_index_by_address() should return Result (throws if not able to)

I'm less comfortable with the Rust side of things — would you be OK with making that change in a follow-on PR? (For that matter, we probably only need get_address_index_by_address in Rust, since is_controlled_address would just be calling get_address_index_by_address anyway.)

And in the meantime, could you give this PR a 👍🏻 since the other items are resolved?

packages/router/src/grpc/custody/authorize/assert-valid-swaps.ts

jessepinho · 2024-03-25T19:52:36Z

@TalDerei:

minor nit; to maintain consistency, can we deactivate the swap button until the user enters a non-zero amount, similar to how we handle send transactions?

This is out of scope for this PR, but maybe we should separately consider adding a minor delay between swap and swap claim toasts, or slight offset in the UI?

Could you please create separate tickets for these?

grod220 · 2024-03-25T21:27:25Z

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

+  assertSwapClaimAddressesBelongToCurrentUser(req.plan, address =>
+    isControlledAddress(fullViewingKey, address),
+  );


I get a sense we will be doing many more of these guards in the future. I wonder if there is a more general pattern we can have for this 🤔

Maybe not for this PR, unless you're feeling inspired.

I think it makes sense to think of these as policies, and have one function per policy that approves or denies, and call each function in sequence

I can dig it 👌🏻

grod220 · 2024-03-25T21:27:55Z

packages/wasm/src/address.ts

 ): AddressIndex | undefined => {
-  const res = is_controlled_address(fullViewingKey, address) as JsonValue;
+  const res = is_controlled_address(fullViewingKey, bech32Address(address)) as JsonValue;


jessepinho force-pushed the jessepinho/reject-swaps-with-foreign-claim-addresses-web-548 branch from 7e91a09 to 60a2982 Compare March 23, 2024 02:10

jessepinho commented Mar 23, 2024

View reviewed changes

packages/wasm/src/address.ts Show resolved Hide resolved

jessepinho changed the title ~~WIP: Reject Swap approvals with foreign claim addresses~~ Reject Swap approvals with foreign claim addresses Mar 23, 2024

jessepinho marked this pull request as ready for review March 23, 2024 02:34

jessepinho requested review from Valentine1898, TalDerei, grod220 and turbocrime March 23, 2024 02:35

TalDerei reviewed Mar 25, 2024

View reviewed changes

packages/router/src/grpc/custody/authorize/assert-valid-swaps.ts Outdated Show resolved Hide resolved

grod220 reviewed Mar 25, 2024

View reviewed changes

jessepinho mentioned this pull request Mar 25, 2024

Should not be able to select same asset in/out #533

Closed

turbocrime reviewed Mar 25, 2024

View reviewed changes

packages/router/src/grpc/custody/authorize/assert-valid-swaps.ts Outdated Show resolved Hide resolved

jessepinho added 12 commits March 25, 2024 12:44

Fix mock typing

c68d5f5

Assert valid swaps when authorizing a transaction

26b6069

Fix typings issues

a1b4369

Rename helper

3d1ccd7

Simply accept an Address

7e6eb93

Create new isControlledAddress helper

d4e012d

Fix mock

572bd5c

Use the new helper

e5fe472

Remove claim address from SwapViewComponent

fd7dd2c

Simplify syntax

4c5ce19

Rename helper

1bb3565

Add back claim address to UI

e113b86

jessepinho force-pushed the jessepinho/reject-swaps-with-foreign-claim-addresses-web-548 branch from f5a720d to e113b86 Compare March 25, 2024 19:50

grod220 approved these changes Mar 25, 2024

View reviewed changes

This was referenced Mar 25, 2024

Disable swap button #829

Closed

More stateful error message for swap approvals with foreign claim addresses #831

Closed

jessepinho merged commit 2479c24 into main Mar 25, 2024
6 checks passed

jessepinho deleted the jessepinho/reject-swaps-with-foreign-claim-addresses-web-548 branch March 25, 2024 22:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reject `Swap` approvals with foreign claim addresses #821

Reject `Swap` approvals with foreign claim addresses #821

jessepinho commented Mar 23, 2024 •

edited

Loading

jessepinho Mar 23, 2024

grod220 Mar 25, 2024

jessepinho Mar 25, 2024

grod220 Mar 25, 2024

hdevalence Mar 25, 2024

hdevalence Mar 25, 2024

jessepinho Mar 23, 2024

TalDerei commented Mar 25, 2024 •

edited

Loading

TalDerei commented Mar 25, 2024 •

edited

Loading

TalDerei commented Mar 25, 2024

grod220 Mar 25, 2024

jessepinho Mar 25, 2024

TalDerei Mar 25, 2024

grod220 Mar 25, 2024

grod220 Mar 25, 2024

jessepinho Mar 25, 2024

grod220 Mar 25, 2024

jessepinho commented Mar 25, 2024

grod220 Mar 25, 2024

hdevalence Mar 25, 2024

jessepinho Mar 25, 2024

grod220 Mar 25, 2024

Reject Swap approvals with foreign claim addresses #821

Reject Swap approvals with foreign claim addresses #821

Conversation

jessepinho commented Mar 23, 2024 • edited Loading

In this PR

Manual testing

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TalDerei commented Mar 25, 2024 • edited Loading

TalDerei commented Mar 25, 2024 • edited Loading

TalDerei commented Mar 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jessepinho commented Mar 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Reject `Swap` approvals with foreign claim addresses #821

Reject `Swap` approvals with foreign claim addresses #821

jessepinho commented Mar 23, 2024 •

edited

Loading

TalDerei commented Mar 25, 2024 •

edited

Loading

TalDerei commented Mar 25, 2024 •

edited

Loading