code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-06-canto/blob/main/Canto/x/coinswap/keeper/swap.go#L26 https://github.com/code-423n4/2023-06-canto/blob/main/Canto/x/coinswap/keeper/swap.go#L18 https://github.com/code-423n4/2023-06-canto/blob/main/Canto/x/coinswap/keeper/swap.go#L203 https://github.com/code-423n4/2023-06-canto/blob/main/Canto/x/onboarding/keeper/ibc_callbacks.go#L93

Vulnerability details

Impact

The onboarding process calls k.coinswapKeeper.TradeInputForExactOutput if needed. There should be two consequences: Either a small portion of the transferred tokens will be swapped so the user receives 4 Canto tokens if the swap succeeds. Or nothing changed if the swap fails. However, swap.swapCoins could cause the user to lose their token without receiving 4 Canto.

Proof of Concept

OnRecvPacket calls k.coinswapKeeper.TradeInputForExactOutput if needed. https://github.com/code-423n4/2023-06-canto/blob/main/Canto/x/onboarding/keeper/ibc_callbacks.go#L93

func (k Keeper) OnRecvPacket(
    ctx sdk.Context,
    packet channeltypes.Packet,
    ack exported.Acknowledgement,
) exported.Acknowledgement {
    …

    if standardCoinBalance.LT(autoSwapThreshold) {
        swappedAmount, err = k.coinswapKeeper.TradeInputForExactOutput(ctx, coinswaptypes.Input{Coin: transferredCoin, Address: recipient.String()}, coinswaptypes.Output{Coin: swapCoins, Address: recipient.String()})
    …
}

In swap.TradeInputForExactOutput, it calls swap.swap to do the swap. https://github.com/code-423n4/2023-06-canto/blob/main/Canto/x/coinswap/keeper/swap.go#L203

func (k Keeper) TradeInputForExactOutput(ctx sdk.Context, input types.Input, output types.Output) (sdk.Int, error) {
    …

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

    return soldTokenAmt, nil
}

And in swap.swapCoins, we can find out that it first transfers the token from the sender to the pool and then transfers canto from the pool to the recipient. https://github.com/code-423n4/2023-06-canto/blob/main/Canto/x/coinswap/keeper/swap.go#L26 https://github.com/code-423n4/2023-06-canto/blob/main/Canto/x/coinswap/keeper/swap.go#L18

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))
}

Both transferal could succeed or fail. Let’s take a look at different situations:

Both of the transferal succeed: The swap succeeds
Both of the transferal fail: The swap fails and nothing changed
The first transferal fails: The swap fails and nothing changed
The first transferal succeeds but the second transferal fails: The user sends the token to the pool but the user doesn’t receive any token.

TradeInputForExactOutput doesn’t handle the last case. It simply returns sdk.ZeroInt(), err. Therefore, if the first transferal succeeds but the second transferal fails, The user loses the token without receiving any Cantos.

Tools Used

Manual Review

Recommended Mitigation Steps

Add a rollback mechanism for this issue: If the first transferal succeeds but the second transferal fails, send back the token to the user.

Assessed type

Error

c4-pre-sort commented 1 year ago

JeffCX marked the issue as duplicate of #5

c4-pre-sort commented 1 year ago

JeffCX marked the issue as duplicate of #80

c4-judge commented 1 year ago

0xean marked the issue as satisfactory