Merge pull request #344 from neutron-org/fix_deposit_spread

Audit Fix: prevent depositor or LP from creating unfair spreads

proto/neutron/dex/params.proto

@@ -8,5 +8,12 @@ option go_package = "github.com/neutron-org/neutron/x/dex/types";
 // Params defines the parameters for the module.
 message Params {
   option (gogoproto.goproto_stringer) = false;
-  repeated uint64 fee_tiers = 1; 
+  repeated uint64 fee_tiers = 1;
+  string max_true_taker_spread = 2 [
+                                    (gogoproto.moretags)   = "yaml:\"max_true_taker_spread\"",
+                                    (gogoproto.customtype) = "github.com/neutron-org/neutron/utils/math.PrecDec",
+                                    (gogoproto.nullable)   = false,
+                                    (gogoproto.jsontag) = "max_true_taker_spread"
+                                    ];
+
 }

tests/ibc/gmp_swap_forward_test.go

@@ -99,10 +99,11 @@ func (s *IBCTestSuite) TestGMPSwapAndForward_Success() {
 	// Check that the funds are moved out of the acc on providerChain
 	s.assertProviderBalance(s.providerAddr, nativeDenom, newProviderBalNative.Sub(ibcTransferAmount))
 
-	// Check that the amountIn is deduced from the neutron account
-	s.assertNeutronBalance(s.neutronAddr, s.providerToNeutronDenom, math.ZeroInt())
-	// Check that neutron account did not keep any of the transfer denom
-	s.assertNeutronBalance(s.neutronAddr, nativeDenom, genesisWalletAmount.Sub(swapAmount))
+	// Check that the amountIn is deducted from the neutron override account
+	overrideAddr := s.ReceiverOverrideAddr(s.neutronTransferPath.EndpointA.ChannelID, s.providerAddr.String())
+	s.assertNeutronBalance(overrideAddr, s.providerToNeutronDenom, math.OneInt())
+	// Check that neutron override account did not keep any of the transfer denom
+	s.assertNeutronBalance(overrideAddr, nativeDenom, math.ZeroInt())
 
 	transferDenomPath := transfertypes.GetPrefixedDenom(
 		transfertypes.PortID,

tests/ibc/swap_forward_test.go

@@ -109,10 +109,11 @@ func (s *IBCTestSuite) TestSwapAndForward_Success() {
 		newProviderBalNative.Sub(ibcTransferAmount),
 	)
 
-	// Check that the amountIn is deducted from the neutron account
-	s.assertNeutronBalance(s.neutronAddr, s.providerToNeutronDenom, math.ZeroInt())
+	// Check that the amountIn is deducted from the neutron overrid receiver account
+	overrideAddr := s.ReceiverOverrideAddr(s.neutronTransferPath.EndpointA.ChannelID, s.providerAddr.String())
+	s.assertNeutronBalance(overrideAddr, s.providerToNeutronDenom, math.OneInt())
 	// Check that neutron account did not keep any of the transfer denom
-	s.assertNeutronBalance(s.neutronAddr, nativeDenom, genesisWalletAmount.Sub(swapAmount))
+	s.assertNeutronBalance(overrideAddr, nativeDenom, math.ZeroInt())
 
 	transferDenomPath := transfertypes.GetPrefixedDenom(
 		transfertypes.PortID,
@@ -284,7 +285,7 @@ func (s *IBCTestSuite) TestSwapAndForward_UnwindIBCDenomSuccess() {
 		s.providerToNeutronDenom,
 		depositAmount,
 		depositAmount,
-		0,
+		1,
 		1,
 		s.neutronAddr)
 
@@ -293,8 +294,8 @@ func (s *IBCTestSuite) TestSwapAndForward_UnwindIBCDenomSuccess() {
 	postDepositNeutronBalNative := genesisWalletAmount.Sub(depositAmount)
 	s.assertNeutronBalance(s.neutronAddr, nativeDenom, postDepositNeutronBalNative)
 
-	swapAmount := math.NewInt(100000)
-	expectedAmountOut := math.NewInt(99990)
+	swapAmount := math.NewInt(100_000)
+	expectedAmountOut := math.NewInt(99980)
 
 	retries := uint8(0)
 
@@ -357,7 +358,7 @@ func (s *IBCTestSuite) TestSwapAndForward_UnwindIBCDenomSuccess() {
 		s.providerAddr,
 		s.neutronAddr,
 		transferDenomNeutronProvider,
-		ibcTransferAmount,
+		swapAmount,
 		string(metadataBz),
 	)
 
@@ -366,6 +367,9 @@ func (s *IBCTestSuite) TestSwapAndForward_UnwindIBCDenomSuccess() {
 	s.Assert().NoError(err)
 	s.coordinator.CommitBlock(s.neutronChain)
 
+	// Check that the amountIn is deducted from the neutron override receiever  account
+	overrideAddr := s.ReceiverOverrideAddr(s.neutronTransferPath.EndpointA.ChannelID, s.providerAddr.String())
+	s.assertNeutronBalance(overrideAddr, nativeDenom, math.OneInt())
 	// Check that the funds are now present on the provider chainer
 	s.assertProviderBalance(
 		s.providerAddr,
@@ -589,6 +593,16 @@ func (s *IBCTestSuite) TestSwapAndForward_ForwardFails() {
 	s.assertNeutronBalance(overrideAddr, s.providerToNeutronDenom, math.ZeroInt())
 	s.assertNeutronBalance(overrideAddr, nativeDenom, math.ZeroInt())
 
+	// Check that nothing made it to chainB
+	transferDenomPath := transfertypes.GetPrefixedDenom(
+		transfertypes.PortID,
+		s.neutronChainBPath.EndpointA.ChannelID,
+		nativeDenom,
+	)
+	transferDenomNeutronChainB := transfertypes.ParseDenomTrace(transferDenomPath).IBCDenom()
+
+	s.assertChainBBalance(chainBAddr, transferDenomNeutronChainB, math.ZeroInt())
+
 	// Check that the refund takes place and the funds are moved back to the account on Gaia
 	s.assertProviderBalance(s.providerAddr, nativeDenom, genesisWalletAmount.Sub(depositAmount))
 }

tests/ibc/swap_test.go

@@ -83,7 +83,8 @@ func (s *IBCTestSuite) TestIBCSwapMiddleware_Success() {
 	s.assertNeutronBalance(s.neutronAddr, nativeDenom, postDepositNeutronBalNative.Add(expectedOut))
 
 	// Check that all of the IBC transfer denom have been used up
-	s.assertNeutronBalance(s.neutronAddr, s.providerToNeutronDenom, math.ZeroInt())
+	overrideAddr := s.ReceiverOverrideAddr(s.neutronTransferPath.EndpointA.ChannelID, s.providerAddr.String())
+	s.assertNeutronBalance(overrideAddr, s.providerToNeutronDenom, math.OneInt())
 }
 
 // TestIBCSwapMiddleware_FailRefund asserts that the IBC swap middleware works as intended with Neutron running as a

x/dex/keeper/integration_deposit_doublesided_test.go

@@ -232,19 +232,19 @@ func (s *DexTestSuite) TestDepositValueAccural() {
 			s.bobLimitSells("TokenA", 10, 1000, types.LimitOrderType_IMMEDIATE_OR_CANCEL)
 		}
 	}
-	s.assertLiquidityAtTickInt(math.NewInt(110516593), math.ZeroInt(), 0, 10)
-	s.assertDexBalancesInt(math.NewInt(110516593), math.ZeroInt())
+	s.assertLiquidityAtTickInt(math.NewInt(110516491), math.ZeroInt(), 0, 10)
+	s.assertDexBalancesInt(math.NewInt(110516491), math.ZeroInt())
 
-	s.assertLiquidityAtTickInt(math.NewInt(110516593), math.ZeroInt(), 0, 10)
+	s.assertLiquidityAtTickInt(math.NewInt(110516491), math.ZeroInt(), 0, 10)
 	// Carol deposits 100TokenA @tick0
 	s.carolDeposits(NewDeposit(100, 0, 0, 10))
-	s.assertLiquidityAtTickInt(math.NewInt(210516593), math.ZeroInt(), 0, 10)
-	s.assertAccountSharesInt(s.carol, 0, 10, math.NewInt(90484150))
+	s.assertLiquidityAtTickInt(math.NewInt(210516491), math.ZeroInt(), 0, 10)
+	s.assertAccountSharesInt(s.carol, 0, 10, math.NewInt(90484233))
 
 	// Alice gets back 100% of the accrued trade value
 	s.aliceWithdraws(NewWithdrawal(100, 0, 10))
-	s.assertAliceBalancesInt(math.NewInt(110516593), math.NewInt(0))
+	s.assertAliceBalancesInt(math.NewInt(110516491), math.NewInt(0))
 	// AND carol get's back only what she put in
-	s.carolWithdraws(NewWithdrawalInt(math.NewInt(90484150), 0, 10))
+	s.carolWithdraws(NewWithdrawalInt(math.NewInt(90484233), 0, 10))
 	s.assertCarolBalances(100, 1)
 }

x/dex/keeper/integration_deposit_singlesided_test.go

@@ -277,7 +277,7 @@ func (s *DexTestSuite) TestDepositSingleSidedCreatingArbToken0() {
 	// Bob arbs
 	s.bobLimitSells("TokenB", -1, 50, types.LimitOrderType_IMMEDIATE_OR_CANCEL)
 	s.bobLimitSells("TokenA", 1, 10)
-	s.assertBobBalancesInt(sdkmath.NewInt(50_000_000), sdkmath.NewInt(53_294_995))
+	s.assertBobBalancesInt(sdkmath.NewInt(50_000_000), sdkmath.NewInt(53_294_996))
 }
 
 func (s *DexTestSuite) TestDepositSingleSidedCreatingArbToken1() {
@@ -302,7 +302,7 @@ func (s *DexTestSuite) TestDepositSingleSidedCreatingArbToken1() {
 	// Bob arbs
 	s.bobLimitSells("TokenA", -1, 50, types.LimitOrderType_IMMEDIATE_OR_CANCEL)
 	s.bobLimitSells("TokenB", -1, 10)
-	s.assertBobBalancesInt(sdkmath.NewInt(53_295_665), sdkmath.NewInt(50_000_000))
+	s.assertBobBalancesInt(sdkmath.NewInt(53_295_666), sdkmath.NewInt(50_000_000))
 }
 
 func (s *DexTestSuite) TestDepositSingleSidedMultiA() {

x/dex/keeper/integration_multihopswap_test.go

@@ -50,9 +50,9 @@ func (s *DexTestSuite) TestMultiHopSwapSingleRoute() {
 
 	// GIVEN liquidity in pools A<>B, B<>C, C<>D,
 	s.SetupMultiplePools(
-		NewPoolSetup("TokenA", "TokenB", 0, 100, 0, 1),
-		NewPoolSetup("TokenB", "TokenC", 0, 100, 0, 1),
-		NewPoolSetup("TokenC", "TokenD", 0, 100, 0, 1),
+		NewPoolSetup("TokenA", "TokenB", 0, 100, -1, 1),
+		NewPoolSetup("TokenB", "TokenC", 0, 100, -1, 1),
+		NewPoolSetup("TokenC", "TokenD", 0, 100, -1, 1),
 	)
 
 	// WHEN alice multihopswaps A<>B => B<>C => C<>D,
@@ -61,12 +61,12 @@ func (s *DexTestSuite) TestMultiHopSwapSingleRoute() {
 
 	// THEN alice gets out 99 TokenD
 	s.assertAccountBalanceWithDenom(s.alice, "TokenA", 0)
-	s.assertAccountBalanceWithDenomInt(s.alice, "TokenD", math.NewInt(99_970_003))
+	s.assertAccountBalanceWithDenom(s.alice, "TokenD", 100)
 
 	s.assertDexBalanceWithDenom("TokenA", 100)
 	s.assertDexBalanceWithDenom("TokenB", 100)
 	s.assertDexBalanceWithDenom("TokenC", 100)
-	s.assertDexBalanceWithDenomInt("TokenD", math.NewInt(29_997))
+	s.assertDexBalanceWithDenom("TokenD", 0)
 }
 
 func (s *DexTestSuite) TestMultiHopSwapInsufficientLiquiditySingleRoute() {
@@ -116,15 +116,15 @@ func (s *DexTestSuite) TestMultiHopSwapMultiRouteOneGood() {
 
 	// GIVEN viable liquidity in pools A<>B, B<>E, E<>X
 	s.SetupMultiplePools(
-		NewPoolSetup("TokenA", "TokenB", 0, 100, 0, 1),
+		NewPoolSetup("TokenA", "TokenB", 0, 100, -1, 1),
 		NewPoolSetup("TokenB", "TokenC", 0, 100, 0, 1),
 		NewPoolSetup("TokenC", "TokenX", 0, 50, 0, 1),
 		NewPoolSetup("TokenC", "TokenX", 0, 50, 2200, 1),
 		NewPoolSetup("TokenB", "TokenD", 0, 100, 0, 1),
 		NewPoolSetup("TokenD", "TokenX", 0, 50, 0, 1),
 		NewPoolSetup("TokenD", "TokenX", 0, 50, 2200, 1),
-		NewPoolSetup("TokenB", "TokenE", 0, 100, 0, 1),
-		NewPoolSetup("TokenE", "TokenX", 0, 100, 0, 1),
+		NewPoolSetup("TokenB", "TokenE", 0, 100, -1, 1),
+		NewPoolSetup("TokenE", "TokenX", 0, 100, -1, 1),
 	)
 
 	// WHEN alice multihopswaps with three routes the first two routes fail and the third works
@@ -137,26 +137,26 @@ func (s *DexTestSuite) TestMultiHopSwapMultiRouteOneGood() {
 
 	// THEN swap succeeds through route A<>B, B<>E, E<>X
 	s.assertAccountBalanceWithDenom(s.alice, "TokenA", 0)
-	s.assertAccountBalanceWithDenomInt(s.alice, "TokenX", math.NewInt(99_970_003))
-	s.assertLiquidityAtTickWithDenomInt(
+	s.assertAccountBalanceWithDenomInt(s.alice, "TokenX", math.NewInt(100_000_000))
+	s.assertLiquidityAtTickWithDenom(
 		&types.PairID{Token0: "TokenA", Token1: "TokenB"},
-		math.NewInt(99_999_999),
-		math.NewInt(10_000),
+		100,
 		0,
+		-1,
 		1,
 	)
-	s.assertLiquidityAtTickWithDenomInt(
+	s.assertLiquidityAtTickWithDenom(
 		&types.PairID{Token0: "TokenB", Token1: "TokenE"},
-		math.NewInt(99_990_000),
-		math.NewInt(19_999),
+		100,
 		0,
+		-1,
 		1,
 	)
-	s.assertLiquidityAtTickWithDenomInt(
+	s.assertLiquidityAtTickWithDenom(
 		&types.PairID{Token0: "TokenE", Token1: "TokenX"},
-		math.NewInt(99_980_001),
-		math.NewInt(29_997),
+		100,
 		0,
+		-1,
 		1,
 	)
 
@@ -276,22 +276,22 @@ func (s *DexTestSuite) TestMultiHopSwapMultiRouteFindBestRoute() {
 	s.assertAccountBalanceWithDenomInt(s.alice, "TokenX", math.NewInt(134_943_366))
 	s.assertLiquidityAtTickWithDenomInt(
 		&types.PairID{Token0: "TokenA", Token1: "TokenB"},
-		math.NewInt(99_999_999),
+		math.NewInt(99_999_998),
 		math.NewInt(10000),
 		0,
 		1,
 	)
 	s.assertLiquidityAtTickWithDenomInt(
 		&types.PairID{Token0: "TokenB", Token1: "TokenE"},
-		math.NewInt(99_990_000),
+		math.NewInt(99_989_999),
 		math.NewInt(19_999),
 		0,
 		1,
 	)
 
 	s.assertLiquidityAtTickWithDenomInt(
 		&types.PairID{Token0: "TokenE", Token1: "TokenX"},
-		math.NewInt(99_980_001),
+		math.NewInt(99_980_000),
 		math.NewInt(865_056_634),
 		-3000,
 		1,
@@ -369,7 +369,7 @@ func (s *DexTestSuite) TestMultiHopSwapLongRouteWithCache() {
 	s.assertAccountBalanceWithDenomInt(s.alice, "TokenX", math.NewInt(99_880_066))
 	s.assertLiquidityAtTickWithDenomInt(
 		&types.PairID{Token0: "TokenM", Token1: "TokenX"},
-		math.NewInt(99_890_055),
+		math.NewInt(99_890_054),
 		math.NewInt(119_934),
 		0,
 		1,

x/dex/keeper/integration_placelimitorder_test.go

@@ -389,7 +389,7 @@ func (s *DexTestSuite) TestPlaceLimitOrderFoK0TotalAmountInNotUsed() {
 	// WHEN alice submits FoK limitOrder for 9998 it succeeds
 	// even though trueAmountIn < specifiedAmountIn due to rounding
 	s.aliceLimitSells("TokenA", 21000, 9998, types.LimitOrderType_FILL_OR_KILL)
-	s.assertAliceBalancesInt(sdkmath.NewInt(5), sdkmath.NewInt(1_353_046_854))
+	s.assertAliceBalancesInt(sdkmath.NewInt(7), sdkmath.NewInt(1_353_046_854))
 }
 
 func (s *DexTestSuite) TestPlaceLimitOrderFoK1TotalAmountInNotUsed() {
@@ -403,7 +403,7 @@ func (s *DexTestSuite) TestPlaceLimitOrderFoK1TotalAmountInNotUsed() {
 	// WHEN alice submits FoK limitOrder for 9998 it succeeds
 	// even though trueAmountIn < specifiedAmountIn due to rounding
 	s.aliceLimitSells("TokenB", -21000, 9998, types.LimitOrderType_FILL_OR_KILL)
-	s.assertAliceBalancesInt(sdkmath.NewInt(135_3046_854), sdkmath.NewInt(5))
+	s.assertAliceBalancesInt(sdkmath.NewInt(135_3046_854), sdkmath.NewInt(7))
 }
 
 func (s *DexTestSuite) TestPlaceLimitOrderFoKMaxOutUsed() {
@@ -417,7 +417,7 @@ func (s *DexTestSuite) TestPlaceLimitOrderFoKMaxOutUsed() {
 	s.aliceLimitSellsWithMaxOut("TokenB", 0, 50, 20)
 
 	// THEN alice swap ~19 BIGTokenB and gets back 20 BIGTokenA
-	s.assertAliceBalancesInt(sdkmath.NewInt(20_000_000), sdkmath.NewInt(31_162_769))
+	s.assertAliceBalancesInt(sdkmath.NewInt(20_000_000), sdkmath.NewInt(31_162_770))
 }
 
 func (s *DexTestSuite) TestPlaceLimitOrderFoKMaxOutUsedMultiTick() {
@@ -433,7 +433,7 @@ func (s *DexTestSuite) TestPlaceLimitOrderFoKMaxOutUsedMultiTick() {
 	s.aliceLimitSellsWithMaxOut("TokenB", 0, 50, 20)
 
 	// THEN alice swap ~19 BIGTokenB and gets back 20 BIGTokenA
-	s.assertAliceBalancesInt(sdkmath.NewInt(20_000_000), sdkmath.NewInt(31_165_594))
+	s.assertAliceBalancesInt(sdkmath.NewInt(20_000_000), sdkmath.NewInt(31_165_596))
 }
 
 // Immediate Or Cancel LimitOrders ////////////////////////////////////////////////////////////////////
@@ -503,6 +503,43 @@ func (s *DexTestSuite) TestPlaceLimitOrderIoCWithLPNoFill() {
 	s.assertLimitLiquidityAtTick("TokenA", 1, 0)
 }
 
+func (s *DexTestSuite) TestPlaceLimitOrderIoCUnfairPriceFails() {
+	s.fundAliceBalances(10, 0)
+	s.fundBobBalances(0, 1)
+	// GIVEN LP of 1 TokenB at tick -20
+	s.bobDeposits(NewDeposit(0, 1, -20, 1))
+	// WHEN alice submits IoC limitOrder for 2 tokenA
+	_, err := s.limitSellsInt(s.alice, "TokenA", 10, sdkmath.NewInt(2), types.LimitOrderType_IMMEDIATE_OR_CANCEL)
+
+	// THEN alice's LimitOrder failswith SwapAmountTooSmall
+	s.ErrorIs(err, types.ErrSwapAmountTooSmall)
+
+	// Nothing is changed
+	s.assertAliceBalances(10, 0)
+	s.assertLiquidityAtTick(0, 1, -20, 1)
+}
+
+func (s *DexTestSuite) TestPlaceLimitOrderIoCUnfairPriceAbortsEarly() {
+	s.fundAliceBalances(1, 0)
+	s.fundBobBalances(0, 2)
+	// GIVEN LP of 1 TokenB at ticks -20 & -21
+	s.bobDeposits(
+		NewDeposit(0, 1, -21, 1),
+		NewDeposit(0, 1, -20, 1),
+	)
+	// WHEN alice submits IoC limitOrder for 999_004 small TokenA
+	_, err := s.limitSellsInt(s.alice, "TokenA", 10, sdkmath.NewInt(998_004), types.LimitOrderType_IMMEDIATE_OR_CANCEL)
+
+	// THEN alice's LimitOrder swaps through the first tick, but does not swap the second tick due to unfair pricing
+	s.NoError(err)
+
+	// The first tick swapped and the second tick is not
+	s.assertLiquidityAtTickInt(sdkmath.NewInt(998_002), sdkmath.ZeroInt(), -21, 1)
+	s.assertLiquidityAtTick(0, 1, -20, 1)
+	// only the first swap is deducted from alices balance
+	s.assertAliceBalancesInt(sdkmath.NewInt(1998), sdkmath.NewInt(1_000_000))
+}
+
 // Just In Time Limit Orders //////////////////////////////////////////////////
 
 func (s *DexTestSuite) TestPlaceLimitOrderJITFills() {
@@ -542,7 +579,7 @@ func (s *DexTestSuite) TestPlaceLimitOrderJITBehindEnemyLines() {
 	s.assertLimitLiquidityAtTick("TokenA", 1, 0)
 	// Alice can withdraw ~10 BIGTokenB
 	s.aliceWithdrawsLimitSell(trancheKey)
-	s.assertAliceBalancesInt(sdkmath.ZeroInt(), sdkmath.NewInt(9999000))
+	s.assertAliceBalancesInt(sdkmath.ZeroInt(), sdkmath.NewInt(9998999))
 }
 
 func (s *DexTestSuite) TestPlaceLimitOrderJITNextBlock() {