OnRecvPacket does not roll back failures in Convert coins or Swap coins

[c4-bot-4]

Lines of code

https://github.com/code-423n4/2024-05-canto/blob/d1d51b2293d4689f467b8b1c82bba84f8f7ea008/canto-main/x/onboarding/keeper/ibc_callbacks.go#L137

Vulnerability details

Impact

onboarding#OnRecvPacket will be call
coinswapKeeper.TradeInputForExactOutput and
erc20Keeper.ConvertCoin

However, if the execution of these two functions fails in the middle, for example, the user's token has been deducted, but the execution fails when mint is executed, the transaction will not be rolled back, which will cause the loss of user funds.

Proof of Concept

The x/keeper function will return an error. If the function is executed through RPC, if it fails, the SDK will handle the error and roll back the transaction.

However,OnRecvPacket calls the keeper function directly, and the error returned by the keeper function is not passed.

    func (k Keeper) OnRecvPacket 
    ....
	if _, err = k.erc20Keeper.ConvertCoin(sdk.WrapSDKContext(ctx), convertMsg); err != nil {
		logger.Error("failed to convert coins", "error", err)
		return ack
	}

	// return original acknowledgement
	return ack

The OnRecvPacket function returns the same ack regardless of success or failure.

Therefore, if the erc20Keeper.ConvertCoin function fails halfway through execution, the error will not be handled.

The ConvertCoin function usually first deducts the user's token and then calls the mint or transfer function (k.CallEVM) in the contract.

Since there are many cases where K.CallEVM may fail to execute, if after deducting the user token, K.CallEVM does not execute successfully, the user's token will be permanently lost.

The convertCoinNativeERC20 function checks whether the user's balance is consistent after the conversion, and checks the Approval event in logs,

In this case, these checks are invalid because the returned error will not be handled.

func (k Keeper) convertCoinNativeERC20....{
    ...
    // Escrow Coins on module account
	if err := k.bankKeeper.SendCoinsFromAccountToModule(ctx, sender, types.ModuleName, coins); err != nil {
		return nil, errorsmod.Wrap(err, "failed to escrow coins")
	}
    // Unescrow Tokens and send to receiver
	res, err := k.CallEVM(ctx, erc20, types.ModuleAddress, contract, true, "transfer", receiver, msg.Coin.Amount.BigInt())
	if err != nil {
		return nil, err
	}
    ...
	if r := balanceTokenAfter.Cmp(exp); r != 0 {
		return nil, errorsmod.Wrapf(
			types.ErrBalanceInvariance,
			"invalid token balance - expected: %v, actual: %v", exp, balanceTokenAfter,
		)
	}
	...
	// Check for unexpected `Approval` event in logs
	if err := k.monitorApprovalEvent(res); err != nil {
		return nil, err
	}
}

The same problem occurs when executing coinswapKeeper.TradeInputForExactOutput.

Tools Used

vscode, manual

Recommended Mitigation Steps

func (k Keeper) OnRecvPacket ...{
	if _, err = k.erc20Keeper.ConvertCoin(sdk.WrapSDKContext(ctx), convertMsg); err != nil {
		logger.Error("failed to convert coins", "error", err)
-		return ack
	}
+	if err != nil {
+    	// Transaction failed, rollback
+    	abci.AbortTx(ctx.Height())
+    	return ack
+	}
}

Assessed type

Error

[c4-judge]

3docSec changed the severity to QA (Quality Assurance)

[c4-judge]

3docSec marked the issue as grade-b