Maybe cause panic in ExternalStorage.get

[c4-bot-7]

Lines of code

https://github.com/code-423n4/2024-03-phala-network/blob/main/phala-blockchain/crates/pink/runtime/src/storage/mod.rs#L115-L119

Vulnerability details

Impact

This bug maybe cause the blockchain node panic

Proof of Concept

in this get function, it will accept a param key and pass it into storage(key) function, which will return Result<Option<StorageValue>, Self::Error>, and call expect to extract the Option<StorageValue>, however , It maybe return Err, which will make the blockchain node panic

    pub fn get(&self, key: &[u8]) -> Option<Vec<u8>> {
        self.backend
            .as_trie_backend()
            .storage(key)
            .expect("Failed to get storage key")
    }

how can we get there? There is a extension function code_exists

    #[ink(extension = 19, handle_status = false)]
    fn code_exists(code_hash: Hash, sidevm: bool) -> bool;

and the callstack will like

code_exists() ->
pink_runtime::runtime::extension::CallInQuery::code_exists ->
pink_runtime::storage::external_backend::helper::code_exists ->
pink_runtime::storage::Storage::get

So If the storage is broken, we can call code_exists in query to cause the node panic

Tools Used

Manual Audit

Recommended Mitigation Steps

use match to handle error

Assessed type

DoS

[c4-pre-sort]

141345 marked the issue as primary issue

[c4-pre-sort]

141345 marked the issue as sufficient quality report

[141345]

unhandled err could lead to panic

[kvinwang]

It returns an error only when the data of the underlying KVDB is corrupted. In this situation, panicking is the best choice rather than letting it keep running.

[c4-sponsor]

kvinwang (sponsor) disputed

[OpenCoreCH]

Agree with the sponsor that panicking is the best possible behaviour in such an (unexpected) situation

[c4-judge]

OpenCoreCH marked the issue as nullified

[c4-judge]

OpenCoreCH marked the issue as unsatisfactory: Insufficient proof

[c4-judge]

OpenCoreCH marked the issue as nullified