fix: add fee field manipulation check (#98)

* add fee field manipulation check * add codcov ignore * add fee manipulation checker
initia-labs · Nov 4, 2024 · f0b8db3 · f0b8db3
1 parent 126224b
commit f0b8db3
Show file tree

Hide file tree

Showing 4 changed files with 40 additions and 10 deletions.
diff --git a/.codecov.yml b/.codecov.yml
@@ -35,3 +35,4 @@ ignore:
   - "**/test_utils.go"
   - "**/module.go"
   - "x/ibc/testing"
+  - "**/contracts"
diff --git a/x/evm/keeper/txutils.go b/x/evm/keeper/txutils.go
@@ -215,7 +215,7 @@ func (u *TxUtils) ConvertCosmosTxToEthereumTx(ctx context.Context, sdkTx sdk.Tx)
 		return nil, nil, nil
 	}
 
-	fees := authTx.GetFee()
+	feeAmount := authTx.GetFee()
 	params, err := u.Params.Get(ctx)
 	if err != nil {
 		return nil, nil, err
@@ -225,7 +225,7 @@ func (u *TxUtils) ConvertCosmosTxToEthereumTx(ctx context.Context, sdkTx sdk.Tx)
 		return nil, nil, err
 	}
 
-	if !(len(fees) == 0 || (len(fees) == 1 && fees[0].Denom == params.FeeDenom)) {
+	if !(len(feeAmount) == 0 || (len(feeAmount) == 1 && feeAmount[0].Denom == params.FeeDenom)) {
 		return nil, nil, nil
 	}
 
@@ -258,7 +258,7 @@ func (u *TxUtils) ConvertCosmosTxToEthereumTx(ctx context.Context, sdkTx sdk.Tx)
 	} else if len(sigData.Signature) == 64 {
 		v, r, s = []byte{}, sigData.Signature[:32], sigData.Signature[32:64]
 	} else {
-		return nil, nil, nil
+		return nil, nil, types.ErrTxConversionFailed.Wrap("invalid signature length")
 	}
 
 	gasFeeCap, ok := new(big.Int).SetString(md.GasFeeCap, 10)
@@ -271,6 +271,13 @@ func (u *TxUtils) ConvertCosmosTxToEthereumTx(ctx context.Context, sdkTx sdk.Tx)
 		return nil, nil, err
 	}
 
+	// check if the fee amount is correctly converted
+	computedFeeAmount := sdk.NewCoins(sdk.NewCoin(params.FeeDenom, math.NewIntFromBigInt(computeGasFeeAmount(gasFeeCap, gas, decimals))))
+	if !feeAmount.Equal(computedFeeAmount) {
+		u.Logger(ctx).Error("fee amount manipulation detected", "expected", computedFeeAmount, "actual", feeAmount)
+		return nil, nil, types.ErrTxConversionFailed.Wrap("fee amount manipulation detected")
+	}
+
 	var to *common.Address
 	var input []byte
 	var value *big.Int
@@ -397,13 +404,13 @@ func (u *TxUtils) IsEthereumTx(ctx context.Context, sdkTx sdk.Tx) (bool, error)
 		return false, nil
 	}
 
-	fees := authTx.GetFee()
+	feeAmount := authTx.GetFee()
 	params, err := u.Params.Get(ctx)
 	if err != nil {
 		return false, err
 	}
 
-	if !(len(fees) == 0 || (len(fees) == 1 && fees[0].Denom == params.FeeDenom)) {
+	if !(len(feeAmount) == 0 || (len(feeAmount) == 1 && feeAmount[0].Denom == params.FeeDenom)) {
 		return false, nil
 	}
 

diff --git a/x/evm/keeper/txutils_test.go b/x/evm/keeper/txutils_test.go
@@ -10,11 +10,11 @@ import (
 	"github.com/stretchr/testify/require"
 
 	"cosmossdk.io/math"
-	authtx "github.com/cosmos/cosmos-sdk/x/auth/tx"
-
+	"github.com/cosmos/cosmos-sdk/client"
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	"github.com/cosmos/cosmos-sdk/types/tx/signing"
 	authsigning "github.com/cosmos/cosmos-sdk/x/auth/signing"
+	authtx "github.com/cosmos/cosmos-sdk/x/auth/tx"
 	"github.com/ethereum/go-ethereum/common"
 	"github.com/ethereum/go-ethereum/common/hexutil"
 	coretypes "github.com/ethereum/go-ethereum/core/types"
@@ -112,7 +112,8 @@ func Test_DynamicFeeTxConversion(t *testing.T) {
 	})
 
 	authTx := sdkTx.(authsigning.Tx)
-	require.Equal(t, authTx.GetFee(), sdk.NewCoins(sdk.NewCoin(sdk.DefaultBondDenom, math.NewIntFromBigInt(feeAmount).AddRaw(1))))
+	expectedFeeAmount := sdk.NewCoins(sdk.NewCoin(sdk.DefaultBondDenom, math.NewIntFromBigInt(feeAmount).AddRaw(1)))
+	require.Equal(t, authTx.GetFee(), expectedFeeAmount)
 
 	sigs, err := authTx.GetSignaturesV2()
 	require.NoError(t, err)
@@ -137,6 +138,12 @@ func Test_DynamicFeeTxConversion(t *testing.T) {
 	ethTx2, _, err := keeper.NewTxUtils(&input.EVMKeeper).ConvertCosmosTxToEthereumTx(ctx, sdkTx)
 	require.NoError(t, err)
 	EqualEthTransaction(t, signedTx, ethTx2)
+
+	// manipulate the fee amount
+	txBuilder := sdkTx.(client.TxBuilder)
+	txBuilder.SetFeeAmount(expectedFeeAmount.Add(sdk.NewCoin(sdk.DefaultBondDenom, math.NewInt(1))))
+	_, _, err = keeper.NewTxUtils(&input.EVMKeeper).ConvertCosmosTxToEthereumTx(ctx, txBuilder.GetTx())
+	require.ErrorIs(t, err, types.ErrTxConversionFailed)
 }
 
 func Test_AccessTxConversion(t *testing.T) {
@@ -298,7 +305,8 @@ func Test_AccessTxConversion(t *testing.T) {
 	})
 
 	authTx = sdkTx.(authsigning.Tx)
-	require.Equal(t, authTx.GetFee(), sdk.NewCoins(sdk.NewCoin(sdk.DefaultBondDenom, math.NewIntFromBigInt(feeAmount).AddRaw(1))))
+	expectedFeeAmount := sdk.NewCoins(sdk.NewCoin(sdk.DefaultBondDenom, math.NewIntFromBigInt(feeAmount).AddRaw(1)))
+	require.Equal(t, authTx.GetFee(), expectedFeeAmount)
 
 	sigs, err = authTx.GetSignaturesV2()
 	require.NoError(t, err)
@@ -323,6 +331,12 @@ func Test_AccessTxConversion(t *testing.T) {
 	ethTx2, _, err = keeper.NewTxUtils(&input.EVMKeeper).ConvertCosmosTxToEthereumTx(ctx, sdkTx)
 	require.NoError(t, err)
 	EqualEthTransaction(t, signedTx, ethTx2)
+
+	// manipulate the fee amount
+	txBuilder := sdkTx.(client.TxBuilder)
+	txBuilder.SetFeeAmount(expectedFeeAmount.Add(sdk.NewCoin(sdk.DefaultBondDenom, math.NewInt(1))))
+	_, _, err = keeper.NewTxUtils(&input.EVMKeeper).ConvertCosmosTxToEthereumTx(ctx, txBuilder.GetTx())
+	require.ErrorIs(t, err, types.ErrTxConversionFailed)
 }
 
 func Test_LegacyTxConversion(t *testing.T) {
@@ -394,7 +408,8 @@ func Test_LegacyTxConversion(t *testing.T) {
 	})
 
 	authTx := sdkTx.(authsigning.Tx)
-	require.Equal(t, authTx.GetFee(), sdk.NewCoins(sdk.NewCoin(sdk.DefaultBondDenom, math.NewIntFromBigInt(feeAmount).AddRaw(1))))
+	expectedFeeAmount := sdk.NewCoins(sdk.NewCoin(sdk.DefaultBondDenom, math.NewIntFromBigInt(feeAmount).AddRaw(1)))
+	require.Equal(t, authTx.GetFee(), expectedFeeAmount)
 
 	sigs, err := authTx.GetSignaturesV2()
 	require.NoError(t, err)
@@ -419,6 +434,12 @@ func Test_LegacyTxConversion(t *testing.T) {
 	ethTx2, _, err := keeper.NewTxUtils(&input.EVMKeeper).ConvertCosmosTxToEthereumTx(ctx, sdkTx)
 	require.NoError(t, err)
 	EqualEthTransaction(t, signedTx, ethTx2)
+
+	// manipulate the fee amount
+	txBuilder := sdkTx.(client.TxBuilder)
+	txBuilder.SetFeeAmount(expectedFeeAmount.Add(sdk.NewCoin(sdk.DefaultBondDenom, math.NewInt(1))))
+	_, _, err = keeper.NewTxUtils(&input.EVMKeeper).ConvertCosmosTxToEthereumTx(ctx, txBuilder.GetTx())
+	require.ErrorIs(t, err, types.ErrTxConversionFailed)
 }
 
 func Test_IsEthereumTx(t *testing.T) {

diff --git a/x/evm/types/errors.go b/x/evm/types/errors.go
@@ -37,6 +37,7 @@ var (
 	ErrFailedToGetERC20WrapperAddr = errorsmod.Register(ModuleName, 28, "Failed to get ERC20 wrapper address")
 	ErrInvalidNumRetainBlockHashes = errorsmod.Register(ModuleName, 29, "Invalid num retain block hashes")
 	ErrExceedMaxRecursiveDepth     = errorsmod.Register(ModuleName, 30, "Exceed max recursive depth")
+	ErrTxConversionFailed          = errorsmod.Register(ModuleName, 31, "Tx conversion failed")
 )
 
 func NewRevertError(revert []byte) error {