Rename Amount Field to DeliverMax (to prevent incorrect handling of Partial Payments)

[shortthefomo]

There are numerous examples and it is even documented in the XRPL docs that using the "Amount" field in a partial payment can cause incorrect interpretation of transactions. There also have been numerous documented accounts XRPForensics has provided multiple examples before.

What I would like to suggest, although it would be a large undertaking, is renaming the "Amount" field to something more descriptive. ~Like "possible_amount_delivered".~ Link to existing doc and field referenced https://xrpl.org/partial-payments.html#partial-payments-exploit

This would probably require a deprecated flag on this function. Documentation changes, along with a slow roll out. Over multiple releases.

The reason I flip this requirement back on to the XRPL is that there continue to be numerous cases that arise from this issue. A simple field name change would "fix" all newer implementations of this.

[yxxyun]

maybe add a new transaction type “PartialPayment” dedicated to partial payment. Abandoned the tfPartialPayment flag for payment transaction.

[MarkusTeufelberger]

A name change would impact the whole ecosystem and a transaction type change could lead to not accepting partial payments because they are rare anyways so not seen worthy to implement. I honestly have very little sympathy for someone implementing something in their closed-source code clearly without reading the documentation (PartialPayments are one of the first items on "How to list XRP as an exchange") or engaging in any way with the developer community (other than getting probed and hacked by black-hat developers apparently).

How many exchanges are actually impacted by a change vs. by partial payments ("multiple" or "numerous" can be 2 or 3...)? There are a lot of other things in the protocol that are not intuitive at all and that don't have easily visible solutions (trustline QualityIn/Out comes to mind...).

Lastly, if you want such a change, you can already do it easily (similar to how xPring acts as a middleware): Write a proxy that forwards requests/answers to/from rippled and add (or even replace) that field you want to see when that flag is set. All you then need to do is to convince vulnerable exchanges to use your solution (which would be less than a screenful of code in most programming languages). If there's a lot of usage, I'm sure it would be easier to see that there's actually demand for changing the protocol rather than reading the documentation, gateway bulletins, working with community/professional experts or doing proper accounting (seconds after the first such deposit, your books won't balance any more and your system should immediately shut down anyways because some assertion fires!).

[nbougalis]

One of your recommendations (renaming the Amount field) is actually not as simple as it sounds but I do agree that something along those lines may help (although, to be perfectly honest, I think people will still find a way to shoot themselves in the foot).

I explored several possibilities along those lines a couple of years ago. Take a look at these:

https://github.com/nbougalis/rippled/commit/52d04bfcb0f63ffad58fdd495c036497c78f695c https://github.com/nbougalis/rippled/commit/f35f8fd94b6638d8198f8daff15f4246b4a9f18f

I think that we should revisit this issue; the above should be fairly easy to rebase, and they are both fairly straight forward. @carlhua, any thoughts?

[RareData]

Reading the documentation should be expected of anyone working as a developer for an exchange, but vulnerable exchanges keep popping up and it's time to do something about it.

I prefer: https://github.com/nbougalis/rippled/commit/52d04bfcb0f63ffad58fdd495c036497c78f695c This would protect new exchanges, while old exchanges could turn it off manually.

Making partial payments off by default would probably take away most of its utility though. Depending on the current use, it might be wiser to remove partial payments completely.

[WietseWind]

I think it would be a shame to block receiving partial payments by default. Removing Partial Payments: even a bigger shame. If the problem is that exchanges/devs (who didn't RTFM) look at the "instruction" (being the Amount field) instead of the tx outcome, I'd say renaming the Amount field for partial payments would make most sense to me. That way faulty implementations will simply look at a field that doesn't exist, which (if the implementation is even ignoring errors) effectively is "0". That'll protect hem. Eg. PartialAmount for transactions with the Partial Payment flag set.

[RareData]

If we don't want to take away its utility, I agree that our best option is to rename the field. PartialAmount is a great suggestion.

[WietseWind]

@RareData Maybe even UnsafePartialAmount, so devs not looking at the docs coding a condition to look at Amount and then at the Partial field are warned that it's an Unsafe amount field. If they then STILL credit the full amount...

P.S. (Re: my previous comment) My problem with removing partial payments isn't only the fact that I actually like the feature: it's also that a matter of principle: if we start removing all potential interesting features because a few people don't RTFM, where does it end?

[sublimator]

where does it end?

totalitarian nanny coin

[MarkusTeufelberger]

PartialAmount or similar don't really capture the "maximum amount to be transferred" intention here imho, but that's a naming discussion and thus one step further already.

[Silkjaer]

Speaking to the scope: I've seen 4-5 successful attacks within a month, all on new implementations, but ranging from small to big VASPs. There's a couple of entities constantly probing new implementations, and they often discover implementations faster than they're identified by bithomp, xrpscan, or xrplorer.

Even though it is not a bug/exploit in the XRPL directly, that's not necessarily the way it is communicated by exploited entities, indirectly damaging the reputation of the XRPL. So while I agree with MarkusTeufelberger and have very little sympathy for implementers not reading the documentation, I also think that it's on due time to reconsider, if anything can be done to prevent future bad implementations, in the least intrusive way possible.

I like the idea of renaming the Amount field to UnsafePartialAmount or similar better covering name, and maybe even only on the API response level?

[sublimator]

@MarkusTeufelberger Agreed, I was thinking the same thing. DeliverMax ?

[RareData]

I'll throw another suggestion into the mix: PotentialAmount

[MarkusTeufelberger]

4-5 successful attacks per month consistently or did just a lot of particularly incompetent players show up recently (who don't even follow basic accounting practices, such as balancing their books...)?

As I already said, the least intrusive way would be to filter out this field and/or replacing it in middleware instead of changing the lowest-level API available.

One of the issues is probably anyways that potentially a lot of these vulnerable entities use the public servers supplied by Ripple, not running their own infrastructure. So a good first step could be to filter out this field from any response from s1.ripple.com etc. and/or adding severe rate limiting there so it is impossible to use these servers in production, forcing people to read documentation to run their own infrastructure.

[RareData]

Consistently and surprisingly many big name exchanges/developers/projects.

[MarkusTeufelberger]

Yet they are not named, not even in this ticket... Maybe a list of these many big name people would help to draw some attention to the issue from people that re-implement something for the nth time in their closed source corporate environment?

[Silkjaer]

I do not have long-term data at hand, but no, not consistently 4-5 per month. It should, however, be compared to the number of new exchange implementations. I don't think it makes the concern less valid though: many “implementers” do not read the documentation.

I think affected parties can easily be running their own nodes, in which case filtering on public nodes will not have an effect.

[RareData]

Well, I can name at least R3's Corda Settler that I helped patch in 2019. Thomas and Wietse have helped patch numerous others, not sure they can/want to name them publicly.

[WietseWind]

We tested over 100 exchanges, about half of them were vulnerable and credited the proposed XRP in the Amount field to users. Some really big names amongst them. Most of them patched within hours/days. We have a list but I don't feel comfortable sharing that publicly, indeed. None of them (their devs) checked the XRPL docs.

[sublimator]

woaaaah

ripple is one of those things that even when you're working on it full time, you can confuse yourself

any efforts to make it simpler/more foolproof (without sacrificing features) is probably a "Good Thing TM"

[MarkusTeufelberger]

Then I'm unsure if changing a field and breaking the API for the entire ecosystem would be enough to fix this systemic issue. "Oh, Amount is now named MaxAmount or whatever on some transactions starting with 1.7.0? Let's just add that..."

[shortthefomo]

@MarkusTeufelberger can you point me in the direction of that please? "Oh, Amount is now named MaxAmount or whatever on some transactions starting with 1.7.0? Let's just add that..."

Is that a patch some where?

Thank you kindly.

[movitto]

One additional thing to consider is complexity of XRPL client logic, by renaming this field with a cutoff rippled / ledger version, clients that need to handle historical ledgers will have to have yet another special edge case for before/after this patch is applied.

I feel a flag to block partial payments as implemented in nbougalis@52d04bf is a good tradeoff and would go even further to recommend that partial payments are disabled by default. The logic being that if exchanges / other endpoints don't explicitly implement logic to handle the partial payments edge case they are vulnerable, disabling this flag (enabling partial payments) would simply be one additional step ontop of addressing the issue.

[Silkjaer]

If a partial payment flag is introduced for accounts and it is disabled by default, the entire partial payment feature might just as well be deprecated, in my honest opinion. In many of the cases where partial payments are beneficial, the receiving entity will not be aware that they received a partial payment – this could e.g. be in cases where a regular user is doing a market sell/buy to swap currencies.

[yxxyun]

Accounts should be allowed to receive partial payment by default, just explicit the transaction is partial payment , so I prefer to add "partialpayment" transaction.

[movitto]

This sounds like a "have your cake and eat it too" situation. If the user wants to leverage partial payments but is not aware of it, they are potentially exposed to the original vulnerability where amount != partial payment amount. By requiring the user to explicitly "opt-in" to partial payments, the feature can be supported but with the explicit authorization of the end-user.

Explicit PartialPayment transactions could work but is also another edge case / logic fork clients would have to handle. With the flag / functionality disabled by default, clients could ensure their application handles delivered_amount vs amount, enable the flag and then forget about it.

[Silkjaer]

The users who are potentially affected by the bad implementation bug is virtual asset service providers that maintain a separate ledger to keep track of their users' funds, where an average user has an account and uses a software or hardware wallet to sign and submit transactions.

The average user will never be directly affected by the vulnerability that arises from not balancing the books and the technical obstacle of understanding flags and partial payment will keep them from using the feature, which will lead to fewer clients (software wallets etc.) implementing a feature to do partial payment-powered currency swaps, effectively killing the feature.

It should be solved in a way that doesn't make the learning curve any steeper for average users. I think that renaming the field in an API-level would solve most problems without anyone noticing a disruption of service unless they were in actual danger:

1. average users who are using the service through various clients will not depend on the fields of the transaction but on the change of balance.
2. VASPs receiving partial payments will either read the correct field and balance the books correctly, or they would have been vulnerable and the change of name would have done its job

It would be interesting to look at how much the feature is actually used, as I think it is mainly used for exploits and arbitrage circular payments.

[yxxyun]

well, since partial payment is a path payment, so separate it to a new transaction type maybe is not a good idea. but disable it by default also impact the path payment.

[WietseWind]

That something isn't used a lot today, doesn't mean it will not make a lot of sense if it gets used more often in the future. For Payment transactions sending another asset than the asset to be delivered, it makes sense send a Partial Payment to take care of the transfer fees on all used (send/receive) pairs.

[movitto]

@Silkjaer you bring up a good point about the different classes of users who would be affected by a flag, I agree that 'average' users should not be impacted by any fix here since the Virtual Asset Service Providers (eg exchanges, etc) are the ones affected by the "vulnerability".

Looking at @nbougalis' "BlockPartialPayments" flag patch, it prevents partial payments from being received, not preventing them from being sent. Eg end users can send partial payments at will, but sending to destinations that do not allow it (whether or not by default) will result in tecNO_PARTIAL_PAYMENTS.

Thus end users/client (who have to explicitly set tfPartialPayment on a transaction anyways) don't have to worry about setting/clearing lsfNoPartialPayment on their account, rather the onus would be on the VASPs.

[WietseWind]

@movitto If the default is to block incoming partial payments for all new accounts, that effectively blocks the entire feature, as no one can send it to all those new accounts with the setting enabled and incoming partial payments blocked.

It prevents partial payments from being received, not preventing them from being sent.

When blocking incoming partial payments, there's not much use in sending them: they won't be accepted.

[movitto]

All in all, there is another approach that we haven't explored. Instead of a technical solution, a marketing / publicity solution would also be effective.

We are trying to figure out a workaround make it impossible or very hard to "mess up" partial payment vulnerabilities, which are the result of improper client implementations and not XRPL itself. That being said the point about the "reputation" of the XRPL is a valid one, and while in an ideal world the vulnerable exchanges would take the publicity hit, if too many are affected the XRPL could suffer the consequences.

Since I'm assuming the known vulnerable VASPs have already been contacted, we could get ahead of the issue via a public outreach campaign from XRPL developers. I'm not suggesting calling out exchanges without first attempting to contact them directly/privately a few times, but end-users that use those exchanges would be affected if all funds are siphoned out and there are no more funds to access in the long run. I know there has been some outreach done via twitter / other channels, but a timed / coordinated publicity campaign would go a long way to driving awareness of this issue (especially if @ripple got involved)

[movitto]

@movitto If the default is to block incoming partial payments for all new accounts, that effectively blocks the entire feature, as no one can send it to all those new accounts with the setting enabled and incoming partial payments blocked.

It prevents partial payments from being received, not preventing them from being sent.

When blocking incoming partial payments, there's not much use in sending them: they won't be accepted.

Is there interest in continuing this conversation (and others) via a virtual developer meetup? DNP would be happy to set one up and schedule a zoom call for the near future (perhaps in a few weeks) with a formal agenda, system to submit topics, etc.

[MarkusTeufelberger]

I much prefer a written format, ideally even in RFC format if something substantial should be changed. Voice/video tends to be more restrictive beyond a handful of people usually and is horrible to document well (also the restriction to live conversations is really not that nice if you're in a different time zone). General discussions, sure - decisions, please no.

[shortthefomo]

@movitto ~ I do like that suggestion "marketing / publicity solution " as a fallback where by some sort of badge or something could be worked into the solution, possibly even provide some sort of path for validator dev's to provide a service with a badge/stamp of approval for exchanges.

But Im hoping for some possible work around, where by a "stupid proof" solution could help the ledger it's self far more. Know from all my years of software development, that this goes a very long way.

[movitto]

@MarkusTeufelberger my fear is that the conversation is starting to move in circles, especially with regard to whether or not it would be worth disabling partial payments by default. I agree notes would need to be taken (I'll rope someone in to help us out with that) but often voice / video chat is a much more efficient mechanism to communicate topics / have discussions.

@lathanbritz I like the badge idea, and perhaps it could even be taken a step further, with the implementation being manifested via an IOU on the XRPL. Of course revoking it in that case would be challenging, but thinking off the top of my head perhaps that's a new feature that could be added to the XRPL ('special' / revokable IOUs where the issuer can deduct a specified amount of an IOU from a particular user at a given time... thoughts?)

As far as writing "stupid proof" software I also agree, the challenge comes in with the code complexity tradeoff. As @MarkusTeufelberger mentioned above it often is simpler to implement user checks via a higher level abstraction / decorator so that the underlying codebase doesn't have to handle all the edge cases (ala the unix philosophy, "do one thing well")

[ximinez]

Just thinking out loud here: It sounds like we're looking for a way to minimize the effect on folks that have read the documentation and are aware of the issues, and maximize the effect on folks who haven't and are at risk of being exploited. What about this?

1. Define a new account flag, but instead of enable/disable partial payments, it would basically mean "I understand how partial payments work". It would default to false for everybody (existing and newly created accounts). Existing users that have a correct working implementation could send a single AccountSet transaction and be done. To reduce the chances of someone setting it by accident, it would only be defined in the same documentation that describes the exploit.
2. If a partial payment is sent to an account with that flag set, that fact will be saved in the metadata, much in the same way the delivered amount is stored.
3. However, the value of that metadata flag will never be returned on the API. Instead, if it is set, the transaction will be returned unaltered. If it is not set / not present on a partial Payment transaction the Amount field will be renamed to something like UnsafePotentialAmount at the API level.

Upsides: Minimal effect on existing exchanges that have a correct implementation. Flags very obviously problems for those that have not. It may even crash incorrectly implemented client code, which is better than a wrong answer. Downsides: Historical transactions will always have the field renamed. It will be more difficult to separately verify the signature on any transaction that has the field renamed.

[sublimator]

@ximinez Sounds reasonable

[MarkusTeufelberger]

Minimal effect on existing exchanges that have a correct implementation.

To keep using their existing implementation, everyone(!) that does it correctly needs to send an AccountSet transaction though, right? Also even their correct implementation will break between the time this amendment enables and the transaction is sent (unless there's e.g. a few weeks/months of grace period where you can set the flag but still get the current behavior regardless) if they need the "Amount" field for some other reason.

[RareData]

There's no need to introduce additional flags. There's already a huge feature set to support by wallets and would introduce a significant lag to e.g. hardware wallets that even here in 2020 only support direct XRP payments (no other transaction types or fields such as memos, yet alone account settings). Simply renaming the field for partial payments (at some level) would be enough.

[ximinez]

To keep using their existing implementation, everyone(!) that does it correctly needs to send an AccountSet transaction though, right?

That is correct.

Also even their correct implementation will break between the time this amendment enables and the transaction is sent (unless there's e.g. a few weeks/months of grace period where you can set the flag but still get the current behavior regardless) if they need the "Amount" field for some other reason.

That's a good point. I think it would be pretty feasible to roll the functionality out in such a way that there would be plenty of time to set that flag. I also think it would be pretty feasible to have some more advanced functionality when returning transactions from before this change. E.g. Perhaps for any "legacy" transactions that don't have that flag in the metadata, look up the account and check the flag value there. There would probably be a performance hit, so we'll have to figure out if something like that is worth it.

[intelliot]

Going back to the original post: I think renaming the Amount field, if done in a certain way, should be easier than most of us are assuming.

rippled uses a binary data format. The binary format is the canonical format; JSON is just for convenience. The Amount field is currently named as such in the JSON format, but it could be renamed. @thejohnfreeman previously suggested an unambiguous name: MaximumAmountSent. It looked weird to me at first, but it has grown on me over time. The more I think about it, the more sense it makes. In the context of partial payments, it is the maximum amount sent by the payment.

I believe we can migrate to MaximumAmountSent without changing anything about the underlying ledger. It is rare that users want MaximumAmountSent anyway; delivered_amount is usually what they actually wanted to know.

1. For methods that return JSON, add MaximumAmountSent alongside Amount. This is backward compatible as it only adds a new field.
2. Document that Amount is deprecated, and MaximumAmountSent or delivered_amount should be used instead. Warn that the Amount field will be removed in about a year or two.
3. Update ripple-binary-codec to support the changes.
4. Use the new api_version functionality to remove the dangerously-named Amount field. For example, when you specify api_version: 3 with your request, there will be no Amount field at all. That value is provided as the MaximumAmountSent.
5. After 2-4 releases (~1 year), make api_version: 3 the default.

[ximinez]

I like @intelliot's idea. It's a lot simpler, and takes advantage of the API versioning that we finally have available. One thought:

• It's definitely good to document that Amount will be deprecated, but remember the whole issue is because of people who don't read the documentation. That makes me wonder, though, how are people learning about the API if they aren't reading the docs. I suspect they're doing some trial and error and visually looking at the results, so maybe we can also bring the documentation to where those users are. For example: on the command line --help output, add a warning in the description for tx. Any time the API (RPC, websocket, etc) returns a delivered_amount, add a warning field embedded in the tx data. I'm thinking something like: tx: { Account: foo, TransactionType: Payments, warnings: [ "Some payments may not deliver the full amount provided in Amount. See delivered_amount and https://xrpl.org/partial-payments.html#partial-payments-exploit" ] }. If it is a partial payment: warnings: [ "Some payments may not deliver the full amount provided in Amount. See delivered_amount and https://xrpl.org/partial-payments.html#partial-payments-exploit", "THIS PAYMENT DID NOT DELIVER THE FULL AMOUNT." ].

[nbougalis]

I appreciate the lively conversation!

I understand and agree that partial payments are useful and it sucks that we have to think about making that functionality less accessible by having a flag and/or a new transaction type simply because implementers are careless.

And while I don't believe we should bubble-wrap the world in an attempt to prevent accidents, I also think that we should consider the fact that so many people have failed to implement the feature correctly. Documentation aside, is this the UX we offer?

We should try and figure out what percentage of payment transactions have the tfPartialPayment flag set, excluding self-payments, before we make a decision on what, if anything, we should do about this.

[ximinez]

I think in this case the percentage of payments with tfPartialPayment is less important than the financial impact on those who have implemented it wrong, and the impact on our reputation every time a "new" "exploit" hits the news.

[Silkjaer]

As for statistics about tfPartialPayment, it is tricky to define circular payments, as it appears many of the circular payment accounts are migrating away from “self-payment” and using pairs of accounts to do the pathfinding arbitraging.

I will think about filtering it to fetch some stats of the, in my opinion, intended uses of partial payments.

[Silkjaer]

The more I have looked into the use of partial payments, the more I start to think the XRPL would be better off without it: as long as it is there, it will be misused (in my opinion) for two things:

1. Exploiting bad VASP implementations (we're trying to fix that). This has the least effect on the XRPL itself, as it's just a minor percentage of transactions sent, but it has a bigger impact on the image/reputation of the XRPL.
2. Automatic arbitrating in circular payments. This is resolved, I think, in an amendment in 1.6.0. However, it does look like the same setup is being copied by using multiple accounts, technically not making it a circular payment.

I have extracted statistics from the XRPL since January 1, 2020, and tried to classify the data:

1. failed: The payment failed (any error code). The majority is tecPATH_DRY, as a result of submitting arbitraging circular payments.
2. identical_sender_and_destination: sender and recipient are the same (self-payment/circular payment)
3. spam: excluding payments from accounts identified as spammers (fake transactions for a Ponzi arbitrage scheme)
4. no_sendmax: tfPartialPayment was set, but not SendMax
5. destination_is_amount_issuer: Destination equals Amount issuer. Special issuer value. Often the case for circular payments too.
6. sender_is_sendmax_issuer: Account equals SendMax issuer. Special issuer value. Often the case for circular payments too.
7. destination_is_sendmax_issuer: Destination equals SendMax issuer
8. sender_is_amount_issuer: Account equals Amount issuer
9. likely_exploit: Payment setup matches the typical exploit payments
10. legit: All the rest.

Think of the list as a switch/case in the order of listed above. If it falls into the failed classification, it is not checked for identical_sender_and_destination etc.

I have plotted the data into an interactive chart. A CSV with the data (and Accounts) is available here.

[yxxyun]

How many payment transactions with paths are set tfPartialPayment?

[Silkjaer]

How many payment transactions with paths are set tfPartialPayment?

I used March 13 as a sample for this. Here are the number of payments with paths:

╒═══════╤══════════════════╤═════════╕
│"count"│"tfPartialPayment"│"success"│
╞═══════╪══════════════════╪═════════╡
│871634 │true              │false    │
├───────┼──────────────────┼─────────┤
│5147   │true              │true     │
├───────┼──────────────────┼─────────┤
│331    │false             │true     │
├───────┼──────────────────┼─────────┤
│23     │false             │false    │
└───────┴──────────────────┴─────────┘

[shortthefomo]

Heads up looks like the new FTX exchanged just got "tested" with this issue. The new FTX exchange: https://xrpscan.com/account/rPFXvVo2fYXVPdV9gCHQouHsMgMhQ2aUwM

Notice the partial payment that just went in there to day. https://xrpscan.com/tx/957AF5856DF4265AB8A44537B62081D78CA4ECA6FA38F89358883510380A0EA7

0.050426 delivered of the 10,211 XRP ~ will be interesting to monitor if they get nailed by this. I hope not!

[mDuo13]

For what it's worth, I continue to be supportive of renaming Amount field in JSON as @intelliot and others have suggested, though I'm partial to DeliverMax because it mirrors DeliverMin and SendMax. The mere existence of both DeliverMax and SendMax is likely to make people realize that there can be a difference between the amount "sent" and the amount "delivered".

P.S. part of the reason I like this change is that it breaks fewer correct implementations than you might think. As long as transaction serialization continues to accept the old Amount field name as input while always outputting DeliverMax, it does nothing to people sending transactions; and anyone who was reading the Amount field of a transaction (instead of delivered_amount) was likely making a mistake. Except, I suppose, ledger explorers and data archivers that just read everything, but those need to update to adjust for new fields anyway.

P.P.S. There may be some technical hurdles because the Amount field definition is reused across other use cases where DeliverMax would make less sense, though...