`shrine.get_threshold_and_value` function might use invalid assets prices

[c4-bot-3]

Lines of code

https://github.com/code-423n4/2024-01-opus/blob/4720e9481a4fb20f4ab4140f9cc391a23ede3817/src/core/shrine.cairo#L1306-L1337 https://github.com/code-423n4/2024-01-opus/blob/4720e9481get_recent_price_froma4fb20f4ab4140f9cc391a23ede3817/src/core/shrine.cairo#L1180-L1187

Vulnerability details

Impact

• shrine.get_threshold_and_value function is used by:

  • shrine.get_trove_health function: to check the healthiness of the trove by calculating its assets value to its yin debt if passing liquidation threshold, and the trove is considered unhealthy and can be liquidated if its LTV is above lqiquidation threshold.
  • shrine.get_shrine_health function: to get the health of the shrine contract.
  • shrine.redistribute_helper function: to redistribute bad debt and yang of between troves.

• Knowing that the collateral tokens prices (that are used to calculate the trove value) are updated in the following cases:

  1. when the time of the prices update comes, where seer.execute_task function can be invoked by anyone.

  2. automatically when an absorbtion happens (liquidation of an unhealthy trove using the yin balance of the absorber contract), where the price update is enforced regardless of the price being invalid/stale, and this is done via seer.update_prices function.

• But it was noticed that get_threshold_and_value might be using stale prices as there's no check if the prices are old and need to be updated or not (prices are extracted from the saved prices by calling the get_recent_price_from function):

   fn get_recent_price_from(self: @ContractState, yang_id: u32, interval: u64) -> (Wad, Wad, u64) {
            let (price, cumulative_price) = self.yang_prices.read((yang_id, interval));

            if price.is_non_zero() {
                return (price, cumulative_price, interval);
            }
            self.get_recent_price_from(yang_id, interval - 1)
        }

• So as can be noticed; the collateral price is returned once a price > 0 is catched, regardless of this price being stale or not, which will result in using stale prices leading to an inaccuracy calculations of health and redistributed yang and yin amounts (from bad debts)

• Also, if the actual asset price drops while the protocol hasn't updated it; this will lead users to borrow more yin against their collaterals as their trove values will be calculated based on the old high price instead of the new unfetched price.

Proof of Concept

shrine.get_threshold_and_value function

      fn get_threshold_and_value(
            self: @ContractState, mut yang_balances: Span<YangBalance>, interval: u64
        ) -> (Ray, Wad) {
            let mut weighted_threshold_sum: Ray = RayZeroable::zero();
            let mut total_value: Wad = WadZeroable::zero();

            loop {
                match yang_balances.pop_front() {
                    Option::Some(yang_balance) => {
                        // Update cumulative values only if the yang balance is greater than 0
                        if (*yang_balance.amount).is_non_zero() {
                            let yang_threshold: Ray = self.get_yang_threshold_helper(*yang_balance.yang_id);
                            let (price, _, _) = self.get_recent_price_from(*yang_balance.yang_id, interval);

                            let yang_deposited_value = *yang_balance.amount * price;
                            total_value += yang_deposited_value;
                            weighted_threshold_sum += wadray::wmul_rw(yang_threshold, yang_deposited_value);
                        }
                    },
                    Option::None => { break; },
                };
            };

            // Catch division by zero
            let threshold: Ray = if total_value.is_non_zero() {
                wadray::wdiv_rw(weighted_threshold_sum, total_value)
            } else {
                RayZeroable::zero()
            };

            (threshold, total_value)
        }

shrine.get_recent_price_from function

fn get_recent_price_from(self: @ContractState, yang_id: u32, interval: u64) -> (Wad, Wad, u64) {
            let (price, cumulative_price) = self.yang_prices.read((yang_id, interval));

            if price.is_non_zero() {
                return (price, cumulative_price, interval);
            }
            self.get_recent_price_from(yang_id, interval - 1)
        }

Tools Used

Manual Review.

Recommended Mitigation Steps

Update shrine.get_threshold_and_value function to check for the latest collaterl price, not relying on the old recorded one as it might return price for an old interval which might be a stale price.

Assessed type

Context

[c4-pre-sort]

bytes032 marked the issue as sufficient quality report

[c4-pre-sort]

bytes032 marked the issue as duplicate of #111

[c4-judge]

alex-ppg marked the issue as unsatisfactory: Invalid