Logic continues even if swap fails

[code423n4]

Lines of code

https://github.com/code-423n4/2023-06-canto/blob/main/Canto/x/onboarding/keeper/ibc_callbacks.go#L92-L108

Vulnerability details

The code tries to perform a coin swap if the standardCoinBalance is less than autoSwapThreshold:

    if standardCoinBalance.LT(autoSwapThreshold) {
        swappedAmount, err = k.coinswapKeeper.TradeInputForExactOutput(ctx, coinswaptypes.Input{Coin: transferredCoin, Address: recipient.String()}, coinswaptypes.Output{Coin: swapCoins, Address: recipient.String()})
        if err != nil {
            logger.Error("failed to swap coins", "error", err)
        } else {
                ctx.EventManager().EmitEvent(
                ...
        }
}

https://github.com/code-423n4/2023-06-canto/blob/main/Canto/x/onboarding/keeper/ibc_callbacks.go#L92-L108

If the swap fails (via k.coinswapKeeper.TradeInputForExactOutput call), it will log the error and proceed with the logic. It doesn't return or break the function execution.

We can see that swappedAmount := sdk.ZeroInt() will be zero as it is the initial value. As a result of this, the logic will continue and reach:

    convertCoin := sdk.NewCoin(transferredCoin.Denom, transferredCoin.Amount.Sub(swappedAmount))

https://github.com/code-423n4/2023-06-canto/blob/main/Canto/x/onboarding/keeper/ibc_callbacks.go#L124

Where swappedAmount = 0 and result in calling .Sub with zero amount, meaning that the entire transferredCoin amount will be converted.

In addition. the code tries to convert the remaining amount of transferred coins into ERC20 tokens:

    // Use MsgConvertCoin to convert the Cosmos Coin to an ERC20
    if _, err = k.erc20Keeper.ConvertCoin(sdk.WrapSDKContext(ctx), convertMsg); err != nil {
        logger.Error("failed to convert coins", "error", err)
        return ack
    }

https://github.com/code-423n4/2023-06-canto/blob/main/Canto/x/onboarding/keeper/ibc_callbacks.go#L132-L136

If the conversion fails, it logs the error and immediately returns the acknowledgement.

Now, we can consider two scenarios, one where the swap operation fails and yet the logic continues into the coin conversion and the other scenario where the swap operation is successful (i.e., coins are successfully swapped), but the subsequent conversion operation fails. In this case, the function logs the error and returns the acknowledgement, as per the logic in the conversion section. However, the swapped coins are left in their swapped state without being converted as originally intended.

This situation may or may not be a problem, depending on your specific requirements or expectations, but these are edge cases that's currently not being explicitly handled.

It's possible that handling this scenario may require additional logic to either revert the coin swap operation when conversion fails or provide a way to retry the failed conversion operation later.

Recommended Mitigation Steps

One option is to add

    return channeltypes.NewErrorAcknowledgement(err.Error())

As it is used in several code snippets throughout the code such as when calling ibc.GetTransferSenderRecipient(packet):

    _, recipient, senderBech32, recipientBech32, err := ibc.GetTransferSenderRecipient(packet)
    if err != nil {
        return channeltypes.NewErrorAcknowledgement(err.Error())
    }

Assessed type

Error

[c4-pre-sort]

JeffCX marked the issue as low quality report

[c4-pre-sort]

JeffCX marked the issue as high quality report

[c4-pre-sort]

JeffCX marked the issue as duplicate of #80

[c4-judge]

0xean marked the issue as satisfactory