Lack of deadline parameter when executing swaps

[code423n4]

Lines of code

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

Vulnerability details

Impact

Deadline is not checked. The transaction may stay unexecuted for a long time, resulting in unfavourable trade when the transaction is finally executed.

Proof of Concept

The function OnRecvPacket is used to help users outside of Canto onboard seamlessly. The module automatically swaps a portion of the assets being transferred to Canto network via IBC transfer for Canto if the user has less than 4 Canto without the need for a manual process, and converts the remaining assets to ERC20 tokens on Canto.

        if standardCoinBalance.LT(autoSwapThreshold) {
                swappedAmount, err = k.coinswapKeeper.TradeInputForExactOutput(ctx, coinswaptypes.Input{Coin: transferredCoin, Address: recipient.String()}, coinswaptypes.Output{Coin: swapCoins, Address: recipient.String()})
                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()),
                                        sdk.NewAttribute(coinswaptypes.AttributeValueSender, recipient.String()),
                                        sdk.NewAttribute(coinswaptypes.AttributeValueRecipient, recipient.String()),
                                        sdk.NewAttribute(coinswaptypes.AttributeValueIsBuyOrder, strconv.FormatBool(true)),
                                        sdk.NewAttribute(coinswaptypes.AttributeValueTokenPair, coinswaptypes.GetTokenPairByDenom(transferredCoin.Denom, swapCoins.Denom)),
                                ),
                        )
                }

When converting the tokens to Canto, TradeInputForExactOutput is called in swap.go, which calls calculateWithExactOutput. There is no deadline check when swapping the positions through the pool.

func (k Keeper) calculateWithExactOutput(ctx sdk.Context, exactBoughtCoin sdk.Coin, soldTokenDenom string) (sdk.Int, error) {
        lptDenom, err := k.GetLptDenomFromDenoms(ctx, exactBoughtCoin.Denom, soldTokenDenom)
        if err != nil {
                return sdk.ZeroInt(), err
        }

        poolAddr := types.GetReservePoolAddr(lptDenom).String()
        reservePool, err := k.GetPoolBalances(ctx, poolAddr)
        if err != nil {
                return sdk.ZeroInt(), err
        }

        outputReserve := reservePool.AmountOf(exactBoughtCoin.Denom)
        inputReserve := reservePool.AmountOf(soldTokenDenom)

        if !inputReserve.IsPositive() {
                return sdk.ZeroInt(), sdkerrors.Wrap(types.ErrInsufficientFunds, fmt.Sprintf("reserve pool insufficient balance: [%s%s]", inputReserve.String(), soldTokenDenom))
        }
        if !outputReserve.IsPositive() {
                return sdk.ZeroInt(), sdkerrors.Wrap(types.ErrInsufficientFunds, fmt.Sprintf("reserve pool insufficient balance: [%s%s]", outputReserve.String(), exactBoughtCoin.Denom))
        }
        if exactBoughtCoin.Amount.GTE(outputReserve) {
                return sdk.ZeroInt(), sdkerrors.Wrap(types.ErrInsufficientFunds, fmt.Sprintf("reserve pool insufficient balance of %s, user expected: %s, actual: %s", exactBoughtCoin.Denom, exactBoughtCoin.Amount.String(), outputReserve.String()))
        }
        param := k.GetParams(ctx)

        soldTokenAmt := GetOutputPrice(exactBoughtCoin.Amount, inputReserve, outputReserve, param.Fee)
        return soldTokenAmt, nil
}

Deadline check is important to maintain favourable trades. For example, if Canto is worth $1 at the moment of the swap and the transaction only executes when Canto reaches $2, then the user has to pay $8 instead of $4 to get their token amounts into the Canto network.

Tools Used

Manual Review

Recommended Mitigation Steps

Recommending setting a deadline parameter in the swap function so that the swap will be declined if too much time has passed without any successful transaction execution.

Assessed type

Other

[c4-pre-sort]

JeffCX marked the issue as primary issue

[tkkwon1998]

There is no need for a deadline, as the swap will either be successful or fail at the time it is called. There's no mempool like Ethereum where TXs can sit in limbo.

[c4-sponsor]

tkkwon1998 marked the issue as sponsor disputed

[c4-judge]

0xean marked the issue as unsatisfactory: Insufficient quality