search

get10101 / 10101

10101 (Ten-Ten-One): Self-custodial derivative trading at your fingertips.
https://10101.finance
MIT License
129 stars 23 forks source link

Invoice descriptions should not be trusted #1147

Closed Restioson closed 12 months ago

Restioson commented 1 year ago

Currently, we trust the invoice description to correctly identify the type of transaction as well as any associated data, such as an order id (an example of CWE-502).

https://github.com/get10101/10101/blob/65f4a2fb766885ffa9e5ec528d9b5305d7fe8c85/mobile/native/src/ln_dlc/mod.rs#L351-L356

What happens if the user creates their own invoice with the description taker-fee-NotAnOrderId?!/.sidfj? Or, worse, what if the user pays an invoice without looking at the description? These would both show up as trade-related payments in the history. This is bad UX at best, and a security vulnerability at worst.

Imagined social engineering attack:

Scammer: hey, do you want to make a bet on the price of bitcoin with me? User: sure! Shall I open a position on 10101 to bet against you? Scammer: I'm actually running my own maker! The software is still in beta, so you need to just transfer me the money directly and the app will set up a DLC for us. User: okay, I'm not sure about this, so can we try with 1000 sats first? Scammer: sure, here is the invoice! [User pays invoice] Scammer: check your wallet history and see the matching fee has been paid! The open position will show in the history soon. (ed: it will not) User: okay, I'll transfer you $1000 now for the bet :)

This is slightly contrived, as we do not expect any of our users to fall for this especially since the beta users should have some idea of how the app works. Note that this scam can still progress to the point of paying 1000 sats in the above example even if this is fixed.

Furthermore, in general, parsing untrusted data is not recommended to be done lightly (see OWASP - Deserialization of untrusted data). Most parsers are not designed to handle untrusted data. What we are doing is probably fine for now (just stripping the prefix), but this could change and introduce a vulnerability.

There should be another way to figure out the type of payment from a trusted source. Perhaps this could be stored in the database? Presumably order info is already stored there.

If we can authenticate that the invoice was created by the user themselves, then we can also trust the description. This might be a bit brittle, though, since it might seem as though we can trust the description in all cases (when in reality we can only trust it if we know the user generated the payment). Maybe, in that case, it is best to also store the associated data in the database.

Restioson commented 1 year ago

cc @luckysori, from our discussion on #1103

github-actions[bot] commented 1 year ago

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.

bonomat commented 1 year ago

Will be fixed with #843 and another ticket which will move the margin into the DLC (https://github.com/get10101/10101/issues/1277)

Restioson commented 1 year ago

Alright, sounds good! Should we not keep this open until then? It just seems like it could be forgotten otherwise :sweat_smile: as it seems a little orthogonal to those two tickets (correct me if I'm wrong)

holzeis commented 1 year ago

Will be closed once the referenced tickets are resolved.