rippled icon indicating copy to clipboard operation
rippled copied to clipboard

Partial Payments Exploit and API response saying *Amount* is misleading developers

Open ioBanker opened this issue 4 years ago • 7 comments

Issue Description

Built-in API response Amount is misleading developers; perhaps common sense while reading any transaction in the world of crypto would assume that Amount in response is the Amount of XRP that is received or sent hence XRPL.org documentation is referring to these partial payments as a feature which causing a vulnerability to developers listening to Amount in API responses.

Perhaps using delivered_amount in API response is a solution for the vulnerability but doesn't solve the misleading issue and fact.

Source: https://xrpl.org/partial-payments.html#partial-payments-exploit

ioBanker avatar Aug 11 '21 11:08 ioBanker

I agree, the Amount field is quit ambiguous in the partial payment context. To avoid any ambiguity between developers, I guess the Amount field should define the actual delivered amount in all the cases, and some other field specify the partial payment (e.g. intended_amount).

smlu avatar Aug 11 '21 13:08 smlu

https://github.com/ripple/rippled/issues/3484

yxxyun avatar Aug 11 '21 13:08 yxxyun

The Amount field is part of the actual transaction, and is really the intended amount. For partial payments, a different amount can end up being delivered. The delivered amount can't be known at the time the transaction is submitted, so it can't possibly be part of the transaction itself. The delivered amount has to be part of the metadata, which is created as part of actually applying the transaction to the ledger. We could possibly try to rename the Amount field to something else, though this is not easy. But regardless, there's no way to put the actual delivered amount in the transaction itself, since that amount can only be computed after the transaction is constructed, signed and submitted. Developers have to parse the metadata for something like this.

cjcobb23 avatar Aug 12 '21 16:08 cjcobb23

The Amount field is part of the actual transaction, and is really the intended amount. For partial payments, a different amount can end up being delivered. The delivered amount can't be known at the time the transaction is submitted, so it can't possibly be part of the transaction itself. The delivered amount has to be part of the metadata, which is created as part of actually applying the transaction to the ledger. We could possibly try to rename the Amount field to something else, though this is not easy. But regardless, there's no way to put the actual delivered amount in the transaction itself, since that amount can only be computed after the transaction is constructed, signed and submitted. Developers have to parse the metadata for something like this.

I was referring to Amount in API GET response and not POST; should be renamed to perhaps Potential_Amount rather than keeping it Amount which is misleading, at least developer would go and search for the meaning of Potential_Amount rather than assuming Amount as final.

ioBanker avatar Aug 13 '21 01:08 ioBanker

We're not very familiar with how rippled API works or designed; perhaps having partial payments disabled by default with a flag/Settings similar to DisallowXRP and can be called AllowPartial; just a thought.

ioBanker avatar Aug 13 '21 01:08 ioBanker

We're not very familiar with how rippled API works or designed; perhaps having partial payments disabled by default with a flag/Settings similar to DisallowXRP and can be called AllowPartial; just a thought.

This has been discussed at length and there are some significant reasons not to do this. (For one, it defeats the intended value of partial payments, which is to let you to bounce payments you didn't want back while spending as little possible extra on fees—if it's opt-in, you won't be able to send these most of the time because the people who sent you the unwanted payments probably didn't opt in.)

mDuo13 avatar Aug 13 '21 01:08 mDuo13

I do think the most reasonable course of action would be to rename the 'Amount' Field/property to something that can be better interpreted by developers not familiar with partial payments and the exploits. Instead of changing the base class, can we not refactor the API response classes instead?

syeduali93 avatar Nov 30 '21 17:11 syeduali93