Merge pull request #15 from iqlusioninc/sam/stride-audit-fixes

Stride LSM audit fixes

docs/core/proto-docs.md

@@ -7611,7 +7611,7 @@ Query/QueryTokenizeShareRecordsOwned RPC method.
 <a name="cosmos.staking.v1beta1.QueryTotalLiquidStaked"></a>
 
 ### QueryTotalLiquidStaked
-QueryQueryTotalLiquidStakedRequest is request type for the
+QueryTotalLiquidStakedRequest is request type for the
 Query/QueryQueryTotalLiquidStaked RPC method.
 
 
@@ -7622,7 +7622,7 @@ Query/QueryQueryTotalLiquidStaked RPC method.
 <a name="cosmos.staking.v1beta1.QueryTotalLiquidStakedResponse"></a>
 
 ### QueryTotalLiquidStakedResponse
-QueryQueryTotalLiquidStakedResponse is response type for the
+QueryTotalLiquidStakedResponse is response type for the
 Query/QueryQueryTotalLiquidStaked RPC method.
 
 

proto/cosmos/staking/v1beta1/query.proto

@@ -442,18 +442,14 @@ message QueryTotalTokenizeSharedAssetsResponse {
   cosmos.base.v1beta1.Coin value = 1 [(gogoproto.nullable) = false];
 }
 
-// QueryQueryTotalLiquidStakedRequest is request type for the
+// QueryTotalLiquidStakedRequest is request type for the
 // Query/QueryQueryTotalLiquidStaked RPC method.
 message QueryTotalLiquidStaked {}
 
-// QueryQueryTotalLiquidStakedResponse is response type for the
+// QueryTotalLiquidStakedResponse is response type for the
 // Query/QueryQueryTotalLiquidStaked RPC method.
 message QueryTotalLiquidStakedResponse {
-  string tokens = 1 [
-    (gogoproto.moretags)   = "yaml:\"tokens\"",
-    (gogoproto.customtype) = "github.com/cosmos/cosmos-sdk/types.Int",
-    (gogoproto.nullable)   = false
-  ];
+  string tokens = 1;
 }
 
 // QueryTokenizeShareLockInfo queries the tokenize share lock information

x/staking/keeper/delegation_test.go

@@ -813,7 +813,8 @@ func TestValidatorBondRedelegate(t *testing.T) {
 	bondDenom := app.StakingKeeper.BondDenom(ctx)
 	notBondedPool := app.StakingKeeper.GetNotBondedPool(ctx)
 
-	require.NoError(t, simapp.FundModuleAccount(app.BankKeeper, ctx, notBondedPool.GetName(), sdk.NewCoins(sdk.NewCoin(bondDenom, startTokens))))
+	startPoolToken := sdk.NewCoins(sdk.NewCoin(bondDenom, startTokens.Mul(sdk.NewInt(2))))
+	require.NoError(t, simapp.FundModuleAccount(app.BankKeeper, ctx, notBondedPool.GetName(), startPoolToken))
 	app.AccountKeeper.SetModuleAccount(ctx, notBondedPool)
 
 	// create a validator and a delegator to that validator
@@ -832,12 +833,17 @@ func TestValidatorBondRedelegate(t *testing.T) {
 	// set total liquid stake
 	app.StakingKeeper.SetTotalLiquidStakedTokens(ctx, sdk.NewInt(100))
 
-	// convert to validator self-bond
-	msgServer := keeper.NewMsgServerImpl(app.StakingKeeper)
-
+	// delegate to each validator
 	validator, _ = app.StakingKeeper.GetValidator(ctx, addrVals[0])
 	err := delegateCoinsFromAccount(ctx, app, addrDels[0], startTokens, validator)
 	require.NoError(t, err)
+
+	validator2, _ = app.StakingKeeper.GetValidator(ctx, addrVals[1])
+	err = delegateCoinsFromAccount(ctx, app, addrDels[1], startTokens, validator2)
+	require.NoError(t, err)
+
+	// convert to validator self-bond
+	msgServer := keeper.NewMsgServerImpl(app.StakingKeeper)
 	_, err = msgServer.ValidatorBond(sdk.WrapSDKContext(ctx), &types.MsgValidatorBond{
 		DelegatorAddress: addrDels[0].String(),
 		ValidatorAddress: addrVals[0].String(),

x/staking/keeper/grpc_query.go

@@ -657,7 +657,7 @@ func (k Querier) TotalLiquidStaked(c context.Context, req *types.QueryTotalLiqui
 		return nil, status.Error(codes.InvalidArgument, "empty request")
 	}
 	ctx := sdk.UnwrapSDKContext(c)
-	totalLiquidStaked := k.GetTotalLiquidStakedTokens(ctx)
+	totalLiquidStaked := k.GetTotalLiquidStakedTokens(ctx).String()
 	return &types.QueryTotalLiquidStakedResponse{
 		Tokens: totalLiquidStaked,
 	}, nil

x/staking/keeper/liquid_stake.go

@@ -4,7 +4,6 @@ import (
 	"time"
 
 	sdk "github.com/cosmos/cosmos-sdk/types"
-	authtypes "github.com/cosmos/cosmos-sdk/x/auth/types"
 	"github.com/cosmos/cosmos-sdk/x/staking/types"
 )
 
@@ -39,22 +38,21 @@ func (k Keeper) GetTotalLiquidStakedTokens(ctx sdk.Context) sdk.Int {
 }
 
 // Check if an account is a owned by a liquid staking provider
-// This is determined by checking if the account is a 32-length module account
-func (k Keeper) AccountIsLiquidStakingProvider(ctx sdk.Context, address sdk.AccAddress) bool {
-	account := k.authKeeper.GetAccount(ctx, address)
-	_, isModuleAccount := account.(*authtypes.ModuleAccount)
-	return isModuleAccount && len(address) == 32
+// This is determined by checking if the account has a 32-length address
+// NOTE: This will have to be refactored before adapting it to chains beyond gaia
+func (k Keeper) AccountIsLiquidStakingProvider(address sdk.AccAddress) bool {
+	return len(address) == 32
 }
 
 // CheckExceedsGlobalLiquidStakingCap checks if a liquid delegation would cause the
 // global liquid staking cap to be exceeded
 // A liquid delegation is defined as either tokenized shares, or a delegation from an ICA Account
 // The total stake is determined by the balance of the bonded pool
-// If the delegation is for a tokenized share, the tokens are already included in the bonded pool
-// If the delegation is from an ICA account, we need to add the tokens to the current bonded pool
-// balance to get the total staked
-// Returns true if the cap is exceeded
-func (k Keeper) CheckExceedsGlobalLiquidStakingCap(ctx sdk.Context, tokens sdk.Int, tokenizingShares bool) bool {
+// If the delegation's shares are already bonded (e.g. in the event of a tokenized share)
+// the tokens are already included in the bonded pool
+// If the delegation's shares are not bonded (e.g. normal delegation),
+// we need to add the tokens to the current bonded pool balance to get the total staked
+func (k Keeper) CheckExceedsGlobalLiquidStakingCap(ctx sdk.Context, tokens sdk.Int, sharesAlreadyBonded bool) bool {
 	liquidStakingCap := k.GlobalLiquidStakingCap(ctx)
 	liquidStakedAmount := k.GetTotalLiquidStakedTokens(ctx)
 
@@ -63,7 +61,7 @@ func (k Keeper) CheckExceedsGlobalLiquidStakingCap(ctx sdk.Context, tokens sdk.I
 	// they would not have been counted yet
 	// If this is for a tokenized delegation, the tokens are already included in the pool balance
 	totalStakedAmount := k.TotalBondedTokens(ctx)
-	if !tokenizingShares {
+	if !sharesAlreadyBonded {
 		totalStakedAmount = totalStakedAmount.Add(tokens)
 	}
 
@@ -80,7 +78,7 @@ func (k Keeper) CheckExceedsGlobalLiquidStakingCap(ctx sdk.Context, tokens sdk.I
 // Returns true if the cap is exceeded
 func (k Keeper) CheckExceedsValidatorBondCap(ctx sdk.Context, validator types.Validator, shares sdk.Dec) bool {
 	validatorBondFactor := k.ValidatorBondFactor(ctx)
-	if validatorBondFactor.Equal(sdk.NewDec(-1)) {
+	if validatorBondFactor.Equal(types.ValidatorBondDisabled) {
 		return false
 	}
 	maxValLiquidShares := validator.TotalValidatorBondShares.Mul(validatorBondFactor)
@@ -103,8 +101,8 @@ func (k Keeper) CheckExceedsValidatorLiquidStakingCap(ctx sdk.Context, validator
 
 // SafelyIncreaseTotalLiquidStakedTokens increments the total liquid staked tokens
 // if the global cap is not surpassed by this delegation
-func (k Keeper) SafelyIncreaseTotalLiquidStakedTokens(ctx sdk.Context, amount sdk.Int, tokenizingShares bool) error {
-	if k.CheckExceedsGlobalLiquidStakingCap(ctx, amount, tokenizingShares) {
+func (k Keeper) SafelyIncreaseTotalLiquidStakedTokens(ctx sdk.Context, amount sdk.Int, sharesAlreadyBonded bool) error {
+	if k.CheckExceedsGlobalLiquidStakingCap(ctx, amount, sharesAlreadyBonded) {
 		return types.ErrGlobalLiquidStakingCapExceeded
 	}
 
@@ -113,8 +111,13 @@ func (k Keeper) SafelyIncreaseTotalLiquidStakedTokens(ctx sdk.Context, amount sd
 }
 
 // DecreaseTotalLiquidStakedTokens decrements the total liquid staked tokens
-func (k Keeper) DecreaseTotalLiquidStakedTokens(ctx sdk.Context, amount sdk.Int) {
-	k.SetTotalLiquidStakedTokens(ctx, k.GetTotalLiquidStakedTokens(ctx).Sub(amount))
+func (k Keeper) DecreaseTotalLiquidStakedTokens(ctx sdk.Context, amount sdk.Int) error {
+	totalLiquidStake := k.GetTotalLiquidStakedTokens(ctx)
+	if amount.GT(totalLiquidStake) {
+		return types.ErrTotalLiquidStakedUnderflow
+	}
+	k.SetTotalLiquidStakedTokens(ctx, totalLiquidStake.Sub(amount))
+	return nil
 }
 
 // SafelyIncreaseValidatorTotalLiquidShares increments the total liquid shares on a validator, if:
@@ -136,9 +139,13 @@ func (k Keeper) SafelyIncreaseValidatorTotalLiquidShares(ctx sdk.Context, valida
 }
 
 // DecreaseValidatorTotalLiquidShares decrements the total liquid shares on a validator
-func (k Keeper) DecreaseValidatorTotalLiquidShares(ctx sdk.Context, validator types.Validator, shares sdk.Dec) {
+func (k Keeper) DecreaseValidatorTotalLiquidShares(ctx sdk.Context, validator types.Validator, shares sdk.Dec) error {
+	if shares.GT(validator.TotalLiquidShares) {
+		return types.ErrValidatorLiquidSharesUnderflow
+	}
 	validator.TotalLiquidShares = validator.TotalLiquidShares.Sub(shares)
 	k.SetValidator(ctx, validator)
+	return nil
 }
 
 // SafelyDecreaseValidatorBond decrements the total validator's self bond
@@ -278,6 +285,21 @@ func (k Keeper) QueueTokenizeSharesAuthorization(ctx sdk.Context, address sdk.Ac
 	return completionTime
 }
 
+// Cancels a pending tokenize share authorization by removing the lock from the queue
+func (k Keeper) CancelTokenizeShareLockExpiration(ctx sdk.Context, address sdk.AccAddress, completionTime time.Time) {
+	authorizations := k.GetPendingTokenizeShareAuthorizations(ctx, completionTime)
+
+	updatedAddresses := []string{}
+	for _, expiringAddress := range authorizations.Addresses {
+		if address.String() != expiringAddress {
+			updatedAddresses = append(updatedAddresses, expiringAddress)
+		}
+	}
+
+	authorizations.Addresses = updatedAddresses
+	k.SetPendingTokenizeShareAuthorizations(ctx, completionTime, authorizations)
+}
+
 // Unlocks all queued tokenize share authorizations that have matured
 // (i.e. have waited the full unbonding period)
 func (k Keeper) RemoveExpiredTokenizeShareLocks(ctx sdk.Context, blockTime time.Time) (unlockedAddresses []string) {
@@ -305,8 +327,8 @@ func (k Keeper) RemoveExpiredTokenizeShareLocks(ctx sdk.Context, blockTime time.
 
 // Calculates and sets the global liquid staked tokens and total liquid shares by validator
 // The totals are determined by looping each delegation record and summing the stake
-// if the delegator is a module account. Checking for a module account will capture
-// ICA accounts, as well as tokenized delegationswhich are owned by module accounts
+// if the delegator has a 32-length address. Checking for a 32-length address will capture
+// ICA accounts, as well as tokenized delegations which are owned by module accounts
 // under the hood
 // This function must be called in the upgrade handler which onboards LSM, as
 // well as any time the liquid staking cap is re-enabled
@@ -336,7 +358,7 @@ func (k Keeper) RefreshTotalLiquidStaked(ctx sdk.Context) error {
 
 		// If the account is a liquid staking provider, increment the global number
 		// of liquid staked tokens, and the total liquid shares on the validator
-		if k.AccountIsLiquidStakingProvider(ctx, delegatorAddress) {
+		if k.AccountIsLiquidStakingProvider(delegatorAddress) {
 			liquidShares := delegation.Shares
 			liquidTokens := validator.TokensFromShares(liquidShares).TruncateInt()
 

x/staking/keeper/liquid_stake_test.go

@@ -1,6 +1,7 @@
 package keeper_test
 
 import (
+	"fmt"
 	"testing"
 	"time"
 
@@ -22,17 +23,30 @@ func createBaseAccount(app *simapp.SimApp, ctx sdk.Context, accountName string)
 	return baseAccountAddress
 }
 
-// Helper function to create a module account from an account name
+// Helper function to create 32-length account
 // Used to mock an liquid staking provider's ICA account
-func createICAAccount(app *simapp.SimApp, ctx sdk.Context, accountName string) sdk.AccAddress {
-	accountAddress := address.Module(accountName, []byte(accountName))
-	account := authtypes.NewModuleAccount(
-		authtypes.NewBaseAccountWithAddress(accountAddress),
-		accountName,
-	)
+func createICAAccount(app *simapp.SimApp, ctx sdk.Context) sdk.AccAddress {
+	icahost := "icahost"
+	connectionID := "connection-0"
+	portID := icahost
+
+	moduleAddress := app.AccountKeeper.GetModuleAddress(icahost)
+	icaAddress := sdk.AccAddress(address.Derive(moduleAddress, []byte(connectionID+portID)))
+
+	account := authtypes.NewBaseAccountWithAddress(icaAddress)
 	app.AccountKeeper.SetAccount(ctx, account)
 
-	return accountAddress
+	return icaAddress
+}
+
+// Helper function to create a module account address from a tokenized share
+// Used to mock the delegation owner of a tokenized share
+func createTokenizeShareModuleAccount(recordID uint64) sdk.AccAddress {
+	record := types.TokenizeShareRecord{
+		Id:            recordID,
+		ModuleAccount: fmt.Sprintf("%s%d", types.TokenizeShareModuleAccountPrefix, recordID),
+	}
+	return record.GetModuleAddress()
 }
 
 // Tests Set/Get TotalLiquidStakedTokens
@@ -71,11 +85,11 @@ func TestAccountIsLiquidStakingProvider(t *testing.T) {
 
 	// Create base and ICA accounts
 	baseAccountAddress := createBaseAccount(app, ctx, "base-account")
-	icaAccountAddress := createICAAccount(app, ctx, "ica-module-account")
+	icaAccountAddress := createICAAccount(app, ctx)
 
 	// Only the ICA module account should be considered a liquid staking provider
-	require.False(t, app.StakingKeeper.AccountIsLiquidStakingProvider(ctx, baseAccountAddress), "base account")
-	require.True(t, app.StakingKeeper.AccountIsLiquidStakingProvider(ctx, icaAccountAddress), "ICA module account")
+	require.False(t, app.StakingKeeper.AccountIsLiquidStakingProvider(baseAccountAddress), "base account")
+	require.True(t, app.StakingKeeper.AccountIsLiquidStakingProvider(icaAccountAddress), "ICA module account")
 }
 
 // Helper function to clear the Bonded pool balances before a unit test
@@ -322,8 +336,13 @@ func TestDecreaseTotalLiquidStakedTokens(t *testing.T) {
 	app.StakingKeeper.SetTotalLiquidStakedTokens(ctx, intitialTotalLiquidStaked)
 
 	// Decrease the total liquid stake and confirm the total was updated
-	app.StakingKeeper.DecreaseTotalLiquidStakedTokens(ctx, decreaseAmount)
+	err := app.StakingKeeper.DecreaseTotalLiquidStakedTokens(ctx, decreaseAmount)
+	require.NoError(t, err, "no error expected when decreasing total liquid staked tokens")
 	require.Equal(t, intitialTotalLiquidStaked.Sub(decreaseAmount), app.StakingKeeper.GetTotalLiquidStakedTokens(ctx))
+
+	// Attempt to decrease by an excessive amount, it should error
+	err = app.StakingKeeper.DecreaseTotalLiquidStakedTokens(ctx, intitialTotalLiquidStaked)
+	require.ErrorIs(t, err, types.ErrTotalLiquidStakedUnderflow)
 }
 
 // Tests CheckExceedsValidatorBondCap
@@ -680,7 +699,7 @@ func TestSafelyIncreaseValidatorTotalLiquidShares(t *testing.T) {
 func TestDecreaseValidatorTotalLiquidShares(t *testing.T) {
 	_, app, ctx := createTestInput()
 
-	initialLiquidShares := sdk.NewDec(0)
+	initialLiquidShares := sdk.NewDec(100)
 	decreaseAmount := sdk.NewDec(10)
 
 	// Create a validator with designated self-bond shares
@@ -695,10 +714,16 @@ func TestDecreaseValidatorTotalLiquidShares(t *testing.T) {
 	app.StakingKeeper.SetValidator(ctx, initialValidator)
 
 	// Decrease the validator liquid shares, and confirm the new share amount has been updated
-	app.StakingKeeper.DecreaseValidatorTotalLiquidShares(ctx, initialValidator, decreaseAmount)
+	err := app.StakingKeeper.DecreaseValidatorTotalLiquidShares(ctx, initialValidator, decreaseAmount)
+	require.NoError(t, err, "no error expected when decreasing validator total liquid shares")
+
 	actualValidator, found := app.StakingKeeper.GetValidator(ctx, valAddress)
 	require.True(t, found)
-	require.Equal(t, initialLiquidShares.Sub(decreaseAmount), actualValidator.TotalLiquidShares, "shares with cap disabled")
+	require.Equal(t, initialLiquidShares.Sub(decreaseAmount), actualValidator.TotalLiquidShares, "total liquid shares")
+
+	// Attempt to decrease by a larger amount than it has, it should fail
+	err = app.StakingKeeper.DecreaseValidatorTotalLiquidShares(ctx, actualValidator, initialLiquidShares)
+	require.ErrorIs(t, err, types.ErrValidatorLiquidSharesUnderflow)
 }
 
 // Tests Add/Remove/Get/SetTokenizeSharesLock
@@ -964,8 +989,9 @@ func TestCalculateTotalLiquidStaked(t *testing.T) {
 	}
 
 	delegations := []struct {
-		delegation types.Delegation
-		isLSTP     bool
+		delegation  types.Delegation
+		isLSTP      bool
+		isTokenized bool
 	}{
 		// Delegator A - Not a liquid staking provider
 		// Number of tokens/shares is irrelevant for this test
@@ -1022,11 +1048,11 @@ func TestCalculateTotalLiquidStaked(t *testing.T) {
 				Shares:           sdk.NewDec(900),
 			},
 		},
-		// Delegator C - Liquid staking provider, tokens included in total
+		// Delegator C - Tokenized shares, tokens included in total
 		// Total liquid staked: 325 + 522 + 75 = 922
 		{
 			// Shares: 325 shares, Exchange Rate: 1.0, Tokens: 325
-			isLSTP: true,
+			isTokenized: true,
 			delegation: types.Delegation{
 				DelegatorAddress: "delC-LSTP",
 				ValidatorAddress: "valA",
@@ -1035,7 +1061,7 @@ func TestCalculateTotalLiquidStaked(t *testing.T) {
 		},
 		{
 			// Shares: 580 shares, Exchange Rate: 0.9, Tokens: 522
-			isLSTP: true,
+			isTokenized: true,
 			delegation: types.Delegation{
 				DelegatorAddress: "delC-LSTP",
 				ValidatorAddress: "valB",
@@ -1044,7 +1070,7 @@ func TestCalculateTotalLiquidStaked(t *testing.T) {
 		},
 		{
 			// Shares: 100 shares, Exchange Rate: 0.75, Tokens: 75
-			isLSTP: true,
+			isTokenized: true,
 			delegation: types.Delegation{
 				DelegatorAddress: "delC-LSTP",
 				ValidatorAddress: "valC",
@@ -1068,9 +1094,12 @@ func TestCalculateTotalLiquidStaked(t *testing.T) {
 	// Create the delegations based on the above (must use actual delegator addresses)
 	for _, delegationCase := range delegations {
 		var delegatorAddress sdk.AccAddress
-		if delegationCase.isLSTP {
-			delegatorAddress = createICAAccount(app, ctx, delegationCase.delegation.DelegatorAddress)
-		} else {
+		switch {
+		case delegationCase.isLSTP:
+			delegatorAddress = createICAAccount(app, ctx)
+		case delegationCase.isTokenized:
+			delegatorAddress = createTokenizeShareModuleAccount(1)
+		default:
 			delegatorAddress = createBaseAccount(app, ctx, delegationCase.delegation.DelegatorAddress)
 		}