Skip to content

Commit

Permalink
fix(evm): issue with infinite recursion in erc20 funtoken contracts (#…
Browse files Browse the repository at this point in the history
…2129)

* fix(evm): issue with infinite recursion in erc20 funtoken contracts

* fix(evm): issue with infinite recursion in erc20 funtoken contracts

* chore: changelog update

* fix: flooring 1/64 of the gas limit
  • Loading branch information
onikonychev authored Dec 28, 2024
1 parent 3954394 commit 1a256f2
Show file tree
Hide file tree
Showing 7 changed files with 450 additions and 6 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ only on the "bankkeeper.BaseKeeper"'s gas consumption.
Remove unnecessary argument in the `VerifyFee` function, which returns the token
payment required based on the effective fee from the tx data. Improve
documentation.
- [#2129](https://github.com/NibiruChain/nibiru/pull/2129) - fix(evm): issue with infinite recursion in erc20 funtoken contracts

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

Expand Down

Large diffs are not rendered by default.

41 changes: 41 additions & 0 deletions x/evm/embeds/contracts/TestInfiniteRecursionERC20.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;

import "./IFunToken.sol";
import "@openzeppelin/contracts/token/ERC20/ERC20.sol";

contract TestInfiniteRecursionERC20 is ERC20 {
constructor(string memory name, string memory symbol, uint8 decimals_)
ERC20(name, symbol) {
_mint(msg.sender, 1000000 * 10**18);
}

function balanceOf(address who) public view virtual override returns (uint256) {
// recurse through funtoken.balance(who, address(this))
address(FUNTOKEN_PRECOMPILE_ADDRESS).staticcall(
abi.encodeWithSignature(
"balance(address,address)",
who,
address(this))
);
return 0;
}

function transfer(address to, uint256 amount) public override returns (bool) {
// recurse through funtoken sendToBank
FUNTOKEN_PRECOMPILE.sendToBank(
address(this),
amount,
"nibi1zaavvzxez0elundtn32qnk9lkm8kmcsz44g7xl" // does not matter, it's not reached
);
return true;
}

function attackBalance() public {
balanceOf(address(0));
}

function attackTransfer() public {
transfer(address(0), 1);
}
}
9 changes: 9 additions & 0 deletions x/evm/embeds/embeds.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@ var (
testNativeSendThenPrecompileSendJson []byte
//go:embed artifacts/contracts/TestPrecompileSelfCallRevert.sol/TestPrecompileSelfCallRevert.json
testPrecompileSelfCallRevertJson []byte
//go:embed artifacts/contracts/TestInfiniteRecursionERC20.sol/TestInfiniteRecursionERC20.json
testInfiniteRecursionERC20Json []byte
)

var (
Expand Down Expand Up @@ -118,6 +120,12 @@ var (
Name: "TestPrecompileSelfCallRevert.sol",
EmbedJSON: testPrecompileSelfCallRevertJson,
}
// SmartContract_TestInfiniteRecursionERC20 is a test contract
// which simulates malicious ERC20 behavior by adding infinite recursion in transfer() and balanceOf() functions
SmartContract_TestInfiniteRecursionERC20 = CompiledEvmContract{
Name: "TestInfiniteRecursionERC20.sol",
EmbedJSON: testInfiniteRecursionERC20Json,
}
)

func init() {
Expand All @@ -132,6 +140,7 @@ func init() {
SmartContract_TestNativeSendThenPrecompileSendJson.MustLoad()
SmartContract_TestERC20TransferThenPrecompileSend.MustLoad()
SmartContract_TestPrecompileSelfCallRevert.MustLoad()
SmartContract_TestInfiniteRecursionERC20.MustLoad()
}

type CompiledEvmContract struct {
Expand Down
1 change: 1 addition & 0 deletions x/evm/embeds/embeds_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,5 +19,6 @@ func TestLoadContracts(t *testing.T) {
embeds.SmartContract_TestFunTokenPrecompileLocalGas.MustLoad()
embeds.SmartContract_TestNativeSendThenPrecompileSendJson.MustLoad()
embeds.SmartContract_TestERC20TransferThenPrecompileSend.MustLoad()
embeds.SmartContract_TestInfiniteRecursionERC20.MustLoad()
})
}
21 changes: 15 additions & 6 deletions x/evm/keeper/erc20.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package keeper

import (
"fmt"
"math"
"math/big"

"cosmossdk.io/errors"
Expand All @@ -26,6 +27,14 @@ const (
Erc20GasLimitExecute uint64 = 200_000
)

// getCallGas returns the gas limit for a call to an ERC20 contract following 63/64 rule (EIP-150)
// protection against recursive calls ERC20 -> precompile -> ERC20.
func getCallGasWithLimit(ctx sdk.Context, gasLimit uint64) uint64 {
availableGas := ctx.GasMeter().GasRemaining()
callGas := availableGas - uint64(math.Floor(float64(availableGas)/64))
return min(callGas, gasLimit)
}

// ERC20 returns a mutable reference to the keeper with an ERC20 contract ABI and
// Go functions corresponding to contract calls in the ERC20 standard like "mint"
// and "transfer" in the ERC20 standard.
Expand Down Expand Up @@ -60,7 +69,7 @@ func (e erc20Calls) Mint(
contract, from, to gethcommon.Address, amount *big.Int,
ctx sdk.Context,
) (evmResp *evm.MsgEthereumTxResponse, err error) {
return e.CallContract(ctx, e.ABI, from, &contract, true, Erc20GasLimitExecute, "mint", to, amount)
return e.CallContract(ctx, e.ABI, from, &contract, true, getCallGasWithLimit(ctx, Erc20GasLimitExecute), "mint", to, amount)
}

/*
Expand All @@ -82,7 +91,7 @@ func (e erc20Calls) Transfer(
return balanceIncrease, nil, errors.Wrap(err, "failed to retrieve recipient balance")
}

resp, err = e.CallContract(ctx, e.ABI, from, &contract, true, Erc20GasLimitExecute, "transfer", to, amount)
resp, err = e.CallContract(ctx, e.ABI, from, &contract, true, getCallGasWithLimit(ctx, Erc20GasLimitExecute), "transfer", to, amount)
if err != nil {
return balanceIncrease, nil, err
}
Expand Down Expand Up @@ -141,7 +150,7 @@ func (e erc20Calls) Burn(
contract, from gethcommon.Address, amount *big.Int,
ctx sdk.Context,
) (evmResp *evm.MsgEthereumTxResponse, err error) {
return e.CallContract(ctx, e.ABI, from, &contract, true, Erc20GasLimitExecute, "burn", amount)
return e.CallContract(ctx, e.ABI, from, &contract, true, getCallGasWithLimit(ctx, Erc20GasLimitExecute), "burn", amount)
}

func (k Keeper) LoadERC20Name(
Expand Down Expand Up @@ -174,7 +183,7 @@ func (k Keeper) LoadERC20String(
evm.EVM_MODULE_ADDRESS,
&erc20Contract,
false,
Erc20GasLimitQuery,
getCallGasWithLimit(ctx, Erc20GasLimitQuery),
methodName,
)
if err != nil {
Expand Down Expand Up @@ -202,7 +211,7 @@ func (k Keeper) loadERC20Uint8(
evm.EVM_MODULE_ADDRESS,
&erc20Contract,
false,
Erc20GasLimitQuery,
getCallGasWithLimit(ctx, Erc20GasLimitQuery),
methodName,
)
if err != nil {
Expand Down Expand Up @@ -232,7 +241,7 @@ func (k Keeper) LoadERC20BigInt(
evm.EVM_MODULE_ADDRESS,
&contract,
false,
Erc20GasLimitQuery,
getCallGasWithLimit(ctx, Erc20GasLimitQuery),
methodName,
args...,
)
Expand Down
67 changes: 67 additions & 0 deletions x/evm/keeper/funtoken_from_erc20_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -385,6 +385,73 @@ func (s *FunTokenFromErc20Suite) TestFunTokenFromERC20MaliciousTransfer() {
s.Require().ErrorContains(err, "gas required exceeds allowance")
}

// TestFunTokenInfiniteRecursionERC20 creates a funtoken from a contract
// with a malicious recursive balanceOf() and transfer() functions.
func (s *FunTokenFromErc20Suite) TestFunTokenInfiniteRecursionERC20() {
deps := evmtest.NewTestDeps()

s.T().Log("Deploy InfiniteRecursionERC20")
metadata := keeper.ERC20Metadata{
Name: "erc20name",
Symbol: "TOKEN",
Decimals: 18,
}
deployResp, err := evmtest.DeployContract(
&deps, embeds.SmartContract_TestInfiniteRecursionERC20,
metadata.Name, metadata.Symbol, metadata.Decimals,
)
s.Require().NoError(err)

erc20Addr := eth.EIP55Addr{
Address: deployResp.ContractAddr,
}

s.T().Log("happy: CreateFunToken for ERC20 with infinite recursion")
s.Require().NoError(testapp.FundAccount(
deps.App.BankKeeper,
deps.Ctx,
deps.Sender.NibiruAddr,
deps.EvmKeeper.FeeForCreateFunToken(deps.Ctx),
))

_, err = deps.EvmKeeper.CreateFunToken(
sdk.WrapSDKContext(deps.Ctx),
&evm.MsgCreateFunToken{
FromErc20: &erc20Addr,
Sender: deps.Sender.NibiruAddr.String(),
},
)
s.Require().NoError(err)

deps.ResetGasMeter()

s.T().Log("happy: call attackBalance()")
res, err := deps.EvmKeeper.CallContract(
deps.Ctx,
embeds.SmartContract_TestInfiniteRecursionERC20.ABI,
deps.Sender.EthAddr,
&erc20Addr.Address,
false,
10_000_000,
"attackBalance",
)
s.Require().NoError(err)
s.Require().NotNil(res)
s.Require().Empty(res.VmError)

s.T().Log("sad: call attackBalance()")
_, err = deps.EvmKeeper.CallContract(
deps.Ctx,
embeds.SmartContract_TestInfiniteRecursionERC20.ABI,
deps.Sender.EthAddr,
&erc20Addr.Address,
true,
10_000_000,
"attackTransfer",
)
s.Require().ErrorContains(err, "execution reverted")
}

type FunTokenFromErc20Suite struct {
suite.Suite
}
Expand Down

0 comments on commit 1a256f2

Please sign in to comment.