Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(evm): consume gas in CallContractWithInput #2180

Merged
merged 10 commits into from
Jan 28, 2025
Merged

fix(evm): consume gas in CallContractWithInput #2180

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
efcfde1
fix(evm): consume gas in CallContractWithInput
k-yang Jan 27, 2025
1cb2068
Update CHANGELOG.md
k-yang Jan 27, 2025
9947330
fix: gas consumption in ApplyEvmMsg
k-yang Jan 27, 2025
53c1500
fix: move commit after gas refund calculation so that we can return a…
k-yang Jan 27, 2025
be10743
refactor: GasToRefund
k-yang Jan 27, 2025
60f3b5c
fix: proper checking of err pointer nil
onikonychev Jan 28, 2025
7387a40
fix: remove fullRefundLeftoverGas
k-yang Jan 28, 2025
4850501
test: add gas assertions
k-yang Jan 28, 2025
52f3293
Merge branch 'main' into fix/evm/consume-evm-gas
k-yang Jan 28, 2025
a55e5b2
Update gas_fees.go
k-yang Jan 28, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ needed to include double quotes around the hexadecimal string.
- [#2173](https://github.com/NibiruChain/nibiru/pull/2173) - fix(evm): clear `StateDB` between calls
- [#2177](https://github.com/NibiruChain/nibiru/pull/2177) - fix(cmd): Continue from #2127 and unwire vesting flags and logic from genaccounts.go
- [#2176](https://github.com/NibiruChain/nibiru/pull/2176) - tests(evm): add dirty state tests from code4rena audit
- [#2180](https://github.com/NibiruChain/nibiru/pull/2180) - fix(evm): apply gas consumption across the entire EVM codebase at `CallContractWithInput`

#### Nibiru EVM | Before Audit 2 - 2024-12-06

Expand Down
10 changes: 6 additions & 4 deletions x/evm/keeper/call_contract.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ func (k Keeper) CallContractWithInput(
) (evmResp *evm.MsgEthereumTxResponse, err error) {
// This is a `defer` pattern to add behavior that runs in the case that the
// error is non-nil, creating a concise way to add extra information.
defer HandleOutOfGasPanic(&err, "CallContractError")
defer HandleOutOfGasPanic(&err, "CallContractError")()
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing parentheses was causing issues with catching OOG errors lol

nonce := k.GetAccNonce(ctx, fromAcc)

unusedBigInt := big.NewInt(0)
Expand All @@ -61,11 +61,13 @@ func (k Keeper) CallContractWithInput(
// sent by a user
txConfig := k.TxConfig(ctx, gethcommon.BigToHash(big.NewInt(0)))
evmResp, err = k.ApplyEvmMsg(
ctx, evmMsg, evmObj, evm.NewNoOpTracer(), commit, txConfig.TxHash, true,
ctx, evmMsg, evmObj, evm.NewNoOpTracer(), commit, txConfig.TxHash,
)
if evmResp != nil {
ctx.GasMeter().ConsumeGas(evmResp.GasUsed, "CallContractWithInput")
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should always consume gas after ApplyEvmMsg(), before returning away from CallContractWithInput(), or else we lose the opportunity to apply the EvmResp.GasUsed variable.

}
if err != nil {
err = errors.Wrap(err, "failed to apply ethereum core message")
return
return nil, errors.Wrap(err, "failed to apply ethereum core message")
}

if evmResp.Failed() {
Expand Down
12 changes: 6 additions & 6 deletions x/evm/keeper/erc20.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,7 +196,7 @@ func (e erc20Calls) loadERC20String(
if err != nil {
return out, err
}
res, err := e.Keeper.CallContractWithInput(
evmResp, err := e.Keeper.CallContractWithInput(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just a minor rename

ctx,
evmObj,
evm.EVM_MODULE_ADDRESS,
Expand All @@ -211,13 +211,13 @@ func (e erc20Calls) loadERC20String(

erc20Val := new(ERC20String)
if err := erc20Abi.UnpackIntoInterface(
erc20Val, methodName, res.Ret,
erc20Val, methodName, evmResp.Ret,
); err == nil {
return erc20Val.Value, err
}

erc20Bytes32Val := new(ERC20Bytes32)
if err := erc20Abi.UnpackIntoInterface(erc20Bytes32Val, methodName, res.Ret); err == nil {
if err := erc20Abi.UnpackIntoInterface(erc20Bytes32Val, methodName, evmResp.Ret); err == nil {
return bytes32ToString(erc20Bytes32Val.Value), nil
}

Expand All @@ -239,7 +239,7 @@ func (e erc20Calls) loadERC20Uint8(
if err != nil {
return out, err
}
res, err := e.Keeper.CallContractWithInput(
evmResp, err := e.Keeper.CallContractWithInput(
ctx,
evmObj,
evm.EVM_MODULE_ADDRESS,
Expand All @@ -254,14 +254,14 @@ func (e erc20Calls) loadERC20Uint8(

erc20Val := new(ERC20Uint8)
if err := erc20Abi.UnpackIntoInterface(
erc20Val, methodName, res.Ret,
erc20Val, methodName, evmResp.Ret,
); err == nil {
return erc20Val.Value, err
}

erc20Uint256Val := new(ERC20BigInt)
if err := erc20Abi.UnpackIntoInterface(
erc20Uint256Val, methodName, res.Ret,
erc20Uint256Val, methodName, evmResp.Ret,
); err == nil {
// We can safely cast to uint8 because it's nonsense for decimals to be larger than 255
return uint8(erc20Uint256Val.Value.Uint64()), err
Expand Down
4 changes: 1 addition & 3 deletions x/evm/keeper/funtoken_from_coin.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ func (k *Keeper) deployERC20ForBankCoin(
k.Bank.StateDB = nil
}()
evmObj := k.NewEVM(ctx, evmMsg, evmCfg, nil /*tracer*/, stateDB)
evmResp, err := k.CallContractWithInput(
_, err = k.CallContractWithInput(
ctx, evmObj, evm.EVM_MODULE_ADDRESS, nil, true /*commit*/, input, Erc20GasLimitDeploy,
)
if err != nil {
Expand All @@ -114,7 +114,5 @@ func (k *Keeper) deployERC20ForBankCoin(
return gethcommon.Address{}, errors.Wrap(err, "failed to commit stateDB")
}

ctx.GasMeter().ConsumeGas(evmResp.GasUsed, "deploy erc20 funtoken contract")
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We no longer need to consume gas here since we consume it at a lower level in CallContractWithInput().


return erc20Addr, nil
}
34 changes: 31 additions & 3 deletions x/evm/keeper/funtoken_from_coin_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ func (s *FunTokenFromCoinSuite) TestCreateFunTokenFromCoin() {
})

s.Run("happy: CreateFunToken for the bank coin", func() {
deps.Ctx = deps.Ctx.WithGasMeter(sdk.NewInfiniteGasMeter())
s.Require().NoError(testapp.FundAccount(
deps.App.BankKeeper,
deps.Ctx,
Expand All @@ -97,6 +98,7 @@ func (s *FunTokenFromCoinSuite) TestCreateFunTokenFromCoin() {
},
)
s.Require().NoError(err)
s.Require().NotZero(deps.Ctx.GasMeter().GasConsumed())

s.Equal(
createFuntokenResp.FuntokenMapping,
Expand Down Expand Up @@ -167,6 +169,7 @@ func (s *FunTokenFromCoinSuite) TestConvertCoinToEvmAndBack() {
funToken := s.fundAndCreateFunToken(deps, 100)

s.T().Log("Convert bank coin to erc-20")
deps.Ctx = deps.Ctx.WithGasMeter(sdk.NewInfiniteGasMeter())
_, err := deps.EvmKeeper.ConvertCoinToEvm(
sdk.WrapSDKContext(deps.Ctx),
&evm.MsgConvertCoinToEvm{
Expand All @@ -178,6 +181,7 @@ func (s *FunTokenFromCoinSuite) TestConvertCoinToEvmAndBack() {
},
)
s.Require().NoError(err)
s.Require().NotZero(deps.Ctx.GasMeter().GasConsumed())

s.T().Log("Check typed event")
testutil.RequireContainsTypedEvent(
Expand Down Expand Up @@ -226,6 +230,7 @@ func (s *FunTokenFromCoinSuite) TestConvertCoinToEvmAndBack() {
deps.Sender.NibiruAddr.String(),
)
s.Require().NoError(err)
deps.Ctx = deps.Ctx.WithGasMeter(sdk.NewInfiniteGasMeter())
evmObj, _ = deps.NewEVM()
_, err = deps.EvmKeeper.CallContractWithInput(
deps.Ctx,
Expand All @@ -237,6 +242,7 @@ func (s *FunTokenFromCoinSuite) TestConvertCoinToEvmAndBack() {
evmtest.FunTokenGasLimitSendToEvm,
)
s.Require().NoError(err)
s.Require().NotZero(deps.Ctx.GasMeter().GasConsumed())

// Check 1: module balance
moduleBalance = deps.App.BankKeeper.GetBalance(deps.Ctx, authtypes.NewModuleAddress(evm.ModuleName), evm.EVMBankDenom)
Expand All @@ -252,6 +258,7 @@ func (s *FunTokenFromCoinSuite) TestConvertCoinToEvmAndBack() {
s.Require().Equal("0", balance.String())

s.T().Log("sad: Convert more erc-20 to back to bank coin, insufficient funds")
deps.Ctx = deps.Ctx.WithGasMeter(sdk.NewInfiniteGasMeter())
evmObj, _ = deps.NewEVM()
_, err = deps.EvmKeeper.CallContractWithInput(
deps.Ctx,
Expand All @@ -263,6 +270,7 @@ func (s *FunTokenFromCoinSuite) TestConvertCoinToEvmAndBack() {
evmtest.FunTokenGasLimitSendToEvm,
)
s.Require().ErrorContains(err, "transfer amount exceeds balance")
s.Require().NotZero(deps.Ctx.GasMeter().GasConsumed())
}

// TestNativeSendThenPrecompileSend tests a race condition where the state DB
Expand Down Expand Up @@ -362,6 +370,7 @@ func (s *FunTokenFromCoinSuite) TestNativeSendThenPrecompileSend() {
newSendAmtSendToBank, /*amount*/
)
s.Require().NoError(err)
deps.Ctx = deps.Ctx.WithGasMeter(sdk.NewInfiniteGasMeter())
evmObj, _ = deps.NewEVM()
evmResp, err := deps.EvmKeeper.CallContractWithInput(
deps.Ctx,
Expand All @@ -373,6 +382,9 @@ func (s *FunTokenFromCoinSuite) TestNativeSendThenPrecompileSend() {
evmtest.FunTokenGasLimitSendToEvm,
)
s.Require().NoError(err)
s.Require().NotZero(deps.Ctx.GasMeter().GasConsumed())
s.Require().NotZero(evmResp.GasUsed)
s.Require().Greaterf(deps.Ctx.GasMeter().GasConsumed(), evmResp.GasUsed, "total gas consumed on cosmos context should be greater than gas used by EVM")
s.Empty(evmResp.VmError)
gasUsedFor2Ops := evmResp.GasUsed

Expand Down Expand Up @@ -404,6 +416,7 @@ func (s *FunTokenFromCoinSuite) TestNativeSendThenPrecompileSend() {
newSendAmtSendToBank, /*amount*/
)
s.Require().NoError(err)
deps.Ctx = deps.Ctx.WithGasMeter(sdk.NewInfiniteGasMeter())
evmObj, _ = deps.NewEVM()
evmResp, err = deps.EvmKeeper.CallContractWithInput(
deps.Ctx,
Expand All @@ -415,6 +428,9 @@ func (s *FunTokenFromCoinSuite) TestNativeSendThenPrecompileSend() {
evmtest.DefaultEthCallGasLimit,
)
s.Require().NoError(err)
s.Require().NotZero(deps.Ctx.GasMeter().GasConsumed())
s.Require().NotZero(evmResp.GasUsed)
s.Require().Greaterf(deps.Ctx.GasMeter().GasConsumed(), evmResp.GasUsed, "total gas consumed on cosmos context should be greater than gas used by EVM")
s.Empty(evmResp.VmError)
gasUsedFor1Op := evmResp.GasUsed

Expand Down Expand Up @@ -517,8 +533,9 @@ func (s *FunTokenFromCoinSuite) TestERC20TransferThenPrecompileSend() {
big.NewInt(9e6), /*amount*/
)
s.Require().NoError(err)
deps.Ctx = deps.Ctx.WithGasMeter(sdk.NewInfiniteGasMeter())
evmObj, _ = deps.NewEVM()
_, err = deps.EvmKeeper.CallContractWithInput(
evmResp, err := deps.EvmKeeper.CallContractWithInput(
deps.Ctx,
evmObj,
deps.Sender.EthAddr, // from
Expand All @@ -528,6 +545,9 @@ func (s *FunTokenFromCoinSuite) TestERC20TransferThenPrecompileSend() {
10_000_000, // gas limit
)
s.Require().NoError(err)
s.Require().NotZero(deps.Ctx.GasMeter().GasConsumed())
s.Require().NotZero(evmResp.GasUsed)
s.Require().Greaterf(deps.Ctx.GasMeter().GasConsumed(), evmResp.GasUsed, "total gas consumed on cosmos context should be greater than gas used by EVM")

evmtest.FunTokenBalanceAssert{
FunToken: funToken,
Expand Down Expand Up @@ -620,6 +640,7 @@ func (s *FunTokenFromCoinSuite) TestPrecompileSelfCallRevert() {
charles := evmtest.NewEthPrivAcc()

s.T().Log("call test contract")
deps.Ctx = deps.Ctx.WithGasMeter(sdk.NewInfiniteGasMeter())
evmObj, _ = deps.NewEVM()
contractInput, err := embeds.SmartContract_TestPrecompileSelfCallRevert.ABI.Pack(
"selfCallTransferFunds",
Expand All @@ -629,7 +650,7 @@ func (s *FunTokenFromCoinSuite) TestPrecompileSelfCallRevert() {
big.NewInt(9e6),
)
s.Require().NoError(err)
_, err = deps.EvmKeeper.CallContractWithInput(
evpResp, err := deps.EvmKeeper.CallContractWithInput(
deps.Ctx,
evmObj,
deps.Sender.EthAddr,
Expand All @@ -639,6 +660,9 @@ func (s *FunTokenFromCoinSuite) TestPrecompileSelfCallRevert() {
evmtest.FunTokenGasLimitSendToEvm,
)
s.Require().NoError(err)
s.Require().NotZero(deps.Ctx.GasMeter().GasConsumed())
s.Require().NotZero(evpResp.GasUsed)
s.Require().Greaterf(deps.Ctx.GasMeter().GasConsumed(), evpResp.GasUsed, "total gas consumed on cosmos context should be greater than gas used by EVM")

evmtest.FunTokenBalanceAssert{
FunToken: funToken,
Expand Down Expand Up @@ -726,8 +750,9 @@ func (s *FunTokenFromCoinSuite) TestPrecompileSendToBankThenErc20Transfer() {
"attack",
)
s.Require().NoError(err)
deps.Ctx = deps.Ctx.WithGasMeter(sdk.NewInfiniteGasMeter())
evmObj, _ := deps.NewEVM()
_, err = deps.EvmKeeper.CallContractWithInput(
evpResp, err := deps.EvmKeeper.CallContractWithInput(
deps.Ctx,
evmObj,
deps.Sender.EthAddr,
Expand All @@ -737,6 +762,9 @@ func (s *FunTokenFromCoinSuite) TestPrecompileSendToBankThenErc20Transfer() {
evmtest.FunTokenGasLimitSendToEvm,
)
s.Require().ErrorContains(err, "execution reverted")
s.Require().NotZero(deps.Ctx.GasMeter().GasConsumed())
s.Require().NotZero(evpResp.GasUsed)
s.Require().Greaterf(deps.Ctx.GasMeter().GasConsumed(), evpResp.GasUsed, "total gas consumed on cosmos context should be greater than gas used by EVM")

evmtest.FunTokenBalanceAssert{
FunToken: funToken,
Expand Down
Loading
Loading