Shentu Chain - audit

Files: - `x/shield/keeper/staking_purchase.go` Commit: - [`3a7e28ae8dec6bbf4d7b35252a745b82a463f781`](https://github.com/shentufoundation/shentu/tree/3a7e28ae8dec6bbf4d7b35252a745b82a463f781) In the shield module, the function `ProcessStakeForShieldExpiration` is called at the conclusion of each blocker to handle expired shields. Funds from expired Shield stakes will be transferred to the user's account, and any remaining shields that are not withdrawn will automatically be used to purchase new shields. ```go=200 renewShieldInt := amount.ToDec().Quo(sPRate).TruncateInt() renewShield := sdk.NewCoins(sdk.NewCoin(bondDenom, renewShieldInt)) if renewShieldInt.IsZero() { return nil } ``` However, the function employs the variable `amount` instead of `renew` to calculate the renewed shield amount. This discrepancy may result in using the entire fund amount of the expired shield to repurchase the shield.

In the shield module, multiple operations are processed at the end of each block: - remove expired purchases and distribute service fees, - process completed withdraws, and - close pools with no shield and shield limits are set to zero. ```go=15 // EndBlocker processes premium payment at every block. func EndBlocker(ctx sdk.Context, k keeper.Keeper) { // Remove expired purchases and distribute service fees. k.RemoveExpiredPurchasesAndDistributeFees(ctx) // Process completed withdraws. k.DequeueCompletedWithdrawQueue(ctx) // Close pools who do not have any shield and shield limits are set to zero. k.ClosePools(ctx) } ``` A large number of operations will be performed in this process. For example, in `RemoveExpiredPurchasesAndDistributeFees()`, fees are distributed to all shield providers: ```go=301 // Distribute service fees. totalCollateral := k.GetTotalCollateral(ctx) providers := k.GetAllProviders(ctx) for _, provider := range providers { ``` An attacker might initiate a Denial of Service (DoS) attack by creating numerous provider accounts and purchases, and initiating a high volume of withdrawals. This strategy could significantly increase the computational resources required at the end of a block, potentially overwhelming the nodes.

According to the current implementation, the function `prepForZeroHeightGenesis()` retrieves an invalid validator address from the KV store, which may be causing the export of the genesis file to fail. The function `prepForZeroHeightGenesis()` is intended to export the genesis state from the Coreum chain, which includes getting and exporting the validator information from the KV store. ```go=142 // Iterate through validators by power descending, reset bond heights, and // update bond intra-tx counters. store := ctx.KVStore(app.keys[stakingtypes.StoreKey]) iter := sdk.KVStoreReversePrefixIterator(store, stakingtypes.ValidatorsKey) counter := int16(0) for ; iter.Valid(); iter.Next() { addr := sdk.ValAddress(iter.Key()[1:]) validator, found := app.StakingKeeper.GetValidator(ctx, addr) if !found { panic("expected validator, not found") } validator.UnbondingHeight = 0 if applyAllowedAddrs && !allowedAddrsMap[addr.String()] { validator.Jailed = true } app.StakingKeeper.SetValidator(ctx, validator) counter++ } ... ``` In line 149, the validator address is fetched via `iter.Key()[1:]`, which excludes the `ValidatorKey`, but it still includes the prefix, the length of the address. This can be demonstrated in the function `GetValidatorKey()`: **cosmos-sdk/x/staking/types/keys.go** ```go=81 // GetValidatorKey creates the key for the validator with the address // VALUE: staking/Validator func GetValidatorKey(operatorAddr sdk.ValAddress) []byte { return append(ValidatorsKey, address.MustLengthPrefix(operatorAddr)...) } ``` Therefore, the `iter.Key()[1:]` returns the bytes of the length of the address plus the address, and it cannot be correctly converted to the validator address via the function `sdk.ValAddress()`. In this case, it will cause panic because it cannot be found in the staking keeper.

Files: - `x/shield/keeper/pool.go` - `x/shield/keeper/keep.go` Commit: - [`3a7e28ae8dec6bbf4d7b35252a745b82a463f781`](https://github.com/shentufoundation/shentu/tree/3a7e28ae8dec6bbf4d7b35252a745b82a463f781) In the context of the `GetPoolsBySponsor` function, it is expected to use the sponsor address as the input parameter. ```go=61 func (k Keeper) GetPoolsBySponsor(ctx sdk.Context, sponsorAddr string) ([]types.Pool, bool) { var ret []types.Pool found := false k.IterateAllPools(ctx, func(pool types.Pool) bool { if pool.SponsorAddr == sponsorAddr { ret = append(ret, pool) found = true } return false }) return ret, found } ``` However, in the `createPool` function, there is a potential issue where the incorrect parameter, `sponsor`, is passed when invoking the `GetPoolsBySponsor` function. This could potentially result in the failure to create a pool due to conflicts in the sponsor's name. ```go=175 if _, found := k.GetPoolsBySponsor(ctx, sponsor); found { return 0, types.ErrSponsorAlreadyExists } ```

Files: - `x/shield/types/msg.go` Commit: - [`3a7e28ae8dec6bbf4d7b35252a745b82a463f781`](https://github.com/shentufoundation/shentu/tree/3a7e28ae8dec6bbf4d7b35252a745b82a463f781) In `MsgUpdateSponsor`'s `ValidationBasic` function, the sponsor address is checked to ensure it is non-empty. However, `sponsorAddr` is decoded from `msg.From` in the implementation, which means `msg.sponsorAddr` is unchecked: ```go=595 sponsorAddr, err := sdk.AccAddressFromBech32(msg.From) if err != nil { panic(err) } if sponsorAddr.Empty() || strings.TrimSpace(msg.Sponsor) == "" { return ErrEmptySponsor } ``` This could potentially result in assigning the zero address to the `sponsorAddr` when updating the sponsor information of a specific pool if the actual `sponsorAddr` is zero.

Files: - `x/shield/keeper.go` Commit: - [`3a7e28ae8dec6bbf4d7b35252a745b82a463f781`](https://github.com/shentufoundation/shentu/tree/3a7e28ae8dec6bbf4d7b35252a745b82a463f781) In the `SecureCollaterals` function, if the provided `purchaseId` does not exist in the purchase list, there is a potential risk of misinterpreting a purchase record with an index of zero (the default value when it is declared) as the intended purchase. ```go=45 var index int for i, entry := range purchaseList.Entries { if entry.PurchaseId == purchaseID { index = i break } } purchase := &purchaseList.Entries[index] if lossAmt.GT(purchase.Shield) { return types.ErrNotEnoughShield } ```

The following test fails with an error indicating a payout was not made from unbonding: ```shell $ go test ./app -run TestFullAppSimulation -Enabled=true -NumBlocks=500 -Genesis= -Verbose=true -Commit=true -Seed=2989 -Period=50 -ExportParamsPath /tmp/sim-logs-2148526441/sim_params-2989.json -ExportStatePath /tmp/sim-logs-2148526441/sim_state-2989.json -v -timeout 24h ... D[2023-12-29|15:30:56.979] absent validator module=x/slashing height=66 validator=cosmosvalcons1r94y8vq7tkftf7r0ey09x77xer8ynz7nrdhg4y missed=66 threshold=0 D[2023-12-29|15:30:56.979] absent validator module=x/slashing height=66 validator=cosmosvalcons1h86dyzynpcfmjsgvmtkzfxupzz078fnlr60fru missed=25 threshold=0 I[2023-12-29|15:30:56.979] ignored equivocation; validator already tombstoned module=x/evidence validator=cosmosvalcons18w9cg8yt7ucrf23hprea64e4lttxq2l9wwtpd2 infraction_height=9 infraction_time="5372-04-29 14:44:18 +0000 UTC" Simulating... block 66/500, operation 50/285. Logs to writing to /home/ubuntu/.simapp/simulations/2023-12-29_15:30:58.log simulate.go:299: error on block 66/500, operation (62/285) from x/gov: recovered: exact pay out was not made from unbondings stack: goroutine 10 [running]: runtime/debug.Stack() /usr/local/go/src/runtime/debug/stack.go:24 +0x65 github.com/cosmos/cosmos-sdk/baseapp.newDefaultRecoveryMiddleware.func1({0x1b985c0, 0x28c77a0}) /home/ubuntu/go/pkg/mod/github.com/cosmos/cosmos-sdk@v0.45.11/baseapp/recovery.go:71 +0x27 github.com/cosmos/cosmos-sdk/baseapp.newRecoveryMiddleware.func1({0x1b985c0?, 0x28c77a0?}) /home/ubuntu/go/pkg/mod/github.com/cosmos/cosmos-sdk@v0.45.11/baseapp/recovery.go:39 +0x30 github.com/cosmos/cosmos-sdk/baseapp.processRecovery({0x1b985c0, 0x28c77a0}, 0xc002ad6900?) /home/ubuntu/go/pkg/mod/github.com/cosmos/cosmos-sdk@v0.45.11/baseapp/recovery.go:28 +0x37 github.com/cosmos/cosmos-sdk/baseapp.processRecovery({0x1b985c0, 0x28c77a0}, 0x28f9060?) /home/ubuntu/go/pkg/mod/github.com/cosmos/cosmos-sdk@v0.45.11/baseapp/recovery.go:33 +0x5e github.com/cosmos/cosmos-sdk/baseapp.(*BaseApp).runTx.func1() /home/ubuntu/go/pkg/mod/github.com/cosmos/cosmos-sdk@v0.45.11/baseapp/baseapp.go:632 +0x110 panic({0x1b985c0, 0x28c77a0}) /usr/local/go/src/runtime/panic.go:884 +0x213 github.com/shentufoundation/shentu/v2/x/shield/keeper.Keeper.MakePayoutByProviderDelegations({{0x28cfa60, 0xc000593820}, {0x28f74d0, 0xc00112a610}, {0x28ecd60, 0xc001131e60}, {0x28f4d68, 0xc0004bc780}, {0x2902c48, 0xc0001f23c0}, ...}, ...) /home/ubuntu/shentu/x/shield/keeper/proposal.go:454 +0x78c github.com/shentufoundation/shentu/v2/x/shield/keeper.Keeper.CreateReimbursement({{0x28cfa60, 0xc000593820}, {0x28f74d0, 0xc00112a610}, {0x28ecd60, 0xc001131e60}, {0x28f4d68, 0xc0004bc780}, {0x2902c48, 0xc0001f23c0}, ...}, ...) /home/ubuntu/shentu/x/shield/keeper/proposal.go:288 +0x805 github.com/shentufoundation/shentu/v2/x/shield.handleShieldClaimProposal({{0x28e8cb8, 0xc000056078}, {0x28f9060, 0xc007abf0c0}, {{0x0, 0x0}, {0x1e9b819, 0xe}, 0x42, {0x0, ...}, ...}, ...}, ...) /home/ubuntu/shentu/x/shield/handler.go:92 +0xed github.com/shentufoundation/shentu/v2/x/shield.NewShieldClaimProposalHandler.func1({{0x28e8cb8, 0xc000056078}, {0x28f9060, 0xc007abf0c0}, {{0x0, 0x0}, {0x1e9b819, 0xe}, 0x42, {0x0, ...}, ...}, ...}, ...) /home/ubuntu/shentu/x/shield/handler.go:80 +0x9d github.com/shentufoundation/shentu/v2/x/gov/keeper.Keeper.SubmitProposal({{{0x7fab585f5ff0, 0xc001177980}, {0x28e9fc0, 0xc0001383f0}, {0x28f18b0, 0xc0004bca80}, {0x7fab585f6018, 0xc0001f2f00}, {0x0, 0x0}, ...}, ...}, ...) /home/ubuntu/shentu/x/gov/keeper/proposal.go:103 +0x2c8 github.com/shentufoundation/shentu/v2/x/gov/keeper.msgServer.SubmitProposal({{{{0x7fab585f5ff0, 0xc001177980}, {0x28e9fc0, 0xc0001383f0}, {0x28f18b0, 0xc0004bca80}, {0x7fab585f6018, 0xc0001f2f00}, {0x0, 0x0}, ...}, ...}}, ...) /home/ubuntu/shentu/x/gov/keeper/msg_server.go:35 +0x238 github.com/cosmos/cosmos-sdk/x/gov/types._Msg_SubmitProposal_Handler.func1({0x28e8d28, 0xc0070a0120}, {0x1e3fb20?, 0xc0055539b0}) /home/ubuntu/go/pkg/mod/github.com/cosmos/cosmos-sdk@v0.45.11/x/gov/types/tx.pb.go:527 +0x78 github.com/cosmos/cosmos-sdk/baseapp.(*MsgServiceRouter).RegisterService.func2.1({0x28e8d28, 0xc0070a00c0}, {0x526b06?, 0x40d88b?}, 0x1e734e0?, 0xc000548888) /home/ubuntu/go/pkg/mod/github.com/cosmos/cosmos-sdk@v0.45.11/baseapp/msg_service_router.go:113 +0x90 github.com/cosmos/cosmos-sdk/x/gov/types._Msg_SubmitProposal_Handler({0x1e7a900?, 0xc001136800}, {0x28e8d28, 0xc0070a00c0}, 0x2696698, 0xc00485a080) /home/ubuntu/go/pkg/mod/git

In the oracle module, a proposer is assigned to each operator when the operator is created: ```go=9 // NewOperator returns an Operator object. func NewOperator(address sdk.AccAddress, proposer sdk.AccAddress, collateral sdk.Coins, accumulatedRewards sdk.Coins, name string) Operator { return Operator{ Address: address.String(), Proposer: proposer.String(), Collateral: collateral, AccumulatedRewards: accumulatedRewards, Name: name, } } ``` The naming implies the proposer is the one who proposes the new operator. However, `MsgCreateOperator` requires `m.Address` (i.e. the operator account) as the signer: ```go=74 // GetSigners defines whose signature is required. func (m MsgCreateOperator) GetSigners() []sdk.AccAddress { addr := sdk.MustAccAddressFromBech32(m.Address) return []sdk.AccAddress{addr} } ``` Moreover, there are no restrictions or checks on the proposer field, meaning the operator can assign any value (for example, the certifier account) as its proposer. This may be misleading to users. Given the issues identified in the oracle module's design (refer to findings ABO-01 and TAS-01 for details), the auditing team seeks clarification from the Shentu team. Specifically, the auditing team want to confirm whether the design intends to restrict proposal rights exclusively to certain roles, such as certifiers or Oracle admins.

Files: - `x/oracle/keeper/task.go` - `x/oracle/types/params.go` Commit: - [`3a7e28ae8dec6bbf4d7b35252a745b82a463f781`](https://github.com/shentufoundation/shentu/tree/3a7e28ae8dec6bbf4d7b35252a745b82a463f781) The `TotalValidTaskCollateral` function determines the total valid collateral for a task across three scenarios: when the task score matches `MinScore`, when the task score is below `ThresholdScore`, and when the task score is equal to or exceeds `ThresholdScore`. ```go=17 var ( MinScore = sdk.NewInt(0) MaxScore = sdk.NewInt(100) DefaultThresholdScore = sdk.NewInt(50) DefaultAggregationResult = sdk.NewInt(50) DefaultExpirationDuration = time.Duration(24) * time.Hour DefaultAggregationWindow = int64(20) DefaultEpsilon1 = sdk.NewInt(1) DefaultEpsilon2 = sdk.NewInt(100) DefaultShortcutQuorum = sdk.NewDecWithPrec(50, 2) DefaultLockedInBlocks = int64(30) DefaultMinimumCollateral = int64(50000) ) ``` With MinScore defaulting to 0 and `ThresholdScore` set to 50, the total valid collateral amount for the scenario where the task score falls below `ThresholdScore` inherently includes the total valid collateral amount for the scenario where the task score equals `MinScore`. This implies that the collateral amount associated with an operator responding with a `MinScore` will also be considered as the total valid collateral amount for the scenario where the task score falls below `ThresholdScore`. ```go=537 } else if task.GetScore() < taskParams.ThresholdScore.Int64() { for _, response := range responses { if response.Score.LT(taskParams.ThresholdScore) { operatorAddr := sdk.MustAccAddressFromBech32(response.Operator) collateral, err := k.GetCollateralAmount(ctx, operatorAddr) if err != nil { continue } totalValidTaskCollateral = totalValidTaskCollateral.Add( amplifier.Mul(collateral).Quo(response.Score.Add(taskParams.Epsilon1)), ) } } } ``` This aspect of the current design could potentially impact an operator's likelihood of successfully scoring a task with a `MinScore`, thereby increasing their chances of receiving a bounty. The auditing team would like to check with the Shentu team if this is an intended design.

Files: - `x/shield/keeper/pool.go` - `x/shield/types/shield.pb.go` Commit: - [`3a7e28ae8dec6bbf4d7b35252a745b82a463f781`](https://github.com/shentufoundation/shentu/tree/3a7e28ae8dec6bbf4d7b35252a745b82a463f781) According to the comments in the `CreatePool` function, the purpose of this function is to create a pool and the sponsor's shield. However, in the actual logic implementation, the shield is paid by the admin account instead of the sponsor. The audit team seeks clarification from the Shentu team to confirm if this aligns with the original design. ```go=169 // CreatePool creates a pool and sponsor's shield. func (k Keeper) CreatePool(ctx sdk.Context, creator sdk.AccAddress, shield sdk.Coins, serviceFees sdk.Coins, sponsor string, sponsorAddr sdk.AccAddress, description string, shieldLimit sdk.Int) (uint64, error) { admin := k.GetAdmin(ctx) if !creator.Equals(admin) { return 0, types.ErrNotShieldAdmin } if _, found := k.GetPoolsBySponsor(ctx, sponsor); found { return 0, types.ErrSponsorAlreadyExists } // Set the new project pool. poolID := k.GetNextPoolID(ctx) pool := types.NewPool(poolID, description, sponsor, sponsorAddr, shieldLimit, sdk.ZeroInt()) k.SetPool(ctx, pool) k.SetNextPoolID(ctx, poolID+1) // Purchase shield for the pool. if _, err := k.purchaseShield(ctx, poolID, shield, "shield for sponsor", creator, serviceFees, sdk.NewCoins()); err != nil { return poolID, err } return poolID, nil } ``` Furthermore, considering the context of the `shield` module, there are no functionalities related to the sponsor. The audit team would appreciate more clarification from the Shentu team regarding the role and functionality of the sponsor in this context.