fix(x/cwerrors): audit remediations (#564)

* ensuring the user doesnt overpay for contract subscriptions

* when user resubscribes, extend the end date of the subscription

* remove redundant contract address being stored for subscriptionendblock collection

* remove redundant errorid being stored for contracterrors and deletionblocks

* Update CHANGELOG.md

CHANGELOG.md

@@ -67,6 +67,7 @@ Contains all the PRs that improved the code without changing the behaviors.
 - [#552](https://github.com/archway-network/archway/pull/552) - Fix issue with x/callback callback error code was not identified correctly when setting cwerrors
 - [#559](https://github.com/archway-network/archway/pull/559) - Fixing wrong bond denom being considered for x/callback and x/cwerrors fees
 - [#562](https://github.com/archway-network/archway/pull/562) - Fixing the account from which x/cwerrors subscription fees are deducted
+- [#564](https://github.com/archway-network/archway/pull/564) - Remediations for x/cwerrors audit
 
 
 ## [v6.0.0](https://github.com/archway-network/archway/releases/tag/v6.0.0)

x/cwerrors/keeper/keeper.go

@@ -27,15 +27,15 @@ type Keeper struct {
 	Params collections.Item[types.Params]
 	// ErrorID key: ErrorsCountKey | value: ErrorID
 	ErrorID collections.Sequence
-	// ContractErrors key: ContractErrorsKeyPrefix + contractAddress + ErrorId | value: ErrorId
-	ContractErrors collections.Map[collections.Pair[[]byte, uint64], uint64]
+	// ContractErrors key: ContractErrorsKeyPrefix + contractAddress + ErrorId | value: nil
+	ContractErrors collections.Map[collections.Pair[[]byte, uint64], []byte]
 	// ContractErrors key: ErrorsKeyPrefix + ErrorId | value: SudoError
 	Errors collections.Map[uint64, types.SudoError]
-	// DeletionBlocks key: DeletionBlocksKeyPrefix + BlockHeight + ErrorId | value: ErrorId
-	DeletionBlocks collections.Map[collections.Pair[int64, uint64], uint64]
+	// DeletionBlocks key: DeletionBlocksKeyPrefix + BlockHeight + ErrorId | value: nil
+	DeletionBlocks collections.Map[collections.Pair[int64, uint64], []byte]
 	// ContractSubscriptions key: ContractSubscriptionsKeyPrefix + contractAddress | value: deletionHeight
 	ContractSubscriptions collections.Map[[]byte, int64]
-	// SubscriptionEndBlock key: SubscriptionEndBlockKeyPrefix + BlockHeight + contractAddress | value: contractAddress
+	// SubscriptionEndBlock key: SubscriptionEndBlockKeyPrefix + BlockHeight + contractAddress | value: nil
 	SubscriptionEndBlock collections.Map[collections.Pair[int64, []byte], []byte]
 }
 
@@ -69,7 +69,7 @@ func NewKeeper(cdc codec.Codec, storeKey storetypes.StoreKey, tStoreKey storetyp
 			types.ContractErrorsKeyPrefix,
 			"contractErrors",
 			collections.PairKeyCodec(collections.BytesKey, collections.Uint64Key),
-			collections.Uint64Value,
+			collections.BytesValue,
 		),
 		Errors: collections.NewMap(
 			sb,
@@ -83,7 +83,7 @@ func NewKeeper(cdc codec.Codec, storeKey storetypes.StoreKey, tStoreKey storetyp
 			types.DeletionBlocksKeyPrefix,
 			"deletionBlocks",
 			collections.PairKeyCodec(collections.Int64Key, collections.Uint64Key),
-			collections.Uint64Value,
+			collections.BytesValue,
 		),
 		ContractSubscriptions: collections.NewMap(
 			sb,

x/cwerrors/keeper/msg_server_test.go

@@ -29,7 +29,8 @@ func (s *KeeperTestSuite) TestSubscribeToError() {
 	)
 	params, err := keeper.GetParams(ctx)
 	s.Require().NoError(err)
-	err = s.chain.GetApp().Keepers.BankKeeper.SendCoins(ctx, contractAdminAcc.Address, contractAddr, sdk.NewCoins(sdk.NewInt64Coin(sdk.DefaultBondDenom, 100)))
+	params.SubscriptionFee = sdk.NewInt64Coin(sdk.DefaultBondDenom, 1)
+	err = keeper.SetParams(ctx, params)
 	s.Require().NoError(err)
 
 	expectedEndHeight := ctx.BlockHeight() + params.SubscriptionPeriod
@@ -92,7 +93,7 @@ func (s *KeeperTestSuite) TestSubscribeToError() {
 				return &types.MsgSubscribeToError{
 					Sender:          contractAddr.String(),
 					ContractAddress: contractAddr.String(),
-					Fee:             sdk.NewInt64Coin(sdk.DefaultBondDenom, 101),
+					Fee:             params.SubscriptionFee,
 				}
 			},
 			expectError: true,
@@ -104,7 +105,7 @@ func (s *KeeperTestSuite) TestSubscribeToError() {
 				return &types.MsgSubscribeToError{
 					Sender:          contractAdminAcc.Address.String(),
 					ContractAddress: contractAddr.String(),
-					Fee:             sdk.NewInt64Coin(sdk.DefaultBondDenom, 100),
+					Fee:             params.SubscriptionFee,
 				}
 			},
 			expectError: false,

x/cwerrors/keeper/subscriptions.go

@@ -19,9 +19,9 @@ func (k Keeper) SetSubscription(ctx sdk.Context, sender, contractAddress sdk.Acc
 		return -1, types.ErrUnauthorized
 	}
 
-	existingSubFound, endHeight := k.GetSubscription(ctx, contractAddress)
+	existingSubFound, existingEndHeight := k.GetSubscription(ctx, contractAddress)
 	if existingSubFound {
-		if err := k.SubscriptionEndBlock.Remove(ctx, collections.Join(endHeight, contractAddress.Bytes())); err != nil {
+		if err := k.SubscriptionEndBlock.Remove(ctx, collections.Join(existingEndHeight, contractAddress.Bytes())); err != nil {
 			return -1, err
 		}
 	}
@@ -30,16 +30,16 @@ func (k Keeper) SetSubscription(ctx sdk.Context, sender, contractAddress sdk.Acc
 		return -1, err
 	}
 
-	if fee.IsLT(params.SubscriptionFee) {
-		return -1, types.ErrInsufficientSubscriptionFee
+	if !fee.IsEqual(params.SubscriptionFee) {
+		return -1, types.ErrIncorrectSubscriptionFee
 	}
 	err = k.bankKeeper.SendCoinsFromAccountToModule(ctx, sender, authtypes.FeeCollectorName, sdk.NewCoins(fee))
 	if err != nil {
 		return -1, err
 	}
 
-	subscriptionEndHeight := ctx.BlockHeight() + params.SubscriptionPeriod
-	if err = k.SubscriptionEndBlock.Set(ctx, collections.Join(subscriptionEndHeight, contractAddress.Bytes()), contractAddress.Bytes()); err != nil {
+	subscriptionEndHeight := max(ctx.BlockHeight(), existingEndHeight) + params.SubscriptionPeriod
+	if err = k.SubscriptionEndBlock.Set(ctx, collections.Join(subscriptionEndHeight, contractAddress.Bytes()), nil); err != nil {
 		return -1, err
 	}
 	return subscriptionEndHeight, k.ContractSubscriptions.Set(ctx, contractAddress, subscriptionEndHeight)
@@ -67,8 +67,8 @@ func (k Keeper) GetSubscription(ctx sdk.Context, contractAddress sdk.AccAddress)
 func (k Keeper) PruneSubscriptionsEndBlock(ctx sdk.Context) (err error) {
 	height := ctx.BlockHeight()
 	rng := collections.NewPrefixedPairRange[int64, []byte](height)
-	err = k.SubscriptionEndBlock.Walk(ctx, rng, func(key collections.Pair[int64, []byte], contractAddress []byte) (bool, error) {
-		if err := k.ContractSubscriptions.Remove(ctx, contractAddress); err != nil {
+	err = k.SubscriptionEndBlock.Walk(ctx, rng, func(key collections.Pair[int64, []byte], _ []byte) (bool, error) {
+		if err := k.ContractSubscriptions.Remove(ctx, key.K2()); err != nil {
 			return true, err
 		}
 		return false, nil

x/cwerrors/keeper/subscriptions_test.go

@@ -42,7 +42,7 @@ func (s *KeeperTestSuite) TestSetSubscription() {
 	})
 	s.Require().NoError(err)
 	_, err = keeper.SetSubscription(ctx, contractAdminAcc.Address, contractAddr, fees)
-	s.Require().ErrorIs(err, types.ErrInsufficientSubscriptionFee)
+	s.Require().ErrorIs(err, types.ErrIncorrectSubscriptionFee)
 	err = keeper.SetParams(ctx, types.DefaultParams())
 	s.Require().NoError(err)
 
@@ -56,13 +56,15 @@ func (s *KeeperTestSuite) TestSetSubscription() {
 	ctx = ctx.WithBlockHeight(ctx.BlockHeight() + 1)
 	subscriptionEndHeight, err = keeper.SetSubscription(ctx, contractAdminAcc.Address, contractAddr, fees)
 	s.Require().NoError(err)
-	s.Require().Equal(subscriptionEndHeight, expectedEndDate+1)
+	expectedEndDate = expectedEndDate + params.SubscriptionPeriod // existing subscription gets extended
+	s.Require().Equal(subscriptionEndHeight, expectedEndDate)
 
 	// TEST CASE 6: Subscription being updated by the contract itself (instead of admin)
 	ctx = ctx.WithBlockHeight(ctx.BlockHeight() + 1)
 	subscriptionEndHeight, err = keeper.SetSubscription(ctx, contractAddr, contractAddr, fees)
 	s.Require().NoError(err)
-	s.Require().Equal(subscriptionEndHeight, expectedEndDate+2)
+	expectedEndDate = expectedEndDate + params.SubscriptionPeriod // existing subscription gets extended
+	s.Require().Equal(subscriptionEndHeight, expectedEndDate)
 }
 
 func (s *KeeperTestSuite) TestHasSubscription() {
@@ -161,7 +163,7 @@ func (s *KeeperTestSuite) TestPruneSubscriptionsEndBlock() {
 
 	ctx = ctx.WithBlockHeight(ctx.BlockHeight() + 1)
 	// extend the subscription for contractAddr3
-	_, err = keeper.SetSubscription(ctx, contractAdminAcc.Address, contractAddr3, fees)
+	newEndHeight, err := keeper.SetSubscription(ctx, contractAdminAcc.Address, contractAddr3, fees)
 	s.Require().NoError(err)
 
 	ctx = ctx.WithBlockHeight(endHeight)
@@ -174,7 +176,7 @@ func (s *KeeperTestSuite) TestPruneSubscriptionsEndBlock() {
 	hasSub = keeper.HasSubscription(ctx, contractAddr3)
 	s.Require().True(hasSub)
 
-	ctx = ctx.WithBlockHeight(ctx.BlockHeight() + 1)
+	ctx = ctx.WithBlockHeight(newEndHeight)
 	err = keeper.PruneSubscriptionsEndBlock(ctx)
 	s.Require().NoError(err)
 	hasSub = keeper.HasSubscription(ctx, contractAddr3)

x/cwerrors/keeper/sudo_errors.go

@@ -37,7 +37,7 @@ func (k Keeper) StoreErrorInState(ctx sdk.Context, contractAddr sdk.AccAddress,
 	}
 
 	// Associate the error with the contract
-	if err = k.ContractErrors.Set(ctx, collections.Join(contractAddr.Bytes(), errorID), errorID); err != nil {
+	if err = k.ContractErrors.Set(ctx, collections.Join(contractAddr.Bytes(), errorID), nil); err != nil {
 		return err
 	}
 
@@ -47,7 +47,7 @@ func (k Keeper) StoreErrorInState(ctx sdk.Context, contractAddr sdk.AccAddress,
 		return err
 	}
 	deletionHeight := ctx.BlockHeight() + params.ErrorStoredTime
-	if err = k.DeletionBlocks.Set(ctx, collections.Join(deletionHeight, errorID), errorID); err != nil {
+	if err = k.DeletionBlocks.Set(ctx, collections.Join(deletionHeight, errorID), nil); err != nil {
 		return err
 	}
 
@@ -78,8 +78,8 @@ func (k Keeper) storeErrorCallback(ctx sdk.Context, sudoErr types.SudoError) err
 // GetErrosByContractAddress returns all errors (in state) for a given contract address
 func (k Keeper) GetErrorsByContractAddress(ctx sdk.Context, contractAddress []byte) (sudoErrs []types.SudoError, err error) {
 	rng := collections.NewPrefixedPairRange[[]byte, uint64](contractAddress)
-	err = k.ContractErrors.Walk(ctx, rng, func(key collections.Pair[[]byte, uint64], errorID uint64) (bool, error) {
-		sudoErr, err := k.Errors.Get(ctx, errorID)
+	err = k.ContractErrors.Walk(ctx, rng, func(key collections.Pair[[]byte, uint64], _ []byte) (bool, error) {
+		sudoErr, err := k.Errors.Get(ctx, key.K2())
 		if err != nil {
 			return true, err
 		}
@@ -110,8 +110,8 @@ func (k Keeper) PruneErrorsCurrentBlock(ctx sdk.Context) (err error) {
 	var errorIDs []uint64
 	height := ctx.BlockHeight()
 	rng := collections.NewPrefixedPairRange[int64, uint64](height)
-	err = k.DeletionBlocks.Walk(ctx, rng, func(key collections.Pair[int64, uint64], errorID uint64) (bool, error) {
-		errorIDs = append(errorIDs, errorID)
+	err = k.DeletionBlocks.Walk(ctx, rng, func(key collections.Pair[int64, uint64], _ []byte) (bool, error) {
+		errorIDs = append(errorIDs, key.K2())
 		return false, nil
 	})
 	if err != nil {

x/cwerrors/types/errors.go

@@ -3,9 +3,9 @@ package types
 import errorsmod "cosmossdk.io/errors"
 
 var (
-	DefaultCodespace               = ModuleName
-	ErrContractNotFound            = errorsmod.Register(DefaultCodespace, 2, "contract with given address not found")
-	ErrUnauthorized                = errorsmod.Register(DefaultCodespace, 3, "sender unauthorized to perform the action")
-	ErrModuleNameMissing           = errorsmod.Register(DefaultCodespace, 4, "module name missing from sudo error")
-	ErrInsufficientSubscriptionFee = errorsmod.Register(DefaultCodespace, 5, "insufficient subscription fee")
+	DefaultCodespace            = ModuleName
+	ErrContractNotFound         = errorsmod.Register(DefaultCodespace, 2, "contract with given address not found")
+	ErrUnauthorized             = errorsmod.Register(DefaultCodespace, 3, "sender unauthorized to perform the action")
+	ErrModuleNameMissing        = errorsmod.Register(DefaultCodespace, 4, "module name missing from sudo error")
+	ErrIncorrectSubscriptionFee = errorsmod.Register(DefaultCodespace, 5, "incorrect subscription fee")
 )