Unnecessary Instantiation of External Storage in code_exists Function

[c4-bot-10]

Lines of code

https://github.com/code-423n4/2024-03-phala-network/blob/a01ffbe992560d8d0f17deadfb9b9a2bed38377e/phala-blockchain/crates/pink/runtime/src/storage/external_backend.rs#L56-L59

Vulnerability details

Impact

The primary impact of this vulnerability is reduced performance due to the unnecessary instantiation of ExternalStorage instances. This inefficiency can result in slower execution times for functions that call code_exists frequently, impacting the overall responsiveness of the contract and potentially increasing transaction costs.

Proof of Concept

The vulnerability lies in the code_exists function within the helper module of the provided Rust contract. In this function, a new instance of ExternalStorage is instantiated every time code_exists is called:

pub fn code_exists(code_hash: &Hash) -> bool {
    let key = code_owner_key(code_hash);
    super::ExternalStorage::instantiate().get(&key).is_some()
}

Creating a new storage instance for each invocation of code_exists can lead to performance degradation and potential inconsistencies in the stored data. This approach is inefficient as it unnecessarily incurs overhead associated with initializing a new storage instance repeatedly. Additionally, it may introduce race conditions or data race issues if multiple threads or processes access the storage concurrently.

Tools Used

Manual

Recommended Mitigation Steps

The code_exists function should be refactored to accept a reference to an existing ExternalStorage instance as a parameter. By passing the storage instance as an argument, the function can reuse the same instance across multiple calls, eliminating the need for redundant instantiation.

Assessed type

Error

[c4-pre-sort]

141345 marked the issue as sufficient quality report

[141345]

no loss

QA is more appropriate

[kvinwang]

First of all, there are no race conditions or data race issues because each ro(query) contract call is executed based on a snapshot of the underlying kvdb, and rw(TX) calls are executed one by one sequentially.

Secondly, ExternalStorage::instantiate() is not heavy; it is just a simple Rust struct with a hash pointer inside. There is no better way to reduce this tiny performance impact due to the execution model.

[c4-sponsor]

kvinwang (sponsor) disputed

[c4-judge]

OpenCoreCH marked the issue as unsatisfactory: Invalid