interledgerjs / ilp-plugin-xrp-asym-server Goto Github PK

This line contains a bug:
https://github.com/interledgerjs/ilp-plugin-xrp-asym-server/blob/master/src/index.ts#L534

The returned asset scale should be the configured value not the constant 6

npm ERR! code 128
npm ERR! Command failed: /usr/local/bin/git checkout 970346aa88cda564308160744bc4c322f79c52af
npm ERR! fatal: reference is not a tree: 970346aa88cda564308160744bc4c322f79c52af

That commit is on mini-accounts. Not sure why it's erroring.

This plugin, together with https://github.com/interledgerjs/ilp-plugin-xrp-asym-client and https://github.com/ripple/ilp-plugin-xrp-paychan, define the paychan-related protocols from interledger/rfcs#381. I think the RFCs repo should describe only the ilp protocol, because the info, channel, fund_channel, channel_signature, claim, last_claim, ripple_channel_id etc. protocols are specific to XRP, and they would be different if you use Ethereum-based payment channels.

Current DEBUG_NAMESPACE shortens log entries to "ilp-plugin-xrp-server"

To ease troubleshooting - change to plugin full name: "ilp-plugin-xrp-asym-server"

It seems that https://github.com/interledgerjs/ilp-plugin-xrp-asym-server/blob/master/index.js#L498 is used instead of https://github.com/interledgerjs/ilp-plugin-xrp-asym-server/blob/master/index.js#L520-L524?

Obviously, since this plugin exposes only one LPI, but in reality represents possibly many connected peers, it's not possible to know who to send money to when plugin.sendMoney is called. The same problem exists for plugin.sendData, which this plugin inherits from ilp-plugin-mini-accounts, but there it's solved with deep packet inspection. That same trick cannot be used here.

Maybe we should just stop pursuing the concept of exposing only one LPI? Otherwise, maybe we should change the LPI to support server plugins?

The code that checks if the balance or bandwidth has been exceeded should return ILP Reject packets instead of throwing errors. Because of interledgerjs/ilp-plugin-mini-accounts#22, throwing errors will make it so no response is returned at all.

Similarly, the code that checks the fulfillment should also return ILP Reject packets

The default bandwidth (even for a new paychan/account) is 2000 drops. An attacker can easily steal from the connector by opening many paychans to the connector, which cost 10 drops a piece, make the connector forward 2000 drops on each paychan and never send a claim.

One mitigation to this problem would be if the connector allows only one paychan per XRP address. An attacker could steal 1990 drops, but not more.

Related to ilp-plugin-btp/issues/4.

Quote from the mini-accounts readme (emphasis mine):

Any secret can be used to authenticate; it is then hashed and becomes your account identifier. This has the advantage of requiring no UI-based signup flow nor any database storing usernames and passwords. It has the disadvantage that password recovery cannot be done, and only randomly generated passwords should be used. Treat your credentials like you would treat a wallet secret for a cryptocurrency.

Comparing it to a wallet secret is not quite right. If a wallet is compromised, the owner of the wallet is out of funds. With mini-accounts, however, if a client's password is compromised the mini-accounts instance is out of funds.

Why? Let's assume an attacker gets a client's password. To steal money, he would authorize to mini-accounts with the stolen password and send a payment up to the bandwidth limit. Since he cannot send a valid claim, further payments are rejected and he cannot steal more (which should not be much, but free money yay). If any legitimate (sane) client realizes that the account is compromised the only option is to close the paychan/account (sending another claim is not an option since the attacker still has the password and it cannot be changed). mini-accounts never got that last claim, so it is out the bandwidth.

So it is really in the best interest of mini-accounts that client passwords are not compromised.

_channelClose() is executed once the server detects that an incoming channel is closing. Should this function also unlink the channel from the account iff the unsecured funds were successfully claimed? Otherwise, the account cannot open a new paychan at a later point.

The plugin should reload the channel amount after it submitted a funding tx.

Follow up to this issue I created on the old repo.

It looks like you took out BTP authentication completely. Now it seems to be enough that a BTP client proves that he opened a paychan to the server, right?

See https://github.com/interledgerjs/ilp-plugin-xrp-asym-server/blob/master/index.js#L352-L366