c4-bot-8 commented 3 months ago

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

c4-pre-sort commented 3 months ago

141345 marked the issue as primary issue

c4-pre-sort commented 3 months ago

141345 marked the issue as sufficient quality report

141345 commented 3 months ago

contradicts doc specs

c4-sponsor commented 3 months ago

kvinwang (sponsor) confirmed

c4-judge commented 3 months ago

OpenCoreCH marked the issue as satisfactory