coinswap liquidity pool susceptible to inflation attacks

[c4-bot-3]

Lines of code

https://github.com/code-423n4/2024-05-canto/blob/d1d51b2293d4689f467b8b1c82bba84f8f7ea008/canto-main/x/coinswap/keeper/keeper.go#L149-L192

Vulnerability details

Impact

The coinswap module implements a constant product AMM enabling users to swap between the standard token and nay other token. However, just like other constant product AMMs, the coinswap module is susceptible to inflation attacks.

The protocol does not implement a minimum liquidity addition amount. So a user can add as little liquidity as they want, even a single wei of each token.

if !exists {
    if err := k.DeductPoolCreationFee(ctx, sender); err != nil {
        return sdk.Coin{}, err
    }
    mintLiquidityAmt = msg.ExactStandardAmt
    // Max value check
    if mintLiquidityAmt.GT(params.MaxStandardCoinPerPool) {
        return sdk.Coin{}, errorsmod.Wrap(types.ErrMaxedStandardDenom, fmt.Sprintf("liquidity amount not met, max standard coin amount: no bigger than %s, actual: %s", params.MaxStandardCoinPerPool.String(), mintLiquidityAmt.String()))
    }
    // slippage check
    if mintLiquidityAmt.LT(msg.MinLiquidity) {
        return sdk.Coin{}, errorsmod.Wrap(types.ErrConstraintNotMet, fmt.Sprintf("liquidity amount not met, user expected: no less than %s, actual: %s", msg.MinLiquidity.String(), mintLiquidityAmt.String()))
    }

    depositToken = sdk.NewCoin(msg.MaxToken.Denom, msg.MaxToken.Amount)
    pool = k.CreatePool(ctx, msg.MaxToken.Denom)

Due to the missing check in the above snippet, a user can open a pool with 1 wei standard and 1 wei token and mint out 1 wei of LP token. Then when the protocol is calculating the balances, it uses the raw balances.

standardReserveAmt := balances.AmountOf(standardDenom)
tokenReserveAmt := balances.AmountOf(msg.MaxToken.Denom)

So the malicious user can donate standard or token amounts externally to the escrow address, and that will change the liquidity in the pool. Say the user donates 1e18 standard and tokens to the pool escrow. The pool will now have 1e18+1 standard and 1e18+1 tokenin the escroe.

When other users try to deposit further liquidity, they will be faced with this steep price. If they deposit 1e18 tokens + 1e18 standard, their Lp will be calculated in the following way.

mintLiquidityAmt = (liquidity.Mul(maxStandardInputAmt)).Quo(standardReserveAmt)

Since liquidity=1 from the last step and maxStandardInputAmt=1e18 and standardReserveAmt=1e18+1, there will be a rounding error and the mintLiquidityAmt will be calculated as 0.

Depositors can prevent this with setting slippage with msg.MinLiquidity, but even a single wei difference will lead to a large loss of funds, and the slippage will always revert if provided. So there will either be a DOS of the pool, or losses by the depositor.

So the earliest depositor can either block the pool with a DOS or steal funds from later depositors. Thus this is a medium severity issue.

Proof of Concept

Say Alice mints out 1 wei of liquidity, by depositing 1 wei standard and 1 wei token. Since there is no minimum mint amount, this is allowed.

Alice then transfers to the contract 1e18 standard and 1e18 token. The pool will now have 1e18+1 standard and 1e18+1 token in the escrow.

Bob comes and tries to deposit 1e18 standard and 1e18 token to the pool. Bob's liquidity amount will be calculated as 1*1e18/(1e18+1) = 0. Thus Bob either loses their deposit, or cannot participate at all.

Tools Used

Manual Review

Recommended Mitigation Steps

Burn the initial 1000 wei of liquidity minted, like is done in uniswap v2 pools.

Assessed type

Math

[poorphd]

• Labels

  • sponsor dispute

• reasoning

  The situation in which the PoC situation described in the raised issue can occur is being prevented by the following code.

  mintLiquidityAmt = (liquidity.Mul(maxStandardInputAmt)).Quo(standardReserveAmt)
  if mintLiquidityAmt.LT(msg.MinLiquidity) {
  	return sdk.Coin{}, errorsmod.Wrap(types.ErrConstraintNotMet, fmt.Sprintf("liquidity amount not met, user expected: no less than %s, actual: %s", msg.MinLiquidity.String(), mintLiquidityAmt.String()))
  }

  Assuming that the issued scenario actually occurs:

  • Alice mints out 1 wei of liquidity, by depositing 1 wei standard and 1 wei token. Since there is no minimum mint amount, this is allowed.
  • Alice then transfers to the contract 1e18 standard and 1e18 token. The pool will now have 1e18+1 standard and 1e18+1 token in the escrow.
  • Bob comes and tries to deposit 1e18 standard and 1e18 token to the pool. Bob's liquidity amount will be calculated as 1*1e18/(1e18+1) = 0. Thus Bob either loses their deposit, or cannot participate at all.

  Although the calculated Bob liquidity is 0, it is possible to set the expected liquidity value through msg.MinLiquidity when calling AddLiquidity, so this scenario can be prevented in advance.

• Severity change requried

  • Mid → not valid
  • Currently, in the implementation of the coinswap module, there is code to protect the user's fund even in the case of this attack scenario. In the worst case, if the user mistakenly sets msg.MinLiquidity to 0, the attack could succeed, but current Canto's swap pools are not for DEX but for onboarding, and the foundation created and manages all the canto/ibc voucher pools (white-listed) for ibc vouchers that can be onboarded, created to the max size. Therefore, the likelihood of the raised issue occurring is close to zero.

• Patch

  • N/A

[3docSec]

Hi @poorphd, my understanding is that a "donation for inflation" attack is an effective way to prevent legitimate liquidity providers away from the pool (or let them in at the price of losing tokens to massive rounding).

However, I am not sure I fully understand this part of your reply:

the foundation created and manages all the canto/ibc voucher pools (white-listed) for ibc vouchers that can be onboarded, created to the max size.

Can you please explain more about this so I have a full picture for judging this group?

Thanks!

[poorphd]

Hi @3docSec,

On Canto, the coinswap pool is used to automatically swap a portion of the assets transferred via IBC for onboarding users. Only a minimum amount of liquidity is provided for this feature, and the max size of the pool is also set.

https://canto-api.synergynodes.com/canto/coinswap/params

{
  "params": {
    "fee": "0.000000000000000000",
    "pool_creation_fee": {
      "denom": "stake",
      "amount": "0"
    },
    "tax_rate": "0.000000000000000000",
    "max_standard_coin_per_pool": "10000000000000000000000",
    "max_swap_amount": [
      {
        "denom": "ibc/17CD484EE7D9723B847D95015FA3EBD1572FD13BC84FB838F55B18A57450F25B",
        "amount": "10000000"
      },
      {
        "denom": "ibc/4F6A2DEFEA52CD8D90966ADCB2BD0593D3993AB0DF7F6AEB3EFD6167D79237B0",
        "amount": "10000000"
      },
      {
        "denom": "ibc/DC186CA7A8C009B43774EBDC825C935CABA9743504CE6037507E6E5CCE12858A",
        "amount": "10000000000000000"
      }
    ]
  }
}

params := k.GetParams(ctx)
	if !params.MaxSwapAmount.AmountOf(msg.MaxToken.Denom).IsPositive() {
		return sdk.Coin{}, errorsmod.Wrapf(types.ErrInvalidDenom,
			"MaxToken %s is not registered in max swap amount", msg.MaxToken.Denom)
	}

Anyone can create a pool, but only pools for the IBC voucher set in the parameter can be created. Currently, Canto has only 3 IBC vouchers whitelisted, and the foundation has already created 3 pools for these vouchers at max size (maximum liquidity of the pool). Therefore, the probability of the attack scenario raised in the issue occurring is very low as you would have to donate a very large amount for the “donation for inflation”.

[3docSec]

Ok thanks for the clarification. In my opinion, there is the possibility to backrun an upgrade or governance proposal to add a new MaxToken, but this can be mitigated by the fact that x/governance proposals like the TextProposal are multi-message, so the team has the tools to atomically add a max token and provide liquidity for its pool at cap.

Adding #7 and #26 to this pool as they are different nuances of the same root cause (donation for inflation).

Marking the whole group as QA because:

• the "monetary" effect of a price manipulation is mitigated by the presence of a max swap amount check
• the inflation attack can effectively prevent other participants from providing liquidity to the pool, so there is the possibility for an availability impact
• however, the attack depends strictly on a particular configuration set to the protocol (MaxToken registered with the pool left to be created), which the team has the knowledge and tools to avoid, so the conditions for this attack to happen can be created by what I consider an admin error

[c4-judge]

3docSec changed the severity to QA (Quality Assurance)

[c4-judge]

3docSec marked the issue as grade-b