Fix serious issue in .toString(16)

[alexdupre]

In some circumstances the hex encoding of big numbers is wrong. In addition to a display issue, given that the the hex string if often used as an intermediate representation in transport/conversion scenarios, the re-constructed big number can actually change its value, creating serious issues.

[ricmoo]

FYI, I have confirmed this issue.

@indutny Would you have an idea of when you would be able to get to this? I am debating including a hack to fix this temporarily into ethers if you don’t have time to get to it. :)

[fanatid]

@alexdupre thanks for the fix. Can you explain what's wrong with current code and how moving if-block will fix it? Test is a good but it would be great to have explanation for the future.

[alexdupre]

The issue is present when these conditions are true:

1. the code is processing the last word
2. carry is 0
3. off is 24 at the start of the iteration

In this case the if doesn't add the full 0-padded 6 characters, because it thinks it's the last iteration, but actually because of the off increment (and consequentially the i decrement) this is the second last iteration, and the full 6 chars have to be added. Does it make sense?

[fanatid]

Make sense, thanks.
I wanted release this as patch but in the same time changes can break something, maybe we should proceed with minor? Any thoughts?

[alexdupre]

If you are 100% sure that the fix is correct, then I think it should be released as patch, this is a serious issue and there cannot be external code that relies on such broken behavior. Personally I don't think this fix can break anything.

[fanatid]

I think changes are correct but it would be great if somebody except @ricmoo confirmed that everything is correct.

[alexdupre]

FWIW I think this may be a good candidate for a property-based testing methodology (it'd be interesting to know if it would have spotted this issue as well).

[alexdupre]

Any update?

[fanatid]

Thank you for the reminder! Published as v5.2.1.

[ricmoo]

@fanatid Could I bug you to also get the version in elliptic bumped to match this too, please? :)

[fanatid]

Only @indutny can do this :/