fix: F-2026-18784 | [Dual Defense] Uppercase Bech32 Fee Payer Bypasses DIRECT_AUX Fee-Payer Guard - #340
Merged
Merged
Conversation
x/tx compares fee payer to signer with a raw string compare, so an uppercase bech32 alias of the victim slips past it. Nothing on Push signs with AUX.
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.
Finding
cosmossdk.io/x/txguards against a fee payer also signing with DIRECT_AUX using a raw string compare (signing/directaux/direct_aux.go:93):BIP-173 permits an all-uppercase bech32 encoding of the same account, so
PUSH1ABC…andpush1abc…are the same account but different strings — the guard fails open. Everything downstream decodes both to identicalAccAddressbytes and deduplicates signers, so no separate fee-payer signature is required.A sponsor holding a victim's valid DIRECT_AUX signature over a fixed
TxBodycan rewriteAuthInfo— raise the fee, set the uppercase payer, drop themselves as a signer — and charge the victim.Version note: the finding cites
x/txv0.13.3. We pin v0.14.0 (go.mod:20, a replace over av1.2.0-alpha.1requirement). I checked v0.14.0 directly: the raw==is still there, so the finding holds against what we actually run.Fix — drop DIRECT_AUX (recommendation 2)
app.gopreviously didappend(tx.DefaultSignModes, SIGN_MODE_TEXTUAL), andDefaultSignModesincludesSIGN_MODE_DIRECT_AUX(SDKx/auth/tx/config.go:62). So the mode was enabled purely by inheriting an SDK default.Nothing on Push uses it:
DIRECT_AUX/DirectAuxanywhere in this repoauthtx.NewTxConfig(cdc, []signing.SignMode{signing.SignMode_SIGN_MODE_DIRECT})(universalClient/pushsigner/pushsigner.go:423)So we were exposing surface for no benefit. The enabled list is now enumerated explicitly, omitting DIRECT_AUX, with a comment recording the mechanism and what has to change upstream before it is restored.
Recommendation 1 (upgrade
x/tx) is not available — v0.14.0 is our pin and still has the raw compare. Worth reporting upstream; not something we can pull down today.Completeness
There is a second place building a tx config from
tx.DefaultSignModes—app/params/proto.go:34, inparams.MakeEncodingConfig(). That one is not on any production path: its only caller is a test (app/ante/account_init_signer_binding_test.go:38). Every production path uses the app's own config —cmd/pchaind/root.go,cmd/puniversald/root.goandapp/encoding.goall taketempApp.TxConfig(). Left alone deliberately rather than changed unnecessarily.Incidental
append(tx.DefaultSignModes, …)appended to a package-level slice. It is a 3-element literal today socap == lenand the append always allocates, but if upstream ever added a fourth element with spare capacity this would write into the shared backing array. The explicit list removes that too.Tests
app/sign_modes_test.go, three cases:TestEnabledSignModes_ExcludesDirectAux— DIRECT_AUX absent fromSupportedModes()TestEnabledSignModes_KeepsTheModesWeActuallyUse— DIRECT, LEGACY_AMINO_JSON and TEXTUAL all still present, so the removal did not take anything else with itTestDefaultSignModesStillContainsDirectAux— asserts upstream's default still contains it; if upstream ever drops it, this fails and the explicit list can be reconsideredThe tests use
setup(t, ChainID, false, 0)rather thanSetup(t)— the latter passes the"testing"chain ID and panics in the EVM configurator, a trapapp/nested_dispatch_test.goalready documents.Compatibility
Removing a sign mode is a client-facing change: any external wallet or tool signing Push transactions with DIRECT_AUX would stop working. Nothing in this repo or the universal client does, so the practical risk looks nil — but it is an ecosystem question rather than a code one and is worth a second opinion before merge.