Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SEP-10 doesn't support custodial wallets #458

Open
msfeldstein opened this issue Nov 15, 2019 · 5 comments
Open

SEP-10 doesn't support custodial wallets #458

msfeldstein opened this issue Nov 15, 2019 · 5 comments

Comments

@msfeldstein
Copy link
Member

@msfeldstein msfeldstein commented Nov 15, 2019

Currently SEP10 authenticates uses based on their stellar account ID. In custodial wallets, there can often be many users sharing a single stellar account, multiplexed by memo to determine who each transaction represents.

SEP10 needs a way to allow authenticatino by custodial accounts. I believe this could be solved by adding an optional memo param to sep10's GET(ecosystem/sep-0010.md#challenge) request, and then storing that memo in the manage_data(key:) field to be retrieved at run-time to choose who it belongs to.

With this however, there's no cryptographic link between the memo and who is signing for it. There's no cryptographic link between the memo, and the actual signature, but this may just be the nature of custodial accounts. However it might also be a vulnerability, as someone who is part of this custodial account could replace the memo before asking the provider to sign the challenge for them. hm.

@leighmcculloch

This comment has been minimized.

Copy link
Member

@leighmcculloch leighmcculloch commented Nov 16, 2019

Any SEP10 authentication process between a custodial wallet and an anchor-server would have the anchor-client be the wallet backend, not on the wallet frontend. There's no reason that I can think of to expose signed proofs to the frontend where it might be intercepted and exploited. I don't think the authentication really needs to be concerned with who the ultimate user is. The authentication only proves possession of a destination account and the memo is not needed for that.

Outside of the authentication we should make it possible for the anchor-client to request a memo gets used by the anchor-server for the deposit, but that would be part of SEP24/26 I think.

So I think SEP10 works for custodial accounts as it is now, and SEP24 supports a memo.

What am I missing?

@msfeldstein

This comment has been minimized.

Copy link
Member Author

@msfeldstein msfeldstein commented Nov 18, 2019

Yeah i guess that makes sense. The only thing missing then is for the /transactions endpoint it uses the JWT to identify which user is making the request, so the anchor-server would need to then filter out the results to whichever of their customers made the request. If the JWT had the memo in it, the client could use the JWT to request transaction statuses directly

@leighmcculloch

This comment has been minimized.

Copy link
Member

@leighmcculloch leighmcculloch commented Nov 19, 2019

This sounds like the user will be contacting the anchor's /transactions directly with a JWT that's been retrieved by the wallet backend. I think there are some problems with this. By using SEP-10 the wallet backend is proving to the anchor that it is in possession of an account, then the anchor is transferring that proof to a JWT. I'm a little worried about the backend sharing with the frontend any credentials including that JWT that acts as a proof of possession of an account, but I can't think of any specific ways this could be exploited, it just feels like there could be something here.

I can see how if we do as you say and include the memo into the SEP-10 authentication process, then that proof would only pertain to that memo.

With this however, there's no cryptographic link between the memo and who is signing for it. There's no cryptographic link between the memo, and the actual signature

The memo is still part of the transaction and so will be signed with the custodial account key. The user doesn't need to prove ownership of anything since the user doesn't actually possess any account, so I think this is as much proof as is going to exist and is required.

Do we expect a custodial wallet will want the user interacting directly with the anchor's API? I would think a custodial wallet would be handling those interactions on the backend since it's essentially a closed system behind the custodial / shared account.

@msfeldstein

This comment has been minimized.

Copy link
Member Author

@msfeldstein msfeldstein commented Nov 19, 2019

You're right, especially if the usage of SEP10 expands and the 'proof of ownership' gains more powers, it may be a problem.

I think we need some input from someone who's built a custodial wallet to chime in here...

@nebolsin

This comment has been minimized.

Copy link
Contributor

@nebolsin nebolsin commented Nov 28, 2019

I think if the multiplexed addresses proposal will be accepted in its CAP variant, only minor changes will be necessary to expand SEP-10 for custodial wallets.

In SEP-23 form we can still use multiplexed addresses when requesting a challenge and also in a sub claim of the final JWT, but cannot use it directly as a client account in the challenge transaction.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.