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

Enforce minimum exchange rates #273

Closed
gakonst opened this issue Sep 3, 2019 · 4 comments · Fixed by #616
Closed

Enforce minimum exchange rates #273

gakonst opened this issue Sep 3, 2019 · 4 comments · Fixed by #616
Assignees
Labels
bug crate/interledger-stream v1.0 Issues which need to be handled for a v1.0 release

Comments

@gakonst
Copy link
Member

gakonst commented Sep 3, 2019

Also... when/if we add that exchange rate logic, we definitely need to fix this: https://github.com/interledger-rs/interledger-rs/blob/master/crates/interledger-stream/src/client.rs#L121

Right now intermediary connectors can steal ALL the money!

Originally posted by @kincaidoneil in #256 (comment)

@kincaidoneil
Copy link
Member

kincaidoneil commented Sep 13, 2019

FWIW, since we now have exchange rates within the store, this might be simple to fix. We'd somehow need the STREAM receiver to send a ConnectionAssetDetails frame when the connection is first created (since it's stateless, one approach would be to include it in response to any request with a ConnectionNewAddress frame). Then the client could lookup the rate from its asset to the destination asset, and determine a reasonable minimum destination amount.

Edit: it also looks like the JS implementation uses a similar approach, by using the ConnectionNewAddress frame to determine if it's a new connection and then responding with a ConnectionAssetDetails frame: https://github.com/interledgerjs/ilp-protocol-stream/blob/bb1e830e418057be094af4fad452b8f2e25cb5cf/src/connection.ts#L627

@kincaidoneil
Copy link
Member

Also, when we implement this, we need to update the STREAM sender to limit the delivered amount based on the minimum amount they set in the Prepare (the delivered amount should be the min of what the receiver claims they got, and what we told them was the minimum they should accept).

https://github.com/interledger-rs/interledger-rs/blob/748591691bf787bee8aafd380ac3d5c5872d82e5/crates/interledger-stream/src/client.rs#L306-L307

After we support invoices (paying by destination amount, rather than source amount), the recipient could fulfill the packet, but lie and say they received 0, which would cause the sender to continue sending indefinitely, draining their account.

JavaScript STREAM is also vulnerable... but Java is not! Woot @sappenin 🎉 (although I think it would incorrectly report the delivered amount to the application)

@kincaidoneil kincaidoneil changed the title Intermediate Connectors can steal all money by manipulating exchange rates Enforce minimum exchange rates Jan 5, 2020
@kincaidoneil kincaidoneil mentioned this issue Jan 9, 2020
12 tasks
@gakonst gakonst added the v1.0 Issues which need to be handled for a v1.0 release label Jan 20, 2020
@gakonst gakonst added this to To do in Towards a v1.0 Jan 20, 2020
@sappenin
Copy link
Contributor

although I think it would incorrectly report the delivered amount to the application

@kincaidoneil can you clarify that one? Is there a bug in the Java logic?

@sappenin
Copy link
Contributor

@kincaidoneil this has been assigned to you - I think it's worth fixing.

@kincaidoneil kincaidoneil moved this from To do to In progress in Towards a v1.0 Jan 27, 2020
@kincaidoneil kincaidoneil moved this from In progress to Waiting Review / Response in Towards a v1.0 Feb 3, 2020
Towards a v1.0 automation moved this from Waiting Review / Response to Done Feb 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug crate/interledger-stream v1.0 Issues which need to be handled for a v1.0 release
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

3 participants