Phat Contract Runtime - zhaojie's results

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

Lines of code

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

Vulnerability details

Impact

An attacker can perform a DoS attack on a worker.

Proof of Concept

Pink Extension provides two functions to make http requests: http_request batch_http_request

The default timeout of http_request is 10 * 1000 ms.

impl<T: PinkRuntimeEnv, E: From<&'static str>> PinkExtBackend for DefaultPinkExtension<'_, T, E> {
    .....
    fn http_request(&self, request: HttpRequest) -> Result<HttpResponse, Self::Error> {
        http_request(request, 10 * 1000).map_err(|err| err.display().into())
    }
    .....
}

The problem is that batch_http_request can pass in a timeout of any value.

batch_http_request sets the maximum number of http requests to 5, but there is no limit to the timeout period:

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

This will lead to DoS attacks. The attacker can launch a large number of requests and specify an infinite timeout period. In this way, a large number of http requests in processing will appear in the worker, which may lead to problems such as memory overflow.

An attacker can set up a malicious web server to make the timeout period as long as possible, such as continuously writing data to the client to maintain the connection.

In addition, the Extension is ultimately run in the worker as a .so file, so the http request code is not executed in the VM, so the DoS cannot be prevented by paying gas.

Tools Used

vscode, manual

Recommended Mitigation Steps

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);
    }
+    if timeout_ms > MAX_TIMEOUT_MS {
+        return Err(ext::HttpRequestError::TooLargeTimeout);
+    }
}

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

An attacker can perform a DoS attack on a worker.

Proof of Concept

chain-extension/lib/async_http_reques titerates request.headers and adds the requested headers to another map:

async fn async_http_request(
    request: HttpRequest,
    timeout_ms: u64,
) -> Result<HttpResponse, HttpRequestError> {
    .....
    let mut headers = HeaderMap::new();
    for (key, value) in &request.headers {
        let key =
            HeaderName::from_str(key.as_str()).or(Err(HttpRequestError::InvalidHeaderName))?;
        let value = HeaderValue::from_str(value).or(Err(HttpRequestError::InvalidHeaderValue))?;
        headers.insert(key, value);
    }
    .....

The request is passed in by the user at the time of invocation and has no size limit.

Therefore, if a malicious user sends a large request.headers and sends a large number of requests at the same time, there may be a memory overflow problem, and the for loop will occupy cpu time, resulting in worker failure, network delay and other problems.

Another factor that can lead to a DoS attack is, http request code is not executed in the VM, so the DoS cannot be prevented by paying gas. The code for the http request is compiled into a native. so file.

Tools Used

vscode, manual

Recommended Mitigation Steps

Limit the size of request.headers

Assessed type

DoS