From b7e8dd8216a6f4e83cde3a875172f686ea9dc913 Mon Sep 17 00:00:00 2001
From: yihuang <huang@crypto.com>
Date: Tue, 16 Nov 2021 22:49:59 +0800
Subject: [PATCH] fix: don't revert gas refund logic when transaction reverted
 (#751)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* fix gas consumption when reverted

Apply suggestions from code review

changelog

* comments

* Update x/evm/keeper/state_transition.go

Co-authored-by: Federico Kunze Küllmer <31522760+fedekunze@users.noreply.github.com>
---
 CHANGELOG.md                     |  1 +
 x/evm/handler_test.go            | 82 ++++++++++++++++++++++++++++++++
 x/evm/keeper/state_transition.go | 14 +++---
 3 files changed, 91 insertions(+), 6 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index c28aba27f7..8f860b7c78 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -42,6 +42,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
 * (evm, ante) [tharsis#620](https://github.com/tharsis/ethermint/pull/620) Add fee market field to EVM `Keeper` and `AnteHandler`.
 * (all) [tharsis#231](https://github.com/tharsis/ethermint/pull/231) Bump go-ethereum version to [`v1.10.9`](https://github.com/ethereum/go-ethereum/releases/tag/v1.10.9)
 * (ante) [tharsis#703](https://github.com/tharsis/ethermint/pull/703) Fix some fields in transaction are not authenticated by signature.
+* (evm) [tharsis#751](https://github.com/tharsis/ethermint/pull/751) don't revert gas refund logic when transaction reverted
 
 ### Features
 
diff --git a/x/evm/handler_test.go b/x/evm/handler_test.go
index 5982077037..a7d6264bb9 100644
--- a/x/evm/handler_test.go
+++ b/x/evm/handler_test.go
@@ -525,3 +525,85 @@ func (suite *EvmTestSuite) TestErrorWhenDeployContract() {
 
 	// TODO: snapshot checking
 }
+
+func (suite *EvmTestSuite) deployERC20Contract() common.Address {
+	k := suite.app.EvmKeeper
+	nonce := k.GetNonce(suite.from)
+	ctorArgs, err := types.ERC20Contract.ABI.Pack("", suite.from, big.NewInt(0))
+	suite.Require().NoError(err)
+	msg := ethtypes.NewMessage(
+		suite.from,
+		nil,
+		nonce,
+		big.NewInt(0),
+		2000000,
+		big.NewInt(1),
+		nil,
+		nil,
+		append(types.ERC20Contract.Bin, ctorArgs...),
+		nil,
+		true,
+	)
+	rsp, err := k.ApplyMessage(msg, nil, true)
+	suite.Require().NoError(err)
+	suite.Require().False(rsp.Failed())
+	return crypto.CreateAddress(suite.from, nonce)
+}
+
+// TestGasRefundWhenReverted check that when transaction reverted, gas refund should still work.
+func (suite *EvmTestSuite) TestGasRefundWhenReverted() {
+	suite.SetupTest()
+	k := suite.app.EvmKeeper
+
+	// the bug only reproduce when there are hooks
+	k.SetHooks(&DummyHook{})
+
+	// add some fund to pay gas fee
+	k.AddBalance(suite.from, big.NewInt(10000000000))
+
+	contract := suite.deployERC20Contract()
+
+	// the call will fail because no balance
+	data, err := types.ERC20Contract.ABI.Pack("transfer", common.BigToAddress(big.NewInt(1)), big.NewInt(10))
+	suite.Require().NoError(err)
+
+	tx := types.NewTx(
+		suite.chainID,
+		k.GetNonce(suite.from),
+		&contract,
+		big.NewInt(0),
+		41000,
+		big.NewInt(1),
+		nil,
+		nil,
+		data,
+		nil,
+	)
+	tx.From = suite.from.String()
+	err = tx.Sign(suite.ethSigner, suite.signer)
+	suite.Require().NoError(err)
+
+	before := k.GetBalance(suite.from)
+
+	txData, err := types.UnpackTxData(tx.Data)
+	suite.Require().NoError(err)
+	_, err = k.DeductTxCostsFromUserBalance(suite.ctx, *tx, txData, "aphoton", true, true, true)
+	suite.Require().NoError(err)
+
+	res, err := k.EthereumTx(sdk.WrapSDKContext(suite.ctx), tx)
+	suite.Require().NoError(err)
+	suite.Require().True(res.Failed())
+
+	after := k.GetBalance(suite.from)
+
+	suite.Require().Equal(uint64(21861), res.GasUsed)
+	// check gas refund works
+	suite.Require().Equal(big.NewInt(21861), new(big.Int).Sub(before, after))
+}
+
+// DummyHook implements EvmHooks interface
+type DummyHook struct{}
+
+func (dh *DummyHook) PostTxProcessing(ctx sdk.Context, txHash common.Hash, logs []*ethtypes.Log) error {
+	return nil
+}
diff --git a/x/evm/keeper/state_transition.go b/x/evm/keeper/state_transition.go
index cd05a1d9d4..1b3b0d20c1 100644
--- a/x/evm/keeper/state_transition.go
+++ b/x/evm/keeper/state_transition.go
@@ -218,12 +218,6 @@ func (k *Keeper) ApplyTransaction(tx *ethtypes.Transaction) (*types.MsgEthereumT
 		return nil, stacktrace.Propagate(err, "failed to apply ethereum core message")
 	}
 
-	// refund gas prior to handling the vm error in order to match the Ethereum gas consumption instead of the default SDK one.
-	err = k.RefundGas(msg, msg.Gas()-res.GasUsed, cfg.Params.EvmDenom)
-	if err != nil {
-		return nil, stacktrace.Propagate(err, "failed to refund gas leftover gas to sender %s", msg.From())
-	}
-
 	res.Hash = txHash.Hex()
 
 	logs := k.GetTxLogsTransient(txHash)
@@ -241,6 +235,14 @@ func (k *Keeper) ApplyTransaction(tx *ethtypes.Transaction) (*types.MsgEthereumT
 		}
 	}
 
+	// change to original context
+	k.WithContext(ctx)
+
+	// refund gas according to Ethereum gas accounting rules.
+	if err := k.RefundGas(msg, msg.Gas()-res.GasUsed, cfg.Params.EvmDenom); err != nil {
+		return nil, stacktrace.Propagate(err, "failed to refund gas leftover gas to sender %s", msg.From())
+	}
+
 	if len(logs) > 0 {
 		res.Logs = types.NewLogsFromEth(logs)
 		// Update transient block bloom filter