diff --git a/CHANGELOG.md b/CHANGELOG.md index 92b279089..4064e1bbc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,12 @@ ### BUG FIXES +- Run the gov precompile's `getTallyResult` against a throwaway cache. The SDK's `TallyResult` query + delegates to `Keeper.Tally`, which deletes every vote it counts; that is safe on the SDK's own + query paths because a gRPC query runs against a context that is never committed, but precompiles + commit their state DB cache context unconditionally. A plain EVM call to `getTallyResult` on a + proposal in the voting period therefore deleted that proposal's votes permanently. The query's + store writes are now discarded, leaving the tally itself unchanged. - Verify the Ethereum sender in `Keeper.EthereumTx` before applying the transaction. The EVM ante handler only runs over a tx's top-level messages, so an `MsgEthereumTx` nested inside a dispatching module (`x/authz`, `x/group`, `x/gov`, a CosmWasm stargate/`Any` message, ICA) reached diff --git a/precompiles/gov/query.go b/precompiles/gov/query.go index 0b389b9a0..ff7482f4b 100644 --- a/precompiles/gov/query.go +++ b/precompiles/gov/query.go @@ -4,6 +4,8 @@ import ( "github.com/ethereum/go-ethereum/accounts/abi" "github.com/ethereum/go-ethereum/core/vm" + vmstoretypes "github.com/cosmos/evm/x/vm/store/types" + sdk "github.com/cosmos/cosmos-sdk/types" ) @@ -136,7 +138,21 @@ func (p *Precompile) GetTallyResult( return nil, err } - res, err := p.govQuerier.TallyResult(ctx, queryTallyResultReq) + // The SDK's TallyResult query is *not* read-only for a proposal in the voting + // period: it delegates to the gov Keeper.Tally, which deletes every vote it + // counts (x/gov/keeper/tally.go). That is safe on the SDK's own query paths + // because a gRPC query runs against a context whose writes are never committed. + // + // Precompiles break that invariant: RunNativeAction commits the state DB cache + // context unconditionally, so a store write performed here would survive the + // enclosing EVM transaction and a plain CALL to getTallyResult would wipe the + // proposal's votes. Run the query against a throwaway cache instead, so the + // deletions are discarded while the tally itself -- delegations, weighted votes, + // validator defaults -- is still the one the SDK computes. + cacheCtx, discardWrites := newDiscardedCacheContext(ctx) + defer discardWrites() + + res, err := p.govQuerier.TallyResult(cacheCtx, queryTallyResultReq) if err != nil { return nil, err } @@ -145,6 +161,29 @@ func (p *Precompile) GetTallyResult( return method.Outputs.Pack(output.TallyResult) } +// newDiscardedCacheContext returns a context to run a query on, together with the +// function that throws away every store write the query performed. The caller is +// expected to always call it -- the writes are never meant to reach the parent +// store. +// +// sdk.Context.CacheContext alone is not enough inside a precompile. It defers to +// the underlying multi store's CacheMultiStore, and while the SDK's stores return +// a detached cache whose writes only reach the parent through the (here dropped) +// write function, the store the EVM runs precompiles on does not: +// x/vm/store/snapshotmulti pushes a snapshot layer and returns the very same +// store, so the layer stays live and is flushed along with everything else when +// the state DB commits. On that store the layer has to be popped explicitly, +// which is what the snapshot/revert pair below does. +func newDiscardedCacheContext(ctx sdk.Context) (sdk.Context, func()) { + if snapshotter, ok := ctx.MultiStore().(vmstoretypes.Snapshotter); ok { + snapshot := snapshotter.Snapshot() + return ctx, func() { snapshotter.RevertToSnapshot(snapshot) } + } + + cacheCtx, _ := ctx.CacheContext() // write function intentionally discarded + return cacheCtx, func() {} +} + // GetProposal implements the query logic for getting a proposal func (p *Precompile) GetProposal( ctx sdk.Context, diff --git a/tests/integration/precompiles/gov/test_gov.go b/tests/integration/precompiles/gov/test_gov.go index b2a2aee96..fca69a6c4 100644 --- a/tests/integration/precompiles/gov/test_gov.go +++ b/tests/integration/precompiles/gov/test_gov.go @@ -13,6 +13,11 @@ import ( "github.com/cosmos/evm/testutil" "github.com/cosmos/evm/x/vm/statedb" evmtypes "github.com/cosmos/evm/x/vm/types" + + "cosmossdk.io/collections" + + sdk "github.com/cosmos/cosmos-sdk/types" + govv1 "github.com/cosmos/cosmos-sdk/x/gov/types/v1" ) func (s *PrecompileTestSuite) TestIsTransaction() { @@ -136,3 +141,162 @@ func (s *PrecompileTestSuite) TestRun() { }) } } + +// runPrecompile executes the gov precompile with the given calldata, mirroring +// what the EVM does for a delivered transaction: the state DB is committed after +// the call so that every store write performed by the precompile is flushed to +// the underlying context. +func (s *PrecompileTestSuite) runPrecompile(ctx sdk.Context, input []byte, readOnly bool) []byte { + baseFee := s.network.App.GetEVMKeeper().GetBaseFee(ctx) + + contract := vm.NewPrecompile(s.keyring.GetAddr(0), s.precompile.Address(), uint256.NewInt(0), uint64(1e6)) + contract.Input = input + + contractAddr := contract.Address() + // Build and sign the Ethereum transaction backing the call. + txArgs := evmtypes.EvmTxArgs{ + ChainID: evmtypes.GetEthChainConfig().ChainID, + Nonce: 0, + To: &contractAddr, + Amount: nil, + GasLimit: 100000, + GasPrice: testutil.ExampleMinGasPrices, + GasFeeCap: baseFee, + GasTipCap: big.NewInt(1), + Accesses: ðtypes.AccessList{}, + } + msg, err := s.factory.GenerateGethCoreMsg(s.keyring.GetPrivKey(0), txArgs) + s.Require().NoError(err) + + cfg, err := s.network.App.GetEVMKeeper().EVMConfig(ctx, ctx.BlockHeader().ProposerAddress) + s.Require().NoError(err, "failed to instantiate EVM config") + + stDB := statedb.New(ctx, s.network.App.GetEVMKeeper(), statedb.NewEmptyTxConfig()) + evm := s.network.App.GetEVMKeeper().NewEVM(ctx, *msg, cfg, nil, stDB) + + precompiles, found, err := s.network.App.GetEVMKeeper().GetPrecompileInstance(ctx, contractAddr) + s.Require().NoError(err, "failed to instantiate precompile") + s.Require().True(found, "not found precompile") + evm.WithPrecompiles(precompiles.Map) + + bz, err := s.precompile.Run(evm, contract, readOnly) + s.Require().NoError(err, "expected no error when running the precompile") + s.Require().NotNil(bz, "expected returned bytes not to be nil") + + // A delivered tx always commits the state DB, which flushes the precompile's + // cache context onto the underlying multi store. + s.Require().NoError(stDB.Commit(), "failed to commit the state DB") + + return bz +} + +// proposalVoters returns the voters that currently have a vote stored for the +// given proposal. +func (s *PrecompileTestSuite) proposalVoters(ctx sdk.Context, proposalID uint64) []string { + govKeeper := s.network.App.GetGovKeeper() + + voters := []string{} + err := govKeeper.Votes.Walk( + ctx, + collections.NewPrefixedPairRange[uint64, sdk.AccAddress](proposalID), + func(_ collections.Pair[uint64, sdk.AccAddress], vote govv1.Vote) (bool, error) { + voters = append(voters, vote.Voter) + return false, nil + }, + ) + s.Require().NoError(err, "failed to walk the proposal votes") + + return voters +} + +// TestGetTallyResultDoesNotDeleteVotes is the regression test for F-2026-18187. +// +// The SDK's gov TallyResult query delegates to Keeper.Tally, which deletes every +// vote it counts. That is safe on the SDK's own query paths because a gRPC query +// runs against a context whose writes are never committed. Precompiles break that +// invariant: cmn.Precompile.runNativeAction commits the state DB cache context +// unconditionally, so before the fix a plain committing EVM call to +// getTallyResult wiped the votes of any proposal in the voting period. +// +// Query classification does not help here: getTallyResult is absent from +// IsTransaction, but that only feeds the staticcall guard and the gas price, so +// both a CALL and a STATICCALL reach the mutating SDK code. Both are covered. +func (s *PrecompileTestSuite) TestGetTallyResultDoesNotDeleteVotes() { + const proposalID uint64 = 1 + + testcases := []struct { + name string + readOnly bool + }{ + { + name: "call", + readOnly: false, + }, + { + name: "staticcall", + readOnly: true, + }, + } + + for _, tc := range testcases { + s.Run(tc.name, func() { + s.SetupTest() + + ctx := s.network.GetContext() + govKeeper := s.network.App.GetGovKeeper() + + // Account 0 is the only delegator of the test network, so it holds all of + // the voting power (3e18). Split its vote across every option so that the + // positive control below exercises the whole tally result. + s.Require().NoError(govKeeper.AddVote(ctx, proposalID, s.keyring.GetAccAddr(0), govv1.WeightedVoteOptions{ + {Option: govv1.OptionYes, Weight: "0.4"}, + {Option: govv1.OptionAbstain, Weight: "0.3"}, + {Option: govv1.OptionNo, Weight: "0.2"}, + {Option: govv1.OptionNoWithVeto, Weight: "0.1"}, + }, "")) + // Accounts 1 and 2 hold no delegations: they add nothing to the tally but + // are stored votes all the same, and Keeper.Tally removes them too. + s.Require().NoError(govKeeper.AddVote(ctx, proposalID, s.keyring.GetAccAddr(1), govv1.NewNonSplitVoteOption(govv1.OptionNo), "")) + s.Require().NoError(govKeeper.AddVote(ctx, proposalID, s.keyring.GetAccAddr(2), govv1.NewNonSplitVoteOption(govv1.OptionYes), "")) + + expVoters := []string{ + s.keyring.GetAccAddr(0).String(), + s.keyring.GetAccAddr(1).String(), + s.keyring.GetAccAddr(2).String(), + } + s.Require().ElementsMatch(expVoters, s.proposalVoters(ctx, proposalID), "expected the votes to be stored before the query") + + proposal, err := govKeeper.Proposals.Get(ctx, proposalID) + s.Require().NoError(err) + s.Require().Equal(govv1.StatusVotingPeriod, proposal.Status, "the proposal must be in the voting period for the tally to mutate state") + + input, err := s.precompile.Pack(gov.GetTallyResultMethod, proposalID) + s.Require().NoError(err, "failed to pack input") + + bz := s.runPrecompile(ctx, input, tc.readOnly) + + // Regression assertion: the query must leave every vote in place. + s.Require().ElementsMatch(expVoters, s.proposalVoters(ctx, proposalID), "getTallyResult must not delete the proposal's votes") + + // Positive control: the tally must still be the one the SDK computes, so + // we do not trade a mutating query for a wrong one. + expTally := gov.TallyResultData{ + Yes: "1200000000000000000", // 40% of 3e18 + Abstain: "900000000000000000", // 30% of 3e18 + No: "600000000000000000", // 20% of 3e18 + NoWithVeto: "300000000000000000", // 10% of 3e18 + } + + var out gov.TallyResultOutput + s.Require().NoError(s.precompile.UnpackIntoInterface(&out, gov.GetTallyResultMethod, bz)) + s.Require().Equal(expTally, out.TallyResult, "unexpected tally result") + + // And the query must be repeatable: a second call sees the same votes and + // therefore returns the same numbers. + var secondOut gov.TallyResultOutput + s.Require().NoError(s.precompile.UnpackIntoInterface(&secondOut, gov.GetTallyResultMethod, s.runPrecompile(ctx, input, tc.readOnly))) + s.Require().Equal(expTally, secondOut.TallyResult, "the tally result must be stable across calls") + s.Require().ElementsMatch(expVoters, s.proposalVoters(ctx, proposalID), "the votes must survive repeated queries") + }) + } +}