Prepared by:
HALBORN
Last Updated 09/25/2024
Date of Engagement by: August 5th, 2024 - September 11th, 2024
100% of all REPORTED Findings have been addressed
All findings
6
Critical
0
High
1
Medium
1
Low
2
Informational
2
Zenrock
engaged Halborn to conduct a security assessment on their modules, beginning on Halborn to conduct a security assessment on the appchain modules, beginning on 08/05/2024 and ending on 09/11/2024. The security assessment was scoped to the sections of code that pertain to the modules. Commit hashes and further details can be found in the Scope section of this report.
Halborn was provided 4 weeks for the engagement and assigned 1 full-time security engineer to review the security of the smart contracts in scope. The engineer is a blockchain and smart contract security experts with advanced penetration testing and smart contract hacking skills, and deep knowledge of multiple blockchain protocols.
The purpose of the assessment is to:
- Ensure that the Zenrock Chain Modules operate as intended.
- Identify potential security issues with the Zenrock Chain Modules in the Zenrock Chain.
In summary, Halborn identified some security concerns that were mostly addressed by the Zenrock team
.
Halborn performed a combination of manual and automated security testing to balance efficiency, timeliness, practicality, and accuracy in regard to the scope of the custom modules. While manual testing is recommended to uncover flaws in logic, process, and implementation; automated testing techniques help enhance coverage of structures and can quickly identify items that do not follow security best practices. The following phases and associated tools were used throughout the term of the assessment:
- Research into architecture and purpose.
- Static Analysis of security for scoped repository, and imported functions. (e.g., staticcheck
, gosec
, unconvert
, codeql
, ineffassign
and semgrep
)
- Manual Assessment for discovering security vulnerabilities in the codebase.
- Ensuring the correctness of the codebase.
- Dynamic Analysis of files and modules related to the modules.
EXPLOITABILIY METRIC () | METRIC VALUE | NUMERICAL VALUE |
---|---|---|
Attack Origin (AO) | Arbitrary (AO:A) Specific (AO:S) | 1 0.2 |
Attack Cost (AC) | Low (AC:L) Medium (AC:M) High (AC:H) | 1 0.67 0.33 |
Attack Complexity (AX) | Low (AX:L) Medium (AX:M) High (AX:H) | 1 0.67 0.33 |
IMPACT METRIC () | METRIC VALUE | NUMERICAL VALUE |
---|---|---|
Confidentiality (C) | None (I:N) Low (I:L) Medium (I:M) High (I:H) Critical (I:C) | 0 0.25 0.5 0.75 1 |
Integrity (I) | None (I:N) Low (I:L) Medium (I:M) High (I:H) Critical (I:C) | 0 0.25 0.5 0.75 1 |
Availability (A) | None (A:N) Low (A:L) Medium (A:M) High (A:H) Critical (A:C) | 0 0.25 0.5 0.75 1 |
Deposit (D) | None (D:N) Low (D:L) Medium (D:M) High (D:H) Critical (D:C) | 0 0.25 0.5 0.75 1 |
Yield (Y) | None (Y:N) Low (Y:L) Medium (Y:M) High (Y:H) Critical (Y:C) | 0 0.25 0.5 0.75 1 |
SEVERITY COEFFICIENT () | COEFFICIENT VALUE | NUMERICAL VALUE |
---|---|---|
Reversibility () | None (R:N) Partial (R:P) Full (R:F) | 1 0.5 0.25 |
Scope () | Changed (S:C) Unchanged (S:U) | 1.25 1 |
Severity | Score Value Range |
---|---|
Critical | 9 - 10 |
High | 7 - 8.9 |
Medium | 4.5 - 6.9 |
Low | 2 - 4.4 |
Informational | 0 - 1.9 |
Critical
0
High
1
Medium
1
Low
2
Informational
2
Security analysis | Risk level | Remediation Date |
---|---|---|
Unhandled Error in AddApprover Call Within ApproveAction Function | High | Solved - 09/10/2024 |
Implement Support for ERC-20 transferFrom Method in ParseEthereumTransaction | Medium | Solved - 09/11/2024 |
Implement PartyThreshold Validation in NewKeyring Function | Low | Solved - 09/10/2024 |
Implement Expiration Date Utilization in AddMultiGrant Function | Low | Risk Accepted |
Lack of spec on the modules | Informational | Solved - 09/16/2024 |
Open TO-DOs | Informational | Acknowledged |
// High
In the ApproveAction
function of the msgServer
, there is a call to act.AddApprover(participant)
that does not include any error checking. This omission could lead to silent failures and inconsistent state updates.
participant, err := policy.AddressToParticipant(msg.Creator)
if err != nil {
return nil, err
}
act.AddApprover(participant)
- The AddApprover
method is called without checking its return value or potential error.
- If AddApprover
internally fails (e.g., due to duplicate approver, capacity limits, or other constraints), this failure would go unnoticed.
Modify the code to check for errors returned by AddApprover
.
SOLVED: The Zenrock team solved the issue by adding an error check.
// Medium
The current implementation of ParseEthereumTransaction
only supports parsing of the transfer
method for ERC-20 tokens. It lacks support for the transferFrom
method, which is a common and important function in ERC-20 contracts.
Impact
- Limited functionality in parsing ERC-20 transactions
- Inability to properly handle and validate transferFrom
transactions
- Potential misinterpretation of transferFrom
calls as unsupported contract calls
Add support for parsing the transferFrom
method in the parseCallData
function.
SOLVED: The Zenrock Team solved the issue by adding a support for function.
// Low
The NewKeyring
function in the keeper
package is not validating that the PartyThreshold
is greater than zero. This could lead to the creation of keyrings with invalid thresholds, potentially causing issues in multi-party operations or compromising the security model of the keyring.
func (k msgServer) NewKeyring(goCtx context.Context, msg *types.MsgNewKeyring) (*types.MsgNewKeyringResponse, error) {
keyring := &types.Keyring{
Creator: msg.Creator,
Description: msg.Description,
Admins: []string{msg.Creator},
PartyThreshold: msg.PartyThreshold,
KeyReqFee: msg.KeyReqFee,
SigReqFee: msg.SigReqFee,
IsActive: true,
}
// ... rest of the function
}
The function creates a new keyring with the PartyThreshold
directly set from the message without any validation.
Modify the NewKeyring
function to include a check that ensures the PartyThreshold
is greater than zero.
SOLVED: The Zenrock team solved the issue by adding threshold enforcement.
// Low
The AddMultiGrant
function in the keeper
package is creating authorizations without setting an expiration date. This could lead to permanent authorizations.
func (k msgServer) AddMultiGrant(goCtx context.Context, msg *types.MsgAddMultiGrant) (*types.MsgAddMultiGrantResponse, error) {
// ... (earlier code)
for _, m := range msg.Msgs {
auth := &authztypes.GenericAuthorization{
Msg: m,
}
authAny, err := codectypes.NewAnyWithValue(auth)
if err != nil {
return nil, err
}
_, err = k.authzKeeper.Grant(goCtx, &authztypes.MsgGrant{
Granter: msg.Creator,
Grantee: msg.Grantee,
Grant: authztypes.Grant{
Authorization: authAny,
Expiration: nil, // Expiration is set to nil
},
})
if err != nil {
return nil, err
}
}
// ... (remaining code)
}
The Expiration
field in the Grant
struct is currently set to nil
, which means these authorizations will not expire.
Modify the AddMultiGrant
function to include an expiration date for each grant. This could be done by:
1. Adding an expiration field to the MsgAddMultiGrant
message type.
2. Utilizing this expiration in the Grant
creation.
RISK ACCEPTED: The Zenrock team accepted the risk of the issue.
// Informational
The spec file intends to outline the common structure for specifications within this directory. The modules are missing spec. This documentation is segmented into developer-focused messages and end-user-facing messages. These messages may be shown to the end user (the human) at the time that they will interact with the module.
It is recommended that modules are fully annotated using spec for all available functionalities.
SOLVED: The Zenrock team solved the issue by adding specs.
// Informational
It has been identified Open TO-DOs in the scoped files. Open TO-DOs can point to architecture or programming issues that still need to be resolved. Often these kinds of comments indicate areas of complexity or confusion for developers. This provides value and insight to an attacker who aims to cause damage to the protocol.
/x/identity/module/simulation.go:27: // TODO: Determine the simulation weight value
./x/identity/module/simulation.go:31: // TODO: Determine the simulation weight value
./x/identity/module/simulation.go:35: // TODO: Determine the simulation weight value
./x/identity/module/simulation.go:39: // TODO: Determine the simulation weight value
./x/identity/module/simulation.go:43: // TODO: Determine the simulation weight value
./x/identity/module/simulation.go:47: // TODO: Determine the simulation weight value
./x/identity/module/simulation.go:51: // TODO: Determine the simulation weight value
./x/identity/module/simulation.go:55: // TODO: Determine the simulation weight value
./x/identity/module/simulation.go:59: // TODO: Determine the simulation weight value
./x/identity/module/simulation.go:63: // TODO: Determine the simulation weight value
./x/identity/module/simulation.go:67: // TODO: Determine the simulation weight value
./x/identity/module/simulation.go:71: // TODO: Determine the simulation weight value
./x/identity/module/simulation.go:75: // TODO: Determine the simulation weight value
./x/identity/module/simulation.go:79: // TODO: Determine the simulation weight value
./x/identity/simulation/remove_workspace_owner.go:26: // TODO: Handling the RemoveWorkspaceOwner simulation
./x/identity/simulation/add_keyring_party.go:26: // TODO: Handling the AddKeyringParty simulation
./x/identity/simulation/add_workspace_owner.go:26: // TODO: Handling the AddWorkspaceOwner simulation
./x/identity/simulation/new_keyring.go:26: // TODO: Handling the NewKeyring simulation
./x/identity/simulation/add_keyring_admin.go:25: // TODO: Handling the AddKeyringAdmin simulation
./x/identity/simulation/remove_keyring_party.go:26: // TODO: Handling the RemoveKeyringParty simulation
./x/identity/simulation/update_keyring.go:26: // TODO: Handling the UpdateKeyring simulation
./x/identity/simulation/deactivate_keyring.go:25: // TODO: Handling the DeactivateKeyring simulation
./x/identity/simulation/update_workspace.go:25: // TODO: Handling the UpdateWorkspace simulation
./x/identity/simulation/remove_keyring_admin.go:25: // TODO: Handling the RemoveKeyringAdmin simulation
./x/identity/simulation/new_workspace.go:26: // TODO: Handling the NewWorkspace simulation
./x/identity/simulation/append_child_workspace.go:26: // TODO: Handling the AppendChildWorkspace simulation
./x/identity/simulation/new_child_workspace.go:26: // TODO: Handling the NewChildWorkspace simulation
./x/genutil/types/genesis.go:161:// TODO(@julienrbrt) eventually abstract from CometBFT types
./x/genutil/collect.go:134: // TODO abstract out staking message validation back to staking
./x/genutil/gentx_test.go:98: bankGenesis, err := suite.encodingConfig.Amino.MarshalJSON(bankGenesisState) // TODO switch this to use Marshaler
./x/genutil/gentx_test.go:217: stakingGenesis, err := cdc.MarshalJSON(&stakingtypes.GenesisState{Params: stakingtypes.DefaultParams()}) // TODO switch this to use Marshaler
./x/treasury/types/wallet_solana_test.go:48:// TODO Add more test vectors
./x/treasury/types/wallet_solana_test.go:95:// TODO complete this test
./x/treasury/types/wallet_solana_test.go:107: // TODO: Add test cases.
./x/treasury/types/wallet_solana.go:33:// TODO: are the nil checks necessary?
./x/treasury/types/wallet_solana.go:99:// TODO Check
./x/treasury/types/wallet_ethereum.go:104: callMsg, parsed, err := parseCallData(tx.Data()) // - TODO we should refactor this so that value can be extracted from all known contract calls
./x/treasury/module/simulation.go:28: // TODO: Determine the simulation weight value
./x/treasury/module/simulation.go:32: // TODO: Determine the simulation weight value
./x/treasury/module/simulation.go:36: // TODO: Determine the simulation weight value
./x/treasury/module/simulation.go:40: // TODO: Determine the simulation weight value
./x/treasury/module/simulation.go:44: // TODO: Determine the simulation weight value
./x/treasury/module/simulation.go:48: // TODO: Determine the simulation weight value
./x/treasury/module/simulation.go:52: // TODO: Determine the simulation weight value
./x/treasury/module/simulation.go:56: // TODO: Determine the simulation weight value
./x/treasury/module/simulation.go:60: // TODO: Determine the simulation weight value
./x/treasury/simulation/update_key_request.go:26: // TODO: Handling the UpdateKeyRequest simulation
./x/treasury/simulation/new_zr_sign_signature_request.go:25: // TODO: Handling the NewZrSignSignatureRequest simulation
./x/treasury/simulation/new_ica_transaction_request.go:25: // TODO: Handling the NewIcaTransactionRequest simulation
./x/treasury/simulation/new_key_request.go:26: // TODO: Handling the NewKeyRequest simulation
./x/treasury/simulation/new_sign_transaction_request.go:26: // TODO: Handling the NewSignTransactionRequest simulation
./x/treasury/simulation/transfer_from_keyring.go:25: // TODO: Handling the TransferFromKeyring simulation
./x/treasury/simulation/new_signature_request.go:26: // TODO: Handling the NewSignatureRequest simulation
./x/treasury/simulation/fulfil_ica_transaction_request.go:25: // TODO: Handling the FulfilIcaTransactionRequest simulation
./x/treasury/simulation/fulfil_signature_request.go:26: // TODO: Handling the FulfilSignatureRequest simulation
./x/treasury/keeper/query_keys.go:21: // TODO Remove this comment
./x/treasury/keeper/msg_server_new_ica_transaction_request.go:69: // TODO - add support for different encodings and memo
./x/validation/migrations/v1/types.go:20: // TODO: Justify our choice of default here.
./x/validation/types/params.go:19: // TODO: Justify our choice of default here.
./x/validation/types/validator.go:26: // TODO: Why can't we just have one string description which can be JSON by convention
./x/validation/types/expected_keepers.go:25: // TODO remove with genesis 2-phases refactor https://github.com/cosmos/cosmos-sdk/issues/2862
./x/validation/types/validator_test.go:137:// TODO refactor to make simpler like the AddToken tests above
./x/validation/types/staking.pb.go:1075:// TODO: explore moving this to proto/cosmos/base to separate modules from tendermint dependence
./x/validation/types/authz.go:13:// TODO: Revisit this once we have propoer gas fee framework.
./x/validation/simulation/operations_test.go:151: // require.NoError(t, err) // TODO check if it should be NoError
./x/validation/keeper/params.go:49:// TODO: we might turn this into an on-chain param:
./x/validation/keeper/validator.go:694: Tokens: v.TokensNative.Add(v.TokensAVS), // TODO: Do we want just the native tokens here?
./x/validation/keeper/keeper.go:21:// TODO: change these from hardcoded values to config values
./x/validation/keeper/alias_functions.go:200:// TODO: remove this func, change all usage for iterate functionality
./x/validation/keeper/genesis.go:159: // TODO: remove with genesis 2-phases refactor https://github.com/cosmos/cosmos-sdk/issues/2862
./x/validation/keeper/delegation.go:170: // TODO: Consider calling hooks outside of the store wrapper functions, it's unobvious.
./x/validation/keeper/delegation.go:718: // TODO (Facu): Should we return here? We are ignoring this error
./x/validation/keeper/delegation.go:1087: // TODO: When would the validator not be found?
./x/policy/types/signmethods_passkeys.go:76: // TODO, see go-webauthn/protocol/attestation.go
./x/policy/module/simulation.go:27: // TODO: Determine the simulation weight value
./x/policy/module/simulation.go:31: // TODO: Determine the simulation weight value
./x/policy/module/simulation.go:35: // TODO: Determine the simulation weight value
./x/policy/module/simulation.go:39: // TODO: Determine the simulation weight value
./x/policy/module/simulation.go:43: // TODO: Determine the simulation weight value
./x/policy/module/simulation.go:47: // TODO: Determine the simulation weight value
./x/policy/module/simulation.go:51: // TODO: Determine the simulation weight value
./x/policy/simulation/msg_add_multi_grant.go:25: // TODO: Handling the MsgAddMultiGrant simulation
./x/policy/simulation/msg_remove_multi_grant.go:25: // TODO: Handling the MsgRemoveMultiGrant simulation
./x/policy/simulation/new_policy.go:25: // TODO: Handling the NewPolicy simulation
./x/policy/simulation/add_sign_method.go:25: // TODO: Handling the AddSignMethod simulation
./x/policy/simulation/approve_action.go:25: // TODO: Handling the ApproveAction simulation
./x/policy/simulation/remove_sign_method.go:25: // TODO: Handling the RemoveSignMethod simulation
./x/policy/simulation/revoke_action.go:25: // TODO: Handling the RevokeAction simulation
./x/policy/keeper/msg_server_new_policy_test.go:95: // TODO: uncomment when BoolparsePolicy Validate() is implemented
Consider resolving the TO-DOs before deploying code to a production context. Use an independent issue tracker or other project management software to track development tasks.
ACKNOWLEDGED: The Zenrock team acknowledged the issue.
Halborn strongly recommends conducting a follow-up assessment of the project either within six months or immediately following any material changes to the codebase, whichever comes first. This approach is crucial for maintaining the project’s integrity and addressing potential vulnerabilities introduced by code modifications.
// Download the full report
* Use Google Chrome for best results
** Check "Background Graphics" in the print settings if needed