fix(crypto_addresses): correct testnet segwit prefix and reject non-hex ETH addresses#466
Open
Jordan-Bourillot wants to merge 1 commit into
Conversation
…ex ETH Two correctness bugs in the crypto address validators: btc_address: the segwit branch is taken when the value starts with "bc" or "tb", but the regexp only matched "bc" or "tc". Every valid testnet bech32 address (tb1...) was therefore rejected. Correct the prefix to "tb". eth_address: _validate_eth_checksum_address only checked the length of the stripped address, not that its characters were hexadecimal. Inputs made of non-hex, caseless characters (e.g. "0x" + "*" * 40) passed the checksum loop vacuously and were accepted as valid. Require 40 hex digits before running the checksum. Existing fixtures are unchanged; added testnet bech32 addresses and a non-hex input as regression tests.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This fixes two correctness bugs in the
crypto_addressesvalidators. Both are covered by new regression tests, and the full test suite still passes.1.
btc_addressrejects valid testnet segwit addressesThe segwit branch is selected when the value starts with
bcortb, but the regexp only matchedbcortc:Because the gate accepts
tbwhile the pattern expectstc, every valid testnet bech32 address is rejected:tcis not a valid Bitcoin HRP and is unreachable through thevalue[:2] in ("bc", "tb")gate, so it is a typo fortb. Fix:(bc|tc)→(bc|tb). Mainnetbc1...is unaffected.2.
eth_addressaccepts non-hex "checksum" addresses_validate_eth_checksum_addresschecked only the length of the stripped address, not that its characters were hexadecimal. The EIP-55 loop only constrains the case of letters, so caseless / non-hex characters pass it vacuously:Fix: require the stripped address to be exactly 40 hex digits before running the checksum (this also subsumes the previous length check).
Tests
test_btc_address: added two valid testnet bech32 addresses (BIP173 vectors) to the valid fixtures.test_eth_address: added a non-hex input to the invalid fixtures.Both new cases fail on
masterand pass with this change.ruffreports no issues and the full suite (898 passed) is green.