Inconsistent error handling and lack of proper propagation hinder issue diagnosis.

[c4-bot-7]

Lines of code

https://github.com/code-423n4/2024-03-phala-network/blob/a01ffbe992560d8d0f17deadfb9b9a2bed38377e/phala-blockchain/crates/pink/runtime/src/contract.rs#L149-L230

Vulnerability details

MED: Inconsistent Error Handling and Lack of Proper Error Propagation LOC: pub fn instantiate, pub fn bare_call

Description

There's suffering from inconsistent error handling and a lack of proper error propagation. Some functions, such as instantiate and bare_call, return raw Vec<u8> without proper error propagation, making it difficult to determine the success or failure of the operation. Additionally, the codebase relies on the anyhow crate for error handling, which provides flexibility but may lead to inconsistencies in how errors are handled across different parts of the system.

// contract.rs

pub fn instantiate(
    code_hash: Hash,
    input_data: Vec<u8>,
    salt: Vec<u8>,
    mode: ExecutionMode,
    args: TransactionArguments,
) -> ContractInstantiateResult {
    // ...
    let result = contract_tx(origin.clone(), gas_limit, gas_free, move || {
        Contracts::bare_instantiate(
            // ...
        )
    });
    // ...
    result
}

pub fn bare_call(
    address: AccountId,
    input_data: Vec<u8>,
    mode: ExecutionMode,
    tx_args: TransactionArguments,
) -> ContractExecResult {
    // ...
    let result = contract_tx(origin.clone(), gas_limit, gas_free, move || {
        Contracts::bare_call(
            // ...
        )
    });
    // ...
    result
}

The instantiate and bare_call functions return ContractInstantiateResult and ContractExecResult respectively, which are raw Vec<u8> types. This makes it difficult for the caller to determine the success or failure of the operation and handle errors appropriately.

The issue is the Inconsistent usage of error-handling mechanisms, such as mixing the use of anyhow with other error-handling approaches.

Returning raw Vec<u8> instead of a structured error type, which makes it difficult to distinguish between successful and error cases.

Impact

Errors may be silently swallowed or not properly propagated, making it challenging to diagnose and fix issues.

Tools Used

Manual audit

Recommended Mitigation Steps

1. Define a consistent error handling strategy across the codebase, using a common error type or trait that can be used to represent and propagate errors.

2. Update functions like instantiate and bare_call to return a structured result type, such as Result<T, E>, where T represents the success value and E represents the error type.

3. Ensure that errors are properly propagated and not swallowed silently. Use the ? operator or match expressions to handle and propagate errors explicitly.

4. Provide clear and informative error messages that include relevant context and details about the error, making it easier to diagnose and fix issues.

5. Consider using a more robust error handling framework or library, such as thiserror or anyhow, to define custom error types and provide additional error context.

By establishing a consistent error handling strategy and properly propagating errors, the codebase can be made more maintainable, reliable, and secure. It allows for better error diagnosis, handling, and logging, reducing the chances of unexpected behavior or security vulnerabilities.

Assessed type

Error

[c4-pre-sort]

141345 marked the issue as insufficient quality report

[c4-judge]

OpenCoreCH changed the severity to QA (Quality Assurance)

[c4-judge]

OpenCoreCH marked the issue as grade-c