coinswap pool price manipulation

[c4-bot-7]

Lines of code

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

Vulnerability details

Impact

price manipulation

Proof of Concept

AddLiquidity prevents price manipulation by setting MinLiquidity:

    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()))
    }

    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()))
    }

Unfortunately, RemoveLiquidity invalidates this limit. RemoveLiquidity only limits the minimum amount of Withdraw tokens,
Without checking the amount of liquidity remaining in the pool,

func (k Keeper) RemoveLiquidity(ctx sdk.Context, msg *types.MsgRemoveLiquidity) (sdk.Coins, error) {
    ....
    // There is no limit to the amount of liquidity remaining in the pool
	if tokenWithdrawCoin.Amount.LT(msg.MinToken) {
		return nil, errorsmod.Wrap(types.ErrConstraintNotMet, fmt.Sprintf("token amount not met, user expected: no less than %s, actual: %s", sdk.NewCoin(minTokenDenom, msg.MinToken).String(), tokenWithdrawCoin.String()))
	}
    ....
}

After adding liquidity, the attacker can remove most of the liquidity, and then keep a small amount of liquidity in the pool, by donating tokens to the pool, to manipulate the price of the token pair.

Tools Used

vscode, manual

Recommended Mitigation Steps

Check the amount of liquidity remaining in the pool when removing liquidity

Assessed type

Other

[poorphd]

• Labels
  • sponsor acknowledged
• reasoning
  • It is true that in a general context, the currently implemented coinswap logic can allow for pool price manipulation scenarios as pointed out in the raised issue.
  • However, in Canto, the pools of the coinswap module are not for DEX but for onboarding, and the foundation created and manages all the canto/ibc voucher pool for ibc vouchers that can be onboarded, created to the max size. Therefore, the likelihood of the raised issue occurring is close to zero.
• Severity change required
  • High → Low
  • The probability of the pool's price being manipulated in the context of onboarding in Canto is almost none. There could be problems in the pools created by users, but this has no direct relevance to Canto's security.
• Patch
  • N/A

[3docSec]

Both this finding and #26 incorrectly interpret MinLiquidity as being a minimum quantity of tokens that must be left in the pool, when it's rather a user-provided slippage check.

If we exclude this point that is incorrect, both this and #26 describe a "donation for inflation" attack that is the root cause of the #13 group, and that's why I am merging these two groups

[c4-judge]

3docSec marked the issue as duplicate of #13

[c4-judge]

3docSec changed the severity to QA (Quality Assurance)

[c4-judge]

3docSec marked the issue as grade-b