Shentu - MultiSig

Repository: - multisig-service File: - controller/login.go The `LoginWithWallet` function provides users with a JWT token that includes their wallet address, which is used for authentication. However, both the public key and signature are derived from the message the user submits. In the following code snippet: ```go=31 if !util.VerifyADR036Sign(req.Address, req.PubKey, req.Signature, req.Msg) { logger.Errorw("Invalid signature information", zap.Any("req", req)) SendBadReqResponse(c, errno.InvalidRequestErr, nil) return } ``` the verification process checks whether the provided signature matches the corresponding public key and message. The vulnerability arises because an attacker can create a valid signature for a message and address of another user by utilizing the attacker's own private key. As the signature verification at line 31 only ensures that the signature matches the given public key and message, it would incorrectly validate as true. This enables the attacker to impersonate a legitimate user, allowing unauthorized actions, such as using the task owner's address to accept comments or engage in unauthorized economic activities.

Repository: - multisig-service File: - service/account.go The `CreateAccount` function in the multisig service allows users to create an account using a public key and an address. However, there is no verification process to ensure that the user creating the account actually owns the address they are registering. This issue is exacerbated by the fact that the same address cannot be registered more than once. ```go=106 dbAccount, err := m.Select(m.Address).Where(m.Address.Eq(req.Address)).First() ``` This could lead to a vulnerability where a malicious actor uses their own public key to register an account with someone else's address, effectively squatting on that address. Once an address is registered, the legitimate owner cannot create an account with it. This vulnerability could prevent legitimate users from creating multisig accounts with their own addresses, potentially leading to a denial of service for those users. It could also result in unauthorized control or association of addresses with malicious actors.

Repository: - multisig-service Files: - service/chain_registry.go - service/account.go Two different create account functions `createAccounts` and `CreateAccount` have inconsistent handling of multisig account members. In the first function, `createAccounts`, members are constructed as a comma-separated string, **service/chain_registry.go** ```go=118 for _, pubkey := range account.PubKey.PublicKeys { if len(members) != 0 { members += "," } member, err := util.AddressFromPubKey(pubkey.Key, pubkey.KeyType, bech32Prefix) if err != nil { util.Logger.Errorf("get member err:%s", err.Error()) continue } members += member } ``` while in the second function, `CreateAccount`, members are serialized into JSON format. **service/account.go** ```go=93 membersJson, _ := json.Marshal(req.Members) ``` This inconsistency can lead to issues in data storage, account creation logic, and future account operations. Specifically, if the members are formatted as a comma-separated string, it causes the `RenameMemberAccount` function to fail. **service/account.go** ```go=241 func (s *MultisigService) RenameMemberAccount(callerHexAddr string, req entity.AccountRequest) *errno.Errno { ... var members []entity.MemberAccount err = json.Unmarshal([]byte(dbAccount.Members), &members) if err != nil { s.logger.Errorf("Error decoding JSON: %v", err) return errno.Err(errno.InternalServerErr, "invalid db data") } ... ```

Repository: - multisig-service Files: - service/account.go The `CreateAccount` function in the `MultisigService` lacks proper input validation for the `pubkey` and `members` fields. Specifically, there is no limit on the length of the `pubkey`, and the `members` array does not have a length limit or duplication check. This could lead to excessively large database entries, potentially causing performance issues or even crashing the node if exploited by a malicious user through automated scripts.

Repository: - multisig-service Files: - service/chain_registry.go - dal/model/multisig_account.gen.go In the `createAccounts` function, the `MultisigAccount` struct requires the `Pubkey` and `Status` fields to be set, as they are defined as not null in the database schema. **dal/model/multisig_account.gen.go** ```go=14 type MultisigAccount struct { Address string `gorm:"column:address;primaryKey" json:"address"` Name string `gorm:"column:name" json:"name"` Threshold int32 `gorm:"column:threshold;not null" json:"threshold"` ChainName string `gorm:"column:chain_name;not null" json:"chain_name"` Members string `gorm:"column:members;not null" json:"members"` Status string `gorm:"column:status;not null" json:"status"` Pubkey string `gorm:"column:pubkey;not null" json:"pubkey"` CreateTime time.Time `gorm:"column:create_time" json:"create_time"` } ``` However, these fields are not populated when creating a multisig account, resulting in the database operation failing. This issue leads to the consistent failure of account creation attempts, impacting the functionality where multisig accounts are intended to be created and stored.

Repository: - multisig-service Files: - util/token.go The `ParseToken` function in the code may lead to a nil pointer dereference if an invalid JWT token is passed. Specifically, if the JWT token contains more than two periods (`.`), the `jwt.ParseWithClaims` function may return a `nil` token. Subsequent access to `token.Claims` without checking if `token` is `nil` can result in a runtime panic. ```go=35 func ParseToken(tokenString string) (*CustomClaims, error) { token, err := jwt.ParseWithClaims(tokenString, &CustomClaims{}, func(token *jwt.Token) (interface{}, error) { return []byte(Cfg().Server.TokenKey), nil }) claims, ok := token.Claims.(*CustomClaims) if !ok || token.Valid { return nil, err } if claims.Issuer != common.MultisigIssuer+Cfg().Server.Env { return nil, fmt.Errorf("invalid issuer") } if claims.Subject != common.MultisigSubject { return nil, fmt.Errorf("invalid task") } return claims, nil } ```

Repository: - multisig-service Files: - multisig-service-20241103-778c2ec8/util/token.go The audited code contains private keys stored in plaintext. This practice, initially implemented for early development or testing purposes, poses a significant security risk. These private keys must be removed to ensure the security of the system.

Repository: - multisig-service File: - util/http.go The `HttpGetReq` function in its current form sets an HTTP client timeout of 10 minutes (Line 17). ```go=10 func HttpGetReq(url string) ([]byte, error) { client := &http.Client{} req, err := http.NewRequest("GET", url, nil) if err != nil { Logger.Errorw("", zap.Error(err)) return nil, err } client.Timeout = time.Minute * 10 res, err := client.Do(req) if err != nil { Logger.Errorw("", zap.Error(err)) return nil, err } defer res.Body.Close() body, err := io.ReadAll(res.Body) if err != nil { Logger.Errorw("", zap.Error(err)) return nil, err } //Logger.Debugw("", zap.Any("body", string(body))) return body, nil } ``` This duration is excessive for most use cases, where responses are generally expected within seconds. Setting a long timeout can lead to unnecessary resource consumption, especially in systems handling multiple concurrent requests. Additionally, it could cause delays in detecting network or server issues, thus impacting the overall performance of the application.

Repository: - multisig-service Files: - service/transactions.go The `CreateTx` function includes a validation check that prevents the creation of transactions with the same `txContent` for the same `MultisigAddress`. While this is useful for avoiding duplicate transactions, it may restrict legitimate cases where the same transaction content needs to be submitted multiple times (e.g., due to external errors or retries). This could lead to user frustration or blocked transactions when the same content is expected to be processed under different circumstances. ```go=51 pTx, err := p.Select(p.ID).Where(p.WalletAddress.Eq(req.MultisigAddress), p.TxContent.Eq(string(txContent))).First() if err == nil { s.logger.Errorf("has already uploaded tx content, pending id:%s", pTx.ID) return nil, errno.Err(errno.InvalidRequestErr, "already uploaded tx content") } ``` Additionally, this validation logic does not take into account other relevant factors like transaction `Deleted` status that might warrant processing the same `txContent` again.

Repository: - multisig-service File: - util/token.go The `CreateToken` function in the current implementation of JWT (JSON Web Token) only includes the `ExpiresAt` claim. This omission of other critical claims such as `Issuer`, `Subject`, `Audience`, `IssuedAt`, and `NotBefore` reduces the security and flexibility of the token. ```go=16 func CreateToken(hexAddr string) (string, error) { expTime := time.Now().Add(time.Hour * 720).Unix() claims := CustomClaims{ HexAddr: hexAddr, StandardClaims: jwt.StandardClaims{ ExpiresAt: expTime, }, } token := jwt.NewWithClaims(jwt.SigningMethodHS256, claims) tokenString, err := token.SignedString(signingTokenKey) return tokenString, err } ``` Without claims like `Audience` or `Issuer`, it's difficult to ensure that the token is used only by the intended recipient or in the correct system. This can increase the risk of token misuse or forgery, especially in distributed or multi-service environments