From 1aabb3e49b7949730c1eeaf0f018d220fa855229 Mon Sep 17 00:00:00 2001 From: Nilesh Gupta Date: Wed, 26 Aug 2026 17:36:39 +0530 Subject: [PATCH] fix(uexecutor): keep the module EVM nonce and the manual nonce counter in step Route every module-sender DerivedEVMCall through one helper that reads the module account's EVM nonce, burns one nonce per attempt, and writes both back. --- .../uexecutor/module_nonce_test.go | 206 ++++++++++++++++++ x/uexecutor/keeper/evm.go | 132 +++++------ x/uexecutor/keeper/keeper.go | 62 +++++- x/uexecutor/mocks/mock_evmkeeper.go | 14 ++ x/uexecutor/types/expected_keepers.go | 5 + 5 files changed, 333 insertions(+), 86 deletions(-) create mode 100644 test/integration/uexecutor/module_nonce_test.go diff --git a/test/integration/uexecutor/module_nonce_test.go b/test/integration/uexecutor/module_nonce_test.go new file mode 100644 index 00000000..74bc9cad --- /dev/null +++ b/test/integration/uexecutor/module_nonce_test.go @@ -0,0 +1,206 @@ +package integrationtest + +import ( + "math/big" + "testing" + + sdk "github.com/cosmos/cosmos-sdk/types" + evmtypes "github.com/cosmos/evm/x/vm/types" + "github.com/ethereum/go-ethereum/common" + "github.com/stretchr/testify/require" + + "github.com/pushchain/push-chain-node/app" + utils "github.com/pushchain/push-chain-node/test/utils" + uexecutortypes "github.com/pushchain/push-chain-node/x/uexecutor/types" +) + +// F-2026-18189 — Manual Module EVM Nonce Desync on Reverted Inbound Execution. +// +// Every module-sender DerivedEVMCall in x/uexecutor supplies a *manual* nonce. +// x/vm turns that nonce into the derived transaction's identity: +// +// ethtypes.NewTx(&DynamicFeeTx{Nonce, GasFeeCap, GasTipCap, Gas, To, Value, Data}) +// -> tx.Hash() -> txConfig.TxHash -> res.Hash / the ethereum_tx event attribute +// +// so the nonce is the only field that distinguishes two otherwise byte-identical +// module calls. Two properties therefore have to hold at once, and these two tests +// pin one each: +// +// 1. the nonce the module hands to x/vm must not drift away from the module +// account's own EVM nonce when an attempt fails (Hacken's reported defect), and +// 2. a nonce must be burned by every *attempt*, not just by every committed +// success — otherwise a retry after a failed attempt reproduces a derived tx +// hash that has already been emitted in this block. +// +// (2) is why the naive "read evm.GetNonce(module) immediately before each call and +// drop the counter" fix cannot be shipped: x/vm only advances a sender's nonce for +// a CREATE (state_transition.go bumps it in the contractCreation branch only), so +// for the plain CALLs the module makes, evm.GetNonce(module) is a constant and +// every byte-identical call would collide. The module has to advance that nonce +// itself, unconditionally. + +// moduleDerivedTxHashes returns the ethereum_tx hashes emitted on ctx, in order. +// A derived tx that dies before execution (see the gas-estimation note in +// TestModuleSenderNonceDistinctHashesAcrossFailedAttempt) emits nothing, so this +// is also how the tests tell "attempted and emitted" from "attempted and dropped". +func moduleDerivedTxHashes(ctx sdk.Context) []string { + var out []string + for _, ev := range ctx.EventManager().Events() { + if ev.Type != evmtypes.EventTypeEthereumTx { + continue + } + for _, attr := range ev.Attributes { + if attr.Key == evmtypes.AttributeKeyEthereumTxHash { + out = append(out, attr.Value) + } + } + } + return out +} + +// moduleNonceState reports the two values that must never diverge: the persisted +// uexecutor counter that feeds the manual nonce, and the module account's own EVM +// nonce that x/vm and eth_getTransactionCount read. +func moduleNonceState(t *testing.T, chainApp *app.ChainApp, ctx sdk.Context) (counter, evmNonce uint64) { + t.Helper() + + counter, err := chainApp.UexecutorKeeper.GetModuleAccountNonce(ctx) + require.NoError(t, err) + + moduleAddr, _ := chainApp.UexecutorKeeper.GetUeModuleAddress(ctx) + return counter, chainApp.EVMKeeper.GetNonce(ctx, moduleAddr) +} + +// callDepositPRC20 issues one module-sender depositPRC20Token through the real +// keeper entry point, on an isolated event manager so the caller sees only the +// ethereum_tx events this one call produced. +func callDepositPRC20( + t *testing.T, + chainApp *app.ChainApp, + ctx sdk.Context, + prc20, to common.Address, + amount *big.Int, +) (hashes []string, err error) { + t.Helper() + + callCtx := ctx.WithEventManager(sdk.NewEventManager()) + _, err = chainApp.UexecutorKeeper.CallPRC20Deposit(callCtx, prc20, to, amount) + return moduleDerivedTxHashes(callCtx), err +} + +// TestModuleSenderNonceSurvivesRevertedDeposit is Hacken's stated case: a +// depositPRC20Token that fails must not leave the module's nonce bookkeeping in a +// state that breaks the *next* module-sender call. +// +// The forced failure is a deposit of a PRC20 address that has no code. The +// UniversalCore handler makes a high-level call into it, which reverts. +func TestModuleSenderNonceSurvivesRevertedDeposit(t *testing.T) { + chainApp, ctx, _ := utils.SetAppWithValidators(t) + + prc20 := utils.GetDefaultAddresses().PRC20USDCAddr + // No contract is ever deployed here, so depositPRC20Token reverts on it. + codelessPRC20 := common.HexToAddress("0x000000000000000000000000000000000000dEaD") + recipient := common.HexToAddress("0x0000000000000000000000000000000000001234") + amount := big.NewInt(1_000_000) + + // Sanity: the module account is the EVM sender for all of these calls. + moduleAddr, _ := chainApp.UexecutorKeeper.GetUeModuleAddress(ctx) + require.Equal(t, + sdk.AccAddress(moduleAddr.Bytes()), + chainApp.AccountKeeper.GetModuleAccount(ctx, uexecutortypes.ModuleName).GetAddress(), + ) + + counter, evmNonce := moduleNonceState(t, chainApp, ctx) + require.Equal(t, counter, evmNonce, "module nonce must start in sync") + + // A committed success first, so the reverted attempt below is not the very + // first thing the module ever does. + _, err := callDepositPRC20(t, chainApp, ctx, prc20, recipient, amount) + require.NoError(t, err, "baseline deposit must succeed") + + counter, evmNonce = moduleNonceState(t, chainApp, ctx) + require.Equal(t, counter, evmNonce, "module nonce must stay in sync after a committed deposit") + + beforeRevertCounter, _ := moduleNonceState(t, chainApp, ctx) + + // The reverted deposit. The inbound executors swallow this error and return + // nil, so on-chain nothing else reacts to it — whatever it leaves behind in + // the nonce bookkeeping is what the next call has to live with. + _, err = callDepositPRC20(t, chainApp, ctx, codelessPRC20, recipient, amount) + require.Error(t, err, "depositing a codeless PRC20 must fail") + + afterRevertCounter, afterRevertEvmNonce := moduleNonceState(t, chainApp, ctx) + + // The next module-sender call — Hacken's reported impact is that this one is + // blocked by the nonce the failed attempt left behind. + _, err = callDepositPRC20(t, chainApp, ctx, prc20, recipient, amount) + require.NoError(t, err, "a module-sender call after a reverted one must still succeed") + + // The failed attempt must still have consumed its nonce. A nonce handed back + // on failure is a nonce a byte-identical retry can re-use, and the derived tx + // hash is a pure function of the nonce and the calldata — see + // TestModuleSenderNonceDistinctHashesAcrossFailedAttempt. + require.Equal(t, beforeRevertCounter+1, afterRevertCounter, + "a failed module-sender attempt must still burn its nonce") + + // ...and this is the drift itself: the counter that feeds the manual nonce + // and the module account's own EVM nonce must still agree. + require.Equal(t, afterRevertCounter, afterRevertEvmNonce, + "reverted module call left the manual nonce counter drifted from the module account's EVM nonce") + + counter, evmNonce = moduleNonceState(t, chainApp, ctx) + require.Equal(t, counter, evmNonce, + "module nonce must be back in sync after the follow-up deposit") +} + +// TestModuleSenderNonceDistinctHashesAcrossFailedAttempt is the residual that +// decides the design (recommendation 4 in the write-up). +// +// Making the module account's EVM nonce the source of truth *without* advancing +// it removes the drift, but re-introduces the bug the counter was added for: the +// derived tx hash is a pure function of {Nonce, GasFeeCap, GasTipCap, Gas, To, +// Value, Data}, so byte-identical module calls collide. A failed attempt is the +// sharpest case — it commits nothing at all — but on this EVM fork plain +// successes collide too, because x/vm never advances a CALL sender's nonce. +// +// Note on the failed attempt: a module-sender call passes gasLimit == nil, so +// DerivedEVMCallWithData runs EstimateGasInternal first. For an always-reverting +// call that returns EstimateGasResponse{Gas: 0, VmError: "execution reverted"}, +// and the call then dies in ApplyMessageWithConfig with "intrinsic gas too low" +// before any ethereum_tx event is emitted. So the failed attempt has no hash of +// its own to compare — what it must still do is consume a nonce, so that the +// byte-identical call after it cannot reproduce the hash of the byte-identical +// call before it. +func TestModuleSenderNonceDistinctHashesAcrossFailedAttempt(t *testing.T) { + chainApp, ctx, _ := utils.SetAppWithValidators(t) + + prc20 := utils.GetDefaultAddresses().PRC20USDCAddr + codelessPRC20 := common.HexToAddress("0x000000000000000000000000000000000000dEaD") + recipient := common.HexToAddress("0x0000000000000000000000000000000000001234") + amount := big.NewInt(1_000_000) + + // Three byte-identical calls — same contract, same value, same calldata, same + // gas limit — with a failed attempt wedged between the first and the second. + first, err := callDepositPRC20(t, chainApp, ctx, prc20, recipient, amount) + require.NoError(t, err) + require.Len(t, first, 1, "a committed module deposit must emit exactly one ethereum_tx") + + failed, err := callDepositPRC20(t, chainApp, ctx, codelessPRC20, recipient, amount) + require.Error(t, err, "depositing a codeless PRC20 must fail") + require.Empty(t, failed, "a module call that dies in gas estimation emits no ethereum_tx") + + second, err := callDepositPRC20(t, chainApp, ctx, prc20, recipient, amount) + require.NoError(t, err) + require.Len(t, second, 1) + + third, err := callDepositPRC20(t, chainApp, ctx, prc20, recipient, amount) + require.NoError(t, err) + require.Len(t, third, 1) + + require.NotEqual(t, first[0], second[0], + "byte-identical module calls separated by a failed attempt produced the same derived tx hash") + require.NotEqual(t, second[0], third[0], + "consecutive byte-identical module calls produced the same derived tx hash") + require.NotEqual(t, first[0], third[0], + "byte-identical module calls produced the same derived tx hash") +} diff --git a/x/uexecutor/keeper/evm.go b/x/uexecutor/keeper/evm.go index 98cce158..4ec8d517 100644 --- a/x/uexecutor/keeper/evm.go +++ b/x/uexecutor/keeper/evm.go @@ -7,11 +7,57 @@ import ( "cosmossdk.io/errors" sdk "github.com/cosmos/cosmos-sdk/types" evmtypes "github.com/cosmos/evm/x/vm/types" + "github.com/ethereum/go-ethereum/accounts/abi" "github.com/ethereum/go-ethereum/common" "github.com/pushchain/push-chain-node/x/uexecutor/types" uregistrytypes "github.com/pushchain/push-chain-node/x/uregistry/types" ) +// derivedModuleCall issues one committing EVM call whose sender is the uexecutor +// module account. +// +// It is the only place a module-sender DerivedEVMCall may be made from, because +// it is the only place that maintains the module's nonce (F-2026-18189). The +// nonce is taken from nextModuleSenderNonce and consumed by burnModuleSenderNonce +// whether or not the call succeeded — a reverted attempt commits nothing, so +// giving its nonce back would let a byte-identical retry reproduce a derived tx +// hash that was already emitted. Callers get the EVM error unchanged; the nonce +// bookkeeping is not part of it. +func (k Keeper) derivedModuleCall( + ctx sdk.Context, + contractABI abi.ABI, + moduleAddr, contract common.Address, + value, gasLimit *big.Int, + method string, + args ...interface{}, +) (*evmtypes.MsgEthereumTxResponse, error) { + nonce, err := k.nextModuleSenderNonce(ctx, moduleAddr) + if err != nil { + return nil, err + } + + res, callErr := k.evmKeeper.DerivedEVMCall( + ctx, + contractABI, + moduleAddr, // sender: module account + contract, // destination + value, + gasLimit, + true, // commit = true (real tx, not simulation) + false, // gasless = false (@dev: we need gas to be emitted in the tx receipt) + true, // module sender = true + &nonce, // manual nonce of module + method, + args..., + ) + + if err := k.burnModuleSenderNonce(ctx, moduleAddr, nonce); err != nil { + return nil, err + } + + return res, callErr +} + // CallFactoryToGetUEAAddressForOrigin calls FactoryV1.getUEAForOrigin(...) func (k Keeper) CallFactoryToGetUEAAddressForOrigin( ctx sdk.Context, @@ -273,28 +319,13 @@ func (k Keeper) CallPRC20Deposit( ueModuleAccAddress, _ := k.GetUeModuleAddress(ctx) - // Before sending an EVM tx from module - nonce, err := k.GetModuleAccountNonce(ctx) - if err != nil { - return nil, err - } - - // increment first (safe for internal modules) - if _, err := k.IncrementModuleAccountNonce(ctx); err != nil { - return nil, err - } - - return k.evmKeeper.DerivedEVMCall( + return k.derivedModuleCall( ctx, abi, - ueModuleAccAddress, // sender: module account - handlerAddr, // destination + ueModuleAccAddress, + handlerAddr, big.NewInt(0), nil, - true, // commit = true (real tx, not simulation) - false, // gasless = false (@dev: we need gas to be emitted in the tx receipt) - true, // module sender = true - &nonce, // manual nonce of module "depositPRC20Token", prc20Address, amount, @@ -319,26 +350,13 @@ func (k Keeper) CallUniversalCoreSetChainMeta( ueModuleAccAddress, _ := k.GetUeModuleAddress(ctx) - nonce, err := k.GetModuleAccountNonce(ctx) - if err != nil { - return nil, err - } - - if _, err := k.IncrementModuleAccountNonce(ctx); err != nil { - return nil, err - } - - return k.evmKeeper.DerivedEVMCall( + return k.derivedModuleCall( ctx, abi, ueModuleAccAddress, handlerAddr, big.NewInt(0), nil, - true, - false, - true, - &nonce, "setChainMeta", chainNamespace, price, @@ -560,28 +578,13 @@ func (k Keeper) CallPRC20DepositAutoSwap( ueModuleAccAddress, _ := k.GetUeModuleAddress(ctx) - // Before sending an EVM tx from module - nonce, err := k.GetModuleAccountNonce(ctx) - if err != nil { - return nil, err - } - - // increment first (safe for internal modules) - if _, err := k.IncrementModuleAccountNonce(ctx); err != nil { - return nil, err - } - - return k.evmKeeper.DerivedEVMCall( + return k.derivedModuleCall( ctx, abi, - ueModuleAccAddress, // who is sending the transaction - handlerAddr, // destination: Handler contract + ueModuleAccAddress, + handlerAddr, big.NewInt(0), nil, - true, // commit = true (real tx, not simulation) - false, // gasless = false (@dev: we need gas to be emitted in the tx receipt) - true, // module sender = true - &nonce, // manual nonce of module "depositPRC20WithAutoSwap", prc20Address, amount, @@ -612,27 +615,14 @@ func (k Keeper) CallUniversalCoreRefundUnusedGas( ueModuleAccAddress, _ := k.GetUeModuleAddress(ctx) - nonce, err := k.GetModuleAccountNonce(ctx) - if err != nil { - return nil, err - } - - if _, err := k.IncrementModuleAccountNonce(ctx); err != nil { - return nil, err - } - // fee is uint24 in Solidity — pass as *big.Int (go-ethereum ABI packs non-standard widths as *big.Int) - return k.evmKeeper.DerivedEVMCall( + return k.derivedModuleCall( ctx, abi, ueModuleAccAddress, handlerAddr, big.NewInt(0), nil, - true, - false, - true, - &nonce, "refundUnusedGas", gasToken, amount, @@ -662,25 +652,13 @@ func (k Keeper) CallExecuteUniversalTx( ueModuleAccAddress, _ := k.GetUeModuleAddress(ctx) - nonce, err := k.GetModuleAccountNonce(ctx) - if err != nil { - return nil, err - } - if _, err := k.IncrementModuleAccountNonce(ctx); err != nil { - return nil, err - } - - return k.evmKeeper.DerivedEVMCall( + return k.derivedModuleCall( ctx, recipientABI, ueModuleAccAddress, recipientAddr, big.NewInt(0), nil, - true, - false, - true, - &nonce, "executeUniversalTx", sourceChain, ceaAddress, diff --git a/x/uexecutor/keeper/keeper.go b/x/uexecutor/keeper/keeper.go index 923de5dc..ba240750 100755 --- a/x/uexecutor/keeper/keeper.go +++ b/x/uexecutor/keeper/keeper.go @@ -3,6 +3,7 @@ package keeper import ( "context" "errors" + "fmt" "github.com/cosmos/cosmos-sdk/codec" sdk "github.com/cosmos/cosmos-sdk/types" @@ -348,20 +349,63 @@ func (k Keeper) GetModuleAccountNonce(ctx sdk.Context) (uint64, error) { return nonce, nil } -// IncrementModuleAccountNonce increases the nonce by 1 and stores it back. -func (k Keeper) IncrementModuleAccountNonce(ctx sdk.Context) (uint64, error) { +// SetModuleAccountNonce allows explicitly setting the nonce (optional, for migration or testing). +// It keeps the module account's EVM nonce in step, so the two can never diverge — +// see nextModuleSenderNonce for why that matters. +func (k Keeper) SetModuleAccountNonce(ctx sdk.Context, nonce uint64) error { + if err := k.ModuleAccountNonce.Set(ctx, nonce); err != nil { + return err + } + + acc := k.accountKeeper.GetModuleAccount(ctx, types.ModuleName) + if acc == nil { + return fmt.Errorf("module account %s not found", types.ModuleName) + } + if acc.GetSequence() == nonce { + return nil + } + if err := acc.SetSequence(nonce); err != nil { + return err + } + k.accountKeeper.SetAccount(ctx, acc) + + return nil +} + +// nextModuleSenderNonce picks the nonce for the module's next DerivedEVMCall. +// +// F-2026-18189. The module account's EVM nonce is the source of truth, but x/vm +// will not maintain it: ApplyMessageWithConfig advances a sender's nonce only in +// its contractCreation branch, and every call the module makes is a plain CALL. +// So the module maintains it itself (see burnModuleSenderNonce), and reads it +// back here so that a nonce the EVM *did* advance — a CREATE from the module, or +// a chain upgraded from a build that left the account nonce behind — is picked up +// instead of being re-issued. +func (k Keeper) nextModuleSenderNonce(ctx sdk.Context, moduleAddr common.Address) (uint64, error) { nonce, err := k.GetModuleAccountNonce(ctx) if err != nil { return 0, err } - newNonce := nonce + 1 - if err := k.ModuleAccountNonce.Set(ctx, newNonce); err != nil { - return 0, err + if evmNonce := k.evmKeeper.GetNonce(ctx, moduleAddr); evmNonce > nonce { + nonce = evmNonce } - return newNonce, nil + return nonce, nil } -// SetModuleAccountNonce allows explicitly setting the nonce (optional, for migration or testing). -func (k Keeper) SetModuleAccountNonce(ctx sdk.Context, nonce uint64) error { - return k.ModuleAccountNonce.Set(ctx, nonce) +// burnModuleSenderNonce consumes the nonce handed out by nextModuleSenderNonce. +// +// The advance is unconditional: it happens whether the call committed, reverted, +// or never reached the EVM at all. That is deliberate. The derived tx hash is +// ethtypes.NewTx(&DynamicFeeTx{Nonce, GasFeeCap, GasTipCap, Gas, To, Value, +// Data}).Hash(), so the nonce is the only thing separating two byte-identical +// module calls; a failed attempt that gave its nonce back would let the retry +// reproduce a hash already emitted in this block. Because the counter and the +// account nonce move together, advancing on failure can no longer desync them — +// which is what F-2026-18189 reported. +func (k Keeper) burnModuleSenderNonce(ctx sdk.Context, moduleAddr common.Address, nonce uint64) error { + next := nonce + 1 + if evmNonce := k.evmKeeper.GetNonce(ctx, moduleAddr); evmNonce > next { + next = evmNonce + } + return k.SetModuleAccountNonce(ctx, next) } diff --git a/x/uexecutor/mocks/mock_evmkeeper.go b/x/uexecutor/mocks/mock_evmkeeper.go index 0c1f0487..f33ed601 100644 --- a/x/uexecutor/mocks/mock_evmkeeper.go +++ b/x/uexecutor/mocks/mock_evmkeeper.go @@ -73,6 +73,20 @@ func (mr *MockEVMKeeperMockRecorder) GetCodeHash(ctx, addr interface{}) *gomock. return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetCodeHash", reflect.TypeOf((*MockEVMKeeper)(nil).GetCodeHash), ctx, addr) } +// GetNonce mocks base method. +func (m *MockEVMKeeper) GetNonce(ctx types.Context, addr common.Address) uint64 { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetNonce", ctx, addr) + ret0, _ := ret[0].(uint64) + return ret0 +} + +// GetNonce indicates an expected call of GetNonce. +func (mr *MockEVMKeeperMockRecorder) GetNonce(ctx, addr interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetNonce", reflect.TypeOf((*MockEVMKeeper)(nil).GetNonce), ctx, addr) +} + // DerivedEVMCall mocks base method. func (m *MockEVMKeeper) DerivedEVMCall(ctx types.Context, abi abi.ABI, from, contract common.Address, value, gasLimit *big.Int, commit, gasless, isModuleSender bool, manualNonce *uint64, method string, args ...interface{}) (*types0.MsgEthereumTxResponse, error) { m.ctrl.T.Helper() diff --git a/x/uexecutor/types/expected_keepers.go b/x/uexecutor/types/expected_keepers.go index 788d3e5f..7577b4d3 100644 --- a/x/uexecutor/types/expected_keepers.go +++ b/x/uexecutor/types/expected_keepers.go @@ -53,6 +53,8 @@ type EVMKeeper interface { args ...interface{}, ) (*types.MsgEthereumTxResponse, error) GetCodeHash(ctx sdk.Context, addr common.Address) common.Hash + // GetNonce returns the account nonce (auth sequence) the EVM sees for addr. + GetNonce(ctx sdk.Context, addr common.Address) uint64 } // FeeMarketKeeper defines the expected interface for the fee market module. @@ -94,6 +96,9 @@ type BankKeeper interface { // AccountKeeper defines the expected interface for the auth module type AccountKeeper interface { GetModuleAccount(ctx context.Context, moduleName string) sdk.ModuleAccountI + // SetAccount persists an account. Used to keep the uexecutor module + // account's EVM nonce in step with the nonce handed to DerivedEVMCall. + SetAccount(ctx context.Context, acc sdk.AccountI) } type UValidatorKeeper interface {