c4-bot-5 commented 3 months ago

Lines of code

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

Vulnerability details

Proof of Concept

Take a look at https://github.com/code-423n4/2024-03-phala-network/blob/a01ffbe992560d8d0f17deadfb9b9a2bed38377e/phala-blockchain/crates/pink/runtime/src/runtime/extension.rs#L321-L326

    fn current_event_chain_head(&self) -> Result<(u64, Hash), Self::Error> {
        Ok((
            PalletPink::next_event_block_number(),
            PalletPink::last_event_block_hash().into(),
        ))
    }

This method is used to retrieve the information for the current event chain

Now consider the following snippets from from pallet_pink.rs that are being queried to demonstrate the storage items involved:

    /// The next event chain sequence number
    #[pallet::storage]
    #[pallet::getter(fn next_event_block_number)]
    pub(crate) type NextEventBlockNumber<T: Config> = StorageValue<_, u64, ValueQuery>;

    /// The last emited event block hash
    #[pallet::storage]
    #[pallet::getter(fn last_event_block_hash)]
    pub(crate) type LastEventBlockHash<T: Config> = StorageValue<_, T::Hash, ValueQuery>;

Impact

The current implementation of current_event_chain_head in extension.rs might return inaccurate information about the event chain head. Here's how it impacts the system:

Fururistic Block Number: The function retrieves the value from PalletPink::next_event_block_number(). This function returns the number for the next event block, not the current one. This creates a discrepancy between the reported head and the actual state of the chain.
Stale Last Event Hash: The function retrieves the hash from PalletPink::last_event_block_hash(). This value might not reflect the most recently committed event block's hash if not updated promptly.

This inconsistency leads to issues for applications or functionalities that rely on accurate event chain head information. For example:

Applications expecting to process events from the latest block might miss recent events due to the reported "future" block number.
Applications verifying event block integrity using the reported hash might use an outdated reference, potentially compromising security.

Recommended Mitigation Steps

Refactor the functions not to return any stale or futuristic data for the current event chain head, this should be easily achieved by modifying the current_event_chain_head method.

Assessed type

Context

c4-pre-sort commented 3 months ago

141345 marked the issue as primary issue

c4-pre-sort commented 3 months ago

141345 marked the issue as sufficient quality report

141345 commented 3 months ago

block number and event hash do not correspond to each other

kvinwang commented 3 months ago

The implementation matches the documentation. Chain extension is the low-level API, and more convenient functions can be implemented in a higher-level SDK.