When reverting a cross chain tx with ERC20/native token as coinType, gas will always be underpaid due to inconsistent handling of median gasPrice

[c4-bot-10]

Lines of code

https://github.com/code-423n4/2023-11-zetachain/blob/b237708ed5e86f12c4bddabddfd42f001e81941a/repos/node/x/crosschain/keeper/gas_payment.go#L151
https://github.com/code-423n4/2023-11-zetachain/blob/b237708ed5e86f12c4bddabddfd42f001e81941a/repos/node/x/crosschain/keeper/gas_payment.go#L106
https://github.com/code-423n4/2023-11-zetachain/blob/b237708ed5e86f12c4bddabddfd42f001e81941a/repos/node/x/crosschain/keeper/gas_payment.go#L285

Vulnerability details

Impact

Median gas price is inconsistently handled in gas_payment.go. For the same external chain, a sender will be charged twice as much median gas price if cointType is zeta, while a sender will be charged the exact median gas price if cointType is native gas coin or ERC20.

Proof of Concept

In gas_payment.go, outboundtx gas payment is calculated based on gasLimit and gas price. gas price is based on median gas price of the external chain. However, gas price is inconsistenly handled between different coinTypes.

When coinType is zetacoin, the gas price is fetched in PayGasInZetaAndUpdateCctx(). It gets gasPrice for chainID through k.GetMedianGasPriceInUint(). The resulting gasPrice is then doubled through gasPrice = gasPrice.MulUint64(2). This gasPrice is then used to calculate actual gas token balance to charge.

However, when coinType is ERC20 token or native gas token, gasPrice for chainId is fetched through k.ChainGasParams(ctx, chainID), which under the hood calls the same GetMedianGasPriceInUint() that directly returns gasPrice. However, gasPrice is then directly used to be multiplied by gasLimit without doubling the price as in zetacoin flow. For the same chainId, gasPrice is inconsistently priced. This results in gas to be underpaid when the crosschain tx coinType is ERC20 token or native gas token.

//x/crosschain/keeper/gas_payment.go
func (k Keeper) PayGasInZetaAndUpdateCctx(
	ctx sdk.Context,
	chainID int64,
	cctx *types.CrossChainTx,
	zetaBurnt math.Uint,
	noEthereumTxEvent bool,
) error {
...
	// get the gas price
	gasPrice, isFound := k.GetMedianGasPriceInUint(ctx, chainID)
...
        //@audit gas price is doubled from median gas price of chainId when cointType is zeta 
|>	gasPrice = gasPrice.MulUint64(2) // overpays gas price by 2x

(https://github.com/code-423n4/2023-11-zetachain/blob/b237708ed5e86f12c4bddabddfd42f001e81941a/repos/node/x/crosschain/keeper/gas_payment.go#L285)

//x/crosschain/keeper/gas_payment.go
func (k Keeper) PayGasInERC20AndUpdateCctx(
	ctx sdk.Context,
	chainID int64,
	cctx *types.CrossChainTx,
	inputAmount math.Uint,
	noEthereumTxEvent bool,
) error {
...
	// get gas params
        //@audit When cointype is erc20 token, ChainGasParams() performs the same GetMedianGasPriceInUint() to return median gas price for chainId 
|>	gasZRC20, gasLimit, gasPrice, protocolFlatFee, err := k.ChainGasParams(ctx, chainID)
...
	outTxGasFee := gasLimit.Mul(gasPrice).Add(protocolFlatFee)

(https://github.com/code-423n4/2023-11-zetachain/blob/b237708ed5e86f12c4bddabddfd42f001e81941a/repos/node/x/crosschain/keeper/gas_payment.go#L147)

//x/crosschain/keeper/gas_payment.go
func (k Keeper) ChainGasParams(
	ctx sdk.Context,
	chainID int64,
) (gasZRC20 ethcommon.Address, gasLimit, gasPrice, protocolFee math.Uint, err error) {
...
        //@audit-info ChainGasParams get the same median price as it also calls GetMedianGasPriceInUint()
|>	gasPrice, isFound := k.GetMedianGasPriceInUint(ctx, chainID)
...

(https://github.com/code-423n4/2023-11-zetachain/blob/b237708ed5e86f12c4bddabddfd42f001e81941a/repos/node/x/crosschain/keeper/gas_payment.go#L75)

Same median gas price is fetched for target chainId, however, only when the cctx coinType is zeta coin, the fetched price will be doubled. When the coinType is erc20 or gas native token, gas will be underpaid.

Tools Used

Manual review

Recommended Mitigation Steps

Consider unifying the gasPrice handling between three different coin types.

Assessed type

Error

[c4-pre-sort]

DadeKuma marked the issue as duplicate of #393

[c4-pre-sort]

DadeKuma marked the issue as sufficient quality report

[0xean]

I think this is a separate issue from #393 ( cc @DadeKuma ) - but also not sure it really qualifies as M either. The logic is not consistent, but the impact (besides some transactions having arbitrarily 2x the gas) isn't well captured by the Warden.

[c4-judge]

0xean marked the issue as not a duplicate

[0xean]

@c4-sponsor - care to comment on this one?

[c4-sponsor]

lumtis (sponsor) confirmed

[c4-judge]

0xean changed the severity to QA (Quality Assurance)

[c4-judge]

0xean marked the issue as grade-b