Audit Fix: Fix coins.add usage during deposit

utils/bank.go

@@ -2,6 +2,7 @@ package utils
 
 import (
 	"fmt"
+	"sort"
 
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	"github.com/neutron-org/neutron/x/dex/types"
@@ -105,3 +106,24 @@ func FilteredPaginateAccountBalances(
 
 	return res, nil
 }
+
+// SanitizeCoins takes an unsorted list of coins and sorts them, removes coins with amount zero and combines duplicate coins
+func SanitizeCoins(coins []sdk.Coin) sdk.Coins {
+	sort.SliceStable(coins, func(i, j int) bool {
+		return coins[i].Denom < coins[j].Denom
+	})
+	cleanCoins := sdk.Coins{}
+	lastDenom := ""
+	for _, coin := range coins {
+		if coin.IsZero() {
+			continue
+		}
+		if lastDenom != coin.Denom {
+			cleanCoins = append(cleanCoins, coin)
+		} else {
+			cleanCoins[len(cleanCoins)-1].Add(coin)
+		}
+		lastDenom = coin.Denom
+	}
+	return cleanCoins
+}

x/dex/keeper/core.go

@@ -7,9 +7,10 @@ import (
 	sdkerrors "cosmossdk.io/errors"
 	"cosmossdk.io/math"
 	sdk "github.com/cosmos/cosmos-sdk/types"
+	"github.com/neutron-org/neutron/utils"
 	math_utils "github.com/neutron-org/neutron/utils/math"
 	"github.com/neutron-org/neutron/x/dex/types"
-	"github.com/neutron-org/neutron/x/dex/utils"
+	dexutils "github.com/neutron-org/neutron/x/dex/utils"
 )
 
 // NOTE: Currently we are using TruncateInt in multiple places for converting Decs back into math.Ints.
@@ -76,10 +77,7 @@ func (k Keeper) DepositCore(
 			return nil, nil, nil, types.ErrDepositShareUnderflow
 		}
 
-		if err := k.MintShares(ctx, receiverAddr, outShares); err != nil {
-			return nil, nil, nil, err
-		}
-		sharesIssued = sharesIssued.Add(outShares)
+		sharesIssued = append(sharesIssued, outShares)
 
 		amounts0Deposited[i] = inAmount0
 		amounts1Deposited[i] = inAmount1
@@ -99,6 +97,10 @@ func (k Keeper) DepositCore(
 		))
 	}
 
+	// At this point shares issued is not sorted and may have duplicates
+	// we must sanitize to convert it to a valid set of coins
+	sharesIssued = utils.SanitizeCoins(sharesIssued)
+
 	if totalAmountReserve0.IsPositive() {
 		coin0 := sdk.NewCoin(pairID.Token0, totalAmountReserve0)
 		if err := k.bankKeeper.SendCoinsFromAccountToModule(ctx, callerAddr, types.ModuleName, sdk.Coins{coin0}); err != nil {
@@ -113,6 +115,10 @@ func (k Keeper) DepositCore(
 		}
 	}
 
+	if err := k.MintShares(ctx, receiverAddr, sharesIssued); err != nil {
+		return nil, nil, nil, err
+	}
+
 	return amounts0Deposited, amounts1Deposited, sharesIssued, nil
 }
 
@@ -257,7 +263,7 @@ func (k Keeper) MultiHopSwapCore(
 	if len(routeErrors) == len(routes) {
 		// All routes have failed
 
-		allErr := utils.JoinErrors(types.ErrAllMultiHopRoutesFailed, routeErrors...)
+		allErr := dexutils.JoinErrors(types.ErrAllMultiHopRoutesFailed, routeErrors...)
 
 		return sdk.Coin{}, allErr
 	}

x/dex/keeper/core_helper.go

@@ -77,9 +77,8 @@ func (k Keeper) ValidateFee(ctx sdk.Context, fee uint64) error {
 //                            TOKENIZER UTILS                                //
 ///////////////////////////////////////////////////////////////////////////////
 
-func (k Keeper) MintShares(ctx sdk.Context, addr sdk.AccAddress, shareCoin sdk.Coin) error {
+func (k Keeper) MintShares(ctx sdk.Context, addr sdk.AccAddress, sharesCoins sdk.Coins) error {
 	// mint share tokens
-	sharesCoins := sdk.Coins{shareCoin}
 	if err := k.bankKeeper.MintCoins(ctx, types.ModuleName, sharesCoins); err != nil {
 		return err
 	}

x/dex/keeper/core_helper_test.go

@@ -62,7 +62,7 @@ func (s *CoreHelpersTestSuite) setLPAtFee1Pool(tickIndex int64, amountA, amountB
 
 	totalShares := pool.CalcSharesMinted(amountAInt, amountBInt, existingShares)
 
-	err = s.app.DexKeeper.MintShares(s.ctx, s.alice, totalShares)
+	err = s.app.DexKeeper.MintShares(s.ctx, s.alice, sdk.NewCoins(totalShares))
 	s.Require().NoError(err)
 
 	lowerTick.ReservesMakerDenom = amountAInt

x/dex/keeper/msg_server_test.go

@@ -543,7 +543,7 @@ func (s *DexTestSuite) deposits(
 		s.Assert().Fail("Only 1 pairID can be provided")
 	}
 
-	_, err := s.msgServer.Deposit(s.GoCtx, &types.MsgDeposit{
+	msg := &types.MsgDeposit{
 		Creator:         account.String(),
 		Receiver:        account.String(),
 		TokenA:          tokenA,
@@ -553,7 +553,10 @@ func (s *DexTestSuite) deposits(
 		TickIndexesAToB: tickIndexes,
 		Fees:            fees,
 		Options:         options,
-	})
+	}
+	err := msg.ValidateBasic()
+	s.Assert().NoError(err)
+	_, err = s.msgServer.Deposit(s.GoCtx, msg)
 	s.Assert().Nil(err)
 }
 

x/dex/types/errors.go

@@ -164,4 +164,9 @@ var (
 		1149,
 		"Invalid Address",
 	)
+	ErrDuplicatePoolDeposit = sdkerrors.Register(
+		ModuleName,
+		1150,
+		"Can only provide a single deposit amount for each tick, fee pair",
+	)
 )

x/dex/types/message_deposit.go

@@ -1,6 +1,8 @@
 package types
 
 import (
+	"fmt"
+
 	sdkerrors "cosmossdk.io/errors"
 	"cosmossdk.io/math"
 	sdk "github.com/cosmos/cosmos-sdk/types"
@@ -78,7 +80,13 @@ func (msg *MsgDeposit) ValidateBasic() error {
 		return ErrZeroDeposit
 	}
 
+	poolsDeposited := make(map[string]bool)
 	for i := 0; i < numDeposits; i++ {
+		poolStr := fmt.Sprintf("%d-%d", msg.TickIndexesAToB[i], msg.Fees[i])
+		if _, ok := poolsDeposited[poolStr]; ok {
+			return ErrDuplicatePoolDeposit
+		}
+		poolsDeposited[poolStr] = true
 		if msg.AmountsA[i].LT(math.ZeroInt()) || msg.AmountsB[i].LT(math.ZeroInt()) {
 			return ErrZeroDeposit
 		}

x/dex/types/message_deposit_test.go

@@ -99,6 +99,18 @@ func TestMsgDeposit_ValidateBasic(t *testing.T) {
 			},
 			err: ErrZeroDeposit,
 		},
+		{
+			name: "invalid duplicate deposit",
+			msg: MsgDeposit{
+				Creator:         sample.AccAddress(),
+				Receiver:        sample.AccAddress(),
+				Fees:            []uint64{1, 2, 1},
+				TickIndexesAToB: []int64{0, 0, 0},
+				AmountsA:        []math.Int{math.OneInt(), math.OneInt(), math.OneInt()},
+				AmountsB:        []math.Int{math.OneInt(), math.OneInt(), math.OneInt()},
+			},
+			err: ErrDuplicatePoolDeposit,
+		},
 		{
 			name: "invalid no deposit",
 			msg: MsgDeposit{