fix: F-2026-18200 | [Dual Defense] Variable-Length Signer Addresses Enable Universal Executor Module Impersonation - #316
Open
0xNilesh wants to merge 2 commits into
Open
fix: F-2026-18200 | [Dual Defense] Variable-Length Signer Addresses Enable Universal Executor Module Impersonation#3160xNilesh wants to merge 2 commits into
0xNilesh wants to merge 2 commits into
Conversation
…-20-byte addresses Ante now enforces pubKey.Address() == signer for new gasless accounts, and GetAddressPair / MustConvertCosmosToHex reject anything that is not exactly 20 bytes instead of truncating onto a module address.
Gasless txs skip fee deduction entirely, so charging gas has no economic effect. The TxSigLimit cap alone bounds the multisig work; reverts the NewAccountInitDecorator signature change and the SigGasConsumer wiring.
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.
Problem
A gasless transaction can declare a crafted, over-long Bech32 signer while being signed by an entirely unrelated key, and end up impersonating the
uexecutormodule inside the UEA. Three links:app/ante/account_init_decorator.gocallsauthsigning.VerifySignature(...), which only proves "this key signed this tx" — it never checkspubKey.Address() == signer. The decorator then early-returns, so the SDK'sSetPubKeyDecorator, which does enforce exactly that (x/auth/ante/sigverify.go:94-96, present since v0.46), is never reached.utils.ConvertAnyAddressToBytesreturns whateversdk.AccAddressFromBech32accepts — up to 255 bytes.utils.GetAddressPairusescommon.BytesToAddress(bz), which keeps the rightmost 20 bytes. So a 21-byte signer0x01 ‖ sha256("uexecutor")[:20]collapses to the module EVM address0x14191Ea54B4c176fCf86f51b0FAc7CB1E71Df7d7— which the UEA contract trusts unconditionally.Result: sign with any key, declare any signer, become the module, and drive any UEA. Truncation works for any signer longer than 20 bytes, not just the 21-byte case in the report.
Fix
1. Bind the signer to the signing key in the ante (
app/ante/account_init_decorator.go).verifySignatureForNewAccountnow rejects the tx unlesspubKey.Address() == signers[i], with the SDK's guards mirrored exactly (!simulate && ctx.IsSigverifyTx()), so simulation and gas estimation are unaffected. Because a pubkey address is always 20 bytes, this alone kills every aliased signer of any length.Implementation note: the plan's preferred route was to drop the early return and let the tx fall through to
SetPubKeyDecorator/SigVerificationDecorator. That is not viable: this path exists precisely because the account does not exist yet, so it signs overaccountNumber = 0/sequence = 0, whileSigVerificationDecoratorverifies againstacc.GetAccountNumber()— a number the chain only assigns at creation time and which a client therefore cannot know in advance. Falling through would break every first-time gasless tx. The in-place check was taken instead.2. Also resolves
F-2026-18186(Medium — the same early return skippedValidateSigCount, the quadratic-multisig vector). The new-account path now enforcesparams.TxSigLimitbefore doing any verification work, so an oversized multisig key is rejected outright rather than verified sub-signature by sub-signature.No gas is consumed on this path, deliberately. The gasless path skips fee deduction entirely (
app/ante/fee.go:60), so charging gas in a fee-exempt tx has no economic effect — it costs an attacker nothing and buys no DoS protection, while introducing a behaviour change on the consensus-critical validator vote path (the gasless message list is almost entirelyMsgVote*). The hard count cap is what actually bounds the work;SigGasConsumerwould only have been the economic disincentive, which is meaningless without a fee. Both findings stay closed with zero behaviour change.3. Enforce exactly 20 bytes on Cosmos→EVM conversion (
utils/address.go).GetAddressPairnow errors instead of truncating. Non-breaking: it already returnederrorand has only two callers (x/uexecutor/keeper/msg_server.go:44,59), both of which already handle it.4.
MustConvertCosmosToHexhardened — not named in the report. It usedcommon.Address(bz), a slice→array conversion that panics below 20 bytes and keeps the leftmost 20 above — the opposite end fromBytesToAddressin the same file. It now returns""for anything that is not exactly 20 bytes.5. Outer guard at CheckTx.
MsgExecutePayload.ValidateBasicandMsgMigrateUEA.ValidateBasicreject a signer that does not decode to exactly 20 bytes, so the tx dies before the ante chain runs.Tests
app/ante/account_init_signer_binding_test.go— builds real SIGN_MODE_DIRECT transactions signed by an unrelated key:MsgExecutePayloadandMsgMigrateUEA→ all rejected withErrInvalidPubKey,nextnever called, no account persisted;simulate=true, matching and mismatched → unaffected;TxSigLimit→ErrTooManySignatures(F-2026-18186).Verified as genuine regressions: with the two new checks disabled, all six aliased-signer cases return no error and create the account.
utils/address_test.go—GetAddressPairrejects 19/21/22/32 bytes and accepts 20 (bech32 and 0x); the module-alias case is asserted to truncate underBytesToAddressand to be rejected byGetAddressPair;MustConvertCosmosToHexneither panics nor truncates.x/uexecutor/types/msg_signer_length_test.go—ValidateBasicon both gasless messages rejects 21/22/32-byte signers and accepts 20.test/integration/uexecutor— one fixture used a literal that decoded to 42 bytes (the ASCII of a hex address, bech32-encoded by mistake), previously truncated down to a 20-byte address byGetAddressPair. Replaced with atestSignerconstant holding exactly those 20 bytes, so behaviour is unchanged.Green:
./app/...(-tags=test),./x/...,./utils/...,./test/integration/uexecutor/....Hacken remediation 3 — declined
ExecutePayloadV2withfrom = uexecutor moduleprecisely so the UEA skips EIP-712 — authorisation there comes from validator consensus, not an owner signature. Removing it breaks cross-chain inbound execution.setUEAProxyImplementation) — high blast radius, real migration risk.