On OnRecvPacket, `TradeInputForExactOutput` is called with all the amount of the transferred coin as a maximum which is not safe.

[code423n4]

Lines of code

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

Vulnerability details

Impact

In OnRecvPacket (IBC receive callback), coinswapKeeper.TradeInputForExactOutput is called to swap from transferredCoin to standardDenom (i.e. canto). TradeInputForExactOutput func takes the input as max amount of the token to be paid. This is not safe for the user. Normally, the max amount is set by the user and used as a limit in case the price of the boughtCoin went up. In that case, the transcation will revert and the user is safe. However, in the auto-swap mechanism here, all the transferredCoin amount is passed. If the standardDenom's price went up for whatever reason (e.g. price manipulation), the user will lose funds from his/her transferredCoin.

Proof of Concept

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

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

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

https://github.com/code-423n4/2023-06-canto/blob/main/Canto/x/coinswap/keeper/swap.go#L169-L173

Tools Used

Manual analysis

Recommended Mitigation Steps

Since this is an auto-swap, consider adding a fixd limit at least to protect the user. for example only a certain percentage of the amount is set as a max.

Assessed type

Other

[c4-pre-sort]

JeffCX marked the issue as duplicate of #74

[c4-judge]

0xean marked the issue as unsatisfactory: Invalid