Divergent Behavior in CallInQuery & CallInCommand `untrusted_millis_since_unix_epoch()`

[c4-bot-4]

Lines of code

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

Vulnerability details

Proof of Concept

The Pink runtime framework employs CallInQuery and CallInCommand structures for contract interaction within the runtime environment. These structures provide different implementations for the untrusted_millis_since_unix_epoch function, leading to potential security risks.

Take a look at the following code snippets coined from PinkExtension.rs , source

// CallInQuery retrieves the actual time
impl PinkExtBackend for CallInQuery {
    // ... other functions
    fn untrusted_millis_since_unix_epoch(&self) -> Result<u64, Self::Error> {
        DefaultPinkExtension::new(self).untrusted_millis_since_unix_epoch()
    }
    // ... other functions
}

// CallInCommand always returns 0
impl PinkExtBackend for CallInCommand {
    // ... other functions
    fn untrusted_millis_since_unix_epoch(&self) -> Result<u64, Self::Error> {
        Ok(0)
    }
    // ... other functions
}

Impact

Evidently as hinted in Proof Of Concept:

• CallInQuery: Retrieves the actual time using an unspecified method (likely a system call).
• CallInCommand: Always returns 0, providing a fixed and unreliable value.

This discrepancy can negatively impact applications that rely on timestamps within transactions:

1. Applications assuming a true timestamp within transactions will always receive zero, causing unintended behaviour or security vulnerabilities.
2. Attackers aware of this behaviour can manipulate time-dependent operations within transactions due to the predictability of the returned zero value.

Recommended Mitigation Steps

Always error out since this is not supported within transactions instead of returning 0, or clearly document in the Pink runtime documentation that untrusted_millis_since_unix_epoch within transactions might not provide a reliable timestamp.

Assessed type

Context

[c4-pre-sort]

141345 marked the issue as primary issue

[c4-pre-sort]

141345 marked the issue as sufficient quality report

[141345]

inconsistent implementation for untrusted_millis_since_unix_epoch()

[kvinwang]

clearly document in the Pink runtime documentation that untrusted_millis_since_unix_epoch within transactions might not provide a reliable timestamp

Yes, it is documented only work in query. We always avoid to return Err at the chain ext api level due the limitation of ink! that let the contract revert rather than be able to catch the error. There should be a higher level contract sdk wrapping this funstion to return Result::Err in TX.

[c4-sponsor]

kvinwang (sponsor) disputed

[c4-judge]

OpenCoreCH marked the issue as unsatisfactory: Invalid