code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-06-canto/blob/a4ff2fd2e67e77e36528fad99f9d88149a5e8532/Canto/x/coinswap/keeper/swap.go#L72-L115 https://github.com/code-423n4/2023-06-canto/blob/a4ff2fd2e67e77e36528fad99f9d88149a5e8532/Canto/x/coinswap/keeper/swap.go#L163-L208

Vulnerability details

Impact

As for the business logic, it is expected that the number of Canto tokens in each pool should not exceed the predefined limit, which is specified as MaxStandardCoinPerPool. However, the current implementation in the source code does not enforce control over the amount of Canto tokens in the pools. This will have an unexpected impact on the value of the Canto token.

Proof of Concept

Assume that currently Bob has funds on Canto Network. There is a pool CANTO/BTC with reserves 10000000000 CANTO and 10000000000 BTC. Bob performs a swap action, exchanging exactly 10 CANTO for BTC. With the MaxStandardCoinPerPool is 10000000005, the transaction is still successful and Bob receives 9 BTC.

Unit test:


func (suite *TestSuite) TestTradeExactInputForOutput_ExceedMaxStandardTokenPerPool_Successfully() {
    sender, poolAddr := createReservePool(suite, denomBTC)

    params := suite.app.CoinswapKeeper.GetParams(suite.ctx)
    fmt.Printf("MaxStandardCoinPerPool: %s\n", params.MaxStandardCoinPerPool.String())

    inputCoin := sdk.NewCoin(denomStandard, sdk.NewInt(10))
    outputCoin := sdk.NewCoin(denomBTC, sdk.NewInt(0))
    input := types.Input{
        Address: sender.String(),
        Coin:    inputCoin,
    }
    output := types.Output{
        Address: sender.String(),
        Coin:    outputCoin,
    }

    poolBalances := suite.app.BankKeeper.GetAllBalances(suite.ctx, poolAddr)
    senderBlances := suite.app.BankKeeper.GetAllBalances(suite.ctx, sender)

    fmt.Printf("[BEFORE]\npool balances: %s\nsender balances: %s\n\n", poolBalances.String(), senderBlances.String())

    amt, err := suite.app.CoinswapKeeper.TradeExactInputForOutput(suite.ctx, input, output)
    suite.NoError(err)

    fmt.Printf("output amount: %s%s\n\n", amt.String(), outputCoin.Denom)

    sold := sdk.NewCoins(inputCoin)
    bought := sdk.NewCoins(sdk.NewCoin(outputCoin.Denom, amt))

    pb := poolBalances.Add(sold...).Sub(bought)
    sb := senderBlances.Add(bought...).Sub(sold)

    fmt.Printf("[AFTER]\npool balances: %s\nsender balances: %s\n", pb.String(), sb.String())

    assertResult(suite, poolAddr, sender, pb, sb)
}

Log of the test:

MaxStandardCoinPerPool: 10000000005

[BEFORE]
pool balances: 10000000000btc,10000000000canto
sender balances: 20000000000btc,10000000000lpt-1,20000000000canto

-------------------

output amount: 9btc

-------------------

[AFTER]
pool balances: 9999999991btc,10000000010canto
sender balances: 20000000009btc,10000000000lpt-1,19999999990canto

PASS
ok      github.com/Canto-Network/Canto/v6/x/coinswap/keeper 0.644s

Recommended Mitigation Steps

Add logic to functions TradeExactInputForOutput and TradeInputForExactOutput to check if standard denomination amount exceeds the limit.

Function TradeExactInputForOutput:

func (k Keeper) TradeExactInputForOutput(ctx sdk.Context, input types.Input, output types.Output) (sdk.Int, error) {
    // ... keep the same as original

    if quoteCoinToSwap.Amount.GT(maxSwapAmount.Amount) {
        return sdk.ZeroInt(), sdkerrors.Wrap(types.ErrConstraintNotMet, fmt.Sprintf("expected swap amount %s%s exceeding swap amount limit %s%s", quoteCoinToSwap.Amount.String(), quoteCoinToSwap.Denom, maxSwapAmount.Amount.String(), maxSwapAmount.Denom))
    }

+   if input.Coin.Denom == standardDenom {
+       lptDenom, err := k.GetLptDenomFromDenoms(ctx, input.Coin.Denom, output.Coin.Denom)
+       if err != nil {
+           return sdk.ZeroInt(), err
+       }
+       reservePoolAddress := types.GetReservePoolAddr(lptDenom).String()
+       reservePool, err := k.GetPoolBalances(ctx, reservePoolAddress)
+       if err != nil {
+           return sdk.ZeroInt(), err
+       }
+       standardDenomReserve := reservePool.AmountOf(standardDenom)
+       if standardDenomReserve.Add(input.Coin.Amount).GT(k.GetParams(ctx).MaxStandardCoinPerPool) {
+           return sdk.ZeroInt(), sdkerrors.Wrap(types.ErrConstraintNotMet, fmt.Sprintf("expected swap amount %s%s exceeding max standard coin per pool", input.Coin.Amount.String(), standardDenom))
+       }
+   }

    if err := k.swapCoins(ctx, inputAddress, outputAddress, input.Coin, boughtToken); err != nil {
        return sdk.ZeroInt(), err
    }
    return boughtTokenAmt, nil
}

Function TradeInputForExactOutput:

func (k Keeper) TradeInputForExactOutput(ctx sdk.Context, input types.Input, output types.Output) (sdk.Int, error) {
    // ... keep the same as original

    if quoteCoinToSwap.Amount.GT(maxSwapAmount.Amount) {
        return sdk.ZeroInt(), sdkerrors.Wrap(types.ErrConstraintNotMet, fmt.Sprintf("expected swap amount %s%s exceeding swap amount limit %s%s", quoteCoinToSwap.Amount.String(), quoteCoinToSwap.Denom, maxSwapAmount.Amount.String(), maxSwapAmount.Denom))
    }

+   if soldToken.Denom == standardDenom {
+       lptDenom, err := k.GetLptDenomFromDenoms(ctx, input.Coin.Denom, output.Coin.Denom)
+       if err != nil {
+           return sdk.ZeroInt(), err
+       }
+       reservePoolAddress := types.GetReservePoolAddr(lptDenom).String()
+       reservePool, err := k.GetPoolBalances(ctx, reservePoolAddress)
+       if err != nil {
+           return sdk.ZeroInt(), err
+       }
+       standardDenomReserve := reservePool.AmountOf(standardDenom)
+       if standardDenomReserve.Add(soldTokenAmt).GT(k.GetParams(ctx).MaxStandardCoinPerPool) {
+           return sdk.ZeroInt(), sdkerrors.Wrap(types.ErrConstraintNotMet, fmt.Sprintf("expected swap amount %s%s exceeding max standard coin per pool", soldTokenAmt.String(), standardDenom))
+       }
+   }

    if err := k.swapCoins(ctx, inputAddress, outputAddress, soldToken, output.Coin); err != nil {
        return sdk.ZeroInt(), err
    }

    return soldTokenAmt, nil
}

Assessed type

Other

c4-pre-sort commented 1 year ago

JeffCX marked the issue as primary issue

tkkwon1998 commented 1 year ago

Our docs should've been more clear. The deposits to the pool fail if the number of canto exceeds 10,000. Swaps can still be made even if it increases the canto reserves to above 10,000. There cannot be a hard cap for reserves in an AMM, because then you would be limiting price movement.