Shentu Chain - audit

In the bounty module, the role bounty admin has authority over the following functionalities: - edit a program, - activate a finding, - confirm a finding is paid, and - close a finding. In the cert module, the role certifier has authority over the following functionalities: - issue a certificate, - revoke a certificate, and - certify a platform. In the oracle module, the role certifier has authority over the following functionality: - remove an operator. In the shield module, the role Shield admin has authority over the following functionalities: - create a Shield pool, - update a Shield pool's configurations, - pause an active Shield pool, - activate a paused Shield pool, and - update a Shield pool's sponsor information. Any compromise to the aforementioned roles may allow a hacker to take advantage of this authority and adversely impact the project.

In the oracle module, operators submit task responses and earn rewards accordingly. However, malicious operators have access to all other operators' responses, enabling them to adjust their collaterals in a way that maximizes their rewards or influences the task outcomes. An Oracle task will be closed several blocks (20 blocks by default) after the task creation. Before the task is closed, the task results are calculated by a weighted average of scores provided by operators, where weights are determined by operators'collaterals: ```go=483 result := taskParams.AggregationResult totalCollateral := sdk.NewInt(0) minScoreCollateral := sdk.NewInt(0) responses := task.GetResponses() for i, response := range responses { operatorAddr := sdk.MustAccAddressFromBech32(response.Operator) amount, err := k.GetCollateralAmount(ctx, operatorAddr) if err != nil { continue } result = result.Add(response.Score.Mul(amount)) responses[i].Weight = amount totalCollateral = totalCollateral.Add(amount) if response.Score.Equal(types.MinScore) { minScoreCollateral = minScoreCollateral.Add(amount) } } if totalCollateral.IsPositive() { if minScoreCollateral.MulRaw(3).GTE(totalCollateral) { result = types.MinScore for i, response := range responses { if !response.Score.Equal(types.MinScore) { responses[i].Weight = sdk.NewInt(0) } } } else { result = result.Quo(totalCollateral) } task.SetStatus(types.TaskStatusSucceeded) } else { task.SetStatus(types.TaskStatusFailed) } task.SetScore(result.Int64()) ``` After the result aggregation, task rewards are distributed to operators based on their responses and collaterals: - If the final score is high, operators providing higher scores will gain more rewards; otherwise, operators providing lower scores will gain more rewards. - Operators with larger collateral deposits are eligible for higher rewards. Given that all operators' responses are publicly visible, malicious operators can discern the current task results before they are finalized. This visibility enables them to adopt strategies to either maximize their rewards or manipulate the task outcomes.

Files: - `x/oracle/keeper/task.go` Commit: - [`3a7e28ae8dec6bbf4d7b35252a745b82a463f781`](https://github.com/shentufoundation/shentu/tree/3a7e28ae8dec6bbf4d7b35252a745b82a463f781) The Oracle module collects task bounties from the task creator at the time of task creation. These bounties may consist of a set of coins. ```go=227 func (k Keeper) CreateTask(ctx sdk.Context, creator sdk.AccAddress, task types.TaskI) error { ... if task.GetStatus() == types.TaskStatusPending { k.AddToClosingTaskIDs(ctx, task) if err := k.CollectBounty(ctx, task.GetBounty(), creator); err != nil { return err } if _, ok := task.(*types.TxTask); ok { k.TryShortcut(ctx, task) } } return nil } ``` However, a flaw arises during the bounty distribution process. While the `DistributeBounty` function iterates through each bounty coin to compute the reward, it fails to consider the existence of multiple existing bounty coins when recording the reward with the expression `responses[i].Reward = reward`. ```go for _, bounty := range task.GetBounty() { if task.GetScore() == types.MinScore.Int64() { for i := range responses { if responses[i].Score.Equal(types.MinScore) { ... amount := bounty.Amount.Mul(collateral).Quo(totalValidTaskCollateral) reward := sdk.NewCoins(sdk.NewCoin(bounty.Denom, amount)) if err := k.AddReward(ctx, operatorAddr, reward); err == nil { responses[i].Reward = reward } } } } ... } ``` This oversight results in the reward being overwritten, ultimately causing discrepancies between the reward distributed to the operator and the reward recorded in the task response, with the reward recorded in the task response potentially consistently exceeding the reward already distributed to the operator. As a result, the reward amount documented in the task response is less than the actual bounty distributed to the operator, allowing the task creator to potentially claim a larger portion of the refunded bounty, leading to insufficient Oracle model balance, triggering the `RefundBounty` function to panic. ```go=637 func (k Keeper) RefundBounty(ctx sdk.Context, task types.TaskI) error { taskCreator, err := sdk.AccAddressFromBech32(task.GetCreator()) if err != nil { panic(err) } totalReward := make(sdk.Coins, 0, 1) for _, response := range task.GetResponses() { if response.Reward != nil { totalReward = totalReward.Add(response.Reward...) } } bounties := task.GetBounty() leftBounty := bounties.Sub(totalReward) if leftBounty != nil && leftBounty.IsAllPositive() { oracleAddress := k.accountKeeper.GetModuleAddress(types.ModuleName) spendableCoins := k.bankKeeper.SpendableCoins(ctx, oracleAddress) if ok := spendableCoins.IsAllGTE(leftBounty); !ok { panic("Insufficient oracle model balance") } if err = k.bankKeeper.SendCoinsFromModuleToAccount(ctx, types.ModuleName, taskCreator, leftBounty); err != nil { if err = k.distrKeeper.FundCommunityPool(ctx, leftBounty, oracleAddress); err != nil { panic(err) } return err } } return nil } ``` Ultimately, the disruption in the balance of funds leads to the break of fund invariants. ```go=83 func FundInvariant(k Keeper) sdk.Invariant { return func(ctx sdk.Context) (string, bool) { broken := false var ( sumCollateral, sumReward, sumPendingBounty, sumWithdraw sdk.Coins ) k.IterateAllOperators(ctx, func(op types.Operator) bool { sumCollateral = sumCollateral.Add(op.Collateral...) sumReward = sumReward.Add(op.AccumulatedRewards...) return false }) k.IteratorAllTasks(ctx, func(task types.TaskI) bool { if task.GetStatus() == types.TaskStatusPending { sumPendingBounty = sumPendingBounty.Add(task.GetBounty()...) } return false }) k.IterateAllWithdraws(ctx, func(wd types.Withdraw) bool { sumWithdraw = sumWithdraw.Add(wd.Amount...) return false }) totalCollateral, err := k.GetTotalCollateral(ctx) oracleAddress := k.accountKeeper.GetModuleAddress(types.ModuleName) moduleBalance := k.bankKeeper.SpendableCoins(ctx, oracleAddress) if err != nil { return sdk.FormatInvariant(types.ModuleName, "fund-check", "cannot get module account balance"), false } if !totalCollateral.IsEqual(sumCollateral) { broken = true } if !moduleBalance.Sub(totalCollateral). IsAllGTE(sumReward.Add(sumPendingBounty...)) { broken = true } if !moduleBalance.IsAllGTE(sumCollateral.Add(sumWithdraw...)) { broken = true } return sdk.FormatInvariant(types.ModuleName, "fund-check", fmt.Sprintf("\n\ttotal collateral amount: %s"+ "\n\tmodule account balance: %s"+ "\n\tsum of operator's collateral: %s"+ "\n\tsum of operator's reward: %s"+ "\n\tsum of operator's withdraw: %s"+ "\n\tsum of pending task's bounty: %s\n", totalCollateral, moduleBalance, sumCollateral, sumReward, sumWithdraw, sumPendingBounty)), broken } } ```

Files: - `x/oracle/keeper/withdraw.go` - `x/oracle/keeper/operator.go` Commit: - [`3a7e28ae8dec6bbf4d7b35252a745b82a463f781`](https://github.com/shentufoundation/shentu/tree/3a7e28ae8dec6bbf4d7b35252a745b82a463f781) In the oracle module, an operator can utilize the `MsgReduceCollateral` message to decrease collateral, and the operator can do so multiple times within the same block height. ```go=166 func (k Keeper) ReduceCollateral(ctx sdk.Context, address sdk.AccAddress, decrement sdk.Coins) error { if !k.IsOperator(ctx, address) { return types.ErrNoOperatorFound } operator, err := k.GetOperator(ctx, address) if err != nil { return err } operator.Collateral = operator.Collateral.Sub(decrement) // Finding: Panic for negative coin amount if k.IsBelowMinCollateral(ctx, operator.Collateral) { return types.ErrNoEnoughCollateral } k.SetOperator(ctx, operator) if err := k.ReduceTotalCollateral(ctx, decrement); err != nil { return err } operatorAddr := sdk.MustAccAddressFromBech32(operator.Address) if err := k.CreateWithdraw(ctx, operatorAddr, decrement); err != nil { return err } return nil } ``` However, utilizing the combination of `withdrawAdd` and `DueBlock` as the key for storing withdrawal records may result in overwriting the previous withdrawal record. This occurs because the store key remains the same within the same block for the same operator. Consequently, this could lead to the operator not receiving the expected withdrawal collateral, resulting in potential losses. ```go=12 func (k Keeper) SetWithdraw(ctx sdk.Context, withdraw types.Withdraw) { store := ctx.KVStore(k.storeKey) bz := k.cdc.MustMarshalLengthPrefixed(&withdraw) withdrawAddr := sdk.MustAccAddressFromBech32(withdraw.Address) store.Set(types.WithdrawStoreKey(withdrawAddr, withdraw.DueBlock), bz) } ```

In the oracle module, operators submit responses to tasks and receive rewards based on their answers and collateral. Although the system rewards accurate or "good" responses, it lacks a mechanism to penalize incorrect or "bad" responses. A task's bounty is distributed following these rules: - If the final score is the minimum score, operators responding with the minimum score share the bounty. - If the final score is below the threshold (50 by default), operators responding with scores lower than the threshold share the bounty. The lower the provided score is, the higher the reward is. - Otherwise, operators responding with scores not lower than the threshold share the bounty. The higher the provided score is, the higher the reward is. ```go=578 for _, bounty := range task.GetBounty() { if task.GetScore() == types.MinScore.Int64() { for i := range responses { if responses[i].Score.Equal(types.MinScore) { operatorAddr := sdk.MustAccAddressFromBech32(responses[i].Operator) collateral, err := k.GetCollateralAmount(ctx, operatorAddr) if err != nil { continue } amount := bounty.Amount.Mul(collateral).Quo(totalValidTaskCollateral) reward := sdk.NewCoins(sdk.NewCoin(bounty.Denom, amount)) if err := k.AddReward(ctx, operatorAddr, reward); err == nil { responses[i].Reward = reward } } } } else if task.GetScore() < taskParams.ThresholdScore.Int64() { for i := range responses { if responses[i].Score.LT(taskParams.ThresholdScore) { operatorAddr, err := sdk.AccAddressFromBech32(responses[i].Operator) if err != nil { panic(err) } collateral, err := k.GetCollateralAmount(ctx, operatorAddr) if err != nil { continue } amount := bounty.Amount.Mul( amplifier.Mul(collateral).Quo(responses[i].Score.Add(taskParams.Epsilon1)), ).Quo(totalValidTaskCollateral) reward := sdk.NewCoins(sdk.NewCoin(bounty.Denom, amount)) if err := k.AddReward(ctx, operatorAddr, reward); err == nil { responses[i].Reward = reward } } } } else { for i := range responses { if responses[i].Score.GTE(taskParams.ThresholdScore) { if responses[i].Score.GTE(taskParams.ThresholdScore) { operatorAddr := sdk.MustAccAddressFromBech32(responses[i].Operator) collateral, err := k.GetCollateralAmount(ctx, operatorAddr) if err != nil { continue } amount := bounty.Amount.Mul( amplifier.Mul(collateral).Quo(types.MaxScore.Sub(responses[i].Score).Add(taskParams.Epsilon2)), ).Quo(totalValidTaskCollateral) reward := sdk.NewCoins(sdk.NewCoin(bounty.Denom, amount)) if err := k.AddReward(ctx, operatorAddr, reward); err == nil { responses[i].Reward = reward } } } } } k.SetTask(ctx, task) return nil } ``` Meanwhile, the worst result for an operator who does not respond with a "good" score is not gaining any rewards from the task. It will not lose anything other than the transaction fees. Consequently, an operator might adopt a strategy of consistently submitting the highest (or lowest) scores without carefully assessing the tasks, aiming to secure task rewards. Over time, this approach could discourage operators from providing thoughtful and accurate scores, ultimately resulting in irrelevant and unreliable task outcomes.

Files: - `x/bounty/keeper/msg.server.go` Commit: - [`3a7e28ae8dec6bbf4d7b35252a745b82a463f781`](https://github.com/shentufoundation/shentu/tree/3a7e28ae8dec6bbf4d7b35252a745b82a463f781) The `CreateProgram` function is accessible to users and lacks any access control measures. Any user can call this function to create a program without any restrictions. ```go=25 func (k msgServer) CreateProgram(goCtx context.Context, msg *types.MsgCreateProgram) (*types.MsgCreateProgramResponse, error) { ctx := sdk.UnwrapSDKContext(goCtx) operatorAddr, err := sdk.AccAddressFromBech32(msg.OperatorAddress) if err != nil { return nil, err } _, found := k.GetProgram(ctx, msg.ProgramId) if found { return nil, types.ErrProgramAlreadyExists } createTime := ctx.BlockHeader().Time program, err := types.NewProgram(msg.ProgramId, msg.Name, msg.Detail, operatorAddr, types.ProgramStatusInactive, createTime) if err != nil { return nil, err } k.SetProgram(ctx, program) ctx.EventManager().EmitEvents(sdk.Events{ sdk.NewEvent( types.EventTypeCreateProgram, sdk.NewAttribute(types.AttributeKeyProgramID, msg.ProgramId), ), sdk.NewEvent( sdk.EventTypeMessage, sdk.NewAttribute(sdk.AttributeKeyModule, types.AttributeValueCategory), sdk.NewAttribute(sdk.AttributeKeySender, msg.OperatorAddress), ), }) return &types.MsgCreateProgramResponse{}, nil } ``` This raises concerns that hacker could exploit this by generating numerous useless `program` instances with irrelevant messages, potentially impacting functionality. And hackers can also create `program` that imitate some real projects, making it impossible to distinguish whether these `program` are real or not. Additionally, a question arises: should the creator of the `program` act as the reward provider for confirmed findings? If there's no requirement for the creator to deposit collateral, the program owner might receive findings without any cost.

Debugging messages in the linked lines are identical despite the different operations before these lines, making it difficult to track debugging information in the development.

Files: - `x/oracle/keeper/operator.go` - `x/cert/keeper/proposal_handler.go` Commit: - [`3a7e28ae8dec6bbf4d7b35252a745b82a463f781`](https://github.com/shentufoundation/shentu/tree/3a7e28ae8dec6bbf4d7b35252a745b82a463f781) In the oracle module, the `removeOperator` function is limited to allowing only certifier proposers to remove operators, except when the operator is the same as the proposer. ```go=83 func (k Keeper) RemoveOperator(ctx sdk.Context, operatorAddress, proposerAddress string) error { ... if operatorAddress != proposerAddress && !k.CertKeeper.IsCertifier(ctx, proposerAddr) { return types.ErrUnqualifiedRemover } ... ``` However, there is a potential issue where the proposer assigned to an operator might not be a valid certifier during operator creation, which could result in the operator being unable to be removed, thereby locking the collateral provided by the operator in the Oracle module. ```go=64 func (k Keeper) CreateOperator(ctx sdk.Context, address sdk.AccAddress, collateral sdk.Coins, proposer sdk.AccAddress, name string) error { if k.IsOperator(ctx, address) { return types.ErrOperatorAlreadyExists } if k.IsBelowMinCollateral(ctx, collateral) { return types.ErrNoEnoughCollateral } operator := types.NewOperator(address, proposer, collateral, nil, name) k.SetOperator(ctx, operator) if err := k.AddTotalCollateral(ctx, collateral); err != nil { return err } if err := k.bankKeeper.SendCoinsFromAccountToModule(ctx, address, types.ModuleName, collateral); err != nil { return err } return nil } ``` Furthermore, even if the proposer is a certifier at the time of operator creation, they can still be removed as a certifier in the Cert module, leading to inconsistencies, and impact removing the operator. ```go=10 func HandleCertifierUpdateProposal(ctx sdk.Context, k Keeper, p *types.CertifierUpdateProposal) error { certifierAddr, err := sdk.AccAddressFromBech32(p.Certifier) if err != nil { panic(err) } proposerAddr, err := sdk.AccAddressFromBech32(p.Proposer) if err != nil { panic(err) } switch p.AddOrRemove { case types.Add: if k.IsCertifier(ctx, certifierAddr) { return types.ErrCertifierAlreadyExists } if p.Alias != "" && k.HasCertifierAlias(ctx, p.Alias) { return types.ErrRepeatedAlias } certifier := types.NewCertifier(certifierAddr, p.Alias, proposerAddr, p.Description) k.SetCertifier(ctx, certifier) return nil case types.Remove: certifiers := k.GetAllCertifiers(ctx) if len(certifiers) == 1 { return types.ErrOnlyOneCertifier } return k.deleteCertifier(ctx, certifierAddr) default: return types.ErrAddOrRemove } } ``` Additionally, the `CreateOperator` function is missing validation mechanisms, allowing any user to become an operator. The entity signing the `MsgCreateOperator` message is the address seeking to become an `operator`, rather than the `proposer`. This means any user can execute the `CreateOperator` function to become an operator themselves. Following this, they can deposit and respond to tasks. This presents a scenario where bad actors could potentially exploit this to influence the task's score. The auditing team would like to check with the Shentu team if these are intended designs.

In the `OpCode` `EXTCODECOPY`, gas is consumed when the `GetAccount` function is invoked: ```go maybe.PushError(engine.UseGasNegative(params.Gas, engine.GasGetAccount)) acc := engine.MustGetAccount(st.CallFrame, maybe, address) ``` However, for the `OpCode` `EXTCODEHASH`, additional gas expenditure is not recorded: ```go case EXTCODEHASH: // 0x3F address := stack.PopAddress() acc := engine.GetAccount(st.CallFrame, maybe, address) if acc == nil { // If the account does not exist, 0 is pushed onto the stack. stack.Push64(0) } else { // Computes the keccak256 hash of the contract's code var extcodehash Word256 if len(acc.CodeHash) > 0 { copy(extcodehash[:], acc.CodeHash) } else { copy(extcodehash[:], crypto.Keccak256(acc.Code())) } stack.Push(extcodehash) } ``` The auditing team would like to understand the rationale behind this design choice from the development team.

The CVM module, built on Hyperledger Burrow, faces potential security risks due to Burrow reaching [End of Life (EOL)](https://github.com/hyperledger/toc/issues/34) status, as it may no longer receive updates necessary to maintain security parity with the evolving EVM. The auditing team would like to know if there are any plans to maintain the CVM module to keep it consistent with the latest EVM.