search

indutny / bn.js

BigNum in pure javascript
MIT License
1.19k stars 150 forks source link

Fix serious issue in `.toString(16)` #295

Closed alexdupre closed 2 years ago

alexdupre commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

alexdupre commented 2 years ago

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 commented 2 years ago

Any update?

fanatid commented 2 years ago

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

ricmoo commented 2 years ago

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

fanatid commented 2 years ago

Only @indutny can do this :/