search

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

0 stars 0 forks source link

inconsistent State Due to Unhandled Migration Failures #81

Closed c4-bot-1 closed 6 months ago

c4-bot-1 commented 6 months ago

Lines of code

https://github.com/code-423n4/2024-03-phala-network/blob/a01ffbe992560d8d0f17deadfb9b9a2bed38377e/phala-blockchain/crates/pink/runtime/src/runtime.rs#L157-L166 https://github.com/code-423n4/2024-03-phala-network/blob/a01ffbe992560d8d0f17deadfb9b9a2bed38377e/phala-blockchain/crates/pink/runtime/src/runtime.rs#L194-L200

Vulnerability details

Impact

vulnerability Description: The migration logic within PinkRuntime is designed to sequentially apply a series of migrations (NoopMigration<10>, v11::Migration, v12::Migration<Self, Balances>,) to transition the runtime state across versions. This sequence is crucial for maintaining the integrity and consistency of the runtime state. the mechanism does not explicitly account for handling partial or failed migrations, which can result in an inconsistent or corrupted state if any migration step fails or partially applies. The implementation of the migration sequence in the impl pallet_contracts::Config for PinkRuntime section:

type Migrations = (
    NoopMigration<10>,
    v11::Migration<Self>,
    v12::Migration<Self, Balances>,
    v13::Migration<Self>,
    v14::Migration<Self, Balances>,
    v15::Migration<Self>,
);

And the application of these migrations during the runtime upgrade:

pub fn on_runtime_upgrade() {
    use frame_support::traits::OnRuntimeUpgrade;
    type Migrations = (Migration<PinkRuntime>, AllPalletsWithSystem);
    Migrations::on_runtime_upgrade();
    info!("Runtime database migration done");
}

If any migration step fails without proper rollback mechanisms, it could leave the state in an inconsistent or corrupted state. This scenario can occur due to unexpected errors

Tools Used

manual review

Recommended Mitigation Steps

Assessed type

Other

141345 commented 6 months ago

migration rollback

QA might be more appropriate

c4-pre-sort commented 6 months ago

141345 marked the issue as sufficient quality report

kvinwang commented 6 months ago

This is the standard way doing the pallet-contracts migration. The migration logic is implemented in pallet-contracts, something like:

impl Migration for (
    NoopMigration<10>,
    v11::Migration<Self>,
    v12::Migration<Self, Balances>,
    v13::Migration<Self>,
    v14::Migration<Self, Balances>,
    v15::Migration<Self>,
) { ...

pallet-contracts would take care of the migration steps. In fact it is far more complicate than simple steps error handling in the implementation.

c4-sponsor commented 6 months ago

kvinwang (sponsor) disputed

c4-judge commented 6 months ago

OpenCoreCH changed the severity to QA (Quality Assurance)

c4-judge commented 6 months ago

This previously downgraded issue has been upgraded by OpenCoreCH

c4-judge commented 6 months ago

OpenCoreCH marked the issue as unsatisfactory: Invalid