[V-PHX-VUL-004] Factory can be made to deploy malicious pools

[gangov]

The function create_liquidity_pool in the factory contract is used to deploy new pool contracts:

Code snippet from the create_liquidity_pool function. It can be found in the factory/src/contract.rs file.

let lp_contract_address = deploy_lp_contract(
    &env,
    lp_init_info.lp_wasm_hash,
    &lp_init_info.token_init_info.token_a,
    &lp_init_info.token_init_info.token_b,
);

The deploy_lp_contract function has the following logic:

Logic of the deploy_lp_contract function. It can be found in the factory/src/utils.rs file.

pub fn deploy_lp_contract(
    env: &Env,
    lp_wasm_hash: BytesN<32>,
    token_a: &Address,
    token_b: &Address,
) -> Address {
    let deployer = env.current_contract_address();

    if deployer != env.current_contract_address() {
        deployer.require_auth();
    }

    let mut salt = Bytes::new(env);
    salt.append(&token_a.to_xdr(env));
    salt.append(&token_b.to_xdr(env));
    let salt = env.crypto().sha256(&salt);

    env.deployer()
        .with_current_contract(salt)
        .deploy(lp_wasm_hash)
}

As we can see the deploy_lp_contract function deploys a new contract using the address of the current running contract, the salt computed with tokenA and tokenB, and with the lp_wasm_hash.

The issue is that as we can see from the create_liquidity_pool function is that lp_wasm_hash is provided as input to the function via lp_init_info.lp_wasm_hash. So, a malicious user can make the factory to deploy arbitrary code.

Impact

Users would expect that the code created via the phoenix factory is always the same.

Recommendation

The lp_wasm_hash should be saved into the instance storage of the contract instead of being provided via user input.

[gangov]

btw in that line of thoughts - we have the following:

pub struct TokenInitInfo {
    pub token_wasm_hash: BytesN<32>,
    pub token_a: Address,
    pub token_b: Address,
}

and

pub struct StakeInitInfo {
    pub stake_wasm_hash: BytesN<32>,
    pub min_bond: i128,
    pub max_distributions: u32,
    pub min_reward: i128,
}

should we get rid of wasm there, too?