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.

[Bronek]

Note the new field output field in API will be named DeliverMax rather than deliver_max as is the convention for synthetic fields (e.g. delivered_amount). The reason is to maintain symmetry with the newly added input API field named the same; in effect DeliverMax is not as much synthetic field (even though some implementation details will make it look like one), but an alias to Amount (in API ver 1) or a new name to replace Amount (in API ver 2 and subsequent)

[Bronek]

The draft PR is #4733 (comment) (unfinished, design could change, link pointing to comment on what I am actually doing)

[Bronek]

This change will only impact Payment transaction type.

It could be argued that CheckCash might be within the scope, but I think that's not the case: the Amount in this transaction type will be always equal to delivered amount, or the transaction is unsuccessful. To allow a different delivery amount one would use DeliverMin field and do not populate Amount at all; this is specifically designed to avoid ambiguity. Similarly in CheckCreate we do not have Amount at all, but SendMax field.

[Bronek]

@intelliot there's a subtle problem with change as proposed in my draft PR and I want to discuss it here. The way it's currently written, the user cannot pass both DeliverMax and Amount when submitting and/or signing Payment transaction - instead only one of these must be used and they are treated as synonymous. However when we output transactions to an API v. 1 clients (but not API v. 2 clients) they will see both of these fields, meaning the transaction JSON does not roundtrip. This bothers me.

Ways out I can see are:

• allow both fields on input, as long as they are identical ; or
• do not produce DeliverMax on output to API v. 1 clients ; or
• do not produce Amount on output to API v. 1 clients

The last option is clearly a breaking one, so we can write it off. Of the remaining two I think first is preferable. What's your opinion ?

[intelliot]

Agreed. "allow both fields on input, as long as they are identical" sounds good to me.

[ximinez]

I don't have a strong opinion, but I think allowing identical fields on input is preferable to not outputting the field at all. However, I don't see where there would be any necessity for making a round trip in the first place. A validated transaction is never going to be valid input anyway, so disallowing it is almost a feature. Is there another input case I'm not considering?

[Bronek]

I wouldn't be surprised if some users were relying on the ability to roundtrip (less some, well specified, fields) for test purposes.

[intelliot]

Agreed on both points - I don't think we make any guarantee that it is valid to roundtrip the API response, but it is also nice to keep that in place since it isn't too painful to do so. In other words, go with "allow both fields on input, as long as they are identical"

[ckeshava]

I have a question pertaining to the inclusion of "DeliverMax" field. Why is it included in sign, submit and other such methods?

As per an earlier comment (#3484 (comment)), DeliverMax is to be included only in the API responses, not in the API request. Given that sign, submit commands are only used to specify transaction inputs to the rippled server, why would a user need to specify DeliverMax ?

Additionally, the sign, submit and other methods accept either both Amount, DeliverMax fields or only Amount field in the input. If the input does not contain Amount, then a temMALFORMED error is returned. I just wanted to put this out here.

The PR implementation adheres to the specification, I'm trying to understand the rationale behind the specification.

[intelliot]

As mentioned in #3484 (comment), it's nice that the transaction JSON can roundtrip - i.e. you can take a response (output), perhaps make some changes, and then submit it as a new transaction.

If the input does not contain Amount, then a temMALFORMED error is returned.

I'm not sure what you're trying to say here - what's the relevance or significance?

[Bronek]

I have a question pertaining to the inclusion of "DeliverMax" field. Why is it included in sign, submit and other such methods?

As per an earlier comment (#3484 (comment)), DeliverMax is to be included only in the API responses, not in the API request. Given that sign, submit commands are only used to specify transaction inputs to the rippled server, why would a user need to specify DeliverMax ?

Additionally, the sign, submit and other methods accept either both Amount, DeliverMax fields or only Amount field in the input. If the input does not contain Amount, then a temMALFORMED error is returned. I just wanted to put this out here.

The PR implementation adheres to the specification, I'm trying to understand the rationale behind the specification.

You missed that these methods only accept both Amount and DeliverMax if these are identical, otherwise the call will fail with rpcINVALID_PARAMS. These methods also accept either of DeliverMax or Amount, meaning that the user can switch to use DeliverMax at any point. The error you mention is only returned if the user did not provide either of these (otherwise we internally use DeliverMax as if it the user set Amount https://github.com/XRPLF/rippled/pull/4733/files#diff-9539e1762712dece6e50238b69e231b998ba0d262b2275fb3e438bcb21830fa2R181 )

This allows round-tripping; since these methods return the body of the signed transaction, we want to allow that body to be usable in a similar context (less some well-defined fields), e.g. for test purposes.

The important point here is that thanks to this decision, users can switch to use DeliverMax exclusively, which in turn will enable us to remove (if such decision is made) Amount entirely in some future version of API without breaking user code, which is what this issue is about (note "Rename" in the title).

[ckeshava]

@Bronek Thanks for the reference. I have modified the Payment transaction unit tests to contain only the DeliverMax field, but not the Amount field. I expect these tests to pass. Am I doing it wrong?
(https://github.com/ckeshava/rippled/tree/deliverMaxTests)

[Note: The above unit tests throw an error at https://github.com/ckeshava/rippled/blob/01fdbf43813e9adafb4041e7fb58a38c22e0fe4b/src/ripple/protocol/impl/STParsedJSON.cpp#L106 ]

This PR inserts the alias in the transactionSign interface. I don't think the transaction engine is modified in this PR

[ckeshava]

@intelliot I'm trying to understand: Apropos API inputs, does this PR update all Payment transaction API inputs or is this relevant only to the inputs of sign, submit and sibling commands?

I understand the implications of API responses well.

The client library code has a tight coupling between the input to a general Payment transaction (say through Websocket, JSON-RPC, etc), and the input to sign command's Payment transaction. I'm adding support for DeliverMax field in the client libraries.

[Bronek]

@Bronek Thanks for the reference. I have modified the Payment transaction unit tests to contain only the DeliverMax field, but not the Amount field. I expect these tests to pass. Am I doing it wrong? (https://github.com/ckeshava/rippled/tree/deliverMaxTests)

You will notice that the stack trace of the failing tests looks something like this

#0  0x0000aaaaba2d3c80 in ripple::STParsedJSONDetail::unknown_field (object=..., field=...)
#1  0x0000aaaaba2d74f8 in ripple::STParsedJSONDetail::parseObject (json_name=..., json=..., inName=..., depth=0, error=...)
#2  0x0000aaaaba2d8484 in ripple::STParsedJSONObject::STParsedJSONObject (this=0xffffffffc960, name=..., json=...)
#3  0x0000aaaab98e4d78 in ripple::test::jtx::parse (jv=...)
#4  0x0000aaaab98e4f18 in ripple::test::jtx::sign (jv=..., account=...)
#5  0x0000aaaab987f14c in ripple::test::jtx::Env::autofill_sig (this=0xffffffffd160, jt=...)
#6  0x0000aaaab987f370 in ripple::test::jtx::Env::autofill (this=0xffffffffd160, jt=...)
#7  0x0000aaaab91c068c in ripple::test::jtx::Env::jt<Json::Value, ripple::test::jtx::seq, ripple::test::jtx::fee, ripple::test::jtx::sig> (this=0xffffffffd160, jv=...)
#8  0x0000aaaab91bf250 in ripple::test::jtx::Env::apply<Json::Value, ripple::test::jtx::seq, ripple::test::jtx::fee, ripple::test::jtx::sig> (this=0xffffffffd160, jv=...)
#9  0x0000aaaab987d9f0 in ripple::test::jtx::Env::fund (this=0xffffffffd160, setDefaultRipple=true, amount=..., account=...)
#10 0x0000aaaab90051f4 in ripple::test::jtx::Env::fund_arg (this=0xffffffffd160, amount=..., account=...)
#11 0x0000aaaab9010bd0 in ripple::test::jtx::Env::fund<ripple::test::jtx::Account> (this=0xffffffffd160, amount=..., arg=...)
#12 0x0000aaaab939da8c in ripple::Ticket_test::testTicketCreatePreclaimFail (this=0xffffffffd550)
#13 0x0000aaaab93a168c in ripple::Ticket_test::run (this=0xffffffffd550)

This is because DeliverMax is not defined the same way as other fields are in protocol/impl/TxFormats.cpp. The existing design of protocol does not allow for aliases. When implementing DeliverMax I assessed that adding support for aliases in the protocol layer would be a complex (hence also high risk) change. So instead I implemented the alias behaviour in the rpc layer, as you have seen above. Most of the unit tests do not go through the rpc layer, so they are unable to use an alias. Meaning you should either:

• leave Amount inside pay.cpp, or
• change all the tests to go through rpc layer, or
• rename Amount in TxFormats only for Payment transaction to DeliverMax, then change all the uses of Amount in the context of Payment transaction and finally reverse the current behaviour of rpc layer such that Amount becomes an alias to DeliverMax

I do not think 2nd or 3rd option are worth the trouble.

[Bronek]

@intelliot I'm trying to understand: Apropos API inputs, does this PR update all Payment transaction API inputs or is this relevant only to the inputs of sign, submit and sibling commands?

I understand the implications of API responses well.

The client library code has a tight coupling between the input to a general Payment transaction (say through Websocket, JSON-RPC, etc), and the input to sign command's Payment transaction. I'm adding support for DeliverMax field in the client libraries.

All the uses of Amount in the rpc layer i.e. client API, in the context of Payment, are affected. Both on input and on output.

[ckeshava]

Thanks for the explanation Bronek, the distinction between rpc methods makes it clear. What non-rpc ways are present to access the protocol? I believe a WebSocket connection does not go through the rpc route to process a transaction.

I'm working on updating the client libraries to support the DeliverMax field.

• Options 1 and 2 would retain an inconsistency in the handling of Payment API inputs.
• The client libraries (Python and Typescript) have a strong dependency on SField.h, SField.cpp and TxFormats.h file. The types in these files are used for serialization and deserialization. (You might have looked into server_definitions command or this tool: https://github.com/RichardAH/xrpl-codec-gen/blob/master/gen.js)

As it stands, this PR does not modify these files.

• The client libraries do not make a distinction between rpc and non-rpc ways to initiate a Payment transaction. They assume an identical input/output API format irrespective of the way we query rippled server.

From the perspective of updating client libraries, It would be very convenient if we could do Option-3, but I get that it's a lot of work.

[Bronek]

Thanks for the explanation Bronek, the distinction between rpc methods makes it clear. What non-rpc ways are present to access the protocol? I believe a WebSocket connection does not go through the rpc route to process a transaction.

WebSocket is different only in these two areas:

• connection management and
• envelope parts of the request/response format

The actual implementation of all methods is done by the same rpc handlers as JSON-RPC ; you can see the call to RPC::doCommand inside ServerHandler::processSession(std::shared_ptr<WSSession> const& session, ...). We do not have "second" implementation of all methods that's for WebSocket only. Even if you send commands to rippled via command line, it will also go via rpc layer (we just internally build small RPC request, in RPCCall.cpp).

I'm working on updating the client libraries to support the DeliverMax field.

• Options 1 and 2 would retain an inconsistency in the handling of Payment API inputs.

Since all client API requests go through rpc, they will be actually consistent. You can just switch to using DeliverMax everywhere and simply make sure that your tests do not send requests directly to the protocol internals (unlike env(pay(..)) ) just use env.rpc(...). See for example AccountTx_test.cpp which is communicating with the ledger via env.rpc(...). Incidentally, env.rpc(...) also offers you overloads to explicitly select API version, which you should use to test your code for both API versions 1 and 2 (hint: both accept DeliverMax)

• The client libraries do not make a distinction between rpc and non-rpc ways to initiate a Payment transaction. They assume an identical input/output API format irrespective of the way we query rippled server.

Yes, and they are right not to make such distinction, as I explain above.

PS. I was wrong scoffing rpc(pay(...)) above; it is only using protocol directly for parsing of the request (in order to internally sign it) before sending it through rpc. Unfortunately, parsing directly in the protocol before going to rpc is exactly what you do not want to do in your tests, if you set DeliverMax field.

[ckeshava]

Okay, I get your point.

It still leaves the below problem open.

• The client libraries (Python and Typescript) have a strong dependency on SField.h, SField.cpp and TxFormats.h file. The types in these files are used for serialization and deserialization. (You might have looked into server_definitions command or this tool: https://github.com/RichardAH/xrpl-codec-gen/blob/master/gen.js)

The other alternative is to modify the parsing of server_definitions (which supports the binary-codec in client libraries). I'm new to this codebase, let me explore the alternatives and get back to you

[ximinez]

* The client libraries (Python and Typescript) have a strong dependency on `SField.h`, `SField.cpp` and `TxFormats.h` file. The types in these files are used for serialization and deserialization. (You might have looked into `server_definitions` command or this tool: https://github.com/RichardAH/xrpl-codec-gen/blob/master/gen.js)

As @Bronek mentioned, the aliasing is handled at the rpc layer. Serialized transactions are effectively operating directly at the protocol layer, where there is no aliasing. You can think of the rpc layer as a "client library" to the protocol layer. Any other client library that wants to do serializing (and deserializing) will have to handle the aliasing itself.

[ckeshava]

Hmm right, I'm wondering if it's lesser technical debt to update the protocol, instead of maintaining this alias change across C++, Python and Javascript client libraries. Anyways, thanks for the explanation

[Bronek]

We "only" need to maintain it as long as we support API version 1. No idea how long that would mean in practice (several years ?)

[ximinez]

Additionally, there is a bunch of functionality that is already maintained across different libraries. I don't see this as much different. Serialization itself is an example.