ripple / ripple-binary-codec

Convert between json and hex representations of transactions and ledger entries on the XRP Ledger. Moved to: https://github.com/XRPLF/xrpl.js/tree/develop/packages/ripple-binary-codec
https://github.com/XRPLF/xrpl.js/tree/develop/packages/ripple-binary-codec
ISC License
19 stars 45 forks source link

Input validation - Amount and Fee should not allow fractional XRP drops #31

Closed intelliot closed 5 years ago

intelliot commented 5 years ago

It is possible for a user to set an incorrect Amount or Fee.

In the XRP Ledger's transaction format, an amount that is a string like "100" represents an amount in drops. The existing code does not validate that the string contains only numbers, so it accepts a value such as "100.001" and incorrectly encodes it to the equivalent of "998001". In other words, someone intending a "100.001" drop fee could end up paying 998,001 drops instead. Another example: "120.00001" becomes '119800001'.

This appears to be caused by the bignumber library (bn.js), as it is not expected (and not tested) for Amount or Fee to contain a decimal point; they're supposed to be string-encoded integers representing an amount of drops.

The docs are clear that Amount and Fee must be integers:

Unit tests are permitted to submit values of XRP (not drops) with a decimal point - for example, "1.23" meaning 1.23 XRP. All other cases should always specify XRP in drops, with no decimal point: e.g. "1230000" meaning 1.23 XRP.

~ https://xrpl.org/basic-data-types.html#specifying-currency-amounts

Aside from unit tests, all use cases must always specify XRP in drops with no decimal point.

Integer amount of XRP, in drops, to be destroyed as a cost for distributing this transaction to the network.

~ https://xrpl.org/transaction-common-fields.html

There are two root causes of the issue: 1) The validation is too loose, requiring that the input string just contains a number anywhere in it, so 'foo 123 bar' is valid. We should require that it only contains numbers and no decimal point(s). 2) The bn.js (bignumber) library does not work with decimals.

intelliot commented 5 years ago

This was fixed in version 0.2.2