From 92fa62efef1359de9432d44c9b2597875a92b896 Mon Sep 17 00:00:00 2001 From: Spoorthi <9302666+spoo-bar@users.noreply.github.com> Date: Thu, 25 Apr 2024 13:25:36 +0100 Subject: [PATCH] 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 | 1 + x/cwerrors/keeper/keeper.go | 14 +++++++------- x/cwerrors/keeper/msg_server_test.go | 7 ++++--- x/cwerrors/keeper/subscriptions.go | 16 ++++++++-------- x/cwerrors/keeper/subscriptions_test.go | 12 +++++++----- x/cwerrors/keeper/sudo_errors.go | 12 ++++++------ x/cwerrors/types/errors.go | 10 +++++----- 7 files changed, 38 insertions(+), 34 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 844dc2dd..291d5b23 100644 --- a/CHANGELOG.md +++ b/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) diff --git a/x/cwerrors/keeper/keeper.go b/x/cwerrors/keeper/keeper.go index 180f02d6..1322eaf9 100644 --- a/x/cwerrors/keeper/keeper.go +++ b/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, diff --git a/x/cwerrors/keeper/msg_server_test.go b/x/cwerrors/keeper/msg_server_test.go index 4448111f..b6d4bd4a 100644 --- a/x/cwerrors/keeper/msg_server_test.go +++ b/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, diff --git a/x/cwerrors/keeper/subscriptions.go b/x/cwerrors/keeper/subscriptions.go index 2c706003..507c6c93 100644 --- a/x/cwerrors/keeper/subscriptions.go +++ b/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 diff --git a/x/cwerrors/keeper/subscriptions_test.go b/x/cwerrors/keeper/subscriptions_test.go index f4406c0a..2e0eb324 100644 --- a/x/cwerrors/keeper/subscriptions_test.go +++ b/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) diff --git a/x/cwerrors/keeper/sudo_errors.go b/x/cwerrors/keeper/sudo_errors.go index cfd20210..55db720c 100644 --- a/x/cwerrors/keeper/sudo_errors.go +++ b/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 { diff --git a/x/cwerrors/types/errors.go b/x/cwerrors/types/errors.go index 2ee9f6ce..ab12208c 100644 --- a/x/cwerrors/types/errors.go +++ b/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") )