Depositing wZETA from EVM chain to another external supported chain could cost much more then expected

[c4-bot-8]

Lines of code

https://github.com/code-423n4/2023-11-zetachain/blob/main/repos/node/x/crosschain/keeper/gas_payment.go#L289 https://github.com/code-423n4/2023-11-zetachain/blob/main/repos/node/x/crosschain/keeper/keeper_cross_chain_tx_vote_outbound_tx.go#L169-L172

Vulnerability details

Description

Normal cost of an ERC20 transfer on Ethereum is around 65000 gas unit. User on Ethereum (or any EVM supported chain) can send their wZETA to another EVM supported chain (thanks to CCTX), which from a user perspective should be similar in terms of cost of a normal ERC20.

Unfortunatelly, this is not the reality, in some case this could imply a huge cost or fund loss for the user which seems to warrant High severity.

Impact

High gas fees or fund loss when deposting wZETA to an external chain during the refund flow (see PoC).

Here are two undesired scenario that can occur depending on the inputs:

• High gas, but higher wZETA value: If user send a super high gas limit like 1M gas unit (which is not abnormal) and wZETA transfered was higher in value, the refund transaction will work but that will cost a LOT of money to the user. Considering a 200 wZeta transfer that revert so refunded on Ethereum, at 30 gwei gas price, ETH at 2355 USD and wZETA at 1 USD, (1M gas unit * 60 gwei) sounds like 140 USD in fees for a failed transfer of the 200 wZETA (value of 200 USD), that's 70% of the value being transfered paid in fees! Worst is that the transfer didn't work, and the user is refunded only 30% of the value sent in that case (see PoC).
• High gas, but lower wZETA value: That will cause to return error ErrNotEnoughZetaBurnt in PayGasInZetaAndUpdateCctx and will cause the transaction to be aborded, so funds loss as refund is aborted.

Proof of Concept

Let's consider the CCTX flow described in the whitepaper as follow for Cross-Chain Message Passing.

Let say Alice want to send 200 wZETA from Ethereum to BSC using ZetaConnector.eth::send.

• At step 1-2, this is where the user call ZetaConnector.eth::send which will emit ZetaSent log.
• At step 3, this is where Observer consume the ZetaSent log in GetInboundVoteMsgForZetaSentEvent, do some basics validation (but use the event.DestinationGasLimit as is) and report it to zetacore, which will do some validation when receiving this broadcast in VoteOnObservedInboundTx.
• At step 4-5, this is where the inbound is finalized and the CCTX is created, status is changed to CctxStatus_PendingOutbound as this is going to BSC and processed by Observers in TryProcessOutTx.
• At step 9, unfortunatelly a failure happened (onReceive reverted, that can easily happen as dApp callback is involved, which is out of the control of Zeta protocol), so the refund mechanism will be triggered.

This is where there is an issue in the code. Let's examine more in details VoteOnObservedOutboundTx when the ballot is finalized.

1. [line 157] code branch BallotStatus_BallotFinalized_FailureObservation is selected as there was a revert.
2. [line 161] then the else branch is taken as msg.CoinType == common.CoinType_Zeta
3. [line 163] then case types.CctxStatus_PendingOutbound is selected
4. [line 165] GetRevertGasLimit will return (0, nil) since cctx.InboundTxParams.CoinType == common.CoinType_Zeta.
5. [line 171] code path gasLimit = cctx.OutboundTxParams[0].OutboundTxGasLimit will be reach, which is the problem. This will actually be the value specified originally by the user in the first call to ZetaConnector.eth::send (reminder: event.DestinationGasLimit).

The understanding of a gaslimit is that you can put very high limit to be sure your transaction doesn't lack gas and revert in the middle, as you know the remaining gas will be refunded to you. The problem here is that this is not the case, as Zeta makes the user pay in advance for the gas the TSS will spend to send the refund transaction on the sender chain (Ethereum).

As we can see, gasLimit is assigned to the new OutboundTxParams OutboundTxGasLimit: gasLimit and then PayGasAndUpdateCctx is called.

                switch oldStatus {
                case types.CctxStatus_PendingOutbound:

                    gasLimit, err := k.GetRevertGasLimit(ctx, cctx)
                    if err != nil {
                        return errors.New("can't get revert tx gas limit" + err.Error())
                    }
                    if gasLimit == 0 {
                        // use same gas limit of outbound as a fallback -- should not happen
                        gasLimit = cctx.OutboundTxParams[0].OutboundTxGasLimit
                    }

                    // create new OutboundTxParams for the revert
                    revertTxParams := &types.OutboundTxParams{
                        Receiver:           cctx.InboundTxParams.Sender,
                        ReceiverChainId:    cctx.InboundTxParams.SenderChainId,
                        Amount:             cctx.InboundTxParams.Amount,
                        CoinType:           cctx.InboundTxParams.CoinType,
                        OutboundTxGasLimit: gasLimit,
                    }
                    cctx.OutboundTxParams = append(cctx.OutboundTxParams, revertTxParams)

                    err = k.PayGasAndUpdateCctx(
                        tmpCtx,
                        cctx.InboundTxParams.SenderChainId,
                        &cctx,
                        cctx.OutboundTxParams[0].Amount,
                        false,
                    )

PayGasInZetaAndUpdateCctx is then called as working with common.CoinType_Zeta, and the function will consume ALL the gaslimit.

    // get the gas price
    gasPrice, isFound := k.GetMedianGasPriceInUint(ctx, chainID)
    if !isFound {
        return cosmoserrors.Wrap(types.ErrUnableToGetGasPrice, fmt.Sprintf(" chain %d | Identifiers : %s ",
            chainID,
            cctx.LogIdentifierForCCTX()),
        )
    }
    gasPrice = gasPrice.MulUint64(2) // overpays gas price by 2x

    gasLimit := sdk.NewUint(cctx.GetCurrentOutTxParam().OutboundTxGasLimit)
    outTxGasFee := gasLimit.Mul(gasPrice)

    // get the gas fee in Zeta using system uniswapv2 pool wzeta/gasZRC20 and adding the protocol fee
    outTxGasFeeInZeta, err := k.fungibleKeeper.QueryUniswapV2RouterGetZetaAmountsIn(ctx, outTxGasFee.BigInt(), gasZRC20)
    if err != nil {
        return cosmoserrors.Wrap(err, "PayGasInZetaAndUpdateCctx: unable to QueryUniswapv2RouterGetAmountsIn")
    }
    feeInZeta := types.GetProtocolFee().Add(math.NewUintFromBigInt(outTxGasFeeInZeta))

    // reduce the amount of the outbound tx
    if feeInZeta.GT(zetaBurnt) {
        return cosmoserrors.Wrap(types.ErrNotEnoughZetaBurnt, fmt.Sprintf("feeInZeta(%s) more than zetaBurnt (%s) | Identifiers : %s ",
            feeInZeta,
            zetaBurnt,
            cctx.LogIdentifierForCCTX()),
        )
    }

Cross-Chain Message Passing flow

1. An end user interacts with a Contract C1 on Chain A.
2. The interaction leaves an event or transaction memo, with user specified [chainID, contractAddress, message]. (the message is arbitrarily encoded application data in binary format.
3. ZetaChain observers (in zetaclient) pick up this event/memo and report to zetacore, which verifies the inbound transaction.
4. zetacore modifies the CCTX (cross-chain tx) state variable with OutboundTxParams which instructs the TSS signers (in zetaclient) to build, sign, and broadcast external transaction.
5. The zetaclient TSS signers observe the OutboundTxParams in the CCTX, and build outbound tx, enter into a TSS keysign ceremony to sign the transaction, and then broadcast the signed transaction to the external blockchains. For CCMP, the outbound transaction is mainly calling the user specified contract with specified addresses and parameters.
6. The CCTX structure also tracks the stages/status of the cross-chain transaction.
7. Once the broadcasted transaction is included in one of the blocks (said to be “mined” or “confirmed”), zetaclients will report such confirmation to zetacore, which will update the CCTX status.
8. If the “confirmed” outbound transaction was successful, the CCTX status becomes OutboundMined, and the CCTX is considered in its terminal state and will not be updated anymore. This CCTX processing is completed.
9. If the “confirmed” outbound transaction is failure (e.g. revert on Ethereum chains), then the CCTX will updates it status to PendingRevert if possible, or Aborted if revert is not possible. The CCTX processing is completed if it goes to Aborted status.
10. If the new status is “PendingRevert”, a second OutboundTxParams should be already in the CCTX, which instructs the zetaclients to create a “Revert” outbound tx to the incoming chain & contract, allowing the incoming contract to implement a application level revert function to cleanup contract state.
11. The zetaclients will build the revert transaction, enter into TSS keysign ceremony to sign the transaction, and broadcast to the incoming blockchain (Chain A in this case).
12. Once the revert transaction is “confirmed” on Chain A, the zetaclients will report the transaction status to zetacore.
13. If the revert transaction is successful, the CCTX status becomes Reverted, and the CCTX processing is completed.
14. If the revert transaction is failure, the CCTX status becomes Aborted, and the CCTX processing is completed.

Recommended Mitigation Steps

GetRevertGasLimit is called in two places as follow. The comment fallback -- should not happen is actually true for InboundTx, it's a dead code path. On the other hand, it's an active path for OutboundTx and cause this high severity report.

VoteOnObservedInboundTx I would recommend to remove the dead code path and have no fallback, the code path is anyway dead, so keeping it there make the code more vulnerable.

    gasLimit, err := k.GetRevertGasLimit(ctx, cctx)
-   if err != nil {
+   if err != nil || gasLimit == 0 {
        cctx.CctxStatus.ChangeStatus(types.CctxStatus_Aborted, "can't get revert tx gas limit"+err.Error())
        return &types.MsgVoteOnObservedInboundTxResponse{}, nil
    }
-   if gasLimit == 0 {
-           // use same gas limit of outbound as a fallback -- should not happen
-       gasLimit = msg.GasLimit
-   }

VoteOnObservedOutboundTx This is the crispy one. I don't see why you could not do the following, at least I'm proposing it XD. I understand that this will also impact the Inbound flow, but seems a noop as a failure in k.HandleEVMDeposit will always return isContractReverted false which would not reach this code path.

func (k Keeper) GetRevertGasLimit(ctx sdk.Context, cctx types.CrossChainTx) (uint64, error) {
    if cctx.InboundTxParams == nil {
        return 0, nil
    }

-   if cctx.InboundTxParams.CoinType == common.CoinType_Gas {
+   if cctx.InboundTxParams.CoinType == common.CoinType_Gas || cctx.InboundTxParams.CoinType == common.CoinType_Zeta {
        // get the gas limit of the gas token
        fc, found := k.fungibleKeeper.GetGasCoinForForeignCoin(ctx, cctx.InboundTxParams.SenderChainId)
        if !found {
            return 0, types.ErrForeignCoinNotFound
        }
        gasLimit, err := k.fungibleKeeper.QueryGasLimit(ctx, ethcommon.HexToAddress(fc.Zrc20ContractAddress))
        if err != nil {
            return 0, errors.Wrap(fungibletypes.ErrContractCall, err.Error())
        }
        return gasLimit.Uint64(), nil
    } else if cctx.InboundTxParams.CoinType == common.CoinType_ERC20 {
        // get the gas limit of the associated asset

    gasLimit, err := k.GetRevertGasLimit(ctx, cctx)
    if err != nil || gasLimit == 0 {
           return errors.New("can't get revert tx gas limit" + err.Error())
    }
-   if gasLimit == 0 {
-      // use same gas limit of outbound as a fallback -- should not happen
-      gasLimit = cctx.OutboundTxParams[0].OutboundTxGasLimit
-   }

Assessed type

Token-Transfer

[c4-pre-sort]

DadeKuma marked the issue as duplicate of #241

[c4-pre-sort]

DadeKuma marked the issue as duplicate of #406

[c4-pre-sort]

DadeKuma marked the issue as sufficient quality report

[c4-judge]

0xean marked the issue as satisfactory

[c4-judge]

0xean changed the severity to 2 (Med Risk)

[dontonka]

Two of my issues have been set as duplicate of 406 which seems to be a mistake. One issue is clearly a duplciate (270), so that's good, but not the current submission, which is addressing a different gas issue (see impacts), so when moving wZETA from an external chain to another external chain (or the same external chain as the smoketest TestMessagePassingRevertSuccess do) using the ZetaChain, while 406 is when sending ZETA from ZetaChain itself to an external chain, the flow, impacts and the issue are different.

The current submission expose how the refund mechanism in the indicated scenario will be using the full amount of gas limit specified when calling the send function originally, which can be very high, and could cause a fund user loss (in case the amount of gas is greater than the ZETA value being transfered) or pay excessive gas (in case ZETA value is greater then the amount of gas). This happen where the devs believe a code path is being dead, while it's not the case as proven by my PoC, but also with the following smoketest.

To proove my point on my first impact (paying high gas fee)

I did run the smoketest TestMessagePassingRevertSuccess which is exploiting this code path with 3 scenarios (250k, 1M and 20M gas limit).

1) I've modified source code of TestDApp::sendHelloWorld to use 1M and 20M as destinationGasLimit and recompiled it

    function sendHelloWorld(address destinationAddress, uint256 destinationChainId, uint256 value, bool doRevert) external payable {
        bool success1 = IERC20(zeta).approve(address(connector), value);
        bool success2 = IERC20(zeta).transferFrom(msg.sender, address(this), value);
        if (!(success1 && success2)) revert ErrorTransferringZeta();

        ZetaConnector(connector).send(
            ZetaInterfaces.SendInput({
                destinationChainId: destinationChainId,
                destinationAddress: abi.encodePacked(destinationAddress),
-                destinationGasLimit: 250000,
+                destinationGasLimit: 1000000,
                message: abi.encode(HELLO_WORLD_MESSAGE_TYPE, doRevert),
                zetaValueAndGas: value,
                zetaParams: abi.encode("")
            })
        );
    }

2) I've modified the TestDAppMetaData.Bin to the corresponding recompiled binary.

var TestDAppMetaData = &bind.MetaData{
      ABI: "[{\"inputs\":[{\"internalType\ ...",
-     Bin: "0x608060405234801561001057600080fd5b5060405162000ebe38038062000...",
+     Bin: "0x608060405234801561001057600080fd5b506040516108663803806108668...",
}

3) I've run those 3 scenarios which require to rebuild the smoketest docker (make zetanode) image each time and the following are the results.

--- GAS limit at 250k (original code) --
$$$ Before: SUPPLY OF AZETA: 108402100200000000000000000
ZetaReverted event:
  Dest Addr: 0x0e141A7e7C0A7E15E7d22713Fc0a6187515Fa9BF
  Dest Chain: 1337
  RemainingZetaValue: 5999999999991975926
  Message: 6e0182194bb1deba01849afd3e035a0b70ce7cb069e482ee663519c76cf569b40000000000000000000000000000000000000000000000000000000000000001
$$$ After: SUPPLY OF AZETA: 108402104200000000008024074
$$$ Diff: SUPPLY OF AZETA: 4000000000008024074

--- GAS limit at 1M--
$$$ Before: SUPPLY OF AZETA: 108402100200000000000000000
ZetaReverted event:
  Dest Addr: 0x0e141A7e7C0A7E15E7d22713Fc0a6187515Fa9BF
  Dest Chain: 1337
  RemainingZetaValue: 5999999999967903710
  Message: 6e0182194bb1deba01849afd3e035a0b70ce7cb069e482ee663519c76cf569b40000000000000000000000000000000000000000000000000000000000000001
$$$ After: SUPPLY OF AZETA: 108402104200000000032096290
$$$ Diff: SUPPLY OF AZETA: 4000000000032096290

--- GAS limit at 20M--
$$$ Before: SUPPLY OF AZETA: 108402100200000000000000000
ZetaReverted event:
  Dest Addr: 0x0e141A7e7C0A7E15E7d22713Fc0a6187515Fa9BF
  Dest Chain: 1337
  RemainingZetaValue: 5999999999358074218
  Message: 6e0182194bb1deba01849afd3e035a0b70ce7cb069e482ee663519c76cf569b40000000000000000000000000000000000000000000000000000000000000001
$$$ After: SUPPLY OF AZETA: 108402104200000000641925782
$$$ Diff: SUPPLY OF AZETA: 4000000000641925782

If we look at RemainingZetaValue in those 3 scenarios or even better at Diff: SUPPLY OF AZETA, we can confirm that the gas spent is increasing by the factor indicated in the destinationGasLimit parameter.

• 250k : 4000000000008024074
• 1M : 4000000000032096290
• 20M : 4000000000641925782

Let's break down those fees. The test is sending 10 ZETA from one ETH account to another ETH account using ZetaChain. Since the protocol fee is hardcoded at 2 ZETA, and since there will be 2 outbound transactions (normal outbound which will revert, then the outbound refund tx), the protocol fees are 4 ZETA. Then the remaining is the actual gas spent on the transaction which is what we are interested in, we can see that the increasing gas spend is corresponding to the gas limit factor when calling send. Those are ridiculous in the smoketest as asset price (ETH and ZETA) are not real in the environnement, but if they would be, that would be much impactfull and as described in my report Impacts, which proove my point, and I hope can be understood by everyone. 250k : 8024074 1M : 8024074 4 = 32096296 20M : 8024074 80 = 641925920

To proove my point on my second impact (user fund loss)

I did run the smoketest TestMessagePassingRevertSuccess which is exploiting this code path with a single scenario sending only 4 ZETA instead of 10 ZETA, which will simulate higher gas cost for the refund transaction, which will revert. Granted that the amounts here are ridiculous as indicated, but this is due to the fact how ETH and ZETA are valued in USD in the smoketest environnement, which is not representing the reality. The Impacts described in my report would be more representative of the reality.

The following is from a normal run.

$$$ Before: SUPPLY OF AZETA: 108402100200000000000000000
TestDApp.SendHello tx hash: 0xc01722d2abd2cbf0be2caa9ca18664de4c5942e78929f9e6f52147611026c612
TestDApp.SendHello tx receipt: status 1
Waiting for cctx to be mined by inTxHash: 0xc01722d2abd2cbf0be2caa9ca18664de4c5942e78929f9e6f52147611026c612
Error getting cctx by inTxHash:  rpc error: code = NotFound desc = not found
Waiting for cctx to be mined by inTxHash: 0xc01722d2abd2cbf0be2caa9ca18664de4c5942e78929f9e6f52147611026c612
Deposit receipt cctx index: [0x20feb00f551724ce1acf0830f8555334779362db10c074a55ccaee98da4705e5]
Deposit receipt cctx status: Reverted; The cctx is processed
ZetaReverted event:
  Dest Addr: 0x0e141A7e7C0A7E15E7d22713Fc0a6187515Fa9BF
  Dest Chain: 1337
  RemainingZetaValue: 5999999999991975926
  Message: 6e0182194bb1deba01849afd3e035a0b70ce7cb069e482ee663519c76cf569b40000000000000000000000000000000000000000000000000000000000000001
$$$ After: SUPPLY OF AZETA: 108402104200000000008024074
$$$ Diff: SUPPLY OF AZETA: 4000000000008024074

This is when sending only 4 ZETA, which we can see the status being Aborted, so no refund, user fund loss.

$$$ Before: SUPPLY OF AZETA: 108402100200000000000000000 - ETH price: 8 Gwei, ZETA price: 448 Gwei
TestDApp.SendHello tx hash: 0x47c7fd77afa0039c5c737ff36ce1910ada38ea9a42ccb327603298237ff9f03c
TestDApp.SendHello tx receipt: status 1
Waiting for cctx to be mined by inTxHash: 0x47c7fd77afa0039c5c737ff36ce1910ada38ea9a42ccb327603298237ff9f03c
Error getting cctx by inTxHash:  rpc error: code = NotFound desc = not found
Waiting for cctx to be mined by inTxHash: 0x47c7fd77afa0039c5c737ff36ce1910ada38ea9a42ccb327603298237ff9f03c
Deposit receipt cctx index: [0x37d78331443130871dd60d259b7eb19e93b7bb6eb1ebe903d625c0760cd4b2f0]
Deposit receipt cctx status: Aborted; The cctx is processed
panic: expected cctx to be reverted

Conclusion

For all the mention above, I would ask @0xean to reconsider this issue to be upgraded to High and unique finding and I would also appreciate the feedback of the sponsor @lumtis on the report as he probably hasn't read it. As indicated, since both impacts affect user funds negatively, that clearly seems to warrant High according to C4 rules.

[0xean]

The understanding of a gaslimit is that you can put very high limit to be sure your transaction doesn't lack gas and revert in the middle, as you know the remaining gas will be refunded to you

This alone disqualifies this from being H severity. I do agree that there are multiple gas issues going on and have separated them accordingly. That being said, I think this issue is actually QA. I welcome comment form the sponsor before downgrading.

[brewmaster012]

the issue is correct that the protocol charges twice the specified gas limit in such case of refund/revert. The user's payment for gas is either gasLimit if no revert or 2*gasLimit in case of revert, and I'm not sure that is higher than the user expectation. The revert tx gasLimit cannot be inferred by protocol, and needs to be set by dApp or user, and in this case the outbound gas limit is assumed which is reasonable.

[c4-sponsor]

lumtis (sponsor) acknowledged

[0xean]

I think QA is the correct severity here.

[c4-judge]

0xean marked the issue as not a duplicate

[c4-judge]

0xean changed the severity to QA (Quality Assurance)

[c4-judge]

0xean marked the issue as grade-a