A malicious worker can forcibly have cache data removed

[c4-bot-2]

Lines of code

https://github.com/code-423n4/2024-03-phala-network/blob/a01ffbe992560d8d0f17deadfb9b9a2bed38377e/phala-blockchain/crates/pink/chain-extension/src/local_cache.rs#L100-L125 https://github.com/code-423n4/2024-03-phala-network/blob/a01ffbe992560d8d0f17deadfb9b9a2bed38377e/phala-blockchain/crates/pink/chain-extension/src/local_cache.rs#L58-L90

Vulnerability details

Impact

A malicious worker can forcibly have cache data removed

Proof of Concept

The function set allows a contract to add data to the cache shown here

https://github.com/code-423n4/2024-03-phala-network/blob/a01ffbe992560d8d0f17deadfb9b9a2bed38377e/phala-blockchain/crates/pink/chain-extension/src/local_cache.rs#L100-L125

  fn set(
        &mut self,
        key: Cow<[u8]>,
        value: Cow<[u8]>,
        lifetime: u64,
    ) -> Result<(), StorageQuotaExceeded> {
        _ = self.remove(key.as_ref());
        let data_len = key.len() + value.len();
        let mut store_size = self.size + data_len;
        if store_size > self.max_size {
            self.clear_expired(now());
            store_size = self.size + data_len;
            if store_size > self.max_size {
                return Err(StorageQuotaExceeded);
            }
        }
        self.size = store_size;
        self.kvs.insert(
            key.into_owned(),
            StorageValue {
                expire_at: now().saturating_add(lifetime),
                value: value.into_owned(),
            },
        );
        Ok(())
    }

Also pay attention to the following function fit size that is nested inside the function apply quotas

https://github.com/code-423n4/2024-03-phala-network/blob/a01ffbe992560d8d0f17deadfb9b9a2bed38377e/phala-blockchain/crates/pink/chain-extension/src/local_cache.rs#L55-L79

 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();
    }

 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)
            );
            if max_size == 0 {
                self.storages.remove(contract);
                continue;
            }
            match self.storages.get_mut(contract) {
                Some(store) => {
                    store.max_size = max_size;
                    store.fit_size();
                }
                None => {
                    self.storages
                        .insert(contract.to_vec(), Storage::new(max_size));
                }
            }
        }
    }

Since the cache has a max size, a malicious worker can forcibly remove data by increasing the size of the cache by calling the function set and then calling the function apply quotas, which calls the nested function fit_size. This will allow the bad actor to forcibly remove innocent data that was removed too early by adding fluff data to exceed the max size.

Example:

An attacker discovers that the cache's maximum size is set to 10MB. They decide to exploit the system by inserting "fluff" data into the cache. The attacker creates numerous key-value pairs, where each pair is approximately 1MB in size. They use the set function to insert these pairs one by one into the cache. After inserting the first 10 pairs, the cache reaches its maximum capacity of 10MB.

To exacerbate the situation, the attacker continues to insert additional fluff data. For every new insertion that exceeds the 10MB limit, the cache management system evicts the oldest data to make room for the new data, as dictated by the fit_size method. This process results in legitimate data being prematurely removed from the cache.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider adding a rate limit (e.g. limit per user) to mitigate such an attack

Assessed type

DoS

[c4-pre-sort]

141345 marked the issue as primary issue

[c4-pre-sort]

141345 marked the issue as sufficient quality report

[kvinwang]

Yes, the local cache is designed to allow data loss.

However, there is no function for a worker to insert data into the cache. Workers are always honest as they run in TEE and always be verified before running a contract.

[c4-sponsor]

kvinwang (sponsor) disputed

[c4-judge]

OpenCoreCH marked the issue as unsatisfactory: Invalid