howlbot-integration[bot] commented 4 months ago

Lines of code

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

Vulnerability details

Description

The balances are calculated from k.GetPoolBalances(ctx, pool.EscrowAddress) that actually calls the k.bk.GetAllBalances function.

This function loop through the balance of all the token in the loop. Ultimately, if the array is too large, the function can fail due to insufficient gas.

A malicious actor can create a large number of tokens using AddLiquidity for example.

He could then transfer these tokens to the target pool for an attack.

Impact

This can cause a DoS.

POC

func (k Keeper) GetPoolBalances(ctx sdk.Context, escrowAddress string) (coins sdk.Coins, err error) {
    address, err := sdk.AccAddressFromBech32(escrowAddress)
    if err != nil {
        return coins, err
    }
    acc := k.ak.GetAccount(ctx, address)
    if acc == nil {
        return nil, errorsmod.Wrap(types.ErrReservePoolNotExists, escrowAddress)
    }
    return k.bk.GetAllBalances(ctx, acc.GetAddress()), nil
}

func (k BaseViewKeeper) GetAllBalances(ctx context.Context, addr sdk.AccAddress) sdk.Coins {
    balances := sdk.NewCoins()
    k.IterateAccountBalances(ctx, addr, func(balance sdk.Coin) bool {
        balances = balances.Add(balance)
        return false
    })

    return balances.Sort()
}

Tools Used

Manual review

Recommended Mitigation

bk.GetAllBalances shouldn't be used to get the balances of all tokens. Instead, you need to have the balance of 1 token in the pool.

Assessed type

DoS

poorphd commented 4 months ago

Labels
- sponsor confirmed
reasoning

As raised in the issue, if an attacker sends tokens of various denoms to the reserved pool address, k.GetPoolBalances(ctx, pool.EscrowAddress) could invoke k.bk.GetAllBalances that internally uses iteration, leading to a situation where the operation could fail if the array becomes very large.

However, pool creation is only allowed for whitelisted denoms, so it is impossible to obtain new tokens through AddLiquidity as raised in the issue. (https://github.com/b-harvest/Canto/blob/liquidstaking-module/x/coinswap/keeper/keeper.go#L113-L116, https://github.com/b-harvest/Canto/blob/liquidstaking-module/x/coinswap/types/params.go#L19-L36)
Severity
- Mid → Low
- In the worst-case scenario, the swap or RemoveLiquidity in coinswap might fail, but this only affects the auto swap during onboarding and does not impact the essential functions of the chain.

Patch

We will patch this before v0.50 upgrade.
Change k.GetPoolBalances(ctx, pool.EscrowAddress) so that it does not use k.bk.GetAllBalances and only queries and returns the balance of standard coin, counter party coin, and pool coin.

Make appropriate changes for GetPoolBalances callers

// GetPoolBalances return the liquidity pool by the specified anotherCoinDenom
func (k Keeper) GetPoolBalances(ctx sdk.Context, pool types.Pool) (coins sdk.Coins, err error) {
    address, err := sdk.AccAddressFromBech32(pool.EscrowAddress)
    if err != nil {
        return coins, err
    }
    acc := k.ak.GetAccount(ctx, address)
    if acc == nil {
        return nil, errorsmod.Wrap(types.ErrReservePoolNotExists, pool.EscrowAddress)
    }

    balances := sdk.NewCoins()
    balances.Add(k.bk.GetBalance(ctx, acc.GetAddress(), pool.StandardDenom))
    balances.Add(k.bk.GetBalance(ctx, acc.GetAddress(), pool.CounterpartyDenom))
    balances.Add(k.bk.GetBalance(ctx, acc.GetAddress(), pool.LptDenom))

    return balances, nil
}

3docSec commented 4 months ago

I find M to be appropriate for this group.

Because Canto is connected to other Cosmos networks via IBC, an arbitrary number of token denominations can coexist (and be donated) to an existing pool to DoS its liquidity operations, without any privilege required for an attacker

c4-judge commented 4 months ago

3docSec marked the issue as satisfactory