Chainport2023

CertiK20 Jul 2023Code version: not recorded (see the report)

CertiK reviewed ChainPort Cross-Chain Bridge (Chainport2023) on 20 Jul 2023: 45 findings, 0 critical; 26 marked fixed, 1 acknowledged, 18 open.

No open critical findings

Open the report Open ChainPort Cross-Chain Bridge’s security record

Report hosted on skynet.certik.com.

Trustblock does not review code.

Findings45

Statuses are the security firm’s own, from its report.

Severity	Open	Acknowledged	Fixed
Critical	0	0	0
High	0	1	1
Medium	6	0	3
Low	12	0	22

HighMissing Token Address Whitelist CheckFixed

The function `handle_non_evm_token_transfer_event()` does not check whether the token's address being used belongs to the whitelist defined on the same file. Without this check, the whitelist is useless and users can interact with any token they desire, including malicious contracts.

MediumInhomogeneous addresses form in whitelist check.Fixed

In the function `is_valid_deposit_utxo` the `target_web3_address_whitelist_check` is extracted from `transaction_metadata` to verify its presence in the `whitelisted_addresses` list. The enforced check verifies if the `.lower()` case version of the `target_web3_address_whitelist_check` is present in the `whitelisted_address` list. The problem resides in the fact that addresses from the `whitelisted_address` list are not guaranteed to be lower case. Considering that it is common practice in the EVM ecosystem to provide addresses in their checksummed form, i.e. with the case of the `abcdf` values being different and deterministic, the pointed check, in the current form, would very likely result in false negatives. In fact, the hardcoded list already contains checksummed addresses.

MediumTransaction With "ERROR" Status Would Be Saved AsFixed

On the function `cardano_bridge_utxo_monitor()` the transactions are processed and saved in the database. In case a transaction is valid and had no errors while processing, it is saved with the `transaction_status` property set to 0. However, when a transaction is valid but an Exception occurred while processing it, the transaction would still be saved with the `transaction_status` property set to 0. This issue arises given that the line that changes the status to the value "2" is an equality check "==" rather than an assignment "=".

MediumWrong function name pointed in loggerFixed

The pointed line of code configure the function name as a prefix in the log message, but the wrong function name is used. Reporting `event_by_burn_tx` event as `bridge_event_by_id` can have dangerous implications, as others systems may consume logs and infer from them that bridge event happened instead of burn event, that may have potential harmful implications to a system.

LowDict access mismatch in log messageFixed

The `transaction_metadata` parameter is accessed as a dictionary using the key `type` as a string. If such value does not match the hardcoded one, a log entry is generated with the current value. However when the log entry is generated the current content is accessed again, but using `type` as a Python keyword, not as a string. This very likely results in a `None` value.

LowDouble Variable DeclarationFixed

The function `mint_bridge_tokens()` declares twice the variable `port_obj`. Given that there is no reason to re-instantiate the variable, the second declaration could be removed.

LowEmpty String Assigned to `issuer_address`Fixed

In the function `evm_to_cardano`, every time a new Cardano transaction is created an empty string is assigned to the variable `issuer_address`.

LowEnum Index value Used like magic number Instead OFixed

The function `create_new_cardano_policy()` from `cardano_policy.py` defines the `policy_type` with a value of `1`, like a magic number. Using the index of the `PolicyType` enum instead of using its name, in this case, `one_time_minter` makes the code harder to read.

LowImproper `None` Comparison CheckFixed

The pointed statements compare if the object is distinct from the `None` value. This check is done with the distinct operator `!=` but could be replaced with a more appropriate, readable check. A similar issue occurs with the statement `if object == None` which could be replaced with `if object is None`. Of course behaviour can be overwritten and custom defined for `==` operator, but rule of thumb is to use `is None`. Therefore, it may cause misalignment between different developers working on same codebase , and behavior beyond expectation.

LowImproper type hint for JSON objectsFixed

Starting from the 3.5 version, Python allows to specify type hints for parameters in functions. Such hints, simply suggest the developer on the expected type and do not enforce any static or dynamic check. This does not imply any word can be used as a type and either Python types should be used or custom ones should be defined. Code at the pointed location uses the `json` word as a type hint but it represents a module. Given the context in which it is used, logic expects a JSON data object to use instead of the `json` module.

Show 10 more

LowInconsistency In Function's Return ValueFixed

The linked functions present an inconsistency in the return value they produce. They sometimes return a value and sometimes do not. Indeed, the function assumes that self.api.block_latest() will always return a non-empty value. If this assumption is incorrect, the function will return None, while the return type is set to `int` and it could lead to unexpected behavior.

LowInconsistency in Query Result ManagementFixed

In the functions `session_get_cardano_bridge_event_by_deposit_tx_id` and `session_get_cardano_bridge_event_by_burn_tx_id` the `one_or_none()` method is used to manage the result of the `outer_session.query`, while in function `session_get_cardano_bridge_event_by_id` the `one` getter is used to accomplish the same operation. The [sqlalchemy](https://docs.sqlalchemy.org/en/14/orm/query.html#sqlalchemy.orm.Query.one) documentation suggests that `one_or_none()` returns at most one result or raises an exception and returns `None` if the query selects no rows, while `one()` returns exactly one result or raise an exception. Thus `one()` never returns `None`. Having different strategies to manage the returns value of the session queries generates inconsistency in the codebase error recovery mechanisms.

LowInconsistent Check And Error MessageFixed

The function `is_valid_deposit_utxo` enforces a check verifying if the `transaction_metadata.type` is equal to `deposit_v2` string. However the error message raised when the check is false prints a different string `deposit`.

LowInconsistent Comment and CodeFixed

The comment on the function `generate_unsigned_transaction_with_metadata_v2()` from `cardano.py` is inconsistent with the code. The comment states that the transaction is invalid after `100` blocks, but the code checks if it's invalid after `1000` blocks. The same issue occurs on the function `generate_unsigned_transaction_with_metadata()` from the same file. Behavior is incorrect if comment considered being a specification.

LowMisleading key type or variable nameFixed

The `get_address_from_vkey` function takes a `VerificationKey` for the `vkey` parameter. Instead, the variable name `hot_bridge_skey` initialized with data from the `fetch_keys` parameter suggests a key of type `SigningKey`.

LowMissing check on body correctness in transactionFixed

The `submit_transaction` method in the `Cardano` class is responsible to send a signed transaction to the blockchain through the BlockFrostApi. Such function takes as input the signed transaction as a `Transaction` object and the `TransactionBody`. The body is used to return the transaction hash after the transaction submission. However, there is not any validation that the passed body corresponds to the signed transaction, so that if the caller passed two parameters which do not match, then a `Transaction` is sent but a wrong hash is returned. Such situation should be prevented, so, if buggy software calls the `submit_transaction` with such inconsistency, it is detected through an error.

LowMissing check on issuer addressFixed

The `mint_bridge_tokens` function accepts `issuer_web3_addess` as an optional parameter. In the case in which `None` is passed, then the issuer address is retrieved from the persistent storage. However, code at line 47 always fetches such information from the storage, independently from the provided parameter, so such data is anyway present in the database. In this case, if the `issuer_web3_addess` is provided by the function caller, there is not any check that the supplied value conforms to the fetched one. Moreover, the `issuer_obj` is not used in any part of the `mint_bridge_tokens` function other than line 58

LowToo Many DigitsFixed

Literals with many digits are difficult to read and review.

LowUngrouped ImportsFixed

The linked import statements import different modules from the same package. They could be grouped to improve the code's readability.

LowUnnecessary assignmentFixed

Code at the pointed lines assigns a value to a variable that is already set to the same value. This is an error prone approach since if during subsequent development iterations such value need to be changed, developer needs to remember or notice to do it twice. Moreover by saving an assignment, compute units are saved.

Show 6 more

LowUnnecessary Call To `__len__()`Fixed

The linked statements make use of the `__len__()` method which returns the length of the object. The same result can be obtained by using the `len()` function directly, which is more readable.

LowUnresolved `TODO` in production codebaseFixed

The linked statements are `TODO` comments. Such comments are used to identify unimplemented code logic.

LowUnused ArgumentsFixed

The linked arguments are declared but are not used inside the function's body.

Lowunused id parameter in `get_policy_id`Fixed

`id` parameter in `get_policy_id` is unused.

LowUsage Of `print()` In ProductionFixed

The linked statements are from a `print()` call. Such a function will print to the standard output and is usually meant for testing purposes. Thus, if the value has to be persisted, it is advised to use a more robust logging solution.

LowUsing Integers To Represent StatesFixed

The linked statements use plain integers to represent different states. This technique makes the code more difficult to read as one can not know what that value represents.