#36117 [SC-High] Permanent freezing of tokens when user sends extra tokens as update fee

Submitted on Oct 20th 2024 at 16:42:11 UTC by @savi0ur for IOP | Swaylend

• Report ID: #36117

• Report Type: Smart Contract

• Report severity: High

• Target: https://github.com/Swaylend/swaylend-monorepo/blob/9132747331188b86dd8cbf9a1ca37b811d08dddb/contracts/market/src/main.sw

• Impacts:

Description

Bug Description

Anyone can update the price of an assets using `update_price_feeds_if_necessary` function.

https://github.com/Swaylend/swaylend-monorepo/blob/9132747331188b86dd8cbf9a1ca37b811d08dddb/contracts/market/src/main.sw#L1269-L1273 ```sway #[payable, storage(read)] fn update_price_feeds_if_necessary(price_data_update: PriceDataUpdate) { reentrancy_guard(); update_price_feeds_if_necessary_internal(price_data_update) } ```

https://github.com/Swaylend/swaylend-monorepo/blob/9132747331188b86dd8cbf9a1ca37b811d08dddb/contracts/market/src/main.sw#L1441-L1469 ```sway #[payable, storage(read)] fn update_price_feeds_if_necessary_internal(price_data_update: PriceDataUpdate) { let contract_id = storage.pyth_contract_id.read(); require( contract_id != ContractId::zero(), Error::OracleContractIdNotSet, );

// check if the payment is sufficient
require(
    msg_amount() >= price_data_update
        .update_fee && msg_asset_id() == AssetId::base(),
    Error::InvalidPayment,
); //@audit-issue there is no transfer of remaining tokens back to caller when msg_amount() > update_fee

let oracle = abi(PythCore, contract_id.bits());
oracle
    .update_price_feeds_if_necessary {
        asset_id: AssetId::base().bits(),
        coins: price_data_update.update_fee,
    }(
        price_data_update
            .price_feed_ids,
        price_data_update
            .publish_times,
        price_data_update
            .update_data,
    );

} ```

As we can see, in `update_price_feeds_if_necessary_internal` function, its accepting `msg_amount() >= update_fee`, if `msg_amount` is strictly greater than `update_fee`, their delta i.e., `msg_amount() - update_fee` will remain in the market contract instead of returning it back to caller i.e., `msg_sender()`. Due to this, caller lose those extra tokens and it gets stuck permanently in the contract.

Impact

Permanent freezing of tokens when user sends extra tokens as update fee while updating price using `update_price_feeds_if_necessary` function.

Recommendation

Either return the remaining tokens back to the caller at the end of function OR make sure `update_price_feeds_if_necessary_internal` function accepts `msg_amount()` exactly equal to `update_fee`.

References

• https://github.com/Swaylend/swaylend-monorepo/blob/9132747331188b86dd8cbf9a1ca37b811d08dddb/contracts/market/src/main.sw#L1269-L1273

• https://github.com/Swaylend/swaylend-monorepo/blob/9132747331188b86dd8cbf9a1ca37b811d08dddb/contracts/market/src/main.sw#L1441-L1469

Proof Of Concept

Steps to Run:

• Open terminal and run `cd swaylend-monorepo`

• Paste following rust code in `contracts/market/tests/local_tests/scenarios/price_changes.rs`

• Run test using `cargo test --package market --test integration_tests -- local_tests::scenarios::price_changes::excess_fee_not_returned --exact --show-output`

```rust #[tokio::test] async fn excess_fee_not_returned() { let TestData { alice, market, assets, eth, oracle, price_feed_ids, publish_time, prices, .. } = setup(None).await;

let price_data_update = PriceDataUpdate {
    update_fee: 100,
    price_feed_ids,
    publish_times: vec![publish_time; assets.len()],
    update_data: oracle.create_update_data(&prices).await.unwrap(),
};

let alice_balance_before = alice.get_asset_balance(&eth.asset_id).await.unwrap();
println!("Alice Balance Before: {}", alice_balance_before);

// Prepare calls for multi_call_handler
let tx_policies =
    fuels::types::transaction::TxPolicies::default().with_script_gas_limit(1_000_000);

// Params for update_price_feeds_if_necessary
let extra_fee = 20;
let call_params_update_price = fuels::programs::calls::CallParameters::default()
    .with_amount(price_data_update.update_fee + extra_fee);

println!(
    "Alice is sending `extra fee ({}) + update_fee ({}) = {}` to update_price_feeds_if_necessary",
    extra_fee, price_data_update.update_fee, extra_fee + price_data_update.update_fee
);
// Update price feeds if necessary
let _update_balance_call = market
    .instance
    .with_account(alice.clone())
    .methods()
    .update_price_feeds_if_necessary(price_data_update.clone())
    .with_contracts(&[&oracle.instance])
    .with_tx_policies(tx_policies)
    .call_params(call_params_update_price)
    .unwrap()
    .call()
    .await;

// println!("res: {:#?}", _update_balance_call);
let alice_balance_after = alice.get_asset_balance(&eth.asset_id).await.unwrap();
println!("Alice Balance After: {}", alice_balance_after);
println!(
    "Difference in balance: {}",
    alice_balance_before - alice_balance_after
);
assert!(alice_balance_after < alice_balance_before - price_data_update.update_fee - extra_fee);
println!(
    "NOTE: Its taking complete `extra fee ({}) + update_fee ({}) = {}`, even though update fee is just {}", 
    extra_fee, price_data_update.update_fee, 
    extra_fee + price_data_update.update_fee, 
    price_data_update.update_fee
);

} ```

Console Output:

```shell ---- local_tests::scenarios::price_changes::excess_fee_not_returned stdout ---- Price for UNI = 5 Price for BTC = 70000 Price for ETH = 3500 Price for USDC = 1 Alice Balance Before: 10000000000 Alice is sending `extra fee (20) + update_fee (100) = 120` to update_price_feeds_if_necessary Alice Balance After: 9999999879 Difference in balance: 121 NOTE: Its using complete `extra fee (20) + update_fee (100) = 120`, even though update fee is just 100

successes: local_tests::scenarios::price_changes::excess_fee_not_returned ```

Proof of Concept

Proof of Concept