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

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

Closed
ioBanker opened this issue Aug 11, 2021 · 7 comments

Comments

@ioBanker
Copy link

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

@smlu
Copy link

smlu commented Aug 11, 2021

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).

@yxxyun
Copy link

yxxyun commented Aug 11, 2021

#3484

@cjcobb23
Copy link
Contributor

cjcobb23 commented Aug 12, 2021

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.

@ioBanker
Copy link
Author

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
Copy link
Author

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.

@mDuo13
Copy link
Collaborator

mDuo13 commented Aug 13, 2021

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.)

@syeduali93
Copy link

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?

@intelliot intelliot added this to the 2024 release milestone Sep 8, 2023
@intelliot intelliot modified the milestones: 2024 release, 2.0 Oct 18, 2023
sophiax851 pushed a commit to sophiax851/rippled that referenced this issue Jun 12, 2024
…F#4733)

Using the "Amount" field in Payment transactions can cause incorrect
interpretation. There continue to be problems from the use of this
field. "Amount" is rarely the correct field to use; instead,
"delivered_amount" (or "DeliveredAmount") should be used.

Rename the "Amount" field to "DeliverMax", a less misleading name. With
api_version: 2, remove the "Amount" field from Payment transactions.

- Input: "DeliverMax" in `tx_json` is an alias for "Amount"
  - sign
  - submit (in sign-and-submit mode)
  - submit_multisigned
  - sign_for
- Output: Add "DeliverMax" where transactions are provided by the API
  - ledger
  - tx
  - tx_history
  - account_tx
  - transaction_entry
  - subscribe (transactions stream)
- Output: Remove "Amount" from API version 2

Fix XRPLF#3484

Fix XRPLF#3902
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

11 participants