add check on non-positive shares and redeemed quote quantums

dydxprotocol · Sep 19, 2024 · af7a7f9 · af7a7f9
1 parent 79d9ea7
commit af7a7f9
Show file tree

Hide file tree

Showing 2 changed files with 59 additions and 16 deletions.
diff --git a/protocol/x/vault/keeper/msg_server_withdraw_from_megavault_test.go b/protocol/x/vault/keeper/msg_server_withdraw_from_megavault_test.go
@@ -48,9 +48,9 @@ func TestMsgWithdrawFromMegavault(t *testing.T) {
 		// Vaults.
 		vaults []VaultSetup
 		// Shares to withdraw.
-		sharesToWithdraw uint64
+		sharesToWithdraw int64
 		// Minimum quote quantums to redeem.
-		minQuoteQuantums uint64
+		minQuoteQuantums int64
 
 		/* --- Expectations --- */
 		// Whether DeliverTx should fail.
@@ -101,6 +101,19 @@ func TestMsgWithdrawFromMegavault(t *testing.T) {
 			expectedTotalShares:   0,
 			expectedOwnerShares:   0,
 		},
+		"Success: Withdraw some unlocked shares (1% of total), No sub-vaults, Redeemed quantums rounds down to 0": {
+			mainVaultBalance:      99,
+			totalShares:           200,
+			owner:                 constants.AliceAccAddress.String(),
+			ownerTotalShares:      10,
+			ownerLockedShares:     5,
+			sharesToWithdraw:      2,
+			minQuoteQuantums:      -1,
+			deliverTxFails:        true,
+			redeemedQuoteQuantums: 0,   // 99 * 2 / 200 = 0.99 ~= 0 (rounded down)
+			expectedTotalShares:   200, // unchanged
+			expectedOwnerShares:   10,  // unchanged
+		},
 		"Failure: Withdraw more than locked shares": {
 			mainVaultBalance:    100,
 			totalShares:         500,
@@ -113,6 +126,30 @@ func TestMsgWithdrawFromMegavault(t *testing.T) {
 			expectedTotalShares: 500, // unchanged
 			expectedOwnerShares: 100, // unchanged
 		},
+		"Failure: Withdraw zero shares": {
+			mainVaultBalance:    100,
+			totalShares:         500,
+			owner:               constants.AliceAccAddress.String(),
+			ownerTotalShares:    100,
+			ownerLockedShares:   20,
+			sharesToWithdraw:    0,
+			minQuoteQuantums:    1,
+			deliverTxFails:      true,
+			expectedTotalShares: 500, // unchanged
+			expectedOwnerShares: 100, // unchanged
+		},
+		"Failure: Withdraw negative shares": {
+			mainVaultBalance:    100,
+			totalShares:         500,
+			owner:               constants.AliceAccAddress.String(),
+			ownerTotalShares:    100,
+			ownerLockedShares:   20,
+			sharesToWithdraw:    -1,
+			minQuoteQuantums:    1,
+			deliverTxFails:      true,
+			expectedTotalShares: 500, // unchanged
+			expectedOwnerShares: 100, // unchanged
+		},
 		"Failure: Withdraw some unlocked shares (8% of total), one sub-vault, Redeemed quantums < Min quantums": {
 			mainVaultBalance:  1_234,
 			totalShares:       500,
@@ -164,7 +201,7 @@ func TestMsgWithdrawFromMegavault(t *testing.T) {
 				},
 			},
 			sharesToWithdraw:      4444,
-			minQuoteQuantums:      0,
+			minQuoteQuantums:      -1,
 			deliverTxFails:        false,
 			redeemedQuoteQuantums: 3_950,   // 888_888 * 4444 / 1_000_000 ~= 3950 (sub-vault is skipped)
 			expectedTotalShares:   995_556, // 1_000_000 - 4444
@@ -420,9 +457,9 @@ func TestMsgWithdrawFromMegavault(t *testing.T) {
 					Number: 0,
 				},
 				Shares: vaulttypes.NumShares{
-					NumShares: dtypes.NewIntFromUint64(tc.sharesToWithdraw),
+					NumShares: dtypes.NewInt(tc.sharesToWithdraw),
 				},
-				MinQuoteQuantums: dtypes.NewIntFromUint64(tc.minQuoteQuantums),
+				MinQuoteQuantums: dtypes.NewInt(tc.minQuoteQuantums),
 			}
 
 			preMegavaultEquity, err := tApp.App.VaultKeeper.GetMegavaultEquity(ctx)

diff --git a/protocol/x/vault/keeper/withdraw.go b/protocol/x/vault/keeper/withdraw.go
@@ -123,7 +123,14 @@ func (k Keeper) WithdrawFromMegavault(
 	sharesToWithdraw *big.Int,
 	minQuoteQuantums *big.Int,
 ) (redeemedQuoteQuantums *big.Int, err error) {
-	// 1. Check that the owner is withdrawing less that or equal to their unlocked shares.
+	// 1. Check that shares to withdraw is positive and not more than unlocked shares.
+	if sharesToWithdraw.Sign() <= 0 {
+		return nil, errorsmod.Wrapf(
+			types.ErrInvalidSharesToWithdraw,
+			"sharesToWithdraw: %s",
+			sharesToWithdraw,
+		)
+	}
 	ownerShares, exists := k.GetOwnerShares(ctx, toSubaccount.Owner)
 	if !exists {
 		return nil, types.ErrOwnerNotFound
@@ -161,7 +168,7 @@ func (k Keeper) WithdrawFromMegavault(
 		if err != nil {
 			log.ErrorLogWithError(
 				ctx,
-				"Megavault withdrawal: failed to get vault ID from state key. Skipping this vault",
+				"Megavault withdrawal: error when getting vault ID from state key. Skipping this vault",
 				err,
 			)
 			continue
@@ -173,7 +180,7 @@ func (k Keeper) WithdrawFromMegavault(
 		if err != nil {
 			log.ErrorLogWithError(
 				ctx,
-				"Megavault withdrawal: failed to get perpetual and market. Skipping this vault",
+				"Megavault withdrawal: error when getting perpetual and market. Skipping this vault",
 				err,
 				"Vault ID",
 				vaultId,
@@ -184,7 +191,7 @@ func (k Keeper) WithdrawFromMegavault(
 		if err != nil {
 			log.ErrorLogWithError(
 				ctx,
-				"Megavault withdrawal: failed to get vault leverage and equity. Skipping this vault",
+				"Megavault withdrawal: error when getting vault leverage and equity. Skipping this vault",
 				err,
 				"Vault ID",
 				vaultId,
@@ -204,7 +211,7 @@ func (k Keeper) WithdrawFromMegavault(
 		if err != nil {
 			log.ErrorLogWithError(
 				ctx,
-				"Megavault withdrawal: failed to get vault withdrawal slippage. Skipping this vault",
+				"Megavault withdrawal: error when getting vault withdrawal slippage. Skipping this vault",
 				err,
 				"Vault ID",
 				vaultId,
@@ -228,7 +235,7 @@ func (k Keeper) WithdrawFromMegavault(
 		if err != nil {
 			log.ErrorLogWithError(
 				ctx,
-				"Megavault withdrawal: failed to transfer from sub vault to main vault. Skipping this vault",
+				"Megavault withdrawal: error when transfering from sub vault to main vault. Skipping this vault",
 				err,
 				"Vault ID",
 				vaultId,
@@ -243,8 +250,8 @@ func (k Keeper) WithdrawFromMegavault(
 		megavaultEquity.Add(megavaultEquity, equity)
 	}
 
-	// 4. Return error if less than min quote quantums are redeemed.
-	if redeemedQuoteQuantums.Cmp(minQuoteQuantums) < 0 {
+	// 4. Return error if redeemed quantums are non-positive or less than min quantums.
+	if redeemedQuoteQuantums.Sign() <= 0 || redeemedQuoteQuantums.Cmp(minQuoteQuantums) < 0 {
 		return nil, errorsmod.Wrapf(
 			types.ErrInsufficientRedeemedQuoteQuantums,
 			"redeemed quote quantums: %s, min quote quantums: %s",
@@ -275,11 +282,10 @@ func (k Keeper) WithdrawFromMegavault(
 	}
 
 	// 6. Decrement total and owner shares.
-	err = k.SetTotalShares(
+	if err = k.SetTotalShares(
 		ctx,
 		types.BigIntToNumShares(new(big.Int).Sub(totalShares, sharesToWithdraw)),
-	)
-	if err != nil {
+	); err != nil {
 		return nil, err
 	}
 	if ownerSharesAfterWithdrawal.Sign() == 0 {