Phat Contract Runtime - 0xTheC0der's results

Lines of code

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

Vulnerability details

Impact

According to the documentation (online and in-line), the availability of the balance_of(...) method (see code below) should be any contract instead of system only which is caused by the present ensure_system check.

fn balance_of(
    &self,
    account: ext::AccountId,
) -> Result<(pink::Balance, pink::Balance), Self::Error> {
    self.ensure_system()?;    // @audit Availability should be 'any contract' instead of 'system only'
    let account: AccountId32 = account.convert_to();
    let total = crate::runtime::Balances::total_balance(&account);
    let free = crate::runtime::Balances::free_balance(&account);
    Ok((total, free))
}

The ensure_system(...) method returns a BadOrigin error in case the caller/origin is not the system contract.

fn ensure_system(&self) -> Result<(), DispatchError> {
    let contract: AccountId32 = self.address.convert_to();
    if Some(contract) != PalletPink::system_contract() {
        return Err(DispatchError::BadOrigin);
    }
    Ok(())
}

Consequence:
The availability of the balance_of(...) method is limited to the system contract instead of being accessible to anyone. Therefore, user contracts relying on this method will inevitably fail.

For comparison:
The import_latest_system_code(...) method has consistent system only availability according to the implementation and documentation.

Proof of Concept

Please add the test case below to phala-blockchain/crates/pink/runtime/tests/test_pink_contract.rs and run it with cargo test test_balance_of -- --nocapture.

#[test]
fn test_balance_of() {
    const TEST_ADDRESS: AccountId32 = AccountId32::new([255u8; 32]);

    let (mut cluster, checker) = create_cluster();

    let balance = 114514;

    cluster.tx().deposit(TEST_ADDRESS.clone(), balance);

    let result = checker
        .call()
        .direct_balance_of(TEST_ADDRESS.convert_to())
        .query(&mut cluster);
    assert_eq!(result.unwrap(), (balance, balance));
}

The test will fail with a BadOrigin error as discussed above.

called `Result::unwrap()` on an `Err` value: Failed to execute call: BadOrigin

Tools Used

Manual review

Recommended Mitigation Steps

Remove the ensure_system check from the balance_of(...) method to ensure availability for any contract.

Assessed type

Invalid Validation

Summary

• L-1: js_eval is not implemented for queries
• L-2: Contract address derivation does not include input_data
• R-1: Flush LimitedWriter after use

L-1: js_eval is not implemented for queries

According to the documentation (online and in-line #1,#2), the js_eval(...) method should be available in queries (only in queries because it's non-deterministic). However, it's actually not implemented in the chain-extension.

fn js_eval(&self, _codes: Vec<JsCode>, _args: Vec<String>) -> Result<JsValue, Self::Error> {
    Ok(JsValue::Exception("No Js Runtime".into()))
}

L-2: Contract address derivation does not include input_data

Pink's contact address derivation overrides the one of pallet-contracts as follows.

fn contract_address(
    deploying_address: &AccountIdOf<T>,
    code_hash: &HashOf<T>,
    _input_data: &[u8],   // @audit _input_data ignored
    salt: &[u8],
) -> AccountIdOf<T> {
    let cluster_id = <ClusterId<T>>::get();
    let buf = phala_types::contract::contract_id_preimage(
        deploying_address.as_ref(),
        code_hash.as_ref(),
        cluster_id.as_ref(),
        salt,
    );
    UncheckedFrom::unchecked_from(<T as frame_system::Config>::Hashing::hash(&buf))
}

Although compatibility with pallet contracts v9 is ensured, it's not in line with the latest address derivation.

Consequence: Multiple instances of the same contract with different arguments/input_data cannot be created as expected failing with a DuplicateContract error. It requires to explicitly alter the salt in order to obtain distinct addresses in such a case.

R-1: Flush LimitedWriter after use

The async_http_request(...) method uses the LimitedWriter to write the body chunk by chunk from the HTTP response.

...

let mut writer = LimitedWriter::new(&mut body, MAX_BODY_SIZE);

while let Some(chunk) = response
    .chunk()
    .await
    .or(Err(HttpRequestError::NetworkError))?
{
    writer
        .write_all(&chunk)
        .or(Err(HttpRequestError::ResponseTooLarge))?;
}

...

However, the LimitedWriter is never flushed.

Recommendation: Add writer.flush().or(Err(HttpRequestError::ResponseTooLarge))?; below the above snipppet to ensure that the writer is correctly flushed.