🐞 Fix Base58 decodeWord sanitizer ordering - #1544
Merged
atarpara merged 1 commit intoAug 2, 2026
Merged
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes a sanitizer ordering bug in Base58.decodeWord where an out-of-bounds lookup-table read (mload(c)) could occur before validating the input character, causing pathological memory expansion and out-of-gas instead of reverting with Base58DecodingError.
Changes:
- Reorders
decodeWordvalidation to check the Base58 character bitmask before the lookup-table read. - Separates character validation from overflow checks for clearer failure modes.
- Adds a regression test asserting low ASCII bytes (e.g.
"0"and0x00) revert cleanly withBase58DecodingError.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/utils/Base58.sol | Validates the character before table lookup in decodeWord to prevent OOG on underflowed indices. |
| test/Base58.t.sol | Adds a regression test ensuring low-character inputs revert with Base58DecodingError. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
`decodeWord` performed `mload(c)` before validating the character, so an input byte below `'1'` (0x31) underflowed the lookup index and expanded memory, reverting with out-of-gas instead of a clean `Base58DecodingError`. Validate the character before the load (as `decode` already does), then perform the multiplication/addition overflow check.
cristianizzo
force-pushed
the
fix/base58-decodeword-sanitizer-ordering
branch
from
July 26, 2026 12:42
34f981e to
13703dc
Compare
atarpara
added a commit
that referenced
this pull request
Aug 2, 2026
* 🐞 Fix Base58 `decodeWord` sanitizer ordering (#1544) * Added TestCase --------- Co-authored-by: 0x1220c <cristianizzo@me.com>
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.
Fixes #1543.
Base58.decodeWordperformedmload(c)(the lookup-table read) before validating that the input character is a valid Base58 character. For an input byte below'1'(0x31) — e.g.'0'(0x30) — the indexc = byte - 49underflows to ~2²⁵⁶, somload(c)expands memory past the block gas limit and the call reverts with out-of-gas instead of a cleanBase58DecodingError.This mirrors the ordering already used in
decode: validate the character with theshl(c, 1)bitmask check beforemload(c), then perform the multiplication/addition overflow check.Test
Added
testDecodeWordLowCharacterReverts, which assertsdecodeWord("0")(and a\x00byte) now revert withBase58DecodingError. It fails on the current code (the call OOGs, reverting without the selector) and passes with this fix. The existing differential, overflow, and fuzz tests continue to pass under both the default and--via-irprofiles.Note
The character check must gate
mload(c), so the two conditions can't be folded back into the original single masked check without reintroducing the ordering bug. I kept them as twoBase58DecodingErrorreverts for clarity/correctness — happy to golf the second revert block if you'd prefer.