Phat Contract Runtime - Cryptor's results
Coprocessor for blockchains
General Information
Platform: Code4rena
Start Date: 01/03/2024
Pot Size: $60,500 USDC
Total HM: 4
Participants: 18
Period: 21 days
Judge: Lambda
Total Solo HM: 3
Id: 344
League: POLKADOT
Findings Distribution
Researcher Performance
Rank: 5/18
Findings: 2
Award: $994.09
🌟 Selected for report: 1
🚀 Solo Findings: 0
Findings Information
🌟 Selected for report: Cryptor
Also found by: 0xTheC0der, Bauchibred, Daniel526, XDZIBECX, ihtishamsudo, zhaojie
Labels
Awards
509.1097 USDC - $509.11
External Links
[L01] Ensure_System should be checked after uploading bare code
Current implementation holds little value as a bad actor can still upload malicious system code since the ensure system check check is made before any new code is added, posing significant risk to users.
[L02] No point in using regular http request over batchhttperequest
Since the len in batchhttprequest is not restricted to being > 1, a user can just use batchhttperequest over the regular httprequest as the former is less likely to revert due to the timeout bound being longer.
[L03] CallinCommand is misleading and should be renamed callinTransaction for clarity
[L04] is_running_in_command function is missing or misnamed in the code
the function is_running_in_command (from the documentation) is missing or misnamed in the code to be is_it_in_transaction.
[L05] Functions that are not supported in transaction mode should revert not send empty arrays
Sending empty arrays could be misleading to users. It is recommended to revert and send a error that these functions are not support in transaction mode
[L06] Twox64Concat has a higher risk of a hash collision than blake128Concat
It is recommended to use blake128Concat hash for better security and a lower chance of a collision
[L07] Ensure_System should be called for sign and verify
If the system contract is maliciously upgraded, then calling sign or verify can lead to unexpected results for users
[L08] fn setup cannot be called in estimating mode
The function setup can only be called in execution mode and not in estimating mode
[L09] Masking deposit calculation can lead to unexpected results in certain circumstances
The function coarse gas relies on complex bit math to mask deposits in order to mitigate side channel attacks. Therefore we can consider that masked deposits must be at least higher or equal to the original deposit. However, this invariant can be broken.
Consider the fuzz test below
#[test] fn test_mask_deposit_properties(deposit in 1u128..=u128::MAX, deposit_per_byte in any::<u128>()) { let masked_deposit = mask_deposit(deposit, deposit_per_byte); // Property 1: Masked deposit should be greater than or equal to the original deposit. prop_assert!(masked_deposit >= deposit); }
Here the invariant will be broken if deposit_per_byte is = 664613997892457936451903530140172289. This can be problematic if the cluster creator (the actor who sets the deposit per byte) is acting in a malicious way as this opens the door for the side channel attack that the function is supposed to prevent.
[L10] Not checking the bytecode for system contract can be dangerous as it can be upgraded
The function ensure system doesn't check for correct bytecode, only the correct address. This can be dangerous as the contract can be maliciously upgraded
[L11] No deadline for signature function
It is a common security to add a deadline for signatures to prevent stale transactions or prevent them from maliciously executed by workers
[L12] is_in_transaction return value can be misleading
If a call is in estimating mode, then is_in_transaction will return true since estimating mode is treated the same as transaction, when in reality, it should be false. There should be a function to return whether the call is in estimating mode instead
[L13] MAX_CODE_LEN should equal the schedule
It makes little sense that the MAX_CODE_LEN (equal to 2 *1024 *1024) is not equal to the schedule payload_len (the max size for storage value equal to 1024 *1024). This means that a code size is free to be bigger than schedule max size but it won't be deployed.
[L14] No code size limit check in function put_sidevm_code
[L15] Omission of reserved balance
The protocol tracks the free balance and the total balance of an account but does not track the reserved balance. Total balance in the substrate frame pallet is equal to free balance + reserved balance
[L16] Payer can be any account
#0 - c4-pre-sort
2024-03-25T04:58:03Z
141345 marked the issue as high quality report
#1 - 141345
2024-03-25T09:07:17Z
11 Cryptor l r nc 3 0 11
L 1 i L 2 n L 3 n L 4 n L 5 n L 6 n L 7 i L 8 n L 9 l L 10 l L 11 l L 12 n L 13 n L 14 n L 15 n L 16 n
#2 - kvinwang
2024-03-26T01:59:59Z
[L01] Ensure_System should be checked after uploading bare code ❌，The system code is fetched on-chain by the worker.
[L02] No point in using regular http request over batchhttperequest
❌，len = 1 is a valid batch.
[L03] CallinCommand is misleading and should be renamed callinTransaction for clarity ✅
[L04] is_running_in_command function is missing or misnamed in the code ✅
[L05] Functions that are not supported in transaction mode should revert not send empty arrays Judgable. It did actually revert in the past, but the reverting can not be catched by the contract which leads bad dev experience.
[L06] Twox64Concat has a higher risk of a hash collision than blake128Concat ❌，The key itself if already crypto hash.
[L07] Ensure_System should be called for sign and verify ❌，The sign and verify are nothing about system contract.
[L08] fn setup cannot be called in estimating mode ❌，There is no code path in estimating mode would call this function.
[L09] Masking deposit calculation can lead to unexpected results in certain circumstances If deposit_per_byte is = 664613997892457936451903530140172289, the cluster wouldn't be able to do anything as it is two expensive.
[L10] Not checking the bytecode for system contract can be dangerous as it can be upgraded ❌，The system code upgrading is managed by on-chain goverance.
[L11] No deadline for signature function ❌，Chain extenstions is the low level API, higher level sdk can to the work.
[L12] is_in_transaction return value can be misleading ✅，this is actually on the plan.
[L13] MAX_CODE_LEN should equal the schedule ❌，schedule.payload_len is a config for different thing
[L14] No code size limit check in function put_sidevm_code ✅，might good to have. However, there is already a on-chain limit check for that.
[L15] Omission of reserved balance
❌，As total = free + reserved, the reserved can be calculated external or in the higher level SDK.
[L16] Payer can be any account ❌，It is called by the trusted worker.
#3 - c4-sponsor
2024-03-26T02:00:05Z
kvinwang (sponsor) confirmed
#4 - c4-judge
2024-03-27T15:33:27Z
OpenCoreCH marked the issue as grade-a
#5 - c4-judge
2024-03-27T15:33:30Z
OpenCoreCH marked the issue as selected for report
#6 - OpenCoreCH
2024-03-28T08:55:41Z
Agree with the Lookout's assessment of L / NC and the sponsors assessment of the validity with the following differences: L02: Valid design suggestion (not necessarily better than the current one, but just an alternative one) L05: Also a design suggestion
#7 - thebrittfactor
2024-04-04T20:25:18Z
Just a note that C4 is excluding the invalid entries from the official report.
For confirmation, the judge provided the following comment via DM:
to summarize, invalid are: L-01, L-06, L-07, L-08, L-10, L11, L-13, L-15, L-16
Findings Information
🌟 Selected for report: hunter_w3b
Also found by: 0xepley, Bauchibred, Cryptor, DarkTower, aariiif, albahaca, fouzantanveer, kaveyjoe, popeye, roguereggiant
Labels
Awards
484.9759 USDC - $484.98
External Links
Overview:
Phala network is an extension of the ink framework that adds additional functionality to ink contracts which include http request, evm cross chain functionality and ecdsa signing
Key Concepts:
There are 3 different execution modes in the pink! runtime. The differences are as follows:
| Execution Mode | Return Coarse Gas | Gas Limit | Determinism |
|---|---|---|---|
| Query | True | x*10 | False |
| Transaction | False | x/2 | True |
| Estimating | True | x/2 | True |
There are also several Signature types to take note of
| Signature Type | Determinism |
|---|---|
| ecdsa | True |
| ed25519 | True |
| sr25519 | False |
Contract Flow:
Files Overview:
This is an overview of areas of concern/ weakness throughout the coadebase where there could be vulnerabilities
pink/runtime/src/runtime.rs
const MAX_CODE_LEN: u32 = 2 * 1024 * 1024; parameter_types! { pub DepositPerStorageByte: Balance = Pink::deposit_per_byte(); pub DepositPerStorageItem: Balance = Pink::deposit_per_item(); pub const DefaultDepositLimit: Balance = Balance::max_value(); pub const MaxCodeLen: u32 = MAX_CODE_LEN; pub const MaxStorageKeyLen: u32 = 128; pub const MaxDebugBufferLen: u32 = 128 * 1024; pub DefaultSchedule: Schedule<PinkRuntime> = { let mut schedule = Schedule::<PinkRuntime>::default(); const MB: u32 = 16; // 64KiB * 16 // Each concurrent query would create a VM instance to serve it. We couldn't // allocate too much here. schedule.limits.memory_pages = 4 * MB; schedule.instruction_weights.base = 8000; schedule.limits.runtime_memory = 2048 * 1024 * 1024; // For unittests schedule.limits.payload_len = 1024 * 1024; // Max size for storage value schedule };
Areas of concern:
Why aren’t the MAX CODE LEN and the schedule payload_len equal to each other?
pink/runtime/src/contract.rs
This file is the contracts call/instantiation API
fn mask_deposit(deposit: u128, deposit_per_byte: u128) -> u128 { const MIN_MASKED_BYTES: u128 = 256; let min_masked_value = deposit_per_byte .saturating_mul(MIN_MASKED_BYTES) .saturating_sub(1); let min_mask_bits = 128 - min_masked_value.leading_zeros(); mask_low_bits128(deposit, min_mask_bits) } fn mask_gas(weight: Weight) -> Weight { Weight::from_parts(mask_low_bits64(weight.ref_time(), 28), 0) }
if !gas_free { let refund = gas_limit .checked_sub(&result.gas_consumed) .expect("BUG: consumed gas more than the gas limit"); PalletPink::refund_gas(&origin, refund).expect("BUG: failed to refund gas"); }
Key Questions:
Are deposits and gas correctly masked with no possibility of dirty bits?
Is gas correctly calculated throughout all execution modes?
pink/runtime/src/storage/external_backend.rs
The file is the storage provider for the runtime
Areas of Interest:
/// Check the existence of a particular ink code in the storage. pub fn code_exists(code_hash: &Hash) -> bool { let key = code_owner_key(code_hash); super::ExternalStorage::instantiate().get(&key).is_some() } fn code_owner_key(code_hash: &Hash) -> Vec<u8> { let mut key = Vec::new(); key.extend(twox_128("Contracts".as_bytes())); key.extend(twox_128("CodeInfoOf".as_bytes())); key.extend(&code_hash.encode()); key }
Key Questions:
Is there any way that code_exist doesn’t return valid ink code in the storage?
Is there any advantage or disadvantage to using twox_128 hash over over hashes?
pink/runtime/src/runtime/pallet_pink.rs
This pallet is used to store custom configuration of the runtime
Areas of Interest:
impl<T: Config> Pallet<T> { pub fn set_cluster_id(cluster_id: Hash) { <ClusterId<T>>::put(cluster_id); }
pub fn put_sidevm_code( owner: T::AccountId, code: Vec<u8>, ) -> Result<T::Hash, DispatchError> { let hash = T::Hashing::hash(&code); let bytes = code.len() + hash.as_ref().len(); let fee = Self::deposit_per_byte() .saturating_mul(BalanceOf::<T>::saturated_from(bytes)) .saturating_add(Self::deposit_per_item()); Self::pay(&owner, fee)?; <SidevmCodes<T>>::insert(hash, WasmCode { owner, code }); Ok(hash) }
Key Questions:
Is there any code size limit for the sideevm code?
Can fn put_sidevm_code fee overflow?
Is there or should there be any input validation for the side evm code?
pink/runtime/src/capi/mod.rs
This file shows all of the cross host-lib function calls (ecalls and ocalls)
Areas of Interest:
/// Gets the nonce of the current contract call (if available). #[xcall(id = 17)] fn contract_call_nonce(&self) -> Option<Vec<u8>>;
Key Questions:
Where is the implementation of the call contract nonce?
pink/runtime/src/capi/ecall_impl.rs
The enterward cross boundary call support. This is to be called by the worker
Areas of concern:
impl ecall::ECalls for ECallImpl { fn cluster_id(&self) -> Hash { PalletPink::cluster_id() } fn setup(&mut self, config: ClusterSetupConfig) -> Result<(), String> { on_genesis(); let ClusterSetupConfig { cluster_id,
let result = crate::contract::instantiate( code_hash, selector, vec![], ExecutionMode::Transaction, args, );
Key Questions:
Is cluster_id hash transparent enough to be used here?
Can a contract be instantiated in estimating mode?
pink/runtime/src/runtime/extension.rs
The chain extension implementation of the runtime. Users are expected to use this implemenation to interact with the runtime engine. This is considered to be the most important in scope file of the audit
Areas of concern:
fn sign( &self, sigtype: SigType, key: Cow<[u8]>, message: Cow<[u8]>, ) -> Result<Vec<u8>, Self::Error> { if matches!(sigtype, SigType::Sr25519) { return Ok(vec![]); } self.as_in_query.sign(sigtype, key, message) }
fn import_latest_system_code( &self, payer: ext::AccountId, ) -> Result<Option<Hash>, Self::Error> { self.ensure_system()?; let system_code = OCallImpl.latest_system_code(); if system_code.is_empty() { return Ok(None); } let code_hash = sp_core::blake2_256(&system_code); if !self.code_exists(code_hash, false)? { crate::runtime::Contracts::bare_upload_code( payer.convert_to(), system_code, None, pallet_contracts::Determinism::Enforced, )?; }; Ok(Some(code_hash)) }
Key Questions:
We have CallinQuery and CallinCommand to represent both Query and Execution modes. But where is the implementation for the estimating mode?
The fn ensure_system is not called when executing any function within PinkExtBackend block.
What would happen if the system contract address is upgraded?
Is the determinism for each function (Enforced, Relaxed), correct?
pink/chain-extension/src/lib.rs
This is the library for the chain extension. This is the interface for all of the function that are expected to be called by users utilizing the phat contract
Areas of concern:
pub fn batch_http_request(requests: Vec<HttpRequest>, timeout_ms: u64) -> ext::BatchHttpResult { const MAX_CONCURRENT_REQUESTS: usize = 5; if requests.len() > MAX_CONCURRENT_REQUESTS { return Err(ext::HttpRequestError::TooManyRequests); } block_on(async move { let futs = requests .into_iter() .map(|request| async_http_request(request, timeout_ms)); tokio::time::timeout( Duration::from_millis(timeout_ms + 200), futures::future::join_all(futs), ) .await }) .or(Err(ext::HttpRequestError::Timeout)) }
fn verify( &self, sigtype: SigType, pubkey: Cow<[u8]>, message: Cow<[u8]>, signature: Cow<[u8]>, ) -> Result<bool, Self::Error> { macro_rules! verify_with { ($sigtype:ident) => {{ let pubkey = sp_core::$sigtype::Public::from_slice(&pubkey) .map_err(|_| "Invalid public key")?; let signature = sp_core::$sigtype::Signature::from_slice(&signature) .ok_or("Invalid signature")?; Ok(sp_core::$sigtype::Pair::verify( &signature, message, &pubkey, )) }}; } match sigtype { SigType::Sr25519 => verify_with!(sr25519), SigType::Ed25519 => verify_with!(ed25519), SigType::Ecdsa => verify_with!(ecdsa), } }
Key Question:
How can the determinism of execution and query modes impact various function calls?
Is there any situation where signatures cannot be verified?
Can timeouts be prevented?
Why are the timeouts for batch http requests and regular http requests different?
pink/chain-extension/src/local_cache.rs
This files manages the local cache of the worker. When data is added to the cache, they have an expiration data. When data is expired or the cache size becomes too big, the deleted data is deleted in favor of new data
Areas of concern:
/// Remove the items closest to the expiration date until the size can be fit into `max_size`. fn fit_size(&mut self) { if self.size <= self.max_size { return; } let map = std::mem::take(&mut self.kvs); let mut kvs: Vec<_> = map .into_iter() .map(|(k, v)| (v.expire_at, (k, v))) .collect(); kvs.sort_by_key(|(expire, _)| *expire); self.kvs = kvs .into_iter() .filter_map(|(_, (k, v))| { if self.size <= self.max_size { return Some((k, v)); } self.size -= k.len() + v.value.len(); None }) .collect(); }
Key Questions
Is there a way for users to malicious force cache to expire?
What would happen if the cache becomes too big but data is not expired yet?
Codebase Quality
| Codebase Quality Categories | Comments |
|---|---|
| Documentation | The documentation for the project overall is great. However, it is important to note that some sections could have been explained better such as how interoperability with the evm works. Also, it would have been better to see a diagram regarding the typical user flow. One the main issues I had when auditing this protocol is the amount of relevant information in files that were out of scope. |
| Code Comments | The code comments in general, was good. Complex functions were well explained. It would have been helpful to get a more detailed but higher level overview of what each file does. Like in the previous section, it would have been helpful to get better context about each function without having to ask the sponsor or look in out of scope files |
| Tests | The tests overall was good. It is important to note that many tests were located in different repos so in the context of the contest repo, there were some integration tests that was lacking. It also would have been helpful to see some fuzzing and invariant testing, especially for some of the more complex functions of the protocol. However, there were plenty of unit tests found which demonstrates a high quality codebase. |
| Organization | Codebase is well organized, with in scope files being very easy to locate. |
Systemic Risks:
Systemic Risks: At the core of the Phala Network protocol, are the system contracts. Owners are deemed to be trusted, however a malicious owner can do irrepariable damage to users. This is in large part to the fact that there is no timelock on the any admin functions. This means that a malicious admin has unexpectedly change many of the parameters of the runtime engine causing issues such as making users pay too much gas, making users pay high fees to the treasury account, to reverting valid transactions.
There is also an issue regarding the
Observations/Learnings:
I noticed that several functions that can be called from different execution modes. but considering that there is a difference in gas limit in both the query and transaction modes, it makes no sense to call certain functions in transaction mode for example, as the gas limit is higher, so there is a lower chance of that function reverting.
Many of the high level contracts are out of scope. This made it tricky to track user flow contract flow and decipher valid bugs as there was very little information presented in the in scope files about who can call each function. One example of this is the apply quotas function
pub fn apply_quotas<'a>(&mut self, quotas: impl IntoIterator<Item = (&'a [u8], usize)>) { for (contract, max_size) in quotas.into_iter() { log::trace!( "Applying cache quotas for {} max_size={max_size}", hex_fmt::HexFmt(contract) );
which on the surface, is supposed to be called by the worker but it has no access control on first glance. However ,after confirming with the sponsor, the implementation is used in a file that is out of scope
local_cache::apply_quotas(calc_cache_quotas(&self.contracts));
One particular function that caught my eye was the calculations for coarse gas.
It was very complicated so I decided to try fuzz testing it using proptest with several invariants which is something that I have never done before. As a result, I noticed that those invariants that I had tested, failed in several circumstances.
Weak Points:
Compatibility with EVM blockchains: The Phala network expects to be interoperable with evm blockchains where phat contracts can sent transactions to evm blockchains and vice versa. However, with this comes risk. Substrate blockchains are not the same as evm blockchains and thus compatibility should be considered an area of high concern as there could be overflows. Some specific concerns include the parameter type in substrate vs those in evm blockchains e.g. if block.timestamp is a u32 on the evm, is it the same on substrate?
Calling functions with not enough or too much determinism: Determinism is important for the maintain security and trust on the blockchain. Phala Network has 3 execution modes: estimating, query, and transaction. Estimating and Transaction modes are designed to be deterministic while query mode is not. It is important to make sure that determinism is enforced and relaxed at the right times or else the security and trust in the blockchain could be compromised.
Accurate gas calculations: Gas calculation is important and an area of high concern. It is important for the runtime to correctly charge the right amount of gas for users. However, since the protocol has a several execution modes, with 2 different gas calculations as well as the introduction of coarse gas to mitigate side channel attacks, there is a possibility that gas can be miscalculated in some scenarios
Access Control: Access control is an area of high concern. If a user is able to manipulate sensitive areas such as cache management or manipulate gas fees, then user will be severely impacted. It is important that only trusted users are able to call certain function and that malicious actors are not able to harm innocent users of the phat contract
DOS attacks: DOS is an area of high concern, as there shouldn’t be a way for bad actors to DOS other users by manipulating the cache management in the local cache file or manipulate gas management to force any OOG errors
Worker trustworthyness: The worker is an intergral part of the protocol that is responsible for sending transaction. If they are not trusted actors, then they can cause significant damage to the integrity of the protocol. For example, the worker is trust to manage the cache management in the local cache file. If the worker is not trusted then they can manipulate the cache to remove an honest user’s data.
Methodology:
My methodology was to first review the documentation at https://docs.phala.network/developers/build-on-phat-contract/use-pink-extension as well as look at the docs for substrate https://docs.substrate.io/maintain/runtime-upgrades/. This allowed me to set up some systemic invariants and look at the code for any systemic inconsistencies with the docs. After reviewing the codebase and asking the sponsors a few questions, I proceeded to take a look at some of the higher level files, taking note of several invariants laid out in the previous section of the analysis. After that I proceeded to take a look at the tests and then edited a few of them to test certain properties. I also took a look at some of the real world examples found here
https://github.com/Phala-Network/phat-dashboard/tree/master/phat/contracts
https://github.com/Phala-Network/index-contract/tree/main/contracts
This step was crucial as it gave me additional context for several functions with the protocol.
Additionally, I engaged with the development team to fill in any knowledge gaps and get their insights on certain discoveries that I found in the code. This collaborative approach helped in understanding the rationale behind specific design choices.
Finally, I compiled my findings on the QA report, Analysis and any M/H findings that I discovered.
Conclusion:
Overall, I enjoyed my time on this audit, I further enchanced my knowledge regarding rust, substrate and ink. I do however, wonder whether the protocol was underscoped, as many of the high level functionality was outside the audit scope. Still, it was a very rewarding experience for me as I have not audited a runtime engine before, so it I had to think about what kind of bugs to look for.
Time Spent: 100 hours
Time spent:
60 hours
#0 - c4-pre-sort
2024-03-25T07:13:48Z
141345 marked the issue as sufficient quality report
#1 - OpenCoreCH
2024-03-27T15:56:35Z
I like the key questions per file, answering them would be a nice addition for the reader.
There is also an issue regarding the
Something is missing here
#2 - c4-judge
2024-03-27T15:56:39Z
OpenCoreCH marked the issue as grade-a