diff --git a/CHANGELOG.md b/CHANGELOG.md index fc3ecb5fc..92b279089 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,12 @@ ### BUG FIXES +- Verify the Ethereum sender in `Keeper.EthereumTx` before applying the transaction. The EVM ante + handler only runs over a tx's top-level messages, so an `MsgEthereumTx` nested inside a + dispatching module (`x/authz`, `x/group`, `x/gov`, a CosmWasm stargate/`Any` message, ICA) reached + the executor with its signature never checked, letting a victim-signed transaction be replayed as + the victim. `VerifySender` is now required before `ApplyTransaction`, so `From` is always the + ECDSA-recovered signer regardless of how the message arrived. - [\#690](https://github.com/cosmos/evm/pull/690) Fix Ledger hardware wallet support for coin type 60. - [\#769](https://github.com/cosmos/evm/pull/769) Fix erc20 ibc middleware to not to validate sender address format. - [\#790](https://github.com/cosmos/evm/pull/790) fix panic in historical query due to missing EvmCoinInfo. diff --git a/tests/integration/x/vm/test_msg_server.go b/tests/integration/x/vm/test_msg_server.go index 8f776f9c3..b5f612b36 100644 --- a/tests/integration/x/vm/test_msg_server.go +++ b/tests/integration/x/vm/test_msg_server.go @@ -12,6 +12,7 @@ import ( sdkmath "cosmossdk.io/math" sdktypes "github.com/cosmos/cosmos-sdk/types" + errortypes "github.com/cosmos/cosmos-sdk/types/errors" authtypes "github.com/cosmos/cosmos-sdk/x/auth/types" govtypes "github.com/cosmos/cosmos-sdk/x/gov/types" ) @@ -260,3 +261,104 @@ func (s *KeeperTestSuite) TestRegisterPreinstalls() { s.Require().NoError(err) } } + +// TestEthereumTxSenderVerification is the regression guard for F-2026-18197 +// (nested message dispatch bypasses the EVM ante for MsgEthereumTx). +// +// The EVM ante handler only runs over the top-level messages of a tx. A module +// that unpacks and re-dispatches an embedded sdk.Msg (x/authz MsgExec, x/group +// MsgSubmitProposal/MsgExec, x/gov proposals, a CosmWasm stargate/Any message, +// ICA) hands the nested message to this msg server with the ante already +// behind it, so the signature was never checked. Keeper.EthereumTx therefore +// verifies the sender itself: the declared From must be the ECDSA signer of the +// raw transaction, whatever route the message took to get here. +func (s *KeeperTestSuite) TestEthereumTxSenderVerification() { + testCases := []struct { + name string + mutate func(msg *types.MsgEthereumTx) + expErr bool + }{ + { + // The ante already ran VerifySender for this path, so the keeper + // check is a redundant no-op and normal traffic is unaffected. + name: "pass - untouched victim-signed tx (normal JSON-RPC path)", + mutate: func(*types.MsgEthereumTx) {}, + expErr: false, + }, + { + // The attack: take a victim-signed tx off the mempool and push it + // through a nested dispatcher, declaring the attacker (or a group + // policy / authz grantee account) as the sender so the outer + // permission check passes. Recovery still yields the victim. + name: "fail - From spoofed to a different account", + mutate: func(msg *types.MsgEthereumTx) { + msg.From = s.Keyring.GetAddr(1).Bytes() + }, + expErr: true, + }, + { + // A module account is derived from a name, never from a key pair, + // so no signature can ever recover to it. This is what makes it a + // design invariant that module-driven EVM calls go through + // ApplyMessage* directly and never through this msg server. + name: "fail - From spoofed to a module account", + mutate: func(msg *types.MsgEthereumTx) { + msg.From = authtypes.NewModuleAddress(govtypes.ModuleName).Bytes() + }, + expErr: true, + }, + { + name: "fail - From cleared", + mutate: func(msg *types.MsgEthereumTx) { + msg.From = nil + }, + expErr: true, + }, + } + + s.Run("fail - empty raw transaction", func() { + s.SetupTest() + // Raw unmarshals to a nil transaction when the field is empty, and + // ValidateBasic lives in the same ante path a nested message skips. + _, err := s.Network.App.GetEVMKeeper().EthereumTx(s.Network.GetContext(), &types.MsgEthereumTx{}) + s.Require().Error(err) + s.Require().ErrorIs(err, errortypes.ErrInvalidRequest) + }) + + for _, tc := range testCases { + s.Run(tc.name, func() { + s.SetupTest() + + victim := s.Keyring.GetKey(0) + recipient := s.Keyring.GetAddr(1) + tx, err := s.Factory.GenerateSignedEthTx(victim.Priv, types.EvmTxArgs{ + To: &recipient, + Amount: big.NewInt(1e10), + }) + s.Require().NoError(err) + + msg := tx.GetMsgs()[0].(*types.MsgEthereumTx) + s.Require().Equal(victim.Addr.Bytes(), msg.From, "sanity: tx must be signed by the victim") + tc.mutate(msg) + + ctx := s.Network.GetContext() + res, err := s.Network.App.GetEVMKeeper().EthereumTx(ctx, msg) + + if !tc.expErr { + s.Require().NoError(err) + s.Require().False(res.Failed()) + return + } + + s.Require().Error(err) + s.Require().ErrorIs(err, errortypes.ErrorInvalidSigner) + s.Require().Contains(err.Error(), "signature verification failed") + // The transaction must not have been applied at all. + s.Require().Nil(res) + s.Require().False( + utils.ContainsEventType(ctx.EventManager().Events().ToABCIEvents(), types.EventTypeEthereumTx), + "a rejected tx must not emit an ethereum_tx event", + ) + }) + } +} diff --git a/x/vm/keeper/msg_server.go b/x/vm/keeper/msg_server.go index 27cebf57a..9cc27684c 100644 --- a/x/vm/keeper/msg_server.go +++ b/x/vm/keeper/msg_server.go @@ -4,10 +4,13 @@ import ( "context" "encoding/hex" "fmt" + "math/big" "strconv" "github.com/hashicorp/go-metrics" + ethtypes "github.com/ethereum/go-ethereum/core/types" + cmttypes "github.com/cometbft/cometbft/types" "github.com/cosmos/evm/x/vm/types" @@ -17,6 +20,7 @@ import ( "github.com/cosmos/cosmos-sdk/telemetry" sdk "github.com/cosmos/cosmos-sdk/types" + errortypes "github.com/cosmos/cosmos-sdk/types/errors" govtypes "github.com/cosmos/cosmos-sdk/x/gov/types" ) @@ -30,6 +34,36 @@ func (k *Keeper) EthereumTx(goCtx context.Context, msg *types.MsgEthereumTx) (*t ctx := sdk.UnwrapSDKContext(goCtx) tx := msg.AsTransaction() + // MsgEthereumTx.Raw unmarshals to a nil transaction when the raw field is + // empty, and ValidateBasic is part of the same ante path that a nested + // message skips, so the executor cannot assume it ran. Fail cleanly instead + // of dereferencing nil below. + if tx == nil { + return nil, errorsmod.Wrap(errortypes.ErrInvalidRequest, "invalid transaction: raw ethereum transaction is empty") + } + + // Verify that msg.From really is the ECDSA signer of the transaction. + // + // This is normally established by the EVM ante handler + // (ante/evm/05_signature_verification.go), but the ante chain only runs over + // the *top-level* messages of a tx. Any module that unpacks an embedded + // sdk.Msg and re-dispatches it through the message router - x/authz MsgExec, + // x/group MsgSubmitProposal/MsgExec, x/gov proposals, a CosmWasm + // stargate/Any message, ICA - delivers the nested message here with the ante + // already behind it, so nothing has checked the signature. An attacker could + // then take any victim-signed transaction off the mempool and have it + // executed as the victim. Checking here means the executor never trusts an + // unverified From, whatever route the message took to reach it. + // + // Cost on the normal path is ~zero: msg.AsTransaction() hands back the same + // *ethtypes.Transaction the ante verified, and go-ethereum caches the + // recovered sender on it per signer, so this is a cache hit rather than a + // second ECDSA recovery. + signer := ethtypes.MakeSigner(types.GetEthChainConfig(), big.NewInt(ctx.BlockHeight()), uint64(ctx.BlockTime().Unix())) //#nosec G115 -- int overflow is not a concern here + if err := msg.VerifySender(signer); err != nil { + return nil, errorsmod.Wrapf(errortypes.ErrorInvalidSigner, "signature verification failed: %s", err.Error()) + } + txIndex := k.GetTxIndexTransient(ctx) labels := []metrics.Label{