A cache that times out can be recovered.

[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#L204

Vulnerability details

Impact

Recover a cached value that has timed out, and a malicious user or contract can exploit this bug to fool other users or cause other unknown problems.

Proof of Concept

The LocalCache#set_expire function does not check whether the key has expired when setting a timeout:

    pub fn set_expire(&mut self, id: Cow<[u8]>, key: Cow<[u8]>, expire: u64) {
        //@audit key values that have timed out may not be deleted
        self.maybe_clear_expired();
        if expire == 0 {
            let _ = self.remove(id.as_ref(), key.as_ref());
        } else if let Some(v) = self
            .storages
            .get_mut(id.as_ref())
            .and_then(|storage| storage.kvs.get_mut(key.as_ref()))
        {
            //@audit You can increase the timeout period of a key value that has expired
            v.expire_at = now().saturating_add(expire)
        }
    }
    fn maybe_clear_expired(&mut self) {
        self.sets_since_last_gc += 1;
@>      if self.sets_since_last_gc == self.gc_interval {
            self.clear_expired();
        }
    }

As we can see from the above code, the set_expire function will first call maybe_clear_expired, this function is called maybe_*, so it won't necessarily delete keys that have expired, this function will not clean up expired keys until gc count reaches a certain value.

Therefore, if there are keys that have expired, they are still queried from storages, and then reset the expiration time. In other words, the set_expire function can cause an expired key to be reactivated.

Let's look at another function, LocalCache#get:

    pub fn get(&self, id: &[u8], key: &[u8]) -> Option<Vec<u8>> {
        let entry = self.storages.get(id)?.kvs.get(key)?;
@>      if entry.expire_at <= now() {
            None
        } else {
            Some(entry.value.to_owned())
        }
    }

The get function returns None if it finds that key has expired, this results in inconsistency of cached data.

In this way, when a key value(such as balance signature or debt) has expired, the attacker declares that the value no longer exists. The user is then asked to take some action, and the victim queries LocalCache#get and finds that the value indeed no longer exists. The problem is that an attacker can use set_expire to restore this value

Another attack scenario is: The key value(such as an nft) that the developer thinks has expired no longer exists. However, a malicious user can make this value expire indefinitely if set_expire can be called.

tips: LocalCache#set does not have this problem. LocalCache#set will call Storage#set, which will first call self.remove to remove the existing key.

Tools Used

vscode, manual

Recommended Mitigation Steps

    pub fn set_expire(&mut self, id: Cow<[u8]>, key: Cow<[u8]>, expire: u64) {
-       self.maybe_clear_expired();
+       self.clear_expired();
        if expire == 0 {
            let _ = self.remove(id.as_ref(), key.as_ref());
        } else if let Some(v) = self
            .storages
            .get_mut(id.as_ref())
            .and_then(|storage| storage.kvs.get_mut(key.as_ref()))
        {
            v.expire_at = now().saturating_add(expire)
        }
    }

Assessed type

Other

[c4-pre-sort]

141345 marked the issue as primary issue

[c4-pre-sort]

141345 marked the issue as sufficient quality report

[141345]

the expired cache will only be cleared if sets_since_last_gc num reachs some certain value, which makes it possible to recover expired cache

[kvinwang]

This is a good catch.

I'm not sure if this should be classified as High Risk level. The purpose of the local cache is to store volatile data, such as information fetched from an HTTP server. This data is expected to be lost at any time and may vary between different workers for the same contract. If the data is lost, the contract will re-fetch it from the external system.

Therefore, I don't think it is suitable for storing on-chain assets that users can query.

[c4-sponsor]

kvinwang (sponsor) confirmed

[c4-sponsor]

kvinwang marked the issue as disagree with severity

[c4-judge]

OpenCoreCH changed the severity to 2 (Med Risk)

[OpenCoreCH]

Good finding, but agree that Medium is more appropriate as the finding does not show any direct way how this can be used to steal funds, but only speculates about potential methods (which I am not sure if they can happen in practice and even if, they would have many assumptions)

[c4-judge]

OpenCoreCH marked the issue as selected for report

[DadeKuma]

I'm a bit skeptical about this finding. First of all, the docs state that the cache will store off-chain computations and not on-chain data that users can query/call:

//! The LocalCache provides a local KV cache for contracts to do some offchain computation. //! When we say local, it means that the data stored in the cache is different in different //! machines of the same contract. And the data might loss when the pruntime restart or caused //! by some kind of cache expiring machanism.

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

There is no proof that these off-chain computations can be leveraged to impact the protocol or leak value in any way, especially because:

1) This data is expected to be lost at any time and can vary between different workers 2) These are off-chain computations which are not queriable by users

For these reasons, I believe this finding should be capped to QA/Low, not Medium risk.

[zhaojio]

Although the user cannot query the cached data directly, the user can query it indirectly through the contract. The cache stores off-chain data, which can also cause problems if it is recovered. For example, price data, the attacker let the expired cache recovery, there may be 2 different prices, which will lead to price manipulation. The report says that storing xxxx in the cache is just an assumption.

severity is determined by the judge, and I think it should at least remain Medium.

[DadeKuma]

The set_expire function is never called inside the contest repository, nor in the main Phala repository, and the only proof of how it will be used is inside the docs in the file itself, which points to off-chain computations.

Using this cache to store on-chain data would be a misuse and a user error (and I don't think it would even make sense as the data is volatile/incongruent between the workers). This is also confirmed by the Sponsor in the comment above.

Of course, the Judge will decide the final severity. I was just adding some details that might have been missed in the initial submission.

EDIT: set_expire is actually called, GitHub search is broken; see comment below. My point on docs/normal usage stands.

[zhaojio]

The set_expire function is never called inside the contest repository, nor in the main Phala repository, and the only proof of how it will be used is inside the docs in the file itself, which points to off-chain computations.

Using this cache to store on-chain data would be a misuse and a user error (and I don't think it would even make sense as the data is volatile/incongruent between the workers). This is also confirmed by the Sponsor in the comment above.

Of course, the Judge will decide the final severity. I was just adding some details that might have been missed in the initial submission.

You should search for set_expiration

pub fn set_expiration(contract: &[u8], key: &[u8], expiration: u64) {
    with_global_cache(|cache| cache.set_expire(contract.into(), key.into(), expiration))
}

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

[OpenCoreCH]

First of all, the docs state that the cache will store off-chain computations and not on-chain data that users can query/call:

I agree with that. If it were used for on-chain data such as balances, High would be more appropriate. However, even for off-chain data, this behaviour could lead to problems. For instance, if the cache is used for caching some API / web responses (which seems to be one of the most common use cases for the cache), it is not unreasonable that a developer wants to have a maximum age of the response (and with cache_set_expire, there is an exposed function for exactly doing that, which does not always correctly as the warden has shown). In these cases, it also does not matter that the data is volatile / different between workers, because you care about the maximum age, but fetching newer data is fine. One example I can think of is betting on some result of an API service that refreshes daily where you would set the expiration such that no new requests are made until the next refresh. Of course, this has some assumptions, but I think they are pretty reasonable for such a runtime and it is well possible that there could be contracts that trigger this issue.