fix(AMM): prevent orphaned objects, inconsistent ledger state: (updates XLS-30)

[gregtatcam]

Proposed commit message, including acknowledgement block, for when this PR is squashed and merged:

fix(AMM): prevent orphaned objects, inconsistent ledger state: (#4626)

When an AMM account is deleted, the owner directory entries must be
deleted in order to ensure consistent ledger state.

* When deleting AMM account:
  * Clean up AMM owner dir, linking AMM account and AMM object
  * Delete trust lines to AMM
* Disallow `CheckCreate` to AMM accounts
  * AMM cannot cash a check
* Constrain entries in AuthAccounts array to be accounts
  * AuthAccounts is an array of objects for the AMMBid transaction
* SetTrust (TrustSet): Allow on AMM only for LP tokens
  * If the destination is an AMM account and the trust line doesn't
    exist, then:
    * If the asset is not the AMM LP token, then fail the tx with
      `tecNO_PERMISSION`
    * If the AMM is in empty state, then fail the tx with `tecAMM_EMPTY`
      * This disallows trustlines to AMM in empty state
* Add AMMID to AMM root account
  * Remove lsfAMM flag and use sfAMMID instead
* Remove owner dir entry for ltAMM
* Add `AMMDelete` transaction type to handle amortized deletion
  * Limit number of trust lines to delete on final withdraw + AMMDelete
  * Put AMM in empty state when LPTokens is 0 upon final withdraw
  * Add `tfTwoAssetIfEmpty` deposit option in AMM empty state
  * Fail all AMM transactions in AMM empty state except special deposit
  * Add `tecINCOMPLETE` to indicate that not all AMM trust lines are
    deleted (i.e. partial deletion)
    * This is handled in Transactor similar to deleted offers
  * Fail AMMDelete with `tecINTERNAL` if AMM root account is nullptr
  * Don't validate for invalid asset pair in AMMDelete
* AMMWithdraw deletes AMM trust lines and AMM account/object only if the
  number of trust lines is less than max
  * Current `maxDeletableAMMTrustLines` = 512
  * Check no directory left after AMM trust lines are deleted
  * Enable partial trustline deletion in AMMWithdraw
* Add `tecAMM_NOT_EMPTY` to fail any transaction that expects an AMM in
  empty state
* Clawback considerations
  * Disallow clawback out of AMM account
  * Disallow AMM create if issuer can claw back

This patch applies to the AMM implementation in #4294.

Acknowledgements:
Richard Holland and Nik Bougalis for responsibly disclosing this issue.

Bug Bounties and Responsible Disclosures:
We welcome reviews of the project code and urge researchers to
responsibly disclose any issues they may find.

To report a bug, please send a detailed report to:

    bugs@xrpl.org

High Level Overview of Change

Fixes:

• Withdrawing all funds from AMM automatically deletes AMM account. The owner directory entries must be deleted. This fix addresses:
  • Clean up AMM owner dir linking AMM account and AMM object
  • Delete trustlines to AMM
• Disallow check create to AMM
• Fix unconstrained entries in AuthAccount in Bid transaction

Context of Change

The bugs are introduced in the merged AMM PR. They result in resource leak and inconsistent ledger state.

Type of Change

• Bug fix (non-breaking change which fixes an issue)

Before / After

Fixes the resource leak and unconstrained entries

Test Plan

Added a unit-test to disallow check create and extended withdraw all unit-test.

[RichardAH]

If I can ask, why not refactor the DeleteAccount transactor so you can call it from AMMWithdraw? or possibly even better: don't delete the account on the last withdrawal but allow anyone to delete it with the DeleteAccount transactor. I think having two different places in code where all the complexities of account deletion are dealt with is just asking to introduce bugs later. Some new object will be added at some point and one of the delete routines wont be updated for it. We should have a single canonical way to delete an account.

[gregtatcam]

If I can ask, why not refactor the DeleteAccount transactor so you can call it from AMMWithdraw? or possibly even better: don't delete the account on the last withdrawal but allow anyone to delete it with the DeleteAccount transactor. I think having two different places in code where all the complexities of account deletion are dealt with is just asking to introduce bugs later. Some new object will be added at some point and one of the delete routines wont be updated for it. We should have a single canonical way to delete an account.

Thought of reusing AccountDelete crossed my mind. Except there is not much in common between deleting a regular account and deleting the AMM account. In fact, the former explicitly doesn't delete the trustlines. One common part is the loop to iterate over the owner directory objects. I think making this loop, or as much of AccountDelete::doApply() as possible, into a function with a callback to handle the specifics of account deletion makes sense.

I don't quite see how allowing anyone to delete AMM account with AccountDelete is going to work. It doesn't seem to be fair to all liquidity providers (LP). Even if AccountDelete is restricted to LP only it doesn't feel fair either. Perhaps LP voting on deletion might work but this is quite a bit of rework for both the specs and the implementation.

[RichardAH]

I don't quite see how allowing anyone to delete AMM account with AccountDelete is going to work. It doesn't seem to be fair to all liquidity providers (LP). Even if AccountDelete is restricted to LP only it doesn't feel fair either. Perhaps LP voting on deletion might work but this is quite a bit of rework for both the specs and the implementation.

Hi Greg

This would only be if issued LP tokens = 0. I.e. after final withdraw. Optionally the empty AMM could be left for future users (although you would need to modify AMMDeposit for this I think).

I think making this loop, or as much of AccountDelete::doApply() as possible, into a function with a callback to handle the specifics of account deletion makes sense.

Indeed. Personally would refactor DeleteAccount so the thing apply() calls is a templated function that lets me "dry delete" (try a pretend delete and see if there are issues) and "hard delete" (delete no matter what). For an example of how this can be done see dry/wet runs for IOUEscrow here:

rippled/src/ripple/ledger/View.h

Line 488 in eea7fcb

R dryRun)

There's a few reasons I would do it this way but the most important is destructor consistency. If you consider that ledger objects are created, live on the ledger and then eventually are removed from the ledger, and these objects may have dependent sub objects, then correctly deleting the objects becomes important... such that there are no orphaned objects (and no possibility of orphaned objects). This means there should be a canonical way to delete each type of object, not 2 or 3 different deletion routines throughout the code (that all worked when they were written but open an exploit when unrelated code is added to the ledger later.)

IMHO there should be one canonical place to go to delete an account regardless of why you're deleting it.

[nbougalis]

This change looks reasonable and, on first look, addresses the serious issues that we reported last week.

I would recommend one more addition, probably in TrustSet::preclaim (you could do it in trustCreate, but that function and its 7,000 arguments ought to die): if one side of the trustline is an AMM, then ONLY create the trustline if the asset is the AMM's token. Since AMMs don't issue any other assets, trustlines for anything else are meaningless and just serve to litter the owner directory of the AMM.

I also share some of Richard's concerns about the different deletion strategies. That's something that should be considered for a future cleanup.

Lastly, I would recommend an acknowledgement block in the commit messages, similar to those included previously for responsibly disclosed bugs.

[nbougalis]

Nothing actionable, just a comment: the dirFirst/dirNext iterators are so ugly to use...

[intelliot]

(Moved to top of PR description for better visibility)

[RichardAH]

Suppose I create a trustline to the AMM, increasing my ownercount by 1.
Then the AMM is subsequently deleted, deleting my trustline object.
Where in this code is my ownercount returned to me? I don't believe trustDelete handles this?

[gregtatcam]

Thanks, will update.

[RichardAH]

It doesn't appear that lsfAllowTrustLineClawback is checked for? What happens if an issuer of one of the assets that the AMM holds claws it back out of the AMM?

[gregtatcam]

IMHO there should be one canonical place to go to delete an account regardless of why you're deleting it.

Hi Richard,

This makes sense. It could be addressed by a common function, say cleanupOnAccountDelete(), which is called in AccountDelete::doApply() to handle a regular account clean up and in AMMWithdraw::deleteAccount() as part of the auto-delete on a final withdraw to handle AMM account clean up. This function cleans up the owner directory for either a regular or AMM account. This is more than an iterator with a callback that I suggested before. I think it still makes sense though to keep the auto-delete for the following reasons:

1. This is a big enough change to the way AMMs work that if we made the change we can't just throw it in at the last minute. We're not against making changes that substantially improve things, but unless this does we should keep the current spec.
2. AMMs are different enough from regular accounts that "auto delete" makes a lot of sense, and if we did use "AccountDelete" allowing another account to do the delete is a big change to that transaction.

[gregtatcam]

This makes sense. It could be addressed by a common function, say cleanupOnAccountDelete(), which is called in AccountDelete::doApply() to handle a regular account clean up and in AMMWithdraw::deleteAccount() as part of the auto-delete on a final withdraw to handle AMM account clean up. This function cleans up the owner directory for either a regular or AMM account. This is more than an iterator with a callback that I suggested before.

Actually I think a callback to get a specific object deleter is still needed. Otherwise would have to bring different ledger entry deletion functions from AccountDelete transactor into the module that's going to host this common function (I'm thinking View.h/cpp is a good place for it). So this common function would take nonObligationDeleter for a regular account and some other function to delete the trustlines in case of AMM.

[intelliot]

@gregtatcam - going forward, please sign your commits, as mentioned in CONTRIBUTING. (Past commits are fine to leave as-is)

[nbougalis]

Something occurred to me (several hours later) but the new deletion code still has a problem @gregtatcam: it can be coerced into doing a very large amount of work (since there's no limitation on how many trustlines to the AMM's token can exist). This is a huge problem: not only would the transaction execution take a lot of time, but it could create a very large amount of metadata and that's a separate problem which has been an issue before (see e.g. the offer crossing limits or the remove-found-unfunded-offers-as-they're-found code, which runs even if the offer crossing fails for other reasons.)

Also, @RichardAH that's a nice catch, with the ownercounts. It would have sucked to miss this.

[RichardAH]

Something occurred to me (several hours later) but the new deletion code still has a problem @gregtatcam: it can be coerced into doing a very large amount of work (since there's no limitation on how many trustlines to the AMM's token can exist). This is a huge problem: not only would the transaction execution take a lot of time, but it could create a very large amount of metadata and that's a separate problem which has been an issue before (see e.g. the offer crossing limits or the remove-found-unfunded-offers-as-they're-found code, which runs even if the offer crossing fails for other reasons.)

Also, @RichardAH that's a nice catch, with the ownercounts. It would have sucked to miss this.

This also is a problem indeed. There aren’t many good solutions to it either. Until the final withdrawal all the LP token trustlines are technically valid because those users might obtain LP tokens.

The only (obvious) safe way to do this is the final withdrawal puts the AMM into a cleanup state. Then another transactor AMMDelete can do an amortised deletion of the trustlines with a maximum per iteration. It would either return tesSUCCESS or tecMORE (meaning it needs to run again.) In cleanup mode it should be impossible to add more trustlines.

It’s not a good solution but it is a solution.

[gregtatcam]

This also is a problem indeed. There aren’t many good solutions to it either. Until the final withdrawal all the LP token trustlines are technically valid because those users might obtain LP tokens.

Can we delete a trustline on a withdraw if the account's LP tokens are 0? The auto-delete could only happen when all LP tokens are 0, which means individual LP tokens are also 0. Should this mitigate this issue?

[RichardAH]

This also is a problem indeed. There aren’t many good solutions to it either. Until the final withdrawal all the LP token trustlines are technically valid because those users might obtain LP tokens.

Can we delete a trustline on a withdraw if the account's LP tokens are 0? The auto-delete could only happen when all LP tokens are 0, which means individual LP tokens are also 0. Should this mitigate this issue?

That's fine, the problem arises when there are 100,000 trustlines

[gregtatcam]

That's fine, the problem arises when there are 100,000 trustlines

So it doesn't need to be mitigated?

[gregtatcam]

I would recommend one more addition, probably in TrustSet::preclaim (you could do it in trustCreate, but that function and its 7,000 arguments ought to die): if one side of the trustline is an AMM, then ONLY create the trustline if the asset is the AMM's token. Since AMMs don't issue any other assets, trustlines for anything else are meaningless and just serve to litter the owner directory of the AMM.

@nbougalis

I can't tell from AMM's root account this AMM's LP Token Currency. To figure out I'd need to get AMM object ltAMM to find this AMM's Currency. This means I'd need to iterate over AMM account owner directory entries, which leads back to your concern of iterating over AMM trustlines.

[RichardAH]

So it doesn't need to be mitigated?

It does. It's entirely possible an AMM has a lot of trustlines at the time it's being deleted. Without a cap on the iteration you will exceed the metadata size limit not to mention taking a long time to close the ledger.

[RichardAH]

I can't tell from AMM's root account this AMM's LP Token Currency. To figure out I'd need to get AMM object ltAMM to find this AMM's Currency. This means I'd need to iterate over AMM account owner directory entries, which leads back to your concern of iterating over AMM trustlines.

This is a design flaw, since these objects are inherently linked you should be able to follow a pointer in either direction.
However a workaround if you don't want to add another optional AccountRoot field is just check the first entry in the first directory page. It's always going to be the ltAMM (unless I'm missing something)

[gregtatcam]

is just check the first entry in the first directory page. It's always going to be the ltAMM (unless I'm missing something)

Thanks. Because ltAMM is the first object to be inserted.

[gregtatcam]

It does. It's entirely possible an AMM has a lot of trustlines at the time it's being deleted. Without a cap on the iteration you will exceed the metadata size limit not to mention taking a long time to close the ledger.

Would it not make sense to clean up as much as possible on each withdraw if the account's LP tokens balance is 0? My concern is that there is no incentive to call AMMDelete, if we take this path, except for being a good XRPL citizen.

[intelliot]

Released with 1.12.0 via 58f7aec