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

Merging account with trustlines #324

Open
TorstenStueber opened this issue Jun 11, 2019 · 11 comments
Open

Merging account with trustlines #324

TorstenStueber opened this issue Jun 11, 2019 · 11 comments
Labels
CAP Represents an issue that requires a CAP. help wanted Open especially for those who want to write a CAP/SEP! needs draft This an issue that has no corresponding draft, and as such has not entered the CAP/SEP process.

Comments

@TorstenStueber
Copy link

TorstenStueber commented Jun 11, 2019

In order to merge an account that has a trustline first the following operations need to be executed:

  • a payment operation that transfers the remaining balance of the trusted asset to some other account
  • a change trust operation to remove the trustline

Certain smart contract schemes (such as payment channels) involve refund transactions for escrow accounts that are created and signed well before they are submitted on chain. If such a scheme deals with an assets different from the native asset, then these refund transactions need to contain the following operations (as stated above):

  • a payment operation to clear the balance of that asset
  • remove the trustline of that asset
  • merge account

The refund transaction will fail if the payment operation does not contain the correct balance of the trusted asset at the time the transaction is submitted. Since the refund transaction is created and signed a long time before, this would allow an attacker to invalidate the transaction by sending a single stroop of the trusted asset to the escrow account before the refund transaction is submitted.

A workaround would be to always set the trust limit of the asset to the current balance on the account. However, this has some downsides – e.g., topping up a payment channel would not be possible.

We propose any of the following solutions:

  1. Extend the accountMerge operation: if the account to be merged has trustlines and the account to be merged into has the same trustlines, then merge all assets into the latter account.
  2. Add a "merge asset" operation that behaves like a payment operation but does not specify an amount – it will transfer the complete remaining amount of the specified asset on the source account instead.
@johansten
Copy link
Contributor

For reference, #56

@theaeolianmachine theaeolianmachine added CAP Represents an issue that requires a CAP. help wanted Open especially for those who want to write a CAP/SEP! needs draft This an issue that has no corresponding draft, and as such has not entered the CAP/SEP process. labels Jun 14, 2019
@stanford-scs
Copy link
Contributor

Agree that this is tricky. I believe the two mechanisms you propose still leave some corner cases. For example, the merge asset operation could exceed the trustline or could exceed 2^{63}, or authorization could be revoked by an asset issuer. I think you need two transactions to close a channel (in case one fails because a party does something weird like delete their account or trustline), and it's better just to change the signers on the escrow account to put it under one party's control than to try to merge that account.

@TorstenStueber
Copy link
Author

You mentioned a third alternative that would also work and is currently the only practical solution to that problem (it's also the only "workaround" we found). However, it has big legal implications if neither party is allowed to get complete control over the escrow account – they might require certain licenses first.

Therefore it is very critical for legal reasons to extend the protocol in the sense of one of the first two alternatives I mentioned.

@erasmus
Copy link
Contributor

erasmus commented Feb 20, 2020

I believe the two mechanisms you propose still leave some corner cases.

There may be corner cases, but there is also the happy path, where it is indeed possible to do a "full" merge. Does it not make sense to accommodate this scenario?

@MonsieurNicolas
Copy link
Contributor

@TorstenStueber did you look at leveraging CAP-0023 that we're trying to finalize in the next couple months? It allows to decouple transfers from accounts and seems to have strong desirable properties for smart contracts in general (and we think for payment channels too). If it doesn't work in your particular scenario, we'd like to hear the blockers and potentially incorporate new changes into CAP-0023.

@erasmus
Copy link
Contributor

erasmus commented Feb 25, 2020

@MonsieurNicolas I'm not sure CAP-0023 will solve our problem, but perhaps I'm missing something here. The issue is not the receiving account, the problem is guaranteeing that the escrow account is fully merged.

Example:

  1. In order to avoid gaining control of our customer's funds (control of customer funds would impose additional legal/compliance requirements on us), we put customer funds on an escrow account during a transfer. The escrow account is multi-sig with a time bound and it tends to get multiple trust lines with positive asset balances.
  2. At the time of creating the escrow account, we also need the customer to sign refund tx which will execute at the expiry of the maxTime limit. (This is a safety measure to ensure the funds will not get lost in the case that the customer loses their secret key)
  3. This signed refund tx will fail, if additional assets are sent to the account in the time between signing and the time bound expiry. That is: the payment operation that clears a trust line asset from the account has to specify the exact amount of assets to transfer, and as long as that value can change, there is no guarantee that the operation will really clear the asset. In that case, the asset balance will be positive and the account merge operation will fail.

We're looking for a robust way to fully merge an account without having to specify the exact balance of each trust line asset. It is not an option to let one one account take full control of the account at any point in time.

@TorstenStueber
Copy link
Author

@MonsieurNicolas I agree with what @erasmus said. CAP-0023 does not solve our problem. CAP-0023 would be helpful in a case where the target account that the escrow is to be merged into might not have the proper trust lines (yet).

But this is not our problem: the target account of the merge operation does have the required trust lines. See @erasmus explanation for some more details.

@MonsieurNicolas
Copy link
Contributor

I was not saying that CAP0023 will make merging accounts easier, but that it should make writing payment channels easier.

In the context of payment channels, as transactions can fail, you really don't want to have any of the pre-signed transactions fail (a "merge asset" operation can fail) as the failure modes get very tricky to handle as the number of participants or assets increases.

CAP0023 allows to do just that (we think): as we decouple what the channel does with pre-signed transactions (creates pending transfers, this cannot fail), and what each recipient does ("claims" can fail but can be retried).

@TorstenStueber
Copy link
Author

I understand and that makes sense. But wouldn't that still require the kind of operations I am asking for in my initial post? So maybe CAP0023 would be an additional improvement but we still need some way to merge assets. Or am I missing something?

@MonsieurNicolas
Copy link
Contributor

yeah merging assets becomes interactive and doesn't need to be done from within the smart contract @TorstenStueber so the existing operations should be enough

@jonjove
Copy link
Contributor

jonjove commented Aug 24, 2020

@TorstenStueber what is your current status on this issue? Do you think that there are still fundamental problems here? I have some ideas based on what has been discussed in this thread (although I would want to make sure that I understand the problem fully before proposing any solutions), but obviously if the problem has been resolved then there is nothing of value for me to add.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CAP Represents an issue that requires a CAP. help wanted Open especially for those who want to write a CAP/SEP! needs draft This an issue that has no corresponding draft, and as such has not entered the CAP/SEP process.
Projects
None yet
Development

No branches or pull requests

9 participants
@johansten @theaeolianmachine @erasmus @MonsieurNicolas @stanford-scs @TorstenStueber @jonjove and others