Slippage protection minOut autoSwapThreshold is not effective when swapping the token

[code423n4]

Lines of code

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

Vulnerability details

Impact

In the current model, the minimum output (minOut) amount for the auto-swap is set to match the autoSwapThreshold, which is fixed at 4 CANTO. This configuration might result in potential market risks due to fluctuations in the value of CANTO, potentially causing a significant loss for the user in cases of high slippage.

Proof of Concept

The onboarding model, by design, automatically swaps from an asset to the CANTO token. Here is a relevant excerpt from the code:

    swapCoins := sdk.NewCoin(standardDenom, autoSwapThreshold)
    standardCoinBalance := k.bankKeeper.SpendableCoins(ctx, recipient).AmountOf(standardDenom)
    swappedAmount := sdk.ZeroInt()

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

This section of code link here swaps the input to the exact output. Here, swapCoins represents the 4 CANTO threshold set by the administrator:

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

This function TradeInputForExactOutput swaps the input token to acquire an exact amount of the output token:

*
Buy exact amount of a token by specifying the max amount of another token, one of them must be standard token
@param input : max amount of the token to be paid
@param output : exact amount of the token to be bought
@param sender : address of the sender
@param receipt : address of the receiver
@return : actual amount of the token to be paid
*/
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
    }

    // assert that the calculated amount is less than the
    // max amount the buyer is willing to pay.
    if soldTokenAmt.GT(input.Coin.Amount) {
        return sdk.ZeroInt(), sdkerrors.Wrap(types.ErrConstraintNotMet, fmt.Sprintf("insufficient amount of %s, user expected: %s, actual: %s", input.Coin.Denom, input.Coin.Amount.String(), soldTokenAmt.String()))
    }
    soldToken := sdk.NewCoin(input.Coin.Denom, soldTokenAmt)

Here, the problem arises as the minOut is hardcoded to autoSwapThreshold (4 CANTO), without any mechanism to account for changes in the AMM pool liquidity.

The price of CANTO can vary. Let's assume a price rate of 1 CANTO = 0.1 USD. A user who swaps 5 USDC for CANTO would expect to get 50 CANTO. However, due to the hardcoded slippage protection minOut set to 4 CANTO, another trade could land before the user's trade.

Given that the code does not use sequence numbers to check the order of execution and does not validate the timeout expiration of the message, the user's swap could suffer from high slippage.

Consequently, the user might only get 4 CANTO when they expected to get 50 CANTO.

Tools Used

Code Review and manual analysis

Recommended Mitigation Steps

Consider implementing a dynamic minOut that lets the user specify the minimum amount of CANTO token they are willing to receive. Hardcoding the minOut slippage amount can be risky, as the market price can fluctuate. Instead, you might want to apply slippage protection by percentage (e.g., the user should receive at least 95% of the asset).

For instance, if the price rate is 1 CANTO = 0.1 USD, and a user swaps 5 USDC for CANTO expecting to get 50 CANTO, they should be able to choose to get at least 90% of the expected receive amount (i.e., a minimum of 45 CANTO). If this condition cannot be met, the swap should not proceed. This approach would provide a more robust protection against slippage for users.

Assessed type

Token-Transfer

[c4-pre-sort]

JeffCX marked the issue as primary issue

[tkkwon1998]

The way this mechanism is designed, users who use the onboarding module will always receive 4 canto since this is amount of gas token needed to make necessary first transactions.

Non-onboarding users can also choose to swap regularly, without the exact output being 4. The idea is that this price will stay stable due to arbitrage with the main canto dex.

A possible concern is someone manipulating our pools so that onboarding users are paying more for their 4 canto than they should. However, that's why we have limits on how much of a whitelisted token can be sent into the swap tx. Also, in the case that someone does manipulate the pool, they would essentially be losing money because the difference would be arbed away almost immediately.

[c4-sponsor]

tkkwon1998 marked the issue as sponsor disputed

[c4-judge]

0xean marked the issue as unsatisfactory: Insufficient quality