Canto - Rolezn's results

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.

<ins>Recommended Mitigation Steps</ins>

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

QA Summary<a name="QA Summary">

Total: 4 contexts over 3 issues

<a href="#QA Summary">[QA‑1]</a><a name="QA&#x2011;1"> Loop can finish iterating earlier

In OnRecvPacket function, as soon as s == packet.DestinationChannel is true, it sets found, but still iterates the rest of the channels.

Instead when setting found to true, we can add break that ends the loop, skipping the rest of the params.WhitelistedChannels that didn't get checked yet.

<ins>Proof Of Concept</ins>

	var found bool
	for _, s := range params.WhitelistedChannels {
		if s == packet.DestinationChannel {
			found = true
		}
	}

	if !found {
		return ack
	}

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

<ins>Recommended Mitigation Steps</ins>

	var found bool
	for _, s := range params.WhitelistedChannels {
		if s == packet.DestinationChannel {
			found = true
			break
		}
	}
	if !found {
		return ack
	}

<a href="#QA Summary">[QA‑2]</a><a name="QA&#x2011;2"> Multiple GetParams calls in OnRecvPacket

The following calls k.GetParams(ctx) twice in the OnRecvPacket function. It may be more efficient to call it once at the start of the function and store the result in a variable for later use, in this case params can be reused.

<ins>Proof Of Concept</ins>

39: params := k.GetParams(ctx)

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

87: autoSwapThreshold := k.GetParams(ctx).AutoSwapThreshold

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

<a href="#QA Summary">[QA‑3]</a><a name="QA&#x2011;3"> Module account type check

The OnRecvPacket function retrieves the account associated with the recipient from k.accountKeeper.GetAccount(ctx, recipient). Then it checks if the account is a module account, which is a special type of account typically used by the system modules for specific functionalities (e.g., fee collection, staking pool).

If the recipient account is a module account, the function immediately returns the acknowledgment without proceeding with the rest of the logic. It is indicated that onboarding is not supported for module accounts, which is why the function returns ack.

While this check is a reasonable. It is possible that not all module accounts should be treated the same - some might actually need to go through the rest of the logic in the function. For instance, a staking module account might be required to participate in coin swaps and conversions. In this case, a more fine-grained check is advised.

Instead of returnig ack on all module accounts, the project could maintain a whitelist of specific module accounts that should be allowed to proceed. The specific implementation would depend on the actual requirements of your application. This will also allow future module accounts that their onboarding to be supported simply by adding them to a whitelist.

<ins>Proof Of Concept</ins>

	// onboarding is not supported for module accounts
	if _, isModuleAccount := account.(authtypes.ModuleAccountI); isModuleAccount {
		return ack
	}