Canto - sces60107's results

Lines of code

https://github.com/code-423n4/2023-06-canto/blob/main/Canto/x/coinswap/keeper/swap.go#L212 https://github.com/code-423n4/2023-06-canto/blob/main/Canto/x/coinswap/types/params.go#L34

Vulnerability details

Impact

In the spec, the pre-defined limit of ETH is 0.01 ETHs. But the actual limit in the code is not 0.01 ETH which could result in misleading.

Proof of Concept

In the spec, it said that the pre-defined limit of ETH is 0.01 ETHs https://github.com/code-423n4/2023-06-canto/blob/main/README.md#swap

For risk management purposes, a swap will fail if the input coin amount exceeds a pre-defined limit (10 USDC, 10 USDT, 0.01 ETH) or if the swap amount limit is not defined.

But in x/coinswap/types/params.go, the actual limit of ETH is 1*10e17 which is 0.1 ETH

// Parameter store keys
var (
	KeyFee                    = []byte("Fee")                    // fee key
	KeyPoolCreationFee        = []byte("PoolCreationFee")        // fee key
	KeyTaxRate                = []byte("TaxRate")                // fee key
	KeyStandardDenom          = []byte("StandardDenom")          // standard token denom key
	KeyMaxStandardCoinPerPool = []byte("MaxStandardCoinPerPool") // max standard coin amount per pool
	KeyMaxSwapAmount          = []byte("MaxSwapAmount")          // whitelisted denoms

	DefaultFee                    = sdk.NewDecWithPrec(0, 0)
	DefaultPoolCreationFee        = sdk.NewInt64Coin(sdk.DefaultBondDenom, 0)
	DefaultTaxRate                = sdk.NewDecWithPrec(0, 0)
	DefaultMaxStandardCoinPerPool = sdk.NewIntWithDecimal(10000, 18)
	DefaultMaxSwapAmount          = sdk.NewCoins(
		sdk.NewCoin(UsdcIBCDenom, sdk.NewIntWithDecimal(10, 6)),
		sdk.NewCoin(UsdtIBCDenom, sdk.NewIntWithDecimal(10, 6)),
		sdk.NewCoin(EthIBCDenom, sdk.NewIntWithDecimal(1, 17)),
	)
)

The limit is used in swap.GetMaximumSwapAmount. Wrong could harm the risk management. https://github.com/code-423n4/2023-06-canto/blob/main/Canto/x/coinswap/keeper/swap.go#L212

func (k Keeper) GetMaximumSwapAmount(ctx sdk.Context, denom string) (sdk.Coin, error) {
	params := k.GetParams(ctx)
	for _, coin := range params.MaxSwapAmount {
		if coin.Denom == denom {
			return coin, nil
		}
	}
	return sdk.Coin{}, sdkerrors.Wrap(types.ErrInvalidDenom, fmt.Sprintf("invalid denom: %s, denom is not whitelisted", denom))
}

Tools Used

Manual Review

Recommended Mitigation Steps

0.01 ETH should be sdk.NewIntWithDecimal(1, 16)

Assessed type

Error

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