From 198da04bfbd32a2a31e95acb20b715d4af7e83a3 Mon Sep 17 00:00:00 2001 From: Peter Lee Date: Mon, 24 Aug 2026 23:11:44 -0700 Subject: [PATCH 1/4] fix(security): port full GHSA Aug-2026 statedb hardening to the v37 line Ports the complete balance-hardening set to the release/v37 fork line (node release/zetacore/v37 / testnet), matching the v35/mainnet branch: - SubBalance underflow guard + ParseAmount extended-denom handling - AddBalance overflow guard - StateDB.Commit() atomicity (cache-context staging) - module-account guard gated on non-zero delta (keeps x/fungible working) - snapshot locked balance on statedb account - x/ibc/callbacks onPacketTimeout: cachedCtx, not live ctx - TestCommitAtomicity regression test Co-Authored-By: Claude Opus 4.8 --- precompiles/common/balance_handler.go | 45 +-- precompiles/common/balance_handler_test.go | 59 +++- precompiles/common/utils.go | 11 +- .../precompiles/staking/test_integration.go | 139 +++++++++ .../precompiles/staking/test_staking.go | 4 +- tests/integration/x/vm/test_statedb.go | 269 ++++++++++++++++++ x/erc20/keeper/dynamic_precompiles.go | 19 +- x/ibc/callbacks/keeper/keeper.go | 2 +- x/precisebank/keeper/keeper.go | 4 + x/precisebank/types/interfaces.go | 1 + x/precisebank/types/mocks/MockBankKeeper.go | 49 ++++ x/vm/keeper/keeper.go | 10 + x/vm/keeper/statedb.go | 76 ++++- x/vm/statedb/commit_atomicity_test.go | 139 +++++++++ x/vm/statedb/state_object.go | 49 +++- x/vm/statedb/statedb.go | 15 +- x/vm/statedb/statedb_test.go | 76 +++-- x/vm/types/interfaces.go | 1 + x/vm/types/mocks/BankKeeper.go | 20 ++ x/vm/wrappers/testutil/mock.go | 28 ++ 20 files changed, 911 insertions(+), 105 deletions(-) create mode 100644 x/vm/statedb/commit_atomicity_test.go diff --git a/precompiles/common/balance_handler.go b/precompiles/common/balance_handler.go index 6f48bbe89..aac1fedd6 100644 --- a/precompiles/common/balance_handler.go +++ b/precompiles/common/balance_handler.go @@ -3,13 +3,9 @@ package common import ( "fmt" - "github.com/ethereum/go-ethereum/common" "github.com/ethereum/go-ethereum/core/tracing" - "github.com/holiman/uint256" - "github.com/cosmos/evm/utils" "github.com/cosmos/evm/x/vm/statedb" - evmtypes "github.com/cosmos/evm/x/vm/types" sdk "github.com/cosmos/cosmos-sdk/types" banktypes "github.com/cosmos/cosmos-sdk/x/bank/types" @@ -43,12 +39,12 @@ func (bh *BalanceHandler) AfterBalanceChange(ctx sdk.Context, stateDB *statedb.S for _, event := range events[bh.prevEventsLen:] { switch event.Type { case banktypes.EventTypeCoinSpent: - spenderHexAddr, err := parseHexAddress(event, banktypes.AttributeKeySpender) + spenderHexAddr, err := ParseHexAddress(event, banktypes.AttributeKeySpender) if err != nil { return fmt.Errorf("failed to parse spender address from event %q: %w", banktypes.EventTypeCoinSpent, err) } - amount, err := parseAmount(event) + amount, err := ParseAmount(event) if err != nil { return fmt.Errorf("failed to parse amount from event %q: %w", banktypes.EventTypeCoinSpent, err) } @@ -56,12 +52,12 @@ func (bh *BalanceHandler) AfterBalanceChange(ctx sdk.Context, stateDB *statedb.S stateDB.SubBalance(spenderHexAddr, amount, tracing.BalanceChangeUnspecified) case banktypes.EventTypeCoinReceived: - receiverHexAddr, err := parseHexAddress(event, banktypes.AttributeKeyReceiver) + receiverHexAddr, err := ParseHexAddress(event, banktypes.AttributeKeyReceiver) if err != nil { return fmt.Errorf("failed to parse receiver address from event %q: %w", banktypes.EventTypeCoinReceived, err) } - amount, err := parseAmount(event) + amount, err := ParseAmount(event) if err != nil { return fmt.Errorf("failed to parse amount from event %q: %w", banktypes.EventTypeCoinReceived, err) } @@ -72,36 +68,3 @@ func (bh *BalanceHandler) AfterBalanceChange(ctx sdk.Context, stateDB *statedb.S return nil } - -func parseHexAddress(event sdk.Event, key string) (common.Address, error) { - attr, ok := event.GetAttribute(key) - if !ok { - return common.Address{}, fmt.Errorf("event %q missing attribute %q", event.Type, key) - } - - accAddr, err := sdk.AccAddressFromBech32(attr.Value) - if err != nil { - return common.Address{}, fmt.Errorf("invalid address %q: %w", attr.Value, err) - } - - return common.BytesToAddress(accAddr), nil -} - -func parseAmount(event sdk.Event) (*uint256.Int, error) { - amountAttr, ok := event.GetAttribute(sdk.AttributeKeyAmount) - if !ok { - return nil, fmt.Errorf("event %q missing attribute %q", banktypes.EventTypeCoinSpent, sdk.AttributeKeyAmount) - } - - amountCoins, err := sdk.ParseCoinsNormalized(amountAttr.Value) - if err != nil { - return nil, fmt.Errorf("failed to parse coins from %q: %w", amountAttr.Value, err) - } - - amountBigInt := amountCoins.AmountOf(evmtypes.GetEVMCoinDenom()).BigInt() - amount, err := utils.Uint256FromBigInt(evmtypes.ConvertAmountTo18DecimalsBigInt(amountBigInt)) - if err != nil { - return nil, fmt.Errorf("failed to convert coin amount to Uint256: %w", err) - } - return amount, nil -} diff --git a/precompiles/common/balance_handler_test.go b/precompiles/common/balance_handler_test.go index d9e94b357..aff776c8b 100644 --- a/precompiles/common/balance_handler_test.go +++ b/precompiles/common/balance_handler_test.go @@ -88,7 +88,7 @@ func TestParseHexAddress(t *testing.T) { event := tc.maleate() - addr, err := parseHexAddress(event, tc.key) + addr, err := ParseHexAddress(event, tc.key) if tc.expError { require.Error(t, err) return @@ -103,12 +103,14 @@ func TestParseHexAddress(t *testing.T) { func TestParseAmount(t *testing.T) { testCases := []struct { name string + chainID testconstants.ChainID maleate func() sdk.Event expAmt *uint256.Int expError bool }{ { - name: "valid amount", + name: "valid amount", + chainID: testconstants.ExampleChainID, maleate: func() sdk.Event { coinStr := sdk.NewCoins(sdk.NewInt64Coin(evmtypes.GetEVMCoinDenom(), 5)).String() return sdk.NewEvent("bank", sdk.NewAttribute(sdk.AttributeKeyAmount, coinStr)) @@ -116,14 +118,55 @@ func TestParseAmount(t *testing.T) { expAmt: uint256.NewInt(5), }, { - name: "missing amount", + name: "unrelated denom is ignored", + chainID: testconstants.ExampleChainID, + maleate: func() sdk.Event { + coinStr := sdk.NewCoins(sdk.NewInt64Coin("foobar", 7)).String() + return sdk.NewEvent("bank", sdk.NewAttribute(sdk.AttributeKeyAmount, coinStr)) + }, + expAmt: uint256.NewInt(0), + }, + { + name: "base denom is scaled to 18 decimals", + chainID: testconstants.SixDecimalsChainID, + maleate: func() sdk.Event { + coinStr := sdk.NewCoins(sdk.NewInt64Coin(evmtypes.GetEVMCoinDenom(), 100)).String() + return sdk.NewEvent("bank", sdk.NewAttribute(sdk.AttributeKeyAmount, coinStr)) + }, + expAmt: uint256.NewInt(100_000_000_000_000), + }, + { + name: "extended denom is taken as is", + chainID: testconstants.SixDecimalsChainID, + maleate: func() sdk.Event { + coinStr := sdk.NewCoins(sdk.NewInt64Coin(evmtypes.GetEVMCoinExtendedDenom(), 500)).String() + return sdk.NewEvent("bank", sdk.NewAttribute(sdk.AttributeKeyAmount, coinStr)) + }, + expAmt: uint256.NewInt(500), + }, + { + name: "base and extended denoms are summed", + chainID: testconstants.SixDecimalsChainID, + maleate: func() sdk.Event { + coinStr := sdk.NewCoins( + sdk.NewInt64Coin(evmtypes.GetEVMCoinDenom(), 100), + sdk.NewInt64Coin(evmtypes.GetEVMCoinExtendedDenom(), 500), + ).String() + return sdk.NewEvent("bank", sdk.NewAttribute(sdk.AttributeKeyAmount, coinStr)) + }, + expAmt: uint256.NewInt(100_000_000_000_500), + }, + { + name: "missing amount", + chainID: testconstants.ExampleChainID, maleate: func() sdk.Event { return sdk.NewEvent("bank") }, expError: true, }, { - name: "invalid coins", + name: "invalid coins", + chainID: testconstants.ExampleChainID, maleate: func() sdk.Event { return sdk.NewEvent("bank", sdk.NewAttribute(sdk.AttributeKeyAmount, "invalid")) }, @@ -133,16 +176,18 @@ func TestParseAmount(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - setupBalanceHandlerTest(t) + configurator := evmtypes.NewEVMConfigurator() + configurator.ResetTestConfig() + require.NoError(t, configurator.WithEVMCoinInfo(testconstants.ExampleChainCoinInfo[tc.chainID]).Configure()) - amt, err := parseAmount(tc.maleate()) + amt, err := ParseAmount(tc.maleate()) if tc.expError { require.Error(t, err) return } require.NoError(t, err) - require.True(t, amt.Eq(tc.expAmt)) + require.Equal(t, tc.expAmt.String(), amt.String()) }) } } diff --git a/precompiles/common/utils.go b/precompiles/common/utils.go index 53b8c7867..b9aaf9913 100644 --- a/precompiles/common/utils.go +++ b/precompiles/common/utils.go @@ -2,6 +2,7 @@ package common import ( "fmt" + "math/big" "github.com/ethereum/go-ethereum/common" "github.com/holiman/uint256" @@ -38,8 +39,14 @@ func ParseAmount(event sdk.Event) (*uint256.Int, error) { return nil, fmt.Errorf("failed to parse coins from %q: %w", amountAttr.Value, err) } - amountBigInt := amountCoins.AmountOf(evmtypes.GetEVMCoinDenom()).BigInt() - amount, err := utils.Uint256FromBigInt(evmtypes.ConvertAmountTo18DecimalsBigInt(amountBigInt)) + baseAmount := amountCoins.AmountOf(evmtypes.GetEVMCoinDenom()).BigInt() + amountBigInt := evmtypes.ConvertAmountTo18DecimalsBigInt(baseAmount) + if evmtypes.GetEVMCoinExtendedDenom() != evmtypes.GetEVMCoinDenom() { + extendedAmount := amountCoins.AmountOf(evmtypes.GetEVMCoinExtendedDenom()).BigInt() + amountBigInt = new(big.Int).Add(amountBigInt, extendedAmount) + } + + amount, err := utils.Uint256FromBigInt(amountBigInt) if err != nil { return nil, fmt.Errorf("failed to convert coin amount to Uint256: %w", err) } diff --git a/tests/integration/precompiles/staking/test_integration.go b/tests/integration/precompiles/staking/test_integration.go index 0c4683c0d..4cd30031a 100644 --- a/tests/integration/precompiles/staking/test_integration.go +++ b/tests/integration/precompiles/staking/test_integration.go @@ -22,6 +22,7 @@ import ( "github.com/cosmos/evm/precompiles/testutil" "github.com/cosmos/evm/precompiles/testutil/contracts" cosmosevmutil "github.com/cosmos/evm/testutil/constants" + basefactory "github.com/cosmos/evm/testutil/integration/base/factory" "github.com/cosmos/evm/testutil/integration/evm/network" "github.com/cosmos/evm/testutil/integration/evm/utils" testutiltx "github.com/cosmos/evm/testutil/tx" @@ -35,6 +36,8 @@ import ( sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" "github.com/cosmos/cosmos-sdk/types/query" authtypes "github.com/cosmos/cosmos-sdk/x/auth/types" + vestingtypes "github.com/cosmos/cosmos-sdk/x/auth/vesting/types" + banktypes "github.com/cosmos/cosmos-sdk/x/bank/types" stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types" ) @@ -428,6 +431,90 @@ func TestPrecompileIntegrationTestSuite(t *testing.T, create network.CreateEvmAp }) }) + Context("from a vesting account", func() { + var ( + vestAddr sdk.AccAddress + vestPriv *ethsecp256k1.PrivKey + amtLocked math.Int + amtSpendable math.Int + preBal math.Int + preSupply math.Int + ) + + BeforeEach(func() { + // setup vesting account to delegate from + vestAddr, vestPriv = testutiltx.NewAccAddressAndKey() + amtLocked = math.NewInt(2e18) + amtSpendable = math.NewInt(2e18) + + funder := s.keyring.GetKey(0) + startTime := s.network.GetContext().BlockTime().Unix() + createMsg := &vestingtypes.MsgCreateVestingAccount{ + FromAddress: funder.AccAddr.String(), + ToAddress: vestAddr.String(), + Amount: sdk.NewCoins(sdk.NewCoin(s.bondDenom, amtLocked)), + EndTime: startTime + 365*24*3600, + Delayed: false, + } + sendMsg := banktypes.NewMsgSend( + funder.AccAddr, vestAddr, + sdk.NewCoins(sdk.NewCoin(s.bondDenom, amtSpendable)), + ) + _, err := s.factory.CommitCosmosTx(funder.Priv, basefactory.CosmosTxArgs{ + Msgs: []sdk.Msg{createMsg, sendMsg}, + }) + Expect(err).To(BeNil(), "error while submitting vesting setup tx") + + ctx := s.network.GetContext() + _, ok := s.network.App.GetAccountKeeper().GetAccount(ctx, vestAddr).(*vestingtypes.ContinuousVestingAccount) + Expect(ok).To(BeTrue(), "expected vesting account to persist after tx commit") + spendable := s.network.App.GetBankKeeper().SpendableCoin(ctx, vestAddr, s.bondDenom).Amount + Expect(spendable).To(Equal(amtSpendable), "unexpected spendable balance after vesting setup") + + preBalRes, err := s.grpcHandler.GetBalanceFromBank(vestAddr, s.bondDenom) + Expect(err).To(BeNil(), "error while getting pre balance") + preBal = preBalRes.Balance.Amount + Expect(preBal).To(Equal(amtLocked.Add(amtSpendable)), "expected vester pre bank balance to equal OV + extra") + + preSupplyRes, err := s.grpcHandler.GetTotalSupply() + Expect(err).To(BeNil(), "error while getting pre supply") + preSupply = preSupplyRes.Supply.AmountOf(s.bondDenom) + }) + + It("should preserve bank balance and total supply when delegating within spendable", func() { + // delegating less than spendable balance and less than + // locked balance + delAmt := big.NewInt(1e18) + gasPrice := big.NewInt(1e9) + + callArgs.Args = []interface{}{ + common.BytesToAddress(vestAddr), valAddr.String(), delAmt, + } + delTxArgs := txArgs + delTxArgs.GasPrice = gasPrice + + logCheckArgs := passCheck.WithExpEvents(staking.EventTypeDelegate) + res, _, err := s.factory.CallContractAndCheckLogs( + vestPriv, delTxArgs, callArgs, logCheckArgs, + ) + Expect(err).To(BeNil(), "error while calling the smart contract: %v", err) + Expect(s.network.NextBlock()).To(BeNil()) + + postBalRes, err := s.grpcHandler.GetBalanceFromBank(vestAddr, s.bondDenom) + Expect(err).To(BeNil(), "error while getting post balance") + postSupplyRes, err := s.grpcHandler.GetTotalSupply() + Expect(err).To(BeNil(), "error while getting post supply") + postBal := postBalRes.Balance.Amount + postSupply := postSupplyRes.Supply.AmountOf(s.bondDenom) + + gasCost := new(big.Int).Mul(gasPrice, big.NewInt(res.GasUsed)) + expBalDrop := new(big.Int).Add(delAmt, gasCost) + actualBalDrop := preBal.Sub(postBal).BigInt() + Expect(actualBalDrop).To(Equal(expBalDrop), "vesting bank balance dropped by more than delegation amount + gas") + Expect(postSupply).To(Equal(preSupply), "unexpected total supply after delegating from vesting account") + }) + }) + Context("on behalf of another account", func() { It("should not delegate if delegator address is not the msg.sender", func() { delegator := s.keyring.GetKey(0) @@ -2041,6 +2128,58 @@ func TestPrecompileIntegrationTestSuite(t *testing.T, create network.CreateEvmAp bondedTokensPoolFinalBalance := balRes.Balance Expect(bondedTokensPoolFinalBalance.Amount).To(Equal(bondedTokensPoolInitialBalance.Amount)) }) + + DescribeTable("should not delegate and update balances accordingly across orderings - internal transfer to tokens pool", + func(tc struct { + before bool + after bool + msgAmt *big.Int + }) { + args.MethodName = "testDelegateWithTransfer" + args.Args = []interface{}{ + common.BytesToAddress(bondedTokensPoolAccAddr), + s.keyring.GetAddr(0), valAddr.String(), tc.before, tc.after, + } + + txArgs.To = &contractTwoAddr + if tc.msgAmt != nil { + txArgs.Amount = tc.msgAmt + } + + reverReasonCheck := execRevertedCheck.WithErrContains( + errorsmod.Wrapf( + sdkerrors.ErrUnauthorized, "%s is not allowed to receive funds", bondedTokensPoolAccAddr.String(), + ).Error(), + ) + + _, _, err := s.factory.CallContractAndCheckLogs( + s.keyring.GetPrivKey(0), + txArgs, + args, + reverReasonCheck, + ) + Expect(err).To(BeNil(), "error while calling the smart contract: %v", err) + Expect(s.network.NextBlock()).To(BeNil()) + + balRes, err := s.grpcHandler.GetBalanceFromBank(contractTwoAddr.Bytes(), s.bondDenom) + Expect(err).To(BeNil()) + Expect(balRes.Balance.Amount).To(Equal(contractInitialBalance.Amount)) + + balRes, err = s.grpcHandler.GetBalanceFromBank(bondedTokensPoolAccAddr, s.bondDenom) + Expect(err).To(BeNil()) + Expect(balRes.Balance.Amount).To(Equal(bondedTokensPoolInitialBalance.Amount)) + }, + Entry("internal transfer after precompile call", struct { + before bool + after bool + msgAmt *big.Int + }{before: false, after: true, msgAmt: nil}), + Entry("internal transfer after precompile call with matching amounts", struct { + before bool + after bool + msgAmt *big.Int + }{before: false, after: true, msgAmt: big.NewInt(15)}), + ) }) It("should not delegate when validator does not exist", func() { diff --git a/tests/integration/precompiles/staking/test_staking.go b/tests/integration/precompiles/staking/test_staking.go index b61c2cad1..762ef2a12 100644 --- a/tests/integration/precompiles/staking/test_staking.go +++ b/tests/integration/precompiles/staking/test_staking.go @@ -381,7 +381,7 @@ func (s *PrecompileTestSuite) TestRun() { s.Require().NoError(err, "failed to pack input") return input }, - 1, // use gas > 0 to avoid doing gas estimation + 25000, // use enough gas to avoid out of gas error true, false, "write protection", @@ -391,7 +391,7 @@ func (s *PrecompileTestSuite) TestRun() { func(_ keyring.Key) []byte { return []byte("invalid") }, - 1, // use gas > 0 to avoid doing gas estimation + 25000, // use enough gas to avoid out of gas error false, false, "no method with id", diff --git a/tests/integration/x/vm/test_statedb.go b/tests/integration/x/vm/test_statedb.go index 4df90afdf..48f196f0d 100644 --- a/tests/integration/x/vm/test_statedb.go +++ b/tests/integration/x/vm/test_statedb.go @@ -30,6 +30,7 @@ import ( authtx "github.com/cosmos/cosmos-sdk/x/auth/tx" authtypes "github.com/cosmos/cosmos-sdk/x/auth/types" vestingtypes "github.com/cosmos/cosmos-sdk/x/auth/vesting/types" + stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types" ) func (s *KeeperTestSuite) TestCreateAccount() { @@ -926,6 +927,68 @@ func (s *KeeperTestSuite) TestAddSlotToAccessList() { // } // } +// TestGetAccountLocked verifies Keeper.GetAccount snapshots LockedCoins for +// the EVM denom into the Account at load time. The snapshot powers the commit +// path's bank-balance reconstruction without re-reading LockedCoins after a +// precompile may have mutated DelegatedVesting on a vesting account. +func (s *KeeperTestSuite) TestGetAccountLocked() { + addr := utiltx.GenerateAddress() + bondDenom := s.Network.GetBaseDenom() + + testCases := []struct { + name string + malleate func() + expLocked *big.Int + }{ + { + "non-existent account returns nil", + func() {}, + nil, // GetAccount returns nil entirely; checked separately + }, + { + "base account has zero Locked snapshot", + func() { + ctx := s.Network.GetContext() + err := s.Network.App.GetBankKeeper().SendCoins(ctx, s.Keyring.GetAccAddr(0), addr.Bytes(), sdk.NewCoins(sdk.NewCoin(bondDenom, math.NewInt(100)))) + s.Require().NoError(err) + }, + big.NewInt(0), + }, + { + "vesting account snapshots OriginalVesting at start time", + func() { + ctx := s.Network.GetContext() + accAddr := sdk.AccAddress(addr.Bytes()) + err := s.Network.App.GetBankKeeper().SendCoins(ctx, s.Keyring.GetAccAddr(0), accAddr, sdk.NewCoins(sdk.NewCoin(bondDenom, math.NewInt(100)))) + s.Require().NoError(err) + + baseAccount := s.Network.App.GetAccountKeeper().GetAccount(ctx, accAddr).(*authtypes.BaseAccount) + currTime := ctx.BlockTime().Unix() + acc, err := vestingtypes.NewContinuousVestingAccount(baseAccount, sdk.NewCoins(sdk.NewCoin(bondDenom, math.NewInt(100))), currTime, currTime+100) + s.Require().NoError(err) + s.Network.App.GetAccountKeeper().SetAccount(ctx, acc) + }, + big.NewInt(100), + }, + } + + for _, tc := range testCases { + s.Run(tc.name, func() { + s.SetupTest() + tc.malleate() + acc := s.Network.App.GetEVMKeeper().GetAccount(s.Network.GetContext(), addr) + if tc.expLocked == nil { + s.Require().Nil(acc, "expected nil account") + return + } + s.Require().NotNil(acc, "expected non-nil account") + locked := acc.LockedBalanceSnapshot() + s.Require().NotNil(locked, "expected Locked snapshot to be populated") + s.Require().Zero(tc.expLocked.Cmp(locked), "Locked snapshot mismatch: want %s got %s", tc.expLocked, locked) + }) + } +} + func (s *KeeperTestSuite) TestSetBalance() { amount := common.U2560 totalBalance := common.U2560 @@ -1063,6 +1126,96 @@ func (s *KeeperTestSuite) TestSetBalance() { } } +func (s *KeeperTestSuite) TestSetBalanceWithLocked() { + amount := common.U2560 + var locked *big.Int + addr := utiltx.GenerateAddress() + + testCases := []struct { + name string + addr common.Address + malleate func() + expErr bool + expTotalAmount func() *uint256.Int + expSpendable func() *uint256.Int + }{ + { + "non vesting account: locked overrides reread", + addr, + func() { + amount = uint256.NewInt(100) + locked = big.NewInt(50) + }, + false, + func() *uint256.Int { + return uint256.NewInt(150) + }, + func() *uint256.Int { + // All funds are spendable on a non base account, the snapshot + // only inflates the bank balance reconstruction. + return uint256.NewInt(150) + }, + }, + { + "vesting account: locked snapshot beats current LockedCoins re-read", + addr, + func() { + ctx := s.Network.GetContext() + accAddr := sdk.AccAddress(addr.Bytes()) + err := s.Network.App.GetBankKeeper().SendCoins(ctx, s.Keyring.GetAccAddr(0), accAddr, sdk.NewCoins(sdk.NewCoin(s.Network.GetBaseDenom(), math.NewInt(100)))) + s.Require().NoError(err) + + baseAccount := s.Network.App.GetAccountKeeper().GetAccount(ctx, accAddr).(*authtypes.BaseAccount) + baseDenom := s.Network.GetBaseDenom() + currTime := s.Network.GetContext().BlockTime().Unix() + + // setup vesting account with 40 locked to simulate a spend + acc, err := vestingtypes.NewContinuousVestingAccount( + baseAccount, + sdk.NewCoins(sdk.NewCoin(baseDenom, math.NewInt(40))), + currTime, currTime+100, + ) + s.Require().NoError(err) + s.Network.App.GetAccountKeeper().SetAccount(ctx, acc) + + amount = uint256.NewInt(100) + + // override locked to 100 + locked = big.NewInt(100) + }, + false, + func() *uint256.Int { + // ensure we used the override of 100 + return uint256.NewInt(200) + }, + func() *uint256.Int { + // spendable = bank balance − current LockedCoins = 200 − 40 = 160. + return uint256.NewInt(160) + }, + }, + } + + for _, tc := range testCases { + s.Run(tc.name, func() { + s.SetupTest() + + tc.malleate() + err := s.Network.App.GetEVMKeeper().SetBalanceWithLocked(s.Network.GetContext(), tc.addr, amount, locked) + if tc.expErr { + s.Require().Error(err) + return + } + + balance := s.Network.App.GetEVMKeeper().GetBalance(s.Network.GetContext(), tc.addr) + s.Require().NoError(err) + s.Require().Equal(tc.expTotalAmount(), balance) + + spendable := s.Network.App.GetEVMKeeper().SpendableCoin(s.Network.GetContext(), tc.addr) + s.Require().Equal(tc.expSpendable(), spendable) + }) + } +} + func (s *KeeperTestSuite) TestDeleteAccount() { var ( ctx sdk.Context @@ -1154,3 +1307,119 @@ func (s *KeeperTestSuite) TestDeleteAccount() { }) } } + +func (s *KeeperTestSuite) TestSetBalanceRejectsModuleAccounts() { + type setup struct { + addr common.Address + current *uint256.Int + } + + // NOTE: ZetaChain narrows the upstream guard to fire only on a non-zero + // delta, because x/fungible issues EVM calls from its own module account and + // every such call reaches SetBalance with delta == 0. A zero delta mints and + // burns nothing, so allowing it preserves the security property while keeping + // module-initiated EVM calls working. wantErr encodes that difference. + cases := []struct { + name string + prepare func() setup + amountFn func(current *uint256.Int) *uint256.Int + wantErr bool + }{ + { + name: "mocked module account (isModule arm)", + prepare: func() setup { + ctx := s.Network.GetContext() + ak := s.Network.App.GetAccountKeeper() + acc := authtypes.NewEmptyModuleAccount("test-blocked-stale-overwrite", authtypes.Minter) + ak.NewAccount(ctx, acc) + ak.SetAccount(ctx, acc) + modEth := common.BytesToAddress(acc.GetAddress().Bytes()) + return setup{ + addr: modEth, + current: s.Network.App.GetEVMKeeper().GetBalance(ctx, modEth), + } + }, + amountFn: func(_ *uint256.Int) *uint256.Int { return uint256.NewInt(12345) }, + wantErr: true, + }, + { + // the exploit direction: reconciliation minting into a module account + name: "bonded_tokens_pool, increase (mint direction)", + prepare: func() setup { + modEth := common.BytesToAddress(authtypes.NewModuleAddress(stakingtypes.BondedPoolName).Bytes()) + return setup{ + addr: modEth, + current: s.Network.App.GetEVMKeeper().GetBalance(s.Network.GetContext(), modEth), + } + }, + amountFn: func(cur *uint256.Int) *uint256.Int { + return new(uint256.Int).Add(cur, uint256.NewInt(1)) + }, + wantErr: true, + }, + { + name: "bonded_tokens_pool, decrease", + prepare: func() setup { + modEth := common.BytesToAddress(authtypes.NewModuleAddress(stakingtypes.BondedPoolName).Bytes()) + return setup{ + addr: modEth, + current: s.Network.App.GetEVMKeeper().GetBalance(s.Network.GetContext(), modEth), + } + }, + amountFn: func(cur *uint256.Int) *uint256.Int { + if cur.IsZero() { + return uint256.NewInt(0) + } + return new(uint256.Int).Sub(cur, uint256.NewInt(1)) + }, + wantErr: true, + }, + { + // zero delta: allowed on ZetaChain (see note above), nothing is + // minted or burned, so the balance must be untouched either way + name: "bonded_tokens_pool, equal (zero delta, allowed)", + prepare: func() setup { + modEth := common.BytesToAddress(authtypes.NewModuleAddress(stakingtypes.BondedPoolName).Bytes()) + return setup{ + addr: modEth, + current: s.Network.App.GetEVMKeeper().GetBalance(s.Network.GetContext(), modEth), + } + }, + amountFn: func(cur *uint256.Int) *uint256.Int { return new(uint256.Int).Set(cur) }, + wantErr: false, + }, + } + + for _, tc := range cases { + s.Run(tc.name, func() { + s.SetupTest() + st := tc.prepare() + amount := tc.amountFn(st.current) + + err := s.Network.App.GetEVMKeeper().SetBalance(s.Network.GetContext(), st.addr, amount) + if tc.wantErr { + s.Require().Error(err) + s.Require().Contains(err.Error(), "is not allowed to receive funds") + } else { + s.Require().NoError(err) + } + + // either way the module account's balance must be unchanged: the + // guard rejected the write, or the delta was zero to begin with + after := s.Network.App.GetEVMKeeper().GetBalance(s.Network.GetContext(), st.addr) + s.Require().Equal(st.current, after) + }) + } +} + +func (s *KeeperTestSuite) TestSetBalanceAllowsEOA() { + s.SetupTest() + addr := utiltx.GenerateAddress() + amount := uint256.NewInt(12345) + + err := s.Network.App.GetEVMKeeper().SetBalance(s.Network.GetContext(), addr, amount) + s.Require().NoError(err) + + got := s.Network.App.GetEVMKeeper().GetBalance(s.Network.GetContext(), addr) + s.Require().Equal(amount, got) +} diff --git a/x/erc20/keeper/dynamic_precompiles.go b/x/erc20/keeper/dynamic_precompiles.go index 7eb0e5697..5dad4d73a 100644 --- a/x/erc20/keeper/dynamic_precompiles.go +++ b/x/erc20/keeper/dynamic_precompiles.go @@ -45,21 +45,14 @@ func (k Keeper) RegisterERC20CodeHash(ctx sdk.Context, erc20Addr common.Address) k.evmKeeper.SetCode(ctx, codeHash, bytecode) } - var ( - nonce uint64 - balance = common.U2560 - ) - // keep balance and nonce if account exists - if acc := k.evmKeeper.GetAccount(ctx, erc20Addr); acc != nil { - nonce = acc.Nonce - balance = acc.Balance + // reuse account with modified code hash if it already exists + acc := k.evmKeeper.GetAccount(ctx, erc20Addr) + if acc == nil { + acc = statedb.NewEmptyAccount() } + acc.CodeHash = codeHash - return k.evmKeeper.SetAccount(ctx, erc20Addr, statedb.Account{ - CodeHash: codeHash, - Nonce: nonce, - Balance: balance, - }) + return k.evmKeeper.SetAccount(ctx, erc20Addr, *acc) } // UnRegisterERC20CodeHash sets the codehash for the account to an empty one diff --git a/x/ibc/callbacks/keeper/keeper.go b/x/ibc/callbacks/keeper/keeper.go index eab3a6d87..36cedf0c9 100644 --- a/x/ibc/callbacks/keeper/keeper.go +++ b/x/ibc/callbacks/keeper/keeper.go @@ -424,7 +424,7 @@ func (k ContractKeeper) IBCOnTimeoutPacketCallback( return err } - res, err := k.evmKeeper.CallEVM(ctx, *abi, sender, contractAddr, true, math.NewIntFromUint64(cachedCtx.GasMeter().GasRemaining()).BigInt(), "onPacketTimeout", + res, err := k.evmKeeper.CallEVM(cachedCtx, *abi, sender, contractAddr, true, math.NewIntFromUint64(cachedCtx.GasMeter().GasRemaining()).BigInt(), "onPacketTimeout", packet.GetSourceChannel(), packet.GetSourcePort(), packet.GetSequence(), packet.GetData()) if err != nil { return errorsmod.Wrapf(types.ErrCallbackFailed, "EVM returned error: %s", err.Error()) diff --git a/x/precisebank/keeper/keeper.go b/x/precisebank/keeper/keeper.go index 1185ea518..c9cb2db9b 100644 --- a/x/precisebank/keeper/keeper.go +++ b/x/precisebank/keeper/keeper.go @@ -48,3 +48,7 @@ func (k Keeper) IterateTotalSupply(ctx context.Context, cb func(coin sdk.Coin) b func (k Keeper) GetSupply(ctx context.Context, denom string) sdk.Coin { return k.bk.GetSupply(ctx, denom) } + +func (k Keeper) LockedCoins(ctx context.Context, addr sdk.AccAddress) sdk.Coins { + return k.bk.LockedCoins(ctx, addr) +} diff --git a/x/precisebank/types/interfaces.go b/x/precisebank/types/interfaces.go index bcae3df14..06da61192 100644 --- a/x/precisebank/types/interfaces.go +++ b/x/precisebank/types/interfaces.go @@ -24,6 +24,7 @@ type BankKeeper interface { GetAllBalances(ctx context.Context, addr sdk.AccAddress) sdk.Coins GetBalance(ctx context.Context, addr sdk.AccAddress, denom string) sdk.Coin GetSupply(ctx context.Context, denom string) sdk.Coin + LockedCoins(ctx context.Context, addr sdk.AccAddress) sdk.Coins SpendableCoin(ctx context.Context, addr sdk.AccAddress, denom string) sdk.Coin BlockedAddr(addr sdk.AccAddress) bool diff --git a/x/precisebank/types/mocks/MockBankKeeper.go b/x/precisebank/types/mocks/MockBankKeeper.go index 80ee7f305..bdd58044c 100644 --- a/x/precisebank/types/mocks/MockBankKeeper.go +++ b/x/precisebank/types/mocks/MockBankKeeper.go @@ -531,6 +531,55 @@ func (_c *BankKeeper_IterateTotalSupply_Call) RunAndReturn(run func(context.Cont return _c } +// LockedCoins provides a mock function with given fields: ctx, addr +func (_m *BankKeeper) LockedCoins(ctx context.Context, addr types.AccAddress) types.Coins { + ret := _m.Called(ctx, addr) + + if len(ret) == 0 { + panic("no return value specified for LockedCoins") + } + + var r0 types.Coins + if rf, ok := ret.Get(0).(func(context.Context, types.AccAddress) types.Coins); ok { + r0 = rf(ctx, addr) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(types.Coins) + } + } + + return r0 +} + +// BankKeeper_LockedCoins_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'LockedCoins' +type BankKeeper_LockedCoins_Call struct { + *mock.Call +} + +// LockedCoins is a helper method to define mock.On call +// - ctx context.Context +// - addr types.AccAddress +func (_e *BankKeeper_Expecter) LockedCoins(ctx interface{}, addr interface{}) *BankKeeper_LockedCoins_Call { + return &BankKeeper_LockedCoins_Call{Call: _e.mock.On("LockedCoins", ctx, addr)} +} + +func (_c *BankKeeper_LockedCoins_Call) Run(run func(ctx context.Context, addr types.AccAddress)) *BankKeeper_LockedCoins_Call { + _c.Call.Run(func(args mock.Arguments) { + run(args[0].(context.Context), args[1].(types.AccAddress)) + }) + return _c +} + +func (_c *BankKeeper_LockedCoins_Call) Return(_a0 types.Coins) *BankKeeper_LockedCoins_Call { + _c.Call.Return(_a0) + return _c +} + +func (_c *BankKeeper_LockedCoins_Call) RunAndReturn(run func(context.Context, types.AccAddress) types.Coins) *BankKeeper_LockedCoins_Call { + _c.Call.Return(run) + return _c +} + // MintCoins provides a mock function with given fields: ctx, moduleName, amt func (_m *BankKeeper) MintCoins(ctx context.Context, moduleName string, amt types.Coins) error { ret := _m.Called(ctx, moduleName, amt) diff --git a/x/vm/keeper/keeper.go b/x/vm/keeper/keeper.go index 075c71d87..e1cdde258 100644 --- a/x/vm/keeper/keeper.go +++ b/x/vm/keeper/keeper.go @@ -310,6 +310,16 @@ func (k *Keeper) SpendableCoin(ctx sdk.Context, addr common.Address) *uint256.In return result } +// lockedCoin loads account's locked balance of the gas token. +func (k *Keeper) lockedCoin(ctx sdk.Context, addr common.Address) *big.Int { + cosmosAddr := sdk.AccAddress(addr.Bytes()) + + lockedCoins := k.bankWrapper.LockedCoins(ctx, cosmosAddr) + lockedGasCoin := lockedCoins.AmountOf(types.GetEVMCoinDenom()) + + return lockedGasCoin.BigInt() +} + // GetBalance load account's balance of gas token. func (k *Keeper) GetBalance(ctx sdk.Context, addr common.Address) *uint256.Int { cosmosAddr := sdk.AccAddress(addr.Bytes()) diff --git a/x/vm/keeper/statedb.go b/x/vm/keeper/statedb.go index 828e0da60..666f9cb52 100644 --- a/x/vm/keeper/statedb.go +++ b/x/vm/keeper/statedb.go @@ -10,10 +10,12 @@ import ( "github.com/cosmos/evm/x/vm/statedb" "github.com/cosmos/evm/x/vm/types" + errorsmod "cosmossdk.io/errors" "cosmossdk.io/store/prefix" storetypes "cosmossdk.io/store/types" sdk "github.com/cosmos/cosmos-sdk/types" + errortypes "github.com/cosmos/cosmos-sdk/types/errors" authtypes "github.com/cosmos/cosmos-sdk/x/auth/types" ) @@ -25,13 +27,18 @@ var _ statedb.Keeper = &Keeper{} // GetAccount returns nil if account is not exist func (k *Keeper) GetAccount(ctx sdk.Context, addr common.Address) *statedb.Account { - acct := k.GetAccountWithoutBalance(ctx, addr) + cosmosAddr := sdk.AccAddress(addr.Bytes()) + acct := k.accountKeeper.GetAccount(ctx, cosmosAddr) if acct == nil { return nil } - acct.Balance = k.SpendableCoin(ctx, addr) - return acct + return statedb.NewAccount( + acct.GetSequence(), + k.SpendableCoin(ctx, addr), + k.lockedCoin(ctx, addr), + k.GetCodeHash(ctx, addr).Bytes(), + ) } // GetState loads contract state from database. @@ -108,16 +115,65 @@ func (k *Keeper) ForEachStorage(ctx sdk.Context, addr common.Address, cb func(ke } } -// SetBalance update account's balance, compare with current balance first, then decide to mint or burn. +// SetAccountBalance update account's balance, compare with current balance first, +// then decide to mint or burn. +// +// If account has a Locked balance specified within it, that value is used in +// order to compute the final balance. If Locked is nil, account's locked +// balance is fetched from state first in order to compute the final balance. +func (k *Keeper) SetAccountBalance(ctx sdk.Context, addr common.Address, account statedb.Account) error { + locked := account.LockedBalanceSnapshot() + if locked == nil { + return k.SetBalance(ctx, addr, account.Balance) + } + return k.SetBalanceWithLocked(ctx, addr, account.Balance, locked) +} + +// SetBalance updates an account's balance, compare with current balance first, +// then decide to mint or burn. func (k *Keeper) SetBalance(ctx sdk.Context, addr common.Address, amount *uint256.Int) error { + cosmosAddr := sdk.AccAddress(addr.Bytes()) + lockedCoin := k.bankWrapper.LockedCoins(ctx, cosmosAddr).AmountOf(types.GetEVMCoinDenom()) + return k.SetBalanceWithLocked(ctx, addr, amount, lockedCoin.BigInt()) +} + +// SetBalanceWithLocked updates an account's balance using the provided locked +// value to reconstruct the bank balance, instead of re-reading LockedCoins from +// state. +// +// Locked must be non nil and is used to compute the final balance instead of +// looking it up from state at set time. If you do not know the locked balance +// already, use SetBalance in order to look it up from state at set time. +func (k *Keeper) SetBalanceWithLocked(ctx sdk.Context, addr common.Address, amount *uint256.Int, locked *big.Int) error { if amount == nil { return nil } cosmosAddr := sdk.AccAddress(addr.Bytes()) - coin := k.bankWrapper.SpendableCoin(ctx, cosmosAddr, types.GetEVMCoinDenom()) - balance := coin.Amount.BigInt() - delta := new(big.Int).Sub(amount.ToBig(), balance) + // Reconstruct the target bank balance as spendable + locked snapshot, + // then mint or burn the delta against the current bank balance. + target := new(big.Int).Add(amount.ToBig(), locked) + current := k.bankWrapper.GetBalance(ctx, cosmosAddr, types.GetEVMCoinDenom()).Amount.BigInt() + delta := new(big.Int).Sub(target, current) + + // Module accounts must never have their balance reconciled by the EVM: that + // is the mint/burn primitive the statedb underflow turns into an exploit. + // + // NOTE: deliberate divergence from upstream cosmos/evm, which rejects module + // accounts unconditionally here. ZetaChain's x/fungible issues EVM calls from + // its own module account (ZRC20 deploys, gas-pool swaps, system contract + // calls), which makes the sender dirty in the statedb and reaches this path + // with delta == 0 on every such call. Gating on a non-zero delta keeps the + // security property identical -- a zero delta mints and burns nothing -- while + // leaving module-initiated EVM calls working. + if delta.Sign() != 0 { + if acct := k.accountKeeper.GetAccount(ctx, cosmosAddr); acct != nil { + if _, isModule := acct.(sdk.ModuleAccountI); isModule { + return errorsmod.Wrapf(errortypes.ErrUnauthorized, "%s is not allowed to receive funds", cosmosAddr) + } + } + } + switch delta.Sign() { case 1: // mint @@ -154,7 +210,7 @@ func (k *Keeper) SetAccount(ctx sdk.Context, addr common.Address, account stated } k.accountKeeper.SetAccount(ctx, acct) - if err := k.SetBalance(ctx, addr, account.Balance); err != nil { + if err := k.SetAccountBalance(ctx, addr, account); err != nil { return err } @@ -164,6 +220,7 @@ func (k *Keeper) SetAccount(ctx sdk.Context, addr common.Address, account stated "nonce", account.Nonce, "codeHash", common.BytesToHash(account.CodeHash).Hex(), "balance", account.Balance, + "locked-balance", account.LockedBalanceSnapshot(), ) return nil } @@ -262,8 +319,7 @@ func (k *Keeper) DeleteAccount(ctx sdk.Context, addr common.Address) error { baseAccount := k.accountKeeper.GetAccount(ctx, cosmosAddr) k.accountKeeper.SetAccount(ctx, authtypes.NewBaseAccount(cosmosAddr, baseAccount.GetPubKey(), baseAccount.GetAccountNumber(), baseAccount.GetSequence())) - // clear balance - if err := k.SetBalance(ctx, addr, new(uint256.Int)); err != nil { + if err := k.SetBalanceWithLocked(ctx, addr, new(uint256.Int), new(big.Int)); err != nil { return err } diff --git a/x/vm/statedb/commit_atomicity_test.go b/x/vm/statedb/commit_atomicity_test.go new file mode 100644 index 000000000..1faddecff --- /dev/null +++ b/x/vm/statedb/commit_atomicity_test.go @@ -0,0 +1,139 @@ +package statedb_test + +import ( + "errors" + "math/big" + "testing" + + "github.com/ethereum/go-ethereum/common" + "github.com/ethereum/go-ethereum/core/tracing" + "github.com/holiman/uint256" + "github.com/stretchr/testify/require" + + "github.com/cosmos/evm/x/vm/statedb" + + storetypes "cosmossdk.io/store/types" + + "github.com/cosmos/cosmos-sdk/testutil" + sdk "github.com/cosmos/cosmos-sdk/types" +) + +// atomicTestKeeper routes writes through ctx's real KVStore, unlike the +// in-memory-map mocks elsewhere in this package, so a write discarded via +// CacheContext is actually observable as absent. +type atomicTestKeeper struct { + key *storetypes.KVStoreKey + errAddr common.Address +} + +var _ statedb.Keeper = &atomicTestKeeper{} + +func (k *atomicTestKeeper) store(ctx sdk.Context) storetypes.KVStore { return ctx.KVStore(k.key) } + +func (k *atomicTestKeeper) GetAccount(ctx sdk.Context, addr common.Address) *statedb.Account { + bz := k.store(ctx).Get(addr.Bytes()) + if bz == nil { + return nil + } + return &statedb.Account{Balance: new(uint256.Int).SetBytes(bz)} +} + +func (k *atomicTestKeeper) SetAccount(ctx sdk.Context, addr common.Address, acc statedb.Account) error { + if addr == k.errAddr { + return errors.New("blocked") + } + k.store(ctx).Set(addr.Bytes(), acc.Balance.Bytes()) + return nil +} + +func (k *atomicTestKeeper) DeleteAccount(ctx sdk.Context, addr common.Address) error { + if addr == k.errAddr { + return errors.New("blocked") + } + k.store(ctx).Delete(addr.Bytes()) + return nil +} + +func (k *atomicTestKeeper) GetState(sdk.Context, common.Address, common.Hash) common.Hash { + return common.Hash{} +} +func (k *atomicTestKeeper) GetCode(sdk.Context, common.Hash) []byte { return nil } +func (k *atomicTestKeeper) GetCodeHash(sdk.Context, common.Address) common.Hash { return common.Hash{} } +func (k *atomicTestKeeper) ForEachStorage(sdk.Context, common.Address, func(common.Hash, common.Hash) bool) { +} +func (k *atomicTestKeeper) DeleteState(sdk.Context, common.Address, common.Hash) {} +func (k *atomicTestKeeper) SetState(sdk.Context, common.Address, common.Hash, []byte) {} +func (k *atomicTestKeeper) DeleteCode(sdk.Context, []byte) {} +func (k *atomicTestKeeper) SetCode(sdk.Context, []byte, []byte) {} + +func (k *atomicTestKeeper) KVStoreKeys() map[string]*storetypes.KVStoreKey { + return map[string]*storetypes.KVStoreKey{k.key.Name(): k.key} +} + +// TestCommitAtomicity commits a dirty set sorted [credit, blocked, debit]. A +// late failure on blocked must discard the whole commit, including credit, +// which a non-atomic commit would already have written. +func TestCommitAtomicity(t *testing.T) { + credit := common.BigToAddress(big.NewInt(10)) + blocked := common.BigToAddress(big.NewInt(50)) + debit := common.BigToAddress(big.NewInt(90)) + precompileAddr := common.BigToAddress(big.NewInt(1)) // written via cacheCtx, bypassing the journal + + setup := func(name string, errAddr common.Address) *statedb.StateDB { + key := storetypes.NewKVStoreKey(name) + tkey := storetypes.NewTransientStoreKey(name + "_t") + ctx := testutil.DefaultContext(key, tkey).WithEventManager(sdk.NewEventManager()) + return statedb.New(ctx, &atomicTestKeeper{key: key, errAddr: errAddr}, emptyTxConfig) + } + seed := func(db *statedb.StateDB) { + db.AddBalance(credit, uint256.NewInt(1_000_000), tracing.BalanceChangeUnspecified) + db.AddBalance(blocked, uint256.NewInt(1), tracing.BalanceChangeUnspecified) + db.AddBalance(debit, uint256.NewInt(1), tracing.BalanceChangeUnspecified) + } + // persisted reads through db's own keeper/ctx, bypassing db's in-memory + // cache, to check what actually reached the real store. + persisted := func(db *statedb.StateDB, addr common.Address) *statedb.Account { + return db.Keeper().GetAccount(db.GetContext(), addr) + } + // stageViaPrecompile writes directly through the cache context the way a + // real precompile does, bypassing the journal entirely. + stageViaPrecompile := func(t *testing.T, db *statedb.StateDB) { + t.Helper() + cacheCtx, err := db.GetCacheContext() + require.NoError(t, err) + require.NoError(t, db.Keeper().SetAccount(cacheCtx, precompileAddr, statedb.Account{Balance: uint256.NewInt(7)})) + } + + t.Run("late failure discards the whole commit", func(t *testing.T) { + db := setup("fail", blocked) + seed(db) + require.Error(t, db.Commit()) + require.Nil(t, persisted(db, credit)) + }) + + t.Run("late failure discards precompile-staged writes too", func(t *testing.T) { + db := setup("fail_precompile", blocked) + stageViaPrecompile(t, db) + seed(db) + require.Error(t, db.Commit()) + require.Nil(t, persisted(db, credit)) + require.Nil(t, persisted(db, precompileAddr)) + }) + + t.Run("success still persists everything", func(t *testing.T) { + db := setup("ok", common.Address{}) + seed(db) + require.NoError(t, db.Commit()) + require.Equal(t, uint256.NewInt(1_000_000), persisted(db, credit).Balance) + require.Equal(t, uint256.NewInt(1), persisted(db, debit).Balance) + }) + + t.Run("success persists precompile-staged writes too", func(t *testing.T) { + db := setup("ok_precompile", common.Address{}) + stageViaPrecompile(t, db) + seed(db) + require.NoError(t, db.Commit()) + require.Equal(t, uint256.NewInt(1_000_000), persisted(db, credit).Balance) + require.Equal(t, uint256.NewInt(7), persisted(db, precompileAddr).Balance) + }) +} diff --git a/x/vm/statedb/state_object.go b/x/vm/statedb/state_object.go index a85bcb4df..9b34febe2 100644 --- a/x/vm/statedb/state_object.go +++ b/x/vm/statedb/state_object.go @@ -2,6 +2,8 @@ package statedb import ( "bytes" + "fmt" + "math/big" "sort" "github.com/ethereum/go-ethereum/common" @@ -18,6 +20,11 @@ type Account struct { Nonce uint64 Balance *uint256.Int CodeHash []byte + + // lockedBalance is a snapshot of Account's LockedCoins at load time. Note + // that this is simply a constant offset to recover the full bank balance + // from spendable Balance at commit, not a live updating value. + lockedBalance *big.Int } // NewEmptyAccount returns an empty account. @@ -28,11 +35,30 @@ func NewEmptyAccount() *Account { } } +// NewAccount returns a new account instance. +func NewAccount(nonce uint64, balance *uint256.Int, lockedBalance *big.Int, codeHash []byte) *Account { + return &Account{ + Nonce: nonce, + Balance: balance, + CodeHash: codeHash, + lockedBalance: lockedBalance, + } +} + // IsContract returns if the account contains contract code. func (acct Account) IsContract() bool { return !types.IsEmptyCodeHash(acct.CodeHash) } +// LockedBalanceSnapshot returns a copy of the LockedCoins value observed when +// the account was loaded, or nil if no snapshot was recorded. +func (acct Account) LockedBalanceSnapshot() *big.Int { + if acct.lockedBalance == nil { + return nil + } + return new(big.Int).Set(acct.lockedBalance) +} + // Storage represents in-memory cache/buffer of contract storage. type Storage map[common.Hash]common.Hash @@ -111,7 +137,17 @@ func (s *stateObject) AddBalance(amount *uint256.Int) uint256.Int { if amount.IsZero() { return *(s.Balance()) } - return s.SetBalance(new(uint256.Int).Add(s.Balance(), amount)) + + newBalance, overflowed := new(uint256.Int).AddOverflow(s.Balance(), amount) + if overflowed { + panic(fmt.Sprintf( + "state balance overflow for %s: have=%s add=%s", + s.address.Hex(), + s.Balance().String(), + amount.String(), + )) + } + return s.SetBalance(newBalance) } // SubBalance removes amount from s's balance. @@ -121,7 +157,16 @@ func (s *stateObject) SubBalance(amount *uint256.Int) uint256.Int { if amount.IsZero() { return *(s.Balance()) } - return s.SetBalance(new(uint256.Int).Sub(s.Balance(), amount)) + balance := s.Balance() + if balance.Lt(amount) { + panic(fmt.Sprintf( + "state balance underflow for %s: have=%s sub=%s", + s.address.Hex(), + balance.String(), + amount.String(), + )) + } + return s.SetBalance(new(uint256.Int).Sub(balance, amount)) } // SetBalance updates account balance. diff --git a/x/vm/statedb/statedb.go b/x/vm/statedb/statedb.go index c5b6d8ede..981f55e8c 100644 --- a/x/vm/statedb/statedb.go +++ b/x/vm/statedb/statedb.go @@ -658,9 +658,22 @@ func (s *StateDB) Commit() error { // writeCache func will exist only when there's a call to a precompile. // It applies all the store updates preformed by precompile calls. if s.writeCache != nil { + // fold the remaining dirty set into the precompile cache so the + // state changes are atomic. + if err := s.commitWithCtx(s.cacheCtx); err != nil { + return err + } s.writeCache() + return nil } - return s.commitWithCtx(s.ctx) + + // stage writes here so a late failure leaves s.ctx untouched. + cacheCtx, writeCache := s.ctx.CacheContext() + if err := s.commitWithCtx(cacheCtx); err != nil { + return err + } + writeCache() + return nil } // CommitWithCacheCtx writes the dirty states to keeper using the cacheCtx. diff --git a/x/vm/statedb/statedb_test.go b/x/vm/statedb/statedb_test.go index 07b54bf1c..38aa27c9a 100644 --- a/x/vm/statedb/statedb_test.go +++ b/x/vm/statedb/statedb_test.go @@ -15,8 +15,12 @@ import ( "github.com/stretchr/testify/suite" "github.com/cosmos/evm/x/vm/statedb" + evmtypes "github.com/cosmos/evm/x/vm/types" "github.com/cosmos/evm/x/vm/types/mocks" + storetypes "cosmossdk.io/store/types" + + "github.com/cosmos/cosmos-sdk/testutil" sdk "github.com/cosmos/cosmos-sdk/types" ) @@ -59,7 +63,7 @@ func (suite *StateDBTestSuite) TestAccount() { suite.Require().Empty(acct.Balance) suite.Require().False(acct.IsContract()) - db = statedb.New(sdk.Context{}, keeper, emptyTxConfig) + db = statedb.New(newTestCtx(), keeper, emptyTxConfig) suite.Require().Equal(true, db.Exist(address)) suite.Require().Equal(true, db.Empty(address)) suite.Require().Equal(common.U2560, db.GetBalance(address)) @@ -82,7 +86,7 @@ func (suite *StateDBTestSuite) TestAccount() { suite.Require().NoError(db.Commit()) // SelfDestruct - db = statedb.New(sdk.Context{}, db.Keeper(), emptyTxConfig) + db = statedb.New(newTestCtx(), db.Keeper(), emptyTxConfig) suite.Require().False(db.HasSelfDestructed(address)) db.SelfDestruct(address) @@ -97,7 +101,7 @@ func (suite *StateDBTestSuite) TestAccount() { suite.Require().NoError(db.Commit()) // not accessible from StateDB anymore - db = statedb.New(sdk.Context{}, db.Keeper(), emptyTxConfig) + db = statedb.New(newTestCtx(), db.Keeper(), emptyTxConfig) suite.Require().False(db.Exist(address)) // and cleared in keeper too @@ -135,7 +139,7 @@ func (suite *StateDBTestSuite) TestAccount() { suite.Require().NoError(db.Commit()) // not accessible from StateDB anymore - db = statedb.New(sdk.Context{}, db.Keeper(), emptyTxConfig) + db = statedb.New(newTestCtx(), db.Keeper(), emptyTxConfig) suite.Require().False(db.Exist(address)) // and cleared in keeper too @@ -160,7 +164,7 @@ func (suite *StateDBTestSuite) TestAccount() { suite.Require().NoError(db.Commit()) // SelfDestruct - db = statedb.New(sdk.Context{}, db.Keeper(), emptyTxConfig) + db = statedb.New(newTestCtx(), db.Keeper(), emptyTxConfig) suite.Require().False(db.HasSelfDestructed(address)) _, _ = db.SelfDestruct6780(address) @@ -173,7 +177,7 @@ func (suite *StateDBTestSuite) TestAccount() { suite.Require().NoError(db.Commit()) // Same-tx maintains state - db = statedb.New(sdk.Context{}, db.Keeper(), emptyTxConfig) + db = statedb.New(newTestCtx(), db.Keeper(), emptyTxConfig) suite.Require().True(db.Exist(address)) suite.Require().False(db.HasSelfDestructed(address)) // but code and state are still accessible in dirty state @@ -192,9 +196,9 @@ func (suite *StateDBTestSuite) TestAccount() { } for _, tc := range testCases { suite.Run(tc.name, func() { - ctx := sdk.Context{} + ctx := newTestCtx() keeper := mocks.NewEVMKeeper() - db := statedb.New(sdk.Context{}, keeper, emptyTxConfig) + db := statedb.New(newTestCtx(), keeper, emptyTxConfig) tc.malleate(ctx, db) }) } @@ -202,7 +206,7 @@ func (suite *StateDBTestSuite) TestAccount() { func (suite *StateDBTestSuite) TestAccountOverride() { keeper := mocks.NewEVMKeeper() - db := statedb.New(sdk.Context{}, keeper, emptyTxConfig) + db := statedb.New(newTestCtx(), keeper, emptyTxConfig) // test balance carry over when overwritten amount := uint256.NewInt(1) @@ -234,14 +238,13 @@ func (suite *StateDBTestSuite) TestDBError() { }}, } for _, tc := range testCases { - db := statedb.New(sdk.Context{}, mocks.NewEVMKeeper(), emptyTxConfig) + db := statedb.New(newTestCtx(), mocks.NewEVMKeeper(), emptyTxConfig) tc.malleate(db) suite.Require().Error(db.Commit()) } } func (suite *StateDBTestSuite) TestBalance() { - // NOTE: no need to test overflow/underflow, that is guaranteed by evm implementation. testCases := []struct { name string malleate func(*statedb.StateDB) @@ -266,9 +269,9 @@ func (suite *StateDBTestSuite) TestBalance() { for _, tc := range testCases { suite.Run(tc.name, func() { - ctx := sdk.Context{} + ctx := newTestCtx() keeper := mocks.NewEVMKeeper() - db := statedb.New(sdk.Context{}, keeper, emptyTxConfig) + db := statedb.New(newTestCtx(), keeper, emptyTxConfig) tc.malleate(db) // check dirty state @@ -280,6 +283,16 @@ func (suite *StateDBTestSuite) TestBalance() { } } +func (suite *StateDBTestSuite) TestSubBalanceUnderflowPanics() { + db := statedb.New(newTestCtx().WithEventManager(sdk.NewEventManager()), mocks.NewEVMKeeper(), emptyTxConfig) + db.AddBalance(address, uint256.NewInt(1), tracing.BalanceChangeUnspecified) + + expectedPanic := fmt.Sprintf("state balance underflow for %s: have=%s sub=%s", address.Hex(), "1", "2") + suite.Require().PanicsWithValue(expectedPanic, func() { + db.SubBalance(address, uint256.NewInt(2), tracing.BalanceChangeUnspecified) + }) +} + func (suite *StateDBTestSuite) TestState() { key1 := common.BigToHash(big.NewInt(1)) value1 := common.BigToHash(big.NewInt(1)) @@ -321,9 +334,9 @@ func (suite *StateDBTestSuite) TestState() { for _, tc := range testCases { suite.Run(tc.name, func() { - ctx := sdk.Context{} + ctx := newTestCtx() keeper := mocks.NewEVMKeeper() - db := statedb.New(sdk.Context{}, keeper, emptyTxConfig) + db := statedb.New(newTestCtx(), keeper, emptyTxConfig) tc.malleate(db) suite.Require().NoError(db.Commit()) @@ -333,7 +346,7 @@ func (suite *StateDBTestSuite) TestState() { } // check ForEachStorage - db = statedb.New(sdk.Context{}, keeper, emptyTxConfig) + db = statedb.New(newTestCtx(), keeper, emptyTxConfig) collected := CollectContractStorage(db) if len(tc.expStates) > 0 { suite.Require().Equal(tc.expStates, collected) @@ -366,7 +379,7 @@ func (suite *StateDBTestSuite) TestCode() { for _, tc := range testCases { suite.Run(tc.name, func() { keeper := mocks.NewEVMKeeper() - db := statedb.New(sdk.Context{}, keeper, emptyTxConfig) + db := statedb.New(newTestCtx(), keeper, emptyTxConfig) tc.malleate(db) // check dirty state @@ -377,7 +390,7 @@ func (suite *StateDBTestSuite) TestCode() { suite.Require().NoError(db.Commit()) // check again - db = statedb.New(sdk.Context{}, keeper, emptyTxConfig) + db = statedb.New(newTestCtx(), keeper, emptyTxConfig) suite.Require().Equal(tc.expCode, db.GetCode(address)) suite.Require().Equal(len(tc.expCode), db.GetCodeSize(address)) suite.Require().Equal(tc.expCodeHash, db.GetCodeHash(address)) @@ -431,7 +444,7 @@ func (suite *StateDBTestSuite) TestRevertSnapshot() { } for _, tc := range testCases { suite.Run(tc.name, func() { - ctx := sdk.Context{} + ctx := newTestCtx() keeper := mocks.NewEVMKeeper() { @@ -470,7 +483,7 @@ func (suite *StateDBTestSuite) TestNestedSnapshot() { value1 := common.BigToHash(big.NewInt(1)) value2 := common.BigToHash(big.NewInt(2)) - db := statedb.New(sdk.Context{}, mocks.NewEVMKeeper(), emptyTxConfig) + db := statedb.New(newTestCtx(), mocks.NewEVMKeeper(), emptyTxConfig) rev1 := db.Snapshot() db.SetState(address, key, value1) @@ -487,7 +500,7 @@ func (suite *StateDBTestSuite) TestNestedSnapshot() { } func (suite *StateDBTestSuite) TestInvalidSnapshotId() { - db := statedb.New(sdk.Context{}, mocks.NewEVMKeeper(), emptyTxConfig) + db := statedb.New(newTestCtx(), mocks.NewEVMKeeper(), emptyTxConfig) suite.Require().Panics(func() { db.RevertToSnapshot(1) }) @@ -578,7 +591,7 @@ func (suite *StateDBTestSuite) TestAccessList() { } for _, tc := range testCases { - db := statedb.New(sdk.Context{}, mocks.NewEVMKeeper(), emptyTxConfig) + db := statedb.New(newTestCtx(), mocks.NewEVMKeeper(), emptyTxConfig) tc.malleate(db) } } @@ -591,7 +604,7 @@ func (suite *StateDBTestSuite) TestLog() { txHash, 1, 1, ) - db := statedb.New(sdk.Context{}, mocks.NewEVMKeeper(), txConfig) + db := statedb.New(newTestCtx(), mocks.NewEVMKeeper(), txConfig) data := []byte("hello world") db.AddLog(ðtypes.Log{ Address: address, @@ -643,7 +656,7 @@ func (suite *StateDBTestSuite) TestRefund() { }, 0, true}, } for _, tc := range testCases { - db := statedb.New(sdk.Context{}, mocks.NewEVMKeeper(), emptyTxConfig) + db := statedb.New(newTestCtx(), mocks.NewEVMKeeper(), emptyTxConfig) if !tc.expPanic { tc.malleate(db) suite.Require().Equal(tc.expRefund, db.GetRefund()) @@ -656,7 +669,7 @@ func (suite *StateDBTestSuite) TestRefund() { } func (suite *StateDBTestSuite) TestIterateStorage() { - ctx := sdk.Context{} + ctx := newTestCtx() key1 := common.BigToHash(big.NewInt(1)) value1 := common.BigToHash(big.NewInt(2)) @@ -664,7 +677,7 @@ func (suite *StateDBTestSuite) TestIterateStorage() { value2 := common.BigToHash(big.NewInt(4)) keeper := mocks.NewEVMKeeper() - db := statedb.New(sdk.Context{}, keeper, emptyTxConfig) + db := statedb.New(newTestCtx(), keeper, emptyTxConfig) db.SetState(address, key1, value1) db.SetState(address, key2, value2) @@ -707,6 +720,17 @@ func CollectContractStorage(db vm.StateDB) statedb.Storage { return storage } +var ( + testStoreKey = storetypes.NewKVStoreKey(evmtypes.StoreKey) + testTransientKey = storetypes.NewTransientStoreKey(evmtypes.TransientKey) +) + +// newTestCtx returns a context backed by a real in-memory multistore so that +// StateDB.Commit's cache-context staging works under unit tests. +func newTestCtx() sdk.Context { + return testutil.DefaultContext(testStoreKey, testTransientKey).WithEventManager(sdk.NewEventManager()) +} + func TestStateDBTestSuite(t *testing.T) { suite.Run(t, &StateDBTestSuite{}) } diff --git a/x/vm/types/interfaces.go b/x/vm/types/interfaces.go index 4a9ffba5c..0fd1d8fcc 100644 --- a/x/vm/types/interfaces.go +++ b/x/vm/types/interfaces.go @@ -44,6 +44,7 @@ type BankKeeper interface { authtypes.BankKeeper SpendableCoin(ctx context.Context, addr sdk.AccAddress, denom string) sdk.Coin GetBalance(ctx context.Context, addr sdk.AccAddress, denom string) sdk.Coin + LockedCoins(ctx context.Context, addr sdk.AccAddress) sdk.Coins SendCoinsFromModuleToAccount(ctx context.Context, senderModule string, recipientAddr sdk.AccAddress, amt sdk.Coins) error MintCoins(ctx context.Context, moduleName string, amt sdk.Coins) error BurnCoins(ctx context.Context, moduleName string, amt sdk.Coins) error diff --git a/x/vm/types/mocks/BankKeeper.go b/x/vm/types/mocks/BankKeeper.go index 0e57f8d13..4b03a4f52 100644 --- a/x/vm/types/mocks/BankKeeper.go +++ b/x/vm/types/mocks/BankKeeper.go @@ -76,6 +76,26 @@ func (_m *BankKeeper) IsSendEnabledCoins(ctx context.Context, coins ...types.Coi return r0 } +// LockedCoins provides a mock function with given fields: ctx, addr +func (_m *BankKeeper) LockedCoins(ctx context.Context, addr types.AccAddress) types.Coins { + ret := _m.Called(ctx, addr) + + if len(ret) == 0 { + panic("no return value specified for LockedCoins") + } + + var r0 types.Coins + if rf, ok := ret.Get(0).(func(context.Context, types.AccAddress) types.Coins); ok { + r0 = rf(ctx, addr) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(types.Coins) + } + } + + return r0 +} + // MintCoins provides a mock function with given fields: ctx, moduleName, amt func (_m *BankKeeper) MintCoins(ctx context.Context, moduleName string, amt types.Coins) error { ret := _m.Called(ctx, moduleName, amt) diff --git a/x/vm/wrappers/testutil/mock.go b/x/vm/wrappers/testutil/mock.go index 0f1710955..2c0493db5 100644 --- a/x/vm/wrappers/testutil/mock.go +++ b/x/vm/wrappers/testutil/mock.go @@ -321,6 +321,20 @@ func (mr *MockBankKeeperMockRecorder) IterateTotalSupply(ctx, cb interface{}) *g return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "IterateTotalSupply", reflect.TypeOf((*MockBankKeeper)(nil).IterateTotalSupply), ctx, cb) } +// LockedCoins mocks base method. +func (m *MockBankKeeper) LockedCoins(ctx context.Context, addr types.AccAddress) types.Coins { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "LockedCoins", ctx, addr) + ret0, _ := ret[0].(types.Coins) + return ret0 +} + +// LockedCoins indicates an expected call of LockedCoins. +func (mr *MockBankKeeperMockRecorder) LockedCoins(ctx, addr any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "LockedCoins", reflect.TypeOf((*MockBankKeeper)(nil).LockedCoins), ctx, addr) +} + // MintCoins mocks base method. func (m *MockBankKeeper) MintCoins(ctx context.Context, moduleName string, amt types.Coins) error { m.ctrl.T.Helper() @@ -734,6 +748,20 @@ func (mr *MockBankWrapperMockRecorder) IterateTotalSupply(ctx, cb interface{}) * return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "IterateTotalSupply", reflect.TypeOf((*MockBankWrapper)(nil).IterateTotalSupply), ctx, cb) } +// LockedCoins mocks base method. +func (m *MockBankWrapper) LockedCoins(ctx context.Context, addr types.AccAddress) types.Coins { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "LockedCoins", ctx, addr) + ret0, _ := ret[0].(types.Coins) + return ret0 +} + +// LockedCoins indicates an expected call of LockedCoins. +func (mr *MockBankWrapperMockRecorder) LockedCoins(ctx, addr any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "LockedCoins", reflect.TypeOf((*MockBankWrapper)(nil).LockedCoins), ctx, addr) +} + // MintAmountToAccount mocks base method. func (m *MockBankWrapper) MintAmountToAccount(ctx context.Context, recipientAddr types.AccAddress, amt *big.Int) error { m.ctrl.T.Helper() From 237ce279a9d9804b338e42e53c769cb6d86cb98b Mon Sep 17 00:00:00 2001 From: Peter Lee Date: Thu, 27 Aug 2026 14:00:07 -0700 Subject: [PATCH 2/4] fix(security): gate the module-account guard on the chain's blocked-receive policy (v37) The narrowed guard from the previous commit still bricks x/fungible. Gating on a non-zero delta covers module-initiated EVM calls that move no value (ZRC20 deploys, approvals, system-contract calls), but x/fungible also pays real value out of its own module account: SetupChainGasCoinAndPool sends native ZETA into the gas pool through a payable addLiquidityETH, which is a genuine non-zero delta and was rejected. Caught by node's x/fungible/migrations/v4 suite, which the previous verification did not run -- only evm-repo tests were exercised, and TestCallEVMWithData covers the delta == 0 shape rather than the value-moving one. The live blast radius is MsgDeployFungibleCoinZRC20, i.e. gas-coin onboarding for a new chain. Probing the full x/fungible + x/crosschain suites shows exactly two accounts reach the guard with a non-zero delta -- the x/fungible module account (131 burn, 1 mint) and the x/crosschain module account (13 burn). Neither is in node's blockedReceivingModAcc set, which deliberately lists only the invariant-bearing accounts: distribution, fee collector, bonded and not-bonded pools, gov, evm, feemarket. So direction-based gating does not work either -- there is a legitimate mint -- but the chain's own blocked-receive policy draws exactly the right line. The guard now consults BankKeeper.BlockedAddr instead of testing for a module account. Under the upstream evmd config every module account in maccPerms is blocked, so behaviour there is unchanged and upstream's own guard tests pass untouched; the divergence appears only on chains that deliberately allow a module account to hold and move funds. This is the same check upstream already uses for the analogous property in x/erc20/keeper/mint.go. - x/vm/types: expose BlockedAddr on the BankKeeper interface (BankWrapper embeds it, so no pass-through is needed) - x/vm/keeper: reject only blocked addresses, still only on a non-zero delta - tests: replace the ad-hoc module-account arm, which is not blocked under evmd, with an explicit unblocked-module-account-moves-value arm carrying a premise assertion; let an accepted write land rather than asserting no-op either way Co-Authored-By: Claude Opus 5 (1M context) --- tests/integration/x/vm/test_statedb.go | 40 +++++++++++++++++++------- x/vm/keeper/statedb.go | 40 ++++++++++++++++---------- x/vm/types/interfaces.go | 1 + x/vm/wrappers/testutil/mock.go | 14 +++++++++ 4 files changed, 69 insertions(+), 26 deletions(-) diff --git a/tests/integration/x/vm/test_statedb.go b/tests/integration/x/vm/test_statedb.go index 48f196f0d..3ddad8fa9 100644 --- a/tests/integration/x/vm/test_statedb.go +++ b/tests/integration/x/vm/test_statedb.go @@ -1314,11 +1314,15 @@ func (s *KeeperTestSuite) TestSetBalanceRejectsModuleAccounts() { current *uint256.Int } - // NOTE: ZetaChain narrows the upstream guard to fire only on a non-zero - // delta, because x/fungible issues EVM calls from its own module account and - // every such call reaches SetBalance with delta == 0. A zero delta mints and - // burns nothing, so allowing it preserves the security property while keeping - // module-initiated EVM calls working. wantErr encodes that difference. + // NOTE: ZetaChain narrows the upstream guard in two ways, and wantErr encodes + // both. It fires only on a non-zero delta (a zero delta mints and burns + // nothing, and x/fungible reaches SetBalance with delta == 0 on every + // module-initiated EVM call), and it consults the chain's blocked-address + // policy rather than testing for a module account. Under this test app every + // module account in maccPerms is blocked, so the staking-pool arms behave + // exactly as they do upstream; a module account outside that set is allowed + // to move value, which is what keeps ZetaChain's x/fungible gas-pool path + // (a payable addLiquidityETH from the module account) working. cases := []struct { name string prepare func() setup @@ -1326,21 +1330,33 @@ func (s *KeeperTestSuite) TestSetBalanceRejectsModuleAccounts() { wantErr bool }{ { - name: "mocked module account (isModule arm)", + // A module account the chain has NOT blocked from receiving funds is + // allowed to move value. This is the ZetaChain x/fungible shape: the + // module account pays native coin out through a payable EVM call, so + // the delta is genuinely non-zero and must reconcile. Upstream's + // unconditional module-account rejection broke exactly this, which is + // why the guard consults the blocked-address policy instead. + name: "unblocked module account, non-zero delta (allowed)", prepare: func() setup { ctx := s.Network.GetContext() ak := s.Network.App.GetAccountKeeper() - acc := authtypes.NewEmptyModuleAccount("test-blocked-stale-overwrite", authtypes.Minter) + acc := authtypes.NewEmptyModuleAccount("test-unblocked-value-mover", authtypes.Minter) ak.NewAccount(ctx, acc) ak.SetAccount(ctx, acc) modEth := common.BytesToAddress(acc.GetAddress().Bytes()) + // guard the premise: this account must not be blocked, otherwise + // the case is silently testing the rejection path instead + s.Require().False( + s.Network.App.GetBankKeeper().BlockedAddr(acc.GetAddress()), + "premise broken: the test module account is blocked from receiving", + ) return setup{ addr: modEth, current: s.Network.App.GetEVMKeeper().GetBalance(ctx, modEth), } }, amountFn: func(_ *uint256.Int) *uint256.Int { return uint256.NewInt(12345) }, - wantErr: true, + wantErr: false, }, { // the exploit direction: reconciliation minting into a module account @@ -1404,10 +1420,12 @@ func (s *KeeperTestSuite) TestSetBalanceRejectsModuleAccounts() { s.Require().NoError(err) } - // either way the module account's balance must be unchanged: the - // guard rejected the write, or the delta was zero to begin with after := s.Network.App.GetEVMKeeper().GetBalance(s.Network.GetContext(), st.addr) - s.Require().Equal(st.current, after) + if tc.wantErr { + s.Require().Equal(st.current, after, "a rejected write must not change the balance") + } else { + s.Require().Equal(amount, after, "an allowed write must land") + } }) } } diff --git a/x/vm/keeper/statedb.go b/x/vm/keeper/statedb.go index 666f9cb52..1a765f553 100644 --- a/x/vm/keeper/statedb.go +++ b/x/vm/keeper/statedb.go @@ -156,22 +156,32 @@ func (k *Keeper) SetBalanceWithLocked(ctx sdk.Context, addr common.Address, amou current := k.bankWrapper.GetBalance(ctx, cosmosAddr, types.GetEVMCoinDenom()).Amount.BigInt() delta := new(big.Int).Sub(target, current) - // Module accounts must never have their balance reconciled by the EVM: that - // is the mint/burn primitive the statedb underflow turns into an exploit. + // The EVM must never reconcile a balance into or out of an address the chain + // has declared must not receive funds. That reconciliation is the mint/burn + // primitive the statedb underflow turns into an exploit, and for module + // accounts it would also break the invariants the owning module maintains + // (the staking pools being the motivating case). // - // NOTE: deliberate divergence from upstream cosmos/evm, which rejects module - // accounts unconditionally here. ZetaChain's x/fungible issues EVM calls from - // its own module account (ZRC20 deploys, gas-pool swaps, system contract - // calls), which makes the sender dirty in the statedb and reaches this path - // with delta == 0 on every such call. Gating on a non-zero delta keeps the - // security property identical -- a zero delta mints and burns nothing -- while - // leaving module-initiated EVM calls working. - if delta.Sign() != 0 { - if acct := k.accountKeeper.GetAccount(ctx, cosmosAddr); acct != nil { - if _, isModule := acct.(sdk.ModuleAccountI); isModule { - return errorsmod.Wrapf(errortypes.ErrUnauthorized, "%s is not allowed to receive funds", cosmosAddr) - } - } + // NOTE: two deliberate divergences from upstream cosmos/evm, which rejects + // *any* module account unconditionally at the top of this function. + // + // 1. The check consults the chain's blocked-address policy rather than + // testing whether the account is a module account. Under the upstream + // evmd configuration every module account is blocked, so behaviour there + // is unchanged. Chains that deliberately permit a module account to hold + // and move funds keep working: ZetaChain leaves x/fungible and + // x/crosschain out of its blocked-receive set precisely because they + // originate EVM calls and pay value out of their own accounts + // (SetupChainGasCoinAndPool sends native ZETA into the gas pool via a + // payable addLiquidityETH call, which is a real non-zero delta). + // + // 2. It fires only on a non-zero delta. A zero delta mints and burns + // nothing, so allowing it preserves the security property while letting + // module-initiated EVM calls that move no value through -- ZRC20 + // deploys and system-contract calls make the sender dirty in the statedb + // and reach this path with delta == 0 on every call. + if delta.Sign() != 0 && k.bankWrapper.BlockedAddr(cosmosAddr) { + return errorsmod.Wrapf(errortypes.ErrUnauthorized, "%s is not allowed to receive funds", cosmosAddr) } switch delta.Sign() { diff --git a/x/vm/types/interfaces.go b/x/vm/types/interfaces.go index 0fd1d8fcc..a8a943b6d 100644 --- a/x/vm/types/interfaces.go +++ b/x/vm/types/interfaces.go @@ -45,6 +45,7 @@ type BankKeeper interface { SpendableCoin(ctx context.Context, addr sdk.AccAddress, denom string) sdk.Coin GetBalance(ctx context.Context, addr sdk.AccAddress, denom string) sdk.Coin LockedCoins(ctx context.Context, addr sdk.AccAddress) sdk.Coins + BlockedAddr(addr sdk.AccAddress) bool SendCoinsFromModuleToAccount(ctx context.Context, senderModule string, recipientAddr sdk.AccAddress, amt sdk.Coins) error MintCoins(ctx context.Context, moduleName string, amt sdk.Coins) error BurnCoins(ctx context.Context, moduleName string, amt sdk.Coins) error diff --git a/x/vm/wrappers/testutil/mock.go b/x/vm/wrappers/testutil/mock.go index 2c0493db5..4bedb8f57 100644 --- a/x/vm/wrappers/testutil/mock.go +++ b/x/vm/wrappers/testutil/mock.go @@ -756,6 +756,20 @@ func (m *MockBankWrapper) LockedCoins(ctx context.Context, addr types.AccAddress return ret0 } +// BlockedAddr mocks base method. +func (m *MockBankWrapper) BlockedAddr(addr types.AccAddress) bool { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "BlockedAddr", addr) + ret0, _ := ret[0].(bool) + return ret0 +} + +// BlockedAddr indicates an expected call of BlockedAddr. +func (mr *MockBankWrapperMockRecorder) BlockedAddr(addr any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "BlockedAddr", reflect.TypeOf((*MockBankWrapper)(nil).BlockedAddr), addr) +} + // LockedCoins indicates an expected call of LockedCoins. func (mr *MockBankWrapperMockRecorder) LockedCoins(ctx, addr any) *gomock.Call { mr.mock.ctrl.T.Helper() From 6c1c110ad83771fb795e34441e814f1a1bff5f97 Mon Sep 17 00:00:00 2001 From: Peter Lee Date: Thu, 27 Aug 2026 14:03:35 -0700 Subject: [PATCH 3/4] docs(changelog): record the GHSA Aug-2026 statedb hardening port (v37) Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index d8afe93b6..40ab08005 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,18 @@ ## UNRELEASED +### SECURITY + +- [\#31](https://github.com/zeta-chain/evm/pull/31) Port the upstream `cosmos/evm` + statedb balance hardening to this fork: snapshot the locked balance on the statedb + account, guard `SubBalance` against underflow, guard `AddBalance` against overflow, + make `StateDB.Commit()` atomic so a late failure discards the whole commit, reject + balance reconciliation for addresses the chain blocks from receiving funds, and use + the cached context in the `x/ibc/callbacks` packet-timeout path. The blocked-address + check deliberately replaces upstream's unconditional module-account rejection, which + is equivalent under the `evmd` configuration but allows a chain to let a module + account hold and move funds. + - [\#6](https://github.com/zeta-chain/evm/pull/6 Add legacy ethermint protocol buffer files to support querying legacy transactions ### DEPENDENCIES From f2aae11a52bead62cee2bbd2f0fa0a4cc36daa4a Mon Sep 17 00:00:00 2001 From: skosito Date: Thu, 3 Sep 2026 17:21:41 +0200 Subject: [PATCH 4/4] docs(changelog): point the security entry at this PR (#37) --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 40ab08005..c3807ba2d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,7 +4,7 @@ ### SECURITY -- [\#31](https://github.com/zeta-chain/evm/pull/31) Port the upstream `cosmos/evm` +- [\#37](https://github.com/zeta-chain/evm/pull/37) Port the upstream `cosmos/evm` statedb balance hardening to this fork: snapshot the locked balance on the statedb account, guard `SubBalance` against underflow, guard `AddBalance` against overflow, make `StateDB.Commit()` atomic so a late failure discards the whole commit, reject