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/capi/ecall_impl.rs#L306-L310

Vulnerability details

Impact

It won't be possible to send funds to contracts by using the transfer field of the TransactionArguments struct, as the runtime only takes into account the balances field for that.

Proof of Concept

In Substrate, we use the Balanes pallet to be able to send funds from one address to another. In such a crate, there are several functions to accomplish this, but the ecall's implementation makes use of deposit_creating, which will always add the funds to the callee address (unless some requirements are met). If we go to:

ecall_impl.rs, function handle_deposit

fn handle_deposit(args: &TransactionArguments) {
    if args.deposit > 0 {
        let _ = PalletBalances::deposit_creating(&args.origin, args.deposit);
    }
}

we see that such a function is responsible of this task, which is called by contract_call and contract_instantiate. However, it only checks for the deposit field of the TransactionArguments struct, even though it is possible to transfer funds by setting its value to 0 and setting up the field transfer:

    pub struct TransactionArguments {
        ...

        /// The amount of Balance transferred to the contract.
        pub transfer: Balance,

        ...

        /// The balance to deposit to the caller address, used when the caller's balance is insufficient
        /// for a gas estimation.
        pub deposit: Balance,
    }

That means if the caller tries to transfer funds as explained above, the logical branch inside handle_deposit won't be triggered and no funds will be transfered to the callee at all.

Recommended Mitigation Steps

I would modify the handle_deposit function like the following to take into account both cases:

fn handle_deposit(args: &TransactionArguments) {
    if args.deposit > 0 && args.transfer == 0 {
        let _ = PalletBalances::deposit_creating(&args.origin, args.deposit);
    } else if args.transfer > 0 && args.deposit == 0 {
        let _ = PalletBalances::deposit_creating(&args.origin, args.transfer);
    }
}

so that either users send tokens via the deposit field or the transfer one, but not both at a time.