Canto - hihen's results

Lines of code

https://github.com/code-423n4/2023-06-canto/blob/a4ff2fd2e67e77e36528fad99f9d88149a5e8532/Canto/x/coinswap/keeper/swap.go#L26 https://github.com/code-423n4/2023-06-canto/blob/a4ff2fd2e67e77e36528fad99f9d88149a5e8532/Canto/x/onboarding/keeper/ibc_callbacks.go#L93-L96 https://github.com/code-423n4/2023-06-canto/blob/a4ff2fd2e67e77e36528fad99f9d88149a5e8532/Canto/x/coinswap/keeper/swap.go#L111-L113 https://github.com/code-423n4/2023-06-canto/blob/a4ff2fd2e67e77e36528fad99f9d88149a5e8532/Canto/x/coinswap/keeper/swap.go#L203-L205

Vulnerability details

Impact

If the last statement of the swapCoins() function returns an error, the swap is only half completed, i.e. only the user's assets are deducted (transferred to the pool), but the user's bought assets are not sent to the user, resulting in a loss of the user's assets.

Proof of Concept

The swapCoins() is implemented as follows:

func (k Keeper) swapCoins(ctx sdk.Context, sender, recipient sdk.AccAddress, coinSold, coinBought sdk.Coin) error {
	lptDenom, err := k.GetLptDenomFromDenoms(ctx, coinSold.Denom, coinBought.Denom)
	if err != nil {
		return err
	}

	poolAddr := types.GetReservePoolAddr(lptDenom)
	if err := k.bk.SendCoins(ctx, sender, poolAddr, sdk.NewCoins(coinSold)); err != nil {
		return err
	}

	if recipient.Empty() {
		recipient = sender
	}

	return k.bk.SendCoins(ctx, poolAddr, recipient, sdk.NewCoins(coinBought))
}

If the last statement fails, the function returns an error, but this is already the second step of the swap operation, and the first step of deducting the user's assets (transferred to the pool) has already been successfully executed.

Then, let's look at the code in TradeInputForExactOutput() where the swapCoins() is called:

func (k Keeper) TradeInputForExactOutput(ctx sdk.Context, input types.Input, output types.Output) (sdk.Int, error) {
	soldTokenAmt, err := k.calculateWithExactOutput(ctx, output.Coin, input.Coin.Denom)
	if err != nil {
		return sdk.ZeroInt(), err
	}
  
  ......

  if err := k.swapCoins(ctx, inputAddress, outputAddress, soldToken, output.Coin); err != nil {
		return sdk.ZeroInt(), err
	}

	return soldTokenAmt, nil
}

From the above code we can see that if k.swapCoins() returns an error, it is treated as if the swap was not executed, an amount of 0 (sdk.ZeroInt()) and the error will be returned .

The implementation of TradeExactInputForOutput() is similar to TradeInputForExactOutput().

Finally, let's look at how the onboarding's ibc_callbacks.go#OnRecvPacket() handles the call to TradeInputForExactOutput():

func (k Keeper) OnRecvPacket(
	ctx sdk.Context,
	packet channeltypes.Packet,
	ack exported.Acknowledgement,
) exported.Acknowledgement {
  ......
  
	if standardCoinBalance.LT(autoSwapThreshold) {
		swappedAmount, err = k.coinswapKeeper.TradeInputForExactOutput(......)
		if err != nil {
			logger.Error("failed to swap coins", "error", err)
		} else {
			ctx.EventManager().EmitEvent(
				sdk.NewEvent(
					coinswaptypes.EventTypeSwap,
					sdk.NewAttribute(coinswaptypes.AttributeValueAmount, swappedAmount.String()),
          ......
				),
			)
		}
	}
  
  //convert coins to ERC20 token
	pairID := k.erc20Keeper.GetTokenPairID(ctx, transferredCoin.Denom)
  ......
}

We can found that if coinswapKeeper.TradeInputForExactOutput() returns (0, err), the onboarding will treat as if the trade has not been executed entirely(no assets transferring), only an error log is recorded.

However, in fact, when the last statement of the swapCoins() returns an error (as metioned above), the swap is half executed, some assets transfer has already taken place. The end result is that some of the user's assets are transferred to the coinswap pool, but the user does not receive any Canto.

In summary, although the likelihood of the last statement of coinswap's swapCoins() returning an error is very small, and may only occur in an extreme case or in a future update, since it may return an error, it should be handled carefully and logical correctly. And the current implementation logic does not strive to do so.

Tools Used

VS Code

Recommended Mitigation Steps

We should throw this speciall error up in a special way if the swap fails in mid-execution(some of the transfers have already been made), and the upper layer (e.g. onboarding middleware) must identify this specific error and trigger the rollback/recovery of the transaction (maybe in Recovery Middleware) to ensure the safety of the user's assets.

Assessed type

Other