diff --git a/CHANGELOG.md b/CHANGELOG.md index 90d8e19..a70e545 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,7 @@ Follow the [migration document](docs/migrations/v0.5.x_to_v0.6.0.md) for upgrade ### BUG FIXES +- Fix EVM fee-abstraction gas refund to compute the refund against the full transaction gas (`gasUsed + leftoverGas`) instead of `gasUsed`, bounding the refund by the paid fee and preventing the fee collector from being drained by high gas limit transactions. - [\#15](https://github.com/KiiChain/evm/pull/15) Fix distribution precompile 32-byte withdraw address inflating native supply by skipping non-20-byte accounts when mirroring balance changes to the StateDB. ## v0.5.1 diff --git a/tests/integration/x/vm/test_gas.go b/tests/integration/x/vm/test_gas.go index a3e5636..4f45c1b 100644 --- a/tests/integration/x/vm/test_gas.go +++ b/tests/integration/x/vm/test_gas.go @@ -193,7 +193,7 @@ func (suite *KeeperTestSuite) TestGasRefundGas() { ctx, *coreMsg, tc.leftoverGas, - DefaultCoreMsgGasUsage, + DefaultCoreMsgGasUsage-tc.leftoverGas, baseDenom, ) @@ -214,3 +214,66 @@ func (suite *KeeperTestSuite) TestGasRefundGas() { }) } } + +// TestGasRefundGasNoOverRefund guards against the fee_collector drain where the fee-abstraction +// refund used the consumed gas instead of the gas limit as the denominator. With a gas limit far +// greater than the gas used, the refund must never exceed the value of the unused gas. +func (suite *KeeperTestSuite) TestGasRefundGasNoOverRefund() { + baseDenom := types.GetEVMCoinDenom() + + feeAddress := authtypes.NewModuleAddress(authtypes.FeeCollectorName) + balances := []banktypes.Balance{ + { + Address: feeAddress.String(), + Coins: sdk.NewCoins(sdk.NewCoin(baseDenom, sdkmath.NewInt(6e18))), + }, + } + bankGenesis := banktypes.DefaultGenesisState() + bankGenesis.Balances = balances + customGenesis := network.CustomGenesisState{} + customGenesis[banktypes.ModuleName] = bankGenesis + + keyring := testkeyring.New(2) + unitNetwork := network.NewUnitTestNetwork( + suite.Create, + network.WithPreFundedAccounts(keyring.GetAllAccAddrs()...), + network.WithCustomGenesis(customGenesis), + ) + grpcHandler := grpc.NewIntegrationHandler(unitNetwork) + txFactory := factory.New(unitNetwork, grpcHandler) + + sender := keyring.GetKey(0) + recipient := keyring.GetAddr(1) + + const gasLimit = uint64(100000) + const gasUsed = uint64(21000) + const leftoverGas = gasLimit - gasUsed + + coreMsg, err := txFactory.GenerateGethCoreMsg( + sender.Priv, + types.EvmTxArgs{ + To: &recipient, + Amount: big.NewInt(100), + GasPrice: big.NewInt(DefaultGasPrice), + GasLimit: gasLimit, + }, + ) + suite.Require().NoError(err) + + ctx, _ := unitNetwork.GetContext().CacheContext() + + paidFee := sdkmath.NewInt(int64(gasLimit) * DefaultGasPrice) + ctx = ctx.WithValue(keeper.ContextPaidFeesKey{}, sdk.NewCoins(sdk.NewCoin(baseDenom, paidFee))) + + initialFeeCollector := unitNetwork.App.GetBankKeeper().GetBalance(ctx, feeAddress, baseDenom).Amount + + err = unitNetwork.App.GetEVMKeeper().RefundGas(ctx, *coreMsg, leftoverGas, gasUsed, baseDenom) + suite.Require().NoError(err) + + finalFeeCollector := unitNetwork.App.GetBankKeeper().GetBalance(ctx, feeAddress, baseDenom).Amount + refunded := initialFeeCollector.Sub(finalFeeCollector) + + expectedRefund := sdkmath.NewInt(int64(leftoverGas) * DefaultGasPrice) + suite.Require().Equal(expectedRefund, refunded) + suite.Require().True(refunded.LTE(paidFee)) +} diff --git a/x/vm/keeper/gas.go b/x/vm/keeper/gas.go index c7bdd5f..50ef0d7 100644 --- a/x/vm/keeper/gas.go +++ b/x/vm/keeper/gas.go @@ -31,11 +31,10 @@ func (k *Keeper) GetEthIntrinsicGas(ctx sdk.Context, msg core.Message, cfg *para homestead, istanbul, shanghai) } -// RefundGas transfers the leftover gas to the sender of the message, capped to half of the total gas -// consumed in the transaction. Additionally, the function sets the total gas consumed to the value -// returned by the EVM execution, thus ignoring the previous intrinsic gas consumed during in the -// AnteHandler. -func (k *Keeper) RefundGas(ctx sdk.Context, msg core.Message, leftoverGas uint64, gasUsed uint64, denom string) error { +// RefundGas transfers the value of the leftover gas to the sender of the message. The refund is +// bounded by the paid fee: it is computed against the full transaction gas (gasUsed + leftoverGas), +// so the sender can never receive more than what was charged upfront. +func (k *Keeper) RefundGas(ctx sdk.Context, msg core.Message, leftoverGas, gasUsed uint64, denom string) error { // Return EVM tokens for remaining gas, exchanged at the original rate. remaining := new(big.Int).Mul(new(big.Int).SetUint64(leftoverGas), msg.GasPrice) @@ -67,12 +66,9 @@ func (k *Keeper) RefundGas(ctx sdk.Context, msg core.Message, leftoverGas uint64 denom = paidCoin.Denom amount := paidCoin.Amount.BigInt() - // Calculate the amount to refund - // This is calculated as: - // remaining = amount * leftoverGas / gasUsed remaining = new(big.Int).Div( new(big.Int).Mul(amount, new(big.Int).SetUint64(leftoverGas)), - new(big.Int).SetUint64(gasUsed), + new(big.Int).SetUint64(msg.GasLimit), ) } }