search

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

0 stars 0 forks source link

Users can lose funds when bridging from Pink to Standalone runtime #60

Closed c4-bot-6 closed 3 months ago

c4-bot-6 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.rs#L48 https://github.com/Phala-Network/phala-blockchain/blob/d92a10a8d2b89ff2c3f28dd917d892b43f1a573c/standalone/runtime/src/lib.rs#L469

Vulnerability details

Impact

When bridging funds back from the Pink runtime to the Standalone runtime, accounts may be reaped if the transfer amount is higher than the Existential Deposit (ED) limit of the Pink runtime, but results in a balance below the ED limit of the Standalone runtime, resulting in a loss of funds.

Proof of Concept

According to the Sponsor, users are required to bridge funds from the Standalone to the Pink runtime:

The two runtimes don't operate on the same state. The workers pickup commands from the on-chain state, and interpret the commands and dispatch them to the pink runtime. Commands includes contract call, upload code, transfer tokens .etc. If an account has some assets on-chain, it doesn't automatically usable in pink runtime. It is required to transfer the tokens into the so-called Cluster (where pink runtime runs on) first. So the two runtimes works like on two different chain with a bridge between them.

An issue may arise when they decide to send some of these funds back to the Standalone runtime.

Due to how the ED works, if the balance is under this limit after the transfer, the account will be reaped to avoid wasting storage space, and funds will be burned.

The issue is that on Pink, the ED is just a single unit:

pub const ExistentialDeposit: Balance = 1;

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

But on Standalone it is 1 CENTS:

pub const ExistentialDeposit: Balance = 1 * CENTS;

https://github.com/Phala-Network/phala-blockchain/blob/d92a10a8d2b89ff2c3f28dd917d892b43f1a573c/standalone/runtime/src/lib.rs#L469

Due to this incongruency, a transfer that would not reap an account on Pink might do so when bridging the funds back to the Standalone runtime, as the ED in the latter is higher, resulting in burned funds.

Tools Used

Manual review

Recommended Mitigation Steps

Consider aligning the ED value in the Pink runtime to the Standalone runtime, they should have the same value.

Assessed type

Token-Transfer

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

different minimum amount with Pink and Standalone

relate to https://github.com/code-423n4/2024-03-phala-network-findings/issues/49

kvinwang commented 3 months ago

Sending funds from the pink cluster to the Phala chain is not supported.

c4-sponsor commented 3 months ago

kvinwang (sponsor) disputed

c4-judge commented 3 months ago

OpenCoreCH marked the issue as unsatisfactory: Invalid

DadeKuma commented 3 months ago

Sending funds from the pink cluster to the Phala chain is not supported.

From my understanding, and please correct me if I'm wrong, it is not supported yet, but the plan is to make it possible in the future, but I would need the Sponsor to re-confirm it:

DadeKuma 1) Is it possible to bridge funds back from the Pink runtime to the Standalone runtime? If yes, how? 2) Does Pink runtime use the same native token as the Standalone runtime, or is it "converted" when bridging?

Kevin Wang

  1. not yet. 2. 1:1

When the Pink-Standalone bridge is deployed, the Pink runtime will already have the wrong ED value applied, and this issue will occur.

Regarding the speculation within the future code, I believe this issue is in scope:

Any issue that is not exploitable within the scope of the contest is defined as speculating on future code. Any such speculation only has the potential to be valid if the root cause is demonstrated to be in the contest scope. Warden may make an argument on why a future code change that would make the bug manifest is reasonably likely. Based on likelihood considerations, the Judge may assign a severity rating in any way they see fit.

The root cause is definitely within scope, as the current ED will be utilized even with the current code, as it is an essential component of Substrate. Furthermore, I believe it is reasonably likely to occur, as a wrong ED is already in place and it went unnoticed (see issue #49).

Also note that this is different from 49 as it has:

Different mitigation:

In issue 49, the Sponsor could mitigate it by increasing the ED and thus avoiding the dusting issue. As long as the cost to create an account increases enough to make the attack economically infeasible. But if the new value it's not the same exact amount, this issue will occur. The greater the difference between these two numbers, the more severe the impact.

If it's higher than the Standalone ED, it will have the reverse problem instead (bridging from Standalone to Pink will burn user funds), while a lower value than Standalone will trigger this issue (bridging from Pink to Standalone will burn user funds).

Different impact: Issue 49 impacts the entire runtime as the storage is bloated intentionally by an attacker, causing a net loss for the entire system.

This issue impacts any user who will bridge funds from Pink to Standalone, as their funds may be unintentionally burned. This is a clear loss of value that could affect any user engaging in normal usage of the protocol.

For these reasons, I believe this is a valid Medium severity issue.

kvinwang commented 3 months ago

This issue impacts any user who will bridge funds from Pink to Standalone, as their funds may be unintentionally burned. This is a clear loss of value that could affect any user engaging in normal usage of the protocol.

Even if we implement a backward bridge in the future, I don't think disallowing funds smaller than the on-chain ED to be transfered back is a problem.

DadeKuma commented 3 months ago

This is not about disallowing transfers smaller than the ED. It should be possible if the user wishes to do so (as other Polkadot parachains allow for that).

The issue arises when a transfer that wouldn't normally burn funds in the context of Pink results in a burn when the transfer is outgoing to the Standalone runtime, instead. This is caused by the discrepancy between the two EDs.

Users may expect that funds won't be burned, as these transfers function correctly within Pink runtime, but they will not work as expected in this integration: this occurrence is also highly probable, given the tight coupling between these two runtimes.

kvinwang commented 3 months ago

The issue arises when a transfer that wouldn't normally burn funds in the context of Pink results in a burn when the transfer is outgoing to the Standalone runtime, instead. This is caused by the discrepancy between the two EDs.

Users may expect that funds won't be burned, as these transfers function correctly within Pink runtime, but they will not work as expected in this integration: this occurrence is also highly probable, given the tight coupling between these two runtimes.

If the design decision is to not allow transfer such small funds back, it wouldn't be burned, it would be a tx fail.

OpenCoreCH commented 3 months ago

While I think the finding is a great catch, the judgement still stands for the following reasons: