search

anza-xyz / wallet-adapter

Modular TypeScript wallet adapters and components for Solana applications.
https://anza-xyz.github.io/wallet-adapter/
Apache License 2.0
1.55k stars 944 forks source link

Remove auto approve from api #82

Closed tiago18c closed 3 years ago

tiago18c commented 3 years ago

API consumers shouldn't have access to this knowledge, this should be strictly on the wallet side.

Why? Today I was investigation a couple of phishing websites, and both of them check if the user has auto approved or not. Both were acting differently, but both were abusing this information.

jordaaash commented 3 years ago

The adapter only exposes the state of the wallet. If a dapp wants to abuse autoApprove, they can detect it from the wallet extension directly anyway.

I agree generally that wallets should remove this feature. The limited purposes it solves for traders on trusted dapps is not worth average users losing funds to scams.

I'll check in with the wallet providers that have this feature, as I believe some of them are already phasing it out.

tiago18c commented 3 years ago

Yeah, I figured so, but both cases were using the adapter and decided to start here.

I thought that wallets could still auto-approve without telling the app it is auto-approving? I'm against removing the feature, but it shouldn't be easily accessible to people who don't know what they're doing. Beyond the UX question, my issue really is the apps being able to know if the wallet is doing auto or not, and then having different behaviors trying to force the user to use auto approval.

jordaaash commented 3 years ago

I thought that wallets could still auto-approve without telling the app it is auto-approving?

Maybe they can, but the ones that have the feature expose it on their interface.

I'm against removing the feature

I'm not, and wallet providers seemingly aren't. It's likely to be removed because it encourages a broken security practice.

jordaaash commented 3 years ago

The use of this pattern derives from https://github.com/project-serum/sol-wallet-adapter/blob/be3fb1414425dc8ae64d67599d677f9acc09fe4c/src/index.ts#L163-L165

Most wallets don't actually have this feature, and we don't think it's a good idea, so we should drop it.

tiago18c commented 3 years ago

Its a move in the right direction, should prevent things like this from happening image

jordaaash commented 3 years ago

Yikes, is that real code from one of these malicious dapps?

tiago18c commented 3 years ago

Yes. Another was doing even worse, with specific prompts to ask the user to refresh and select auto-approve.

jordaaash commented 3 years ago

Woof. Thanks for that. So they are actually just detecting it on window.solana which means they aren't checking the adapter state at all then.

I'm all for removing this once we have buy in to hide it from Phantom at least.

tiago18c commented 3 years ago

Whether or not they remove the feature, they should definitely remove the info from their adapter and most importantly the wallet shouldn't expose that info. Can you reach their team with this? Hopefully we get a fix asap, as these scams seem to be more common by the day.

jordaaash commented 3 years ago

I'm in contact with Phantom and Solflare and I'm working on removing it from wallet-adapter now.

tiago18c commented 3 years ago

Cool. Thank you

jordaaash commented 3 years ago

This is a breaking change (can at least break TypeScript in dapps) but I think it's important to make.

jordaaash commented 3 years ago

Published: