diff --git a/app/ante/account_init_decorator.go b/app/ante/account_init_decorator.go index 29e6d4ea..b117c938 100644 --- a/app/ante/account_init_decorator.go +++ b/app/ante/account_init_decorator.go @@ -1,6 +1,7 @@ package ante import ( + "bytes" "fmt" sdk "github.com/cosmos/cosmos-sdk/types" @@ -12,6 +13,7 @@ import ( codectypes "github.com/cosmos/cosmos-sdk/codec/types" sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" "github.com/cosmos/cosmos-sdk/types/tx/signing" + "github.com/cosmos/cosmos-sdk/x/auth/ante" authsigning "github.com/cosmos/cosmos-sdk/x/auth/signing" txpolicy "github.com/pushchain/push-chain-node/app/txpolicy" ) @@ -55,7 +57,7 @@ func (aid AccountInitDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate "address", sdk.AccAddress(newAccAddr).String(), "simulate", simulate, ) - // if account does not exist on chain, bypass rest of ante chain (especially gas and signature verification) here. + // if account does not exist on chain, bypass rest of ante chain here. // Perform signature verification on account number e and sequence number e instead. if err := aid.verifySignatureForNewAccount(ctx, tx, simulate); err != nil { ctx.Logger().Debug("account init decorator: signature verification failed for new account", @@ -103,13 +105,52 @@ func (aid AccountInitDecorator) verifySignatureForNewAccount(ctx sdk.Context, tx return errorsmod.Wrapf(sdkerrors.ErrUnauthorized, "invalid number of signer; expected: %d, got %d", len(signers), len(sigs)) } - newAccAddr := sdk.AccAddress(signers[0]) + params := aid.ak.GetParams(ctx) + + // Enforce the signature count limit before doing any verification work. + // This decorator short-circuits the ante chain for new accounts, so + // ante.ValidateSigCountDecorator never runs for them; without this hard cap + // a gasless tx could carry an arbitrarily large multisig key and force the + // node to verify every sub-signature. Gas is deliberately NOT consumed here: + // gasless txs skip fee deduction entirely, so charging gas would cost an + // attacker nothing - the count cap is what actually bounds the work. + sigCount := 0 for _, sig := range sigs { + if sig.PubKey == nil { + return errorsmod.Wrap(sdkerrors.ErrInvalidPubKey, "pubkey is not provided in signature") + } + sigCount += ante.CountSubKeys(sig.PubKey) + if uint64(sigCount) > params.TxSigLimit { + return errorsmod.Wrapf(sdkerrors.ErrTooManySignatures, + "signatures: %d, limit: %d", sigCount, params.TxSigLimit) + } + } + + newAccAddr := sdk.AccAddress(signers[0]) + for i, sig := range sigs { pubKey := sig.PubKey if pubKey == nil { return errorsmod.Wrap(sdkerrors.ErrInvalidPubKey, "pubkey is not provided in signature") } + // Bind the declared signer to the key that actually signed the tx. + // + // VerifySignature below only proves "this key signed this tx"; it says + // nothing about WHO the tx claims to be from. Because this decorator + // short-circuits the ante chain for new accounts, the SDK's + // SetPubKeyDecorator - which owns this check - never runs, so a tx could + // declare an arbitrary signer while being signed by an unrelated key. + // Bech32 account addresses may be up to 255 bytes, and downstream + // conversion to a 20-byte EVM address keeps only the rightmost bytes, so + // a crafted longer signer could alias a module address. + // + // Guards mirror x/auth/ante/sigverify.go exactly so simulation and gas + // estimation keep working. + if !simulate && ctx.IsSigverifyTx() && !bytes.Equal(pubKey.Address().Bytes(), signers[i]) { + return errorsmod.Wrapf(sdkerrors.ErrInvalidPubKey, + "pubKey does not match signer address %s with signer index: %d", sdk.AccAddress(signers[i]).String(), i) + } + // retrieve signer data chainID := ctx.ChainID() var accSequence uint64 = 0 diff --git a/app/ante/account_init_signer_binding_test.go b/app/ante/account_init_signer_binding_test.go new file mode 100644 index 00000000..4173e17f --- /dev/null +++ b/app/ante/account_init_signer_binding_test.go @@ -0,0 +1,298 @@ +package ante_test + +import ( + "context" + "fmt" + "testing" + + kmultisig "github.com/cosmos/cosmos-sdk/crypto/keys/multisig" + "github.com/cosmos/cosmos-sdk/crypto/keys/secp256k1" + cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types" + sdk "github.com/cosmos/cosmos-sdk/types" + sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" + "github.com/ethereum/go-ethereum/common" + "github.com/stretchr/testify/require" + + clienttx "github.com/cosmos/cosmos-sdk/client/tx" + "github.com/cosmos/cosmos-sdk/std" + "github.com/cosmos/cosmos-sdk/types/tx/signing" + authsigning "github.com/cosmos/cosmos-sdk/x/auth/signing" + authtypes "github.com/cosmos/cosmos-sdk/x/auth/types" + + "github.com/pushchain/push-chain-node/app/ante" + appparams "github.com/pushchain/push-chain-node/app/params" + uexecutortypes "github.com/pushchain/push-chain-node/x/uexecutor/types" +) + +// uexecutorModuleEVMAddr is the EVM address of the uexecutor module account - +// sha256("uexecutor")[:20]. The UEA contract trusts calls coming from it +// unconditionally, which is what makes aliasing onto it so damaging. +const uexecutorModuleEVMAddr = "0x14191Ea54B4c176fCf86f51b0FAc7CB1E71Df7d7" + +const anteTestChainID = "push_9000-1" + +// newSignerBindingEncodingConfig returns an encoding config able to build and +// sign real uexecutor transactions. +func newSignerBindingEncodingConfig(t *testing.T) appparams.EncodingConfig { + t.Helper() + encCfg := appparams.MakeEncodingConfig() + std.RegisterInterfaces(encCfg.InterfaceRegistry) + authtypes.RegisterInterfaces(encCfg.InterfaceRegistry) + uexecutortypes.RegisterInterfaces(encCfg.InterfaceRegistry) + return encCfg +} + +// aliasedSigner returns a `length`-byte address whose RIGHTMOST 20 bytes are the +// uexecutor module account. common.BytesToAddress keeps exactly those bytes, so +// every such address collapses onto the module's EVM address. +func aliasedSigner(t *testing.T, length int) sdk.AccAddress { + t.Helper() + require.Greater(t, length, common.AddressLength) + + moduleAddr := authtypes.NewModuleAddress(uexecutortypes.ModuleName) + require.Len(t, moduleAddr, common.AddressLength) + require.Equal(t, uexecutorModuleEVMAddr, common.BytesToAddress(moduleAddr).Hex()) + + prefix := make([]byte, length-common.AddressLength) + prefix[0] = 0x01 + addr := sdk.AccAddress(append(prefix, moduleAddr...)) + require.Len(t, addr, length) + + // The whole point of the finding: this longer address truncates onto the + // module's EVM address downstream. + require.Equal(t, uexecutorModuleEVMAddr, common.BytesToAddress(addr).Hex()) + return addr +} + +// gaslessMsgFor builds one of the two user-facing gasless messages with the +// given declared signer. +func gaslessMsgFor(t *testing.T, msgType string, signer sdk.AccAddress) sdk.Msg { + t.Helper() + ua := &uexecutortypes.UniversalAccountId{ + ChainNamespace: "eip155", + ChainId: "11155111", + Owner: "0x000000000000000000000000000000000000dead", + } + + switch msgType { + case "MsgExecutePayload": + return &uexecutortypes.MsgExecutePayload{ + Signer: signer.String(), + UniversalAccountId: ua, + UniversalPayload: &uexecutortypes.UniversalPayload{ + To: "0x000000000000000000000000000000000000dead", + Data: "0xabcdef", + }, + VerificationData: "0xabcdef", + } + case "MsgMigrateUEA": + return &uexecutortypes.MsgMigrateUEA{ + Signer: signer.String(), + UniversalAccountId: ua, + MigrationPayload: &uexecutortypes.MigrationPayload{ + Migration: "0x000000000000000000000000000000000000beef", + Nonce: "0", + Deadline: "1", + }, + Signature: "0xabcdef", + } + default: + t.Fatalf("unknown msg type %q", msgType) + return nil + } +} + +// buildSignedTx returns a tx carrying msg whose declared signer is +// `declaredSigner` but which is signed by `priv` - the two need not be related, +// which is exactly the confusion the fix has to reject. +func buildSignedTx(t *testing.T, encCfg appparams.EncodingConfig, msg sdk.Msg, declaredSigner sdk.AccAddress, priv cryptotypes.PrivKey) sdk.Tx { + t.Helper() + + txb := encCfg.TxConfig.NewTxBuilder() + require.NoError(t, txb.SetMsgs(msg)) + txb.SetGasLimit(300_000) + + require.NoError(t, txb.SetSignatures(signing.SignatureV2{ + PubKey: priv.PubKey(), + Data: &signing.SingleSignatureData{SignMode: signing.SignMode_SIGN_MODE_DIRECT}, + Sequence: 0, + })) + + // The gasless new-account path signs over account number 0 / sequence 0, + // since the account does not exist on chain yet. + signerData := authsigning.SignerData{ + Address: declaredSigner.String(), + ChainID: anteTestChainID, + AccountNumber: 0, + Sequence: 0, + PubKey: priv.PubKey(), + } + + sigV2, err := clienttx.SignWithPrivKey( + context.Background(), signing.SignMode_SIGN_MODE_DIRECT, signerData, + txb, priv, encCfg.TxConfig, 0, + ) + require.NoError(t, err) + require.NoError(t, txb.SetSignatures(sigV2)) + + return txb.GetTx() +} + +func newSignerBindingDecorator(t *testing.T, encCfg appparams.EncodingConfig) (ante.AccountInitDecorator, *mockAccountKeeperAnte) { + t.Helper() + ak := newMockAccountKeeperAnte(sdk.AccAddress([]byte("feeCollector"))) + return ante.NewAccountInitDecorator(ak, encCfg.TxConfig.SignModeHandler()), ak +} + +// TestAccountInitDecorator_RejectsAliasedModuleSigner is the regression test for +// F-2026-18200: a gasless tx may not declare an over-long signer that truncates +// onto the uexecutor module address while being signed by an unrelated key. +// +// Hacken's PoC only used the 21-byte case; truncation works for ANY length > 20, +// so 21, 22 and 32 bytes are all covered, against both gasless messages. +func TestAccountInitDecorator_RejectsAliasedModuleSigner(t *testing.T) { + encCfg := newSignerBindingEncodingConfig(t) + + for _, msgType := range []string{"MsgExecutePayload", "MsgMigrateUEA"} { + for _, length := range []int{21, 22, 32} { + t.Run(fmt.Sprintf("%s/%dbytes", msgType, length), func(t *testing.T) { + attackerKey := secp256k1.GenPrivKey() + declaredSigner := aliasedSigner(t, length) + msg := gaslessMsgFor(t, msgType, declaredSigner) + tx := buildSignedTx(t, encCfg, msg, declaredSigner, attackerKey) + + aid, ak := newSignerBindingDecorator(t, encCfg) + ctx := newAnteTestCtx(t, false).WithChainID(anteTestChainID) + + nextCalled := false + _, err := aid.AnteHandle(ctx, tx, false, func(ctx sdk.Context, tx sdk.Tx, simulate bool) (sdk.Context, error) { + nextCalled = true + return ctx, nil + }) + + require.Error(t, err, "aliased signer must not pass the ante chain") + require.True(t, sdkerrors.ErrInvalidPubKey.Is(err), "expected ErrInvalidPubKey, got: %v", err) + require.False(t, nextCalled, "the message must never reach execution") + require.False(t, ak.HasAccount(context.Background(), declaredSigner), + "no account may be persisted for a rejected signer") + }) + } + } +} + +// TestAccountInitDecorator_RejectsMismatchedSigner covers the general case: a +// well-formed 20-byte signer that is simply not the address of the signing key. +func TestAccountInitDecorator_RejectsMismatchedSigner(t *testing.T) { + encCfg := newSignerBindingEncodingConfig(t) + + attackerKey := secp256k1.GenPrivKey() + victimKey := secp256k1.GenPrivKey() + declaredSigner := sdk.AccAddress(victimKey.PubKey().Address()) + + msg := gaslessMsgFor(t, "MsgExecutePayload", declaredSigner) + tx := buildSignedTx(t, encCfg, msg, declaredSigner, attackerKey) + + aid, ak := newSignerBindingDecorator(t, encCfg) + ctx := newAnteTestCtx(t, false).WithChainID(anteTestChainID) + + _, err := aid.AnteHandle(ctx, tx, false, emptyNext) + require.Error(t, err) + require.True(t, sdkerrors.ErrInvalidPubKey.Is(err), "expected ErrInvalidPubKey, got: %v", err) + require.False(t, ak.HasAccount(context.Background(), declaredSigner)) +} + +// TestAccountInitDecorator_AcceptsMatchingSigner is the positive control: a +// normal 20-byte signer whose key matches still creates the account and passes. +func TestAccountInitDecorator_AcceptsMatchingSigner(t *testing.T) { + encCfg := newSignerBindingEncodingConfig(t) + + for _, msgType := range []string{"MsgExecutePayload", "MsgMigrateUEA"} { + t.Run(msgType, func(t *testing.T) { + key := secp256k1.GenPrivKey() + signer := sdk.AccAddress(key.PubKey().Address()) + require.Len(t, signer, common.AddressLength) + + msg := gaslessMsgFor(t, msgType, signer) + tx := buildSignedTx(t, encCfg, msg, signer, key) + + aid, ak := newSignerBindingDecorator(t, encCfg) + ctx := newAnteTestCtx(t, false).WithChainID(anteTestChainID) + + _, err := aid.AnteHandle(ctx, tx, false, emptyNext) + require.NoError(t, err) + + acc := ak.GetAccount(context.Background(), signer) + require.NotNil(t, acc, "the account must be created for a legitimate gasless tx") + require.Equal(t, uint64(1), acc.GetSequence()) + }) + } +} + +// TestAccountInitDecorator_SimulationUnaffected checks that the new binding +// check keeps the SDK's `!simulate` guard, so simulation and gas estimation - +// which carry no usable signature - keep working. +func TestAccountInitDecorator_SimulationUnaffected(t *testing.T) { + encCfg := newSignerBindingEncodingConfig(t) + + attackerKey := secp256k1.GenPrivKey() + victimKey := secp256k1.GenPrivKey() + + for name, declaredSigner := range map[string]sdk.AccAddress{ + "matching_signer": sdk.AccAddress(attackerKey.PubKey().Address()), + "mismatched_signer": sdk.AccAddress(victimKey.PubKey().Address()), + } { + t.Run(name, func(t *testing.T) { + msg := gaslessMsgFor(t, "MsgExecutePayload", declaredSigner) + tx := buildSignedTx(t, encCfg, msg, declaredSigner, attackerKey) + + aid, _ := newSignerBindingDecorator(t, encCfg) + ctx := newAnteTestCtx(t, false).WithChainID(anteTestChainID) + + _, err := aid.AnteHandle(ctx, tx, true /* simulate */, emptyNext) + require.NoError(t, err, "simulation must not be affected by the binding check") + }) + } +} + +// TestAccountInitDecorator_EnforcesSignatureLimit covers F-2026-18186: the +// new-account path short-circuits the ante chain, so it has to enforce the +// signature count limit itself instead of verifying an unbounded multisig for +// free. +func TestAccountInitDecorator_EnforcesSignatureLimit(t *testing.T) { + encCfg := newSignerBindingEncodingConfig(t) + + params := authtypes.DefaultParams() + numKeys := int(params.TxSigLimit) + 1 + + pubKeys := make([]cryptotypes.PubKey, numKeys) + sigs := make([]signing.SignatureData, numKeys) + bitArray := cryptotypes.NewCompactBitArray(numKeys) + for i := 0; i < numKeys; i++ { + pubKeys[i] = secp256k1.GenPrivKey().PubKey() + sigs[i] = &signing.SingleSignatureData{ + SignMode: signing.SignMode_SIGN_MODE_DIRECT, + Signature: []byte("not-checked-the-limit-trips-first"), + } + bitArray.SetIndex(i, true) + } + + multisigPk := kmultisig.NewLegacyAminoPubKey(numKeys, pubKeys) + signer := sdk.AccAddress(multisigPk.Address()) + + txb := encCfg.TxConfig.NewTxBuilder() + require.NoError(t, txb.SetMsgs(gaslessMsgFor(t, "MsgExecutePayload", signer))) + txb.SetGasLimit(300_000) + require.NoError(t, txb.SetSignatures(signing.SignatureV2{ + PubKey: multisigPk, + Data: &signing.MultiSignatureData{BitArray: bitArray, Signatures: sigs}, + Sequence: 0, + })) + + aid, ak := newSignerBindingDecorator(t, encCfg) + ctx := newAnteTestCtx(t, false).WithChainID(anteTestChainID) + + _, err := aid.AnteHandle(ctx, txb.GetTx(), false, emptyNext) + require.Error(t, err) + require.True(t, sdkerrors.ErrTooManySignatures.Is(err), "expected ErrTooManySignatures, got: %v", err) + require.False(t, ak.HasAccount(context.Background(), signer)) +} diff --git a/app/ante/ante_cosmos.go b/app/ante/ante_cosmos.go index 08be3f01..78170d88 100755 --- a/app/ante/ante_cosmos.go +++ b/app/ante/ante_cosmos.go @@ -43,7 +43,8 @@ func NewCosmosAnteHandler(ctx sdk.Context, options HandlerOptions) sdk.AnteHandl // NewAccountInitDecorator must be called before all signature verification decorators and SetPubKeyDecorator // - this // 1. generates the account for the new accounts only for gasless transactions, - // 2. verifies the sig, and + // 2. binds the declared signer to the signing key, enforces the signature + // count limit and verifies the sig, and // 3. bypasses the rest of the ante chain NewAccountInitDecorator(options.AccountKeeper, options.SignModeHandler), // SetPubKeyDecorator must be called before all signature verification decorators diff --git a/test/integration/uexecutor/chain_enabled_test.go b/test/integration/uexecutor/chain_enabled_test.go index ee65cecf..84d5e5df 100644 --- a/test/integration/uexecutor/chain_enabled_test.go +++ b/test/integration/uexecutor/chain_enabled_test.go @@ -220,7 +220,7 @@ func TestExecutePayload_ChainEnabled(t *testing.T) { ms := uexecutorkeeper.NewMsgServerImpl(testApp.UexecutorKeeper) _, err := ms.ExecutePayload(ctx, &uexecutortypes.MsgExecutePayload{ - Signer: "cosmos1xpurwdecvsenyvpkxvmnge3cv93nyd34xuersef38pjnxen9xfsk2dnz8yek2drrv56qmn2ak9", + Signer: testSigner, UniversalAccountId: &uexecutortypes.UniversalAccountId{ ChainNamespace: "eip155", ChainId: "11155111", diff --git a/test/integration/uexecutor/execute_payload_test.go b/test/integration/uexecutor/execute_payload_test.go index 3a1cf313..88e61514 100644 --- a/test/integration/uexecutor/execute_payload_test.go +++ b/test/integration/uexecutor/execute_payload_test.go @@ -15,6 +15,12 @@ import ( "github.com/stretchr/testify/require" ) +// testSigner is the bech32 form of the 20-byte account that these fixtures have +// always resolved to on the EVM side. It replaces an older literal that decoded +// to 42 bytes - GetAddressPair used to truncate it down to exactly these bytes, +// and now rejects it outright. +const testSigner = "cosmos18pjnzwr9xdnx2vnpv5mxywfnv56xxef5cludl5" + func TestExecutePayload(t *testing.T) { app, ctx, _ := utils.SetAppWithValidators(t) @@ -100,7 +106,7 @@ func TestExecutePayload(t *testing.T) { require.NoError(t, err) msg := &uexecutortypes.MsgExecutePayload{ - Signer: "cosmos1xpurwdecvsenyvpkxvmnge3cv93nyd34xuersef38pjnxen9xfsk2dnz8yek2drrv56qmn2ak9", + Signer: testSigner, UniversalAccountId: validUA, UniversalPayload: validUP, VerificationData: "0x91987784d56359fa91c3e3e0332f4f0cffedf9c081eb12874a63b41d5b5e5c660dc827947c2ae26e658d0551ad4b2d2aa073d62691429a0ae239d2cc58055bf11c", @@ -130,7 +136,7 @@ func TestExecutePayload(t *testing.T) { } msg := &uexecutortypes.MsgExecutePayload{ - Signer: "cosmos1xpurwdecvsenyvpkxvmnge3cv93nyd34xuersef38pjnxen9xfsk2dnz8yek2drrv56qmn2ak9", + Signer: testSigner, UniversalAccountId: validUA, UniversalPayload: validUP, } @@ -160,7 +166,7 @@ func TestExecutePayload(t *testing.T) { } msg := &uexecutortypes.MsgExecutePayload{ - Signer: "cosmos1xpurwdecvsenyvpkxvmnge3cv93nyd34xuersef38pjnxen9xfsk2dnz8yek2drrv56qmn2ak9", + Signer: testSigner, UniversalAccountId: validUA, UniversalPayload: validUP, VerificationData: "0xZZZZ", @@ -261,7 +267,7 @@ func TestExecutePayload_AutoDeployOnPreFundedAddress(t *testing.T) { // Submit MsgExecutePayload directly — no standalone DeployUEAV2 call beforehand. msg := &uexecutortypes.MsgExecutePayload{ - Signer: "cosmos1xpurwdecvsenyvpkxvmnge3cv93nyd34xuersef38pjnxen9xfsk2dnz8yek2drrv56qmn2ak9", + Signer: testSigner, UniversalAccountId: validUA, UniversalPayload: validUP, VerificationData: "0x91987784d56359fa91c3e3e0332f4f0cffedf9c081eb12874a63b41d5b5e5c660dc827947c2ae26e658d0551ad4b2d2aa073d62691429a0ae239d2cc58055bf11c", @@ -333,7 +339,7 @@ func TestExecutePayload_RejectWhenUndeployedAndUnfunded(t *testing.T) { } msg := &uexecutortypes.MsgExecutePayload{ - Signer: "cosmos1xpurwdecvsenyvpkxvmnge3cv93nyd34xuersef38pjnxen9xfsk2dnz8yek2drrv56qmn2ak9", + Signer: testSigner, UniversalAccountId: validUA, UniversalPayload: validUP, VerificationData: "0x1234", diff --git a/utils/address.go b/utils/address.go index 0a9ea1f6..cf52c362 100644 --- a/utils/address.go +++ b/utils/address.go @@ -58,22 +58,41 @@ func ConvertAnyAddressesToBytes[T ByteType](addr ...string) ([]T, error) { return res, nil } -// get address pair returns both the cosmos and the 0x addresses, or an error +// GetAddressPair returns both the cosmos and the 0x addresses, or an error. +// +// The address MUST decode to exactly 20 bytes. The Cosmos SDK accepts bech32 +// account addresses of up to 255 bytes, while common.BytesToAddress silently +// keeps only the RIGHTMOST 20 bytes. A longer address would therefore collapse +// onto an unrelated EVM address - e.g. 0x01 || +// truncates to the uexecutor module itself - so reject it instead of +// truncating. func GetAddressPair(addr string) (sdk.AccAddress, common.Address, error) { bz, err := ConvertAnyAddressToBytes(addr) if err != nil { return nil, common.Address{}, err } + if len(bz) != common.AddressLength { + return nil, common.Address{}, fmt.Errorf( + "invalid address length for %q: got %d bytes, want %d", addr, len(bz), common.AddressLength) + } + return sdk.AccAddress(bz), common.BytesToAddress(bz), nil } +// MustConvertCosmosToHex returns the 0x form of addr, or an empty string when +// addr cannot be represented as a 20-byte EVM address. +// +// It never panics and never truncates: the previous common.Address(bz) +// conversion panicked for inputs shorter than 20 bytes and silently kept the +// LEFTMOST 20 bytes for longer ones - the opposite end from +// common.BytesToAddress used elsewhere in this file. func MustConvertCosmosToHex(addr string) string { bz, err := ConvertAnyAddressToBytes(addr) - if err != nil { + if err != nil || len(bz) != common.AddressLength { return "" } - return common.Address(bz).Hex() + return common.BytesToAddress(bz).Hex() } // create an enum for COSMOS, 0x, or EITHER diff --git a/utils/address_test.go b/utils/address_test.go new file mode 100644 index 00000000..7695564c --- /dev/null +++ b/utils/address_test.go @@ -0,0 +1,92 @@ +package utils_test + +import ( + "testing" + + sdk "github.com/cosmos/cosmos-sdk/types" + authtypes "github.com/cosmos/cosmos-sdk/x/auth/types" + "github.com/ethereum/go-ethereum/common" + "github.com/stretchr/testify/require" + + "github.com/pushchain/push-chain-node/utils" +) + +// uexecutorModuleEVMAddr is sha256("uexecutor")[:20] rendered as an EVM address. +const uexecutorModuleEVMAddr = "0x14191Ea54B4c176fCf86f51b0FAc7CB1E71Df7d7" + +// bech32OfLength returns a bech32 account address that decodes to exactly n bytes. +func bech32OfLength(n int) string { + bz := make([]byte, n) + for i := range bz { + bz[i] = byte(i + 1) + } + return sdk.AccAddress(bz).String() +} + +// TestGetAddressPair_RejectsNon20ByteAddresses is the regression test for +// F-2026-18200 remediation 2: anything that does not decode to exactly 20 bytes +// must be rejected rather than silently truncated. +func TestGetAddressPair_RejectsNon20ByteAddresses(t *testing.T) { + for _, length := range []int{19, 21, 22, 32} { + addr := bech32OfLength(length) + _, _, err := utils.GetAddressPair(addr) + require.Error(t, err, "%d-byte address must be rejected", length) + require.Contains(t, err.Error(), "invalid address length") + } +} + +func TestGetAddressPair_Accepts20ByteAddresses(t *testing.T) { + bz := make([]byte, common.AddressLength) + for i := range bz { + bz[i] = byte(i + 1) + } + + cosmosAddr, evmAddr, err := utils.GetAddressPair(sdk.AccAddress(bz).String()) + require.NoError(t, err) + require.Equal(t, sdk.AccAddress(bz), cosmosAddr) + require.Equal(t, common.BytesToAddress(bz), evmAddr) + + // The 0x form must round-trip as well. + cosmosAddr, evmAddr, err = utils.GetAddressPair(common.BytesToAddress(bz).Hex()) + require.NoError(t, err) + require.Equal(t, sdk.AccAddress(bz), cosmosAddr) + require.Equal(t, common.BytesToAddress(bz), evmAddr) +} + +// TestGetAddressPair_ModuleAliasRejected documents the exact attack: an over-long +// address whose rightmost 20 bytes are the uexecutor module account truncates +// onto the module's EVM address, which the UEA trusts unconditionally. +func TestGetAddressPair_ModuleAliasRejected(t *testing.T) { + moduleAddr := authtypes.NewModuleAddress("uexecutor") + require.Len(t, moduleAddr, common.AddressLength) + require.Equal(t, uexecutorModuleEVMAddr, common.BytesToAddress(moduleAddr).Hex()) + + for _, prefixLen := range []int{1, 2, 12} { + aliased := sdk.AccAddress(append(make([]byte, prefixLen), moduleAddr...)) + // Without the length check this collapses onto the module address. + require.Equal(t, uexecutorModuleEVMAddr, common.BytesToAddress(aliased).Hex()) + + _, evmAddr, err := utils.GetAddressPair(aliased.String()) + require.Error(t, err, "aliased %d-byte address must be rejected", len(aliased)) + require.Equal(t, common.Address{}, evmAddr) + } +} + +// TestMustConvertCosmosToHex checks the second truncation site: it must neither +// panic on short input nor keep the leftmost 20 bytes of a long one. +func TestMustConvertCosmosToHex(t *testing.T) { + bz := make([]byte, common.AddressLength) + for i := range bz { + bz[i] = byte(i + 1) + } + require.Equal(t, common.BytesToAddress(bz).Hex(), utils.MustConvertCosmosToHex(sdk.AccAddress(bz).String())) + + for _, length := range []int{19, 21, 22, 32} { + require.NotPanics(t, func() { + require.Empty(t, utils.MustConvertCosmosToHex(bech32OfLength(length)), + "%d-byte address must not be converted", length) + }) + } + + require.Empty(t, utils.MustConvertCosmosToHex("not-a-bech32-address")) +} diff --git a/x/uexecutor/types/msg_execute_payload.go b/x/uexecutor/types/msg_execute_payload.go index 656499f7..45281faf 100644 --- a/x/uexecutor/types/msg_execute_payload.go +++ b/x/uexecutor/types/msg_execute_payload.go @@ -7,6 +7,7 @@ import ( "cosmossdk.io/errors" sdk "github.com/cosmos/cosmos-sdk/types" sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" + "github.com/ethereum/go-ethereum/common" ) var ( @@ -47,10 +48,20 @@ func (msg *MsgExecutePayload) GetSigners() []sdk.AccAddress { // ValidateBasic does a sanity check on the provided data. func (msg *MsgExecutePayload) ValidateBasic() error { - // Validate signer - if _, err := sdk.AccAddressFromBech32(msg.Signer); err != nil { + // Validate signer. + // The length check is deliberate: bech32 account addresses may carry up to + // 255 bytes, and this signer is later converted to a 20-byte EVM address + // that keeps only the rightmost bytes. A longer signer would therefore + // collapse onto an unrelated EVM address, including module addresses that + // the UEA trusts. Reject it here, at CheckTx, before the ante chain runs. + signerBz, err := sdk.AccAddressFromBech32(msg.Signer) + if err != nil { return errors.Wrap(err, "invalid signer address") } + if len(signerBz) != common.AddressLength { + return errors.Wrapf(sdkerrors.ErrInvalidAddress, + "invalid signer address length: got %d bytes, want %d", len(signerBz), common.AddressLength) + } // Validate universalAccountId if msg.UniversalAccountId == nil { diff --git a/x/uexecutor/types/msg_migrate_uea.go b/x/uexecutor/types/msg_migrate_uea.go index 45178c6a..25ec7db3 100644 --- a/x/uexecutor/types/msg_migrate_uea.go +++ b/x/uexecutor/types/msg_migrate_uea.go @@ -4,6 +4,7 @@ import ( "cosmossdk.io/errors" sdk "github.com/cosmos/cosmos-sdk/types" sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" + "github.com/ethereum/go-ethereum/common" ) var ( @@ -44,10 +45,20 @@ func (msg *MsgMigrateUEA) GetSigners() []sdk.AccAddress { // ValidateBasic does a sanity check on the provided data. func (msg *MsgMigrateUEA) ValidateBasic() error { - // Validate signer - if _, err := sdk.AccAddressFromBech32(msg.Signer); err != nil { + // Validate signer. + // The length check is deliberate: bech32 account addresses may carry up to + // 255 bytes, and this signer is later converted to a 20-byte EVM address + // that keeps only the rightmost bytes. A longer signer would therefore + // collapse onto an unrelated EVM address, including module addresses that + // the UEA trusts. Reject it here, at CheckTx, before the ante chain runs. + signerBz, err := sdk.AccAddressFromBech32(msg.Signer) + if err != nil { return errors.Wrap(err, "invalid signer address") } + if len(signerBz) != common.AddressLength { + return errors.Wrapf(sdkerrors.ErrInvalidAddress, + "invalid signer address length: got %d bytes, want %d", len(signerBz), common.AddressLength) + } // Validate universalAccountId if msg.UniversalAccountId == nil { diff --git a/x/uexecutor/types/msg_signer_length_test.go b/x/uexecutor/types/msg_signer_length_test.go new file mode 100644 index 00000000..8b5f5c0a --- /dev/null +++ b/x/uexecutor/types/msg_signer_length_test.go @@ -0,0 +1,107 @@ +package types_test + +import ( + "testing" + + sdk "github.com/cosmos/cosmos-sdk/types" + authtypes "github.com/cosmos/cosmos-sdk/x/auth/types" + "github.com/ethereum/go-ethereum/common" + "github.com/stretchr/testify/require" + + "github.com/pushchain/push-chain-node/x/uexecutor/types" +) + +// uexecutorModuleEVMAddr is sha256("uexecutor")[:20] rendered as an EVM address. +// The UEA contract trusts calls from it unconditionally. +const uexecutorModuleEVMAddr = "0x14191Ea54B4c176fCf86f51b0FAc7CB1E71Df7d7" + +// aliasedModuleSigner returns a bech32 signer of the given byte length whose +// rightmost 20 bytes are the uexecutor module account, so that the downstream +// conversion to a 20-byte EVM address collapses onto the module itself. +func aliasedModuleSigner(t *testing.T, length int) string { + t.Helper() + moduleAddr := authtypes.NewModuleAddress(types.ModuleName) + require.Len(t, moduleAddr, common.AddressLength) + require.Equal(t, uexecutorModuleEVMAddr, common.BytesToAddress(moduleAddr).Hex()) + + prefix := make([]byte, length-common.AddressLength) + prefix[0] = 0x01 + addr := sdk.AccAddress(append(prefix, moduleAddr...)) + require.Equal(t, uexecutorModuleEVMAddr, common.BytesToAddress(addr).Hex()) + return addr.String() +} + +// TestGaslessMsgs_RejectOverlongSigner is the CheckTx-time guard for +// F-2026-18200: both gasless messages must reject a signer that does not decode +// to exactly 20 bytes, before the ante chain ever runs. +func TestGaslessMsgs_RejectOverlongSigner(t *testing.T) { + validUA := &types.UniversalAccountId{ + ChainNamespace: "eip155", + ChainId: "11155111", + Owner: "0x000000000000000000000000000000000000dead", + } + + for _, length := range []int{21, 22, 32} { + signer := aliasedModuleSigner(t, length) + + execMsg := &types.MsgExecutePayload{ + Signer: signer, + UniversalAccountId: validUA, + UniversalPayload: &types.UniversalPayload{ + To: "0x000000000000000000000000000000000000dead", + Data: "0xabcdef", + }, + VerificationData: "abcdef", + } + err := execMsg.ValidateBasic() + require.Error(t, err, "MsgExecutePayload must reject a %d-byte signer", length) + require.Contains(t, err.Error(), "invalid signer address length") + + migrateMsg := &types.MsgMigrateUEA{ + Signer: signer, + UniversalAccountId: validUA, + MigrationPayload: &types.MigrationPayload{ + Migration: "0x000000000000000000000000000000000000beef", + Nonce: "0", + Deadline: "1", + }, + Signature: "abcdef", + } + err = migrateMsg.ValidateBasic() + require.Error(t, err, "MsgMigrateUEA must reject a %d-byte signer", length) + require.Contains(t, err.Error(), "invalid signer address length") + } +} + +// TestGaslessMsgs_Accept20ByteSigner is the positive control. +func TestGaslessMsgs_Accept20ByteSigner(t *testing.T) { + signer := sdk.AccAddress(make([]byte, common.AddressLength)).String() + validUA := &types.UniversalAccountId{ + ChainNamespace: "eip155", + ChainId: "11155111", + Owner: "0x000000000000000000000000000000000000dead", + } + + execMsg := &types.MsgExecutePayload{ + Signer: signer, + UniversalAccountId: validUA, + UniversalPayload: &types.UniversalPayload{ + To: "0x000000000000000000000000000000000000dead", + Data: "0xabcdef", + }, + VerificationData: "abcdef", + } + require.NoError(t, execMsg.ValidateBasic()) + + migrateMsg := &types.MsgMigrateUEA{ + Signer: signer, + UniversalAccountId: validUA, + MigrationPayload: &types.MigrationPayload{ + Migration: "0x000000000000000000000000000000000000beef", + Nonce: "0", + Deadline: "1", + }, + Signature: "abcdef", + } + require.NoError(t, migrateMsg.ValidateBasic()) +}