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

When approving a payment transaction, display currency-code in both HEX and ASCII #19

Open
sappenin opened this issue Mar 16, 2022 · 4 comments

Comments

@sappenin
Copy link

sappenin commented Mar 16, 2022

In the XRPL, there are two different ways to assemble a currency-code. The first way (ISO standard codes) support only three-character ASCII strings (e.g., USD); the other way (non-standard, but still legitimate) is a 160-bit HEX-string value (for codes longer than 3 ASCII characters).

I propose that when using the Nano to sign a payment transaction containing a non-standard currency code, the device will display both the HEX and the ASCII equivalent of the currency code.

Displaying the HEX string is correct IMO (i.e., not every HEX string maps to valid ASCII) and likely just good security practice (i.e., we want users to see the real bytes for what they're about to sign). However, displaying 160-bit HEX strings is a sub-optimal user-experience (if that's all that is displayed), especially given that many issued currency codes are actually just ASCII strings that have more than 3 characters (and are thus non-standard).

For example, I envision something like this scrolling across the device's display, as part of payment transaction signature approval:

5553445400000000000000000000000000000000 - USDT

Curious to hear thoughts around this proposal.

@RareData
Copy link
Contributor

Displaying 160-bit HEX strings is a suboptimal user-experience

Hex strings that can be decoded to all ASCII are supposed to be decoded to ASCII. I see a bug on line https://github.com/LedgerHQ/app-xrp/blob/master/src/xrp/amount.c#L176, that's supposed to be negated. If "XRP" is in the hex string, we should proceed to the else-case and display as hex.

Earlier versions of this firmware properly decoded all ASCII hex strings, e.g. 5553445400000000000000000000000000000000 to USDT. The decoder for all ASCII can be seen here: https://github.com/LedgerHQ/app-xrp/blame/master/src/xrp/strings.c#L21 and it behaves as expected for other decoding of e.g. MemoData.

@RareData
Copy link
Contributor

@RareData
Copy link
Contributor

Ping @TamtamHero, you probably wanna look at the above diff. since it inverted the intended behaviour.

@RareData
Copy link
Contributor

RareData commented Jun 1, 2023

This is still an issue in production. The expected behaviour has changed. Pinging for higher visibility @TamtamHero @fbeutin-ledger @tjoly-ledger @agrojean-ledger @btchip

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

No branches or pull requests

2 participants