code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Impact

It is stated in the README that some spend limit are configured for the swaps. This is a security precaution to avoid spending too much tokens for the default 4 CANTO tokens in order to onboard the users if their balance is less than 4 tokens.

As a default, this spend limit should be ~10$ for all tokens. The inital tokens than can be bridged on Canto are USDC, USDT, and ETH, and so their respective spending limit should be 10, 10, 0.01 (~= 19$)

In the current implementation of the code, this spend limit is fetched from calling the GetMaximumSwapAmount function at the time of swaps.

The default swap limit is set in the params.go and as we can see, it's correct for the USDC and USDT (according to their 6 decimals in all EVM chains), but not for ETH. Indeed, the DefaultMaxSwapAmount for ETH is set initially at 1e17, which equals 0.1 ETH.

Intially, the expected value should have been about 10$ for each asset, but here it's about 190$, which is far more than expected (20x).

The impact is that if in the future, the CANTO price goes up and that some ETH are bridged to onboard a new user, then it may swap much more than the expected 0.01 ETH, and may swap up to 0.1 ETH before aborting the swap operation.

Tools Used

Manual inspection

Recommended Mitigation Steps

Update the DefaultMaxSwapAmount to have a default for ETH at 0.01 ETH

    DefaultMaxSwapAmount          = sdk.NewCoins(
        sdk.NewCoin(UsdcIBCDenom, sdk.NewIntWithDecimal(10, 6)),
        sdk.NewCoin(UsdtIBCDenom, sdk.NewIntWithDecimal(10, 6)),
-    sdk.NewCoin(EthIBCDenom, sdk.NewIntWithDecimal(1, 17)),
+    sdk.NewCoin(EthIBCDenom, sdk.NewIntWithDecimal(1, 16)),
    )

Assessed type

Decimal

c4-pre-sort commented 1 year ago

JeffCX marked the issue as duplicate of #8

c4-judge commented 1 year ago

0xean marked the issue as satisfactory