c4-bot-4 commented 6 months ago

Lines of code

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

Vulnerability details

Impact

Pink Extension provides the possibility to set cache data that can be read from within the same worker.

The high level logic in set function as follows:

Remove the current value if it exists already
```
    _ = self.remove(key.as_ref());
```
local_cache.rs#L106
Check if StorageQuota is exceeded. if the max size is reached, a StorageQuotaExceeded error is returned.
```
        if store_size > self.max_size {
            return Err(StorageQuotaExceeded);
        }
```
local_cache.rs#L112-L115

Insert the new value to the BTreeMap

    self.kvs.insert(
        key.into_owned(),
        StorageValue {
            expire_at: now().saturating_add(lifetime),
            value: value.into_owned(),
        },
    );

local_cache.rs#L117-L124

It seems that removing the old value first is done to calculate correctly the new store_size with the new key/value lengths. However, if it happens that the new key/value length + current size exceeds the max size, the old value will be removed. Though, an error is returned to the developer. This is an incorrect behaviour, the set function shouldn't remove the old value if an error occurs.

This brings incorrectness in the behaviour of set function and breaks the assumption of the developer.

Proof of Concept

Here is the scenario simplified:

Set cache data with key "A" and value "B"
Set other data till we nearly reach the limit
Read value of key "A", we will receive "B"
Update cache data for key "A" with some value e.g. "CCCCCCC"
Assume we exceeded the storage max size, set function will return StorageQuotaExceeded
Read value of key "A", we will NOT receive "B" because the item was removed and not restored before returning StorageQuotaExceeded.

Tools Used

Manual analysis

Recommended Mitigation Steps

Restore the old value before StorageQuotaExceeded is returned.

Assessed type

Other

c4-pre-sort commented 6 months ago

141345 marked the issue as duplicate of #21

c4-judge commented 6 months ago

OpenCoreCH marked the issue as unsatisfactory: Invalid

code-423n4 / 2024-03-phala-network-findings

Incorrectness in the behaviour of cache `set` function when the storage quota exceeded #69