#37343 [SC-Insight] inaccurate check leading to debt miscalculation

#37343 [SC-Insight] Inaccurate Check Leading to Debt Miscalculation

Submitted on Dec 2nd 2024 at 16:03:30 UTC by @Blockian for IOP | Fluid Protocol

• Report ID: #37343

• Report Type: Smart Contract

• Report severity: Insight

• Target: https://github.com/Hydrogen-Labs/fluid-protocol/tree/main/contracts/trove-manager-contract/src/main.sw

• Impacts:

  • Griefing (e.g. no profit motive for an attacker, but damage to the users or the protocol)

  • Contract fails to deliver promised returns, but doesn't lose value

Description

Fluid Bug Report

Inaccurate Check Leading to Debt Miscalculation

Overview

An edge case arises during reward application where a user's pending_usdf is not added to their debt due to an oversight. This miscalculation enables users to withdraw funds without accounting for the additional debt, potentially shifting the burden onto other protocol participants or causing minor fluctuations in the value of USDF due to inaccurate collateral backing.

Deep Dive

The issue stems from the internal_has_pending_rewards function, which verifies changes in the l_asset value but overlooks updates to the l_usdf value. Consequently, in scenarios where l_usdf changes while l_asset remains unchanged, the function erroneously returns false. This prevents the internal_apply_pending_rewards function from updating the pending rewards accurately.

How does l_usdf change without l_asset? The internal_redistribute_debt_and_coll function updates these values based on the latest liquidation data. Here's the critical implementation:

fn internal_redistribute_debt_and_coll(debt: u64, coll: u64) {
    // not really interesting
    let asset_numerator: U128 = U128::from(coll) * U128::from(DECIMAL_PRECISION) + U128::from(storage.last_asset_error_redistribution.read());
    let usdf_numerator: U128 = U128::from(debt) * U128::from(DECIMAL_PRECISION) + U128::from(storage.last_usdf_error_redistribution.read());
    let asset_reward_per_unit_staked = asset_numerator / U128::from(storage.total_stakes.read());
    let usdf_reward_per_unit_staked = usdf_numerator / U128::from(storage.total_stakes.read());

    // not really interesting

    storage
        .l_asset
        .write(storage.l_asset.read() + asset_reward_per_unit_staked.as_u64().unwrap());
    storage
        .l_usdf
        .write(storage.l_usdf.read() + usdf_reward_per_unit_staked.as_u64().unwrap());
    // not really interesting
}

In cases where usdf_reward_per_unit_staked != 0 but asset_reward_per_unit_staked == 0, l_usdf is updated independently. This disparity can occur due to:

1. Significant price differences between the asset and USDF.

2. High total_stakes values combined with a higher USDF price compared to the asset.

3. Growth in last_usdf_error_redistribution.

During this interim period, pending rewards might need adjustment due to events such as liquidations, withdrawals, or redemptions.

Impact

The issue self-corrects once internal_redistribute_debt_and_coll is called again, updating l_asset. However, during the lag:

1. Users can perform actions (e.g., withdrawals) at a lower debt than intended.

2. Accumulated inaccuracies in the debt-to-collateral ratio can destabilize the USDF price if enough users act during this window.

Proposed Solution

Modify internal_has_pending_rewards to also evaluate changes in l_usdf:

fn internal_has_pending_rewards(address: Identity) -> bool {
    if (storage.troves.get(address).read().status != Status::Active)
    {
        return false;
    }
    return (storage.reward_snapshots.get(address).read().asset < storage.l_asset.read() || storage.reward_snapshots.get(address).read().usdf_debt < storage.l_usdf.read());
}

Proof of Concept

Proof of Concept

Run forc test after applying the following steps:

1. Add the following methods to the trove_manager_interface:

    #[storage(read, write)]
    fn test_internal_redistribute_debt_and_coll(debt: u64, coll: u64);

    #[storage(read, write)]
    fn test_issue_setup(total_stake: u64);

    #[storage(read)]
    fn test_get_l_values() -> (u64, u64);

1. Apply the git patch below:

diff --git a/contracts/trove-manager-contract/src/main.sw b/contracts/trove-manager-contract/src/main.sw
index 7e02245..6b93a1e 100644
--- a/contracts/trove-manager-contract/src/main.sw
+++ b/contracts/trove-manager-contract/src/main.sw
@@ -291,6 +291,23 @@ impl TroveManager for Contract {
     fn get_pending_asset_rewards(id: Identity) -> u64 {
         internal_get_pending_asset_reward(id)
     }
+
+    #[storage(read, write)]
+    fn test_internal_redistribute_debt_and_coll(debt: u64, coll: u64) {
+        internal_redistribute_debt_and_coll(debt, coll);
+    }
+
+    #[storage(read, write)]
+    fn test_issue_setup(total_stake: u64) {
+        storage
+        .total_stakes
+        .write(total_stake);
+    }
+
+    #[storage(read)]
+    fn test_get_l_values() -> (u64, u64) {
+        (storage.l_asset.read(), storage.l_usdf.read())
+    }
 }
 #[storage(read, write)]
 fn internal_update_trove_reward_snapshots(id: Identity) {
@@ -676,7 +693,7 @@ fn internal_apply_liquidation(
 }
 #[storage(read, write)]
 fn internal_redistribute_debt_and_coll(debt: u64, coll: u64) {
-    let asset_contract_cache = storage.asset_contract.read();
+    // let asset_contract_cache = storage.asset_contract.read();
     if (debt == 0) {
         return;
     }
@@ -704,11 +721,11 @@ fn internal_redistribute_debt_and_coll(debt: u64, coll: u64) {
     storage
         .l_usdf
         .write(storage.l_usdf.read() + usdf_reward_per_unit_staked.as_u64().unwrap());
-    let active_pool = abi(ActivePool, storage.active_pool_contract.read().into());
-    let default_pool = abi(DefaultPool, storage.default_pool_contract.read().into());
-    active_pool.decrease_usdf_debt(debt, asset_contract_cache);
-    default_pool.increase_usdf_debt(debt, asset_contract_cache);
-    active_pool.send_asset_to_default_pool(coll, asset_contract_cache);
+    // let active_pool = abi(ActivePool, storage.active_pool_contract.read().into());
+    // let default_pool = abi(DefaultPool, storage.default_pool_contract.read().into());
+    // active_pool.decrease_usdf_debt(debt, asset_contract_cache);
+    // default_pool.increase_usdf_debt(debt, asset_contract_cache);
+    // active_pool.send_asset_to_default_pool(coll, asset_contract_cache);
 }
 #[storage(read, write)]
 fn internal_update_stake_and_total_stakes(address: Identity) -> u64 {
@@ -979,3 +996,35 @@ fn internal_update_system_snapshots_exclude_coll_remainder(coll_remainder: u64)
         .total_collateral_snapshot
         .write(active_pool_coll - coll_remainder + liquidated_coll);
 }
+
+#[test]
+fn test_issue_1() {
+    /*
+        for the following price, coll and debt, the ICR will be 100% which makes this theoretical position liquidatable
+    */
+    let price = 3_000_000_000_000;
+    let coll = 1_000_000_000;
+    let debt = 3_000_000_000_000;
+    let usdf_in_stab_pool = 0;
+
+    let needed_total_stake = 996_000_000_000_000_000;
+
+    let caller = abi(TroveManager, CONTRACT_ID);
+
+    caller.test_issue_setup(needed_total_stake);
+
+    let l_before = caller.test_get_l_values();
+    let l_asset_before = l_before.0;
+    let l_usdf_before = l_before.1;
+
+    let mut single_liquidation = LiquidationValues::default();
+    single_liquidation = get_offset_and_redistribution_vals(coll, debt, usdf_in_stab_pool, price);
+    caller.test_internal_redistribute_debt_and_coll(single_liquidation.debt_to_redistribute, single_liquidation.coll_to_redistribute);
+
+    let l_after = caller.test_get_l_values();
+    let l_asset_after = l_after.0;
+    let l_usdf_after = l_after.1;
+
+    assert(l_asset_after == l_asset_before); // no change in l_asset
+    assert(l_usdf_after > l_usdf_before); // there is change in l_usdf
+}
\ No newline at end of file