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

Request: Better Scam / Exploitation protection for malicious Soroban invocations #1092

Closed
mootz12 opened this issue Jan 29, 2024 · 5 comments · Fixed by stellar/freighter-backend#31 or #1095
Assignees
Labels
triage Determining where this fits into upcoming work

Comments

@mootz12
Copy link

mootz12 commented Jan 29, 2024

Documenting a discussion point raised by @esteblock about potential exploitation avenues due to Soroban's flexible authentication system.

See related discord thread: https://discord.com/channels/897514728459468821/1201562954466861157

Problem

Malicious contracts can inject bad invocation authentication requests through the require_auth method that can be difficult for a user to detect. Further, the contract can request authentication for other contract requests (I.E. - sending all of a users XLM to a different location).

This is not a common security concern on Ethereum, as you need to either sign a permit message or call "approve()" to permit contracts the ability to send ERC20 based tokens on your behalf.

An example:

A trusted AMM protocol allows any LP to be created with two SEP-41 compliant tokens.

  1. Someone creates a LP with XLM<>BAD_TOKEN
  2. They wash trade or spoof metrics on the XLM<>BAD_TOKEN pool so it get's displayed on either the trusted AMM protocols frontend or another source
  3. The BAD_TOKEN has a transfer function such that:
    • after some time or a flag is set, "transfer" calls the USDC token "transfer" function instead to send the callers full USDC balance to different destination
  4. The current InvokeHostFunction signature display is too complicated to notice and they get their funds drained on a trusted website

image

A second example:

  1. get user to try and send tokens via Freighter "transfer" for a malicious token they add via contractId (GIF in discord thread above)

Solutions

Opening this as a thread to hear input from others about potential solutions.

My thoughts:

  • All token invocations (especially transfers) need to be clearly shown in the signature screen (shown above in example)
  • Any token invocations to untrusted tokens adds a warning alongside the invocation / sub-invocation information
  • It can likely be detected and flagged if and SEP-41 token function (transfer, transferFrom) have sub-invocations for the user to inspect
@aristidesstaffieri
Copy link
Contributor

Thanks @mootz12

I think there are some obvious improvements we can make, like a more human friendly root invocation representation when signing auth entries, and I have some other thoughts about the rest -

As the token list standard develops, we may choose to use it in order to highlight untrusted or less common tokens.

In the case of the Soroban side sep41 compliant token transfer(payment in the Freighter UI), we can know exactly what the user is attempting to do so I think it would not be crazy to warn or reject attempts to use the "payment" flow in Freighter when the user selects a token and that tokens transfer has any sub calls or interacts with any other contract/unexpected storage entries.

Parsing the tx envelope can expose a lot of info about the interface and tx/call stack but a malicious contract doesn't have to conform to the token standard or make this clear. If a user wants to do a transfer that was constructed in an application or is part of a swap maybe, we can highlight transfers as a part of these invocation trees when they match the spec but that leaves plenty of cases that are harder to identify which still lead to unexpected results. To me, it seems that a richer simulation story could ultimately catch a broader set of situations where the user is taking an action that they don't intend than only relying on the users understanding of the invocation.

@earrietadev
Copy link

I saw that this stellar/stellar-rpc#11 was moved a few hours ago to Next Sprint Proposal from Backlog so that richer simulation story should be possible in the near future

@esteblock
Copy link

think it would not be crazy to warn or reject attempts to use the "payment" flow in Freighter when the user selects a token and that tokens transfer has any sub calls or interacts with any other contract/unexpected storage entries.

I totally agree!

@leighmcculloch
Copy link
Member

I think it could be worth adding some friction for each auth. Not only displaying the full invoke which would show each auth, but it could be worth requiring the user to check a box for each separate invocation/sub-invocation that is being authorized. That way they're encouraged to look and engage with each one.

@heytdep
Copy link

heytdep commented Jan 31, 2024

Pasting here what I shared in the dc chat.

What about wallets keeping a list of safe and audited contracts (similarly to token lists), and notify users = when there's a call in the stack to an unknown contract (i.e a contract that is not on the list). I believe that depending on the contract code, the list could either accept also contract wasm hashes (for contracts that cannot become malicious if badly initialized).

After all, I think that on mainnet we only want general-purpose users to interact with audited contracts and be notified when they aren't. Actually, it wouldn't be a bad idea to integrate documentation for each public contract method when adding the binary to the "safe" contracts list.

@aristidesstaffieri aristidesstaffieri added the triage Determining where this fits into upcoming work label Feb 1, 2024
@aristidesstaffieri aristidesstaffieri self-assigned this Feb 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
triage Determining where this fits into upcoming work
Projects
None yet
6 participants