code423n4 commented 1 year ago

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

c4-pre-sort commented 1 year ago

JeffCX marked the issue as primary issue

tkkwon1998 commented 1 year ago

The warden is correct in that a swap could go only half-completed if the function is used incorrectly, but all of the checks should occur before it ever gets to the swap function to ensure it succeeds fully. In our case, all of the checks are done in TradeInputForExactOutput. If the warden believes we are missing any checks which may cause the swap to only half-complete, then that would be great.

I will mark this as valid because, as the warden mentioned, if any future upgrades use the swap function, there may be a potential that they do not do the necessary checks.

c4-sponsor commented 1 year ago

tkkwon1998 marked the issue as sponsor confirmed

c4-judge commented 1 year ago

0xean marked the issue as satisfactory