`seer.update_prices` function might enforce updating to an invalid collateral prices and risking users troves health

[c4-bot-5]

Lines of code

https://github.com/code-423n4/2024-01-opus/blob/4720e9481a4fb20f4ab4140f9cc391a23ede3817/src/core/seer.cairo#L168-L171 https://github.com/code-423n4/2024-01-opus/blob/4720e9481a4fb20f4ab4140f9cc391a23ede3817/src/core/seer.cairo#L193-L239 https://github.com/code-423n4/2024-01-opus/blob/4720e9481a4fb20f4ab4140f9cc391a23ede3817/src/external/pragma.cairo#L186-L214

Vulnerability details

Impact

• The yang price is set in the shrine by one of the following operations:

  1. when the yang is initially added, where this is done manually without directly fetching the price from the pragma oracle as the pprice will be entered manually.

  2. externally by anyone via seer.execute_task function when the time of the prices update comes.

  3. 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.

• The problem with seer.update_prices is that it enforces updating the collaterals prices reagrdless of the returned prices being stale/invalid or zero, as it will fetch the price from pragma oracle where this check is made when fetching it:

  //@notice: pragma.fetchprice function
  fn fetch_price(ref self: ContractState, yang: ContractAddress, force_update: bool) -> Result<Wad, felt252> {
            let response: PragmaPricesResponse = self.oracle.read().get_data_median(DataType::SpotEntry(pair_id));
  
        //some code...
            if force_update || self.is_valid_price_update(response) {
                return Result::Ok(price);
            }
        //rest of the function...
  }

• So even if the price is invalid (not fresh and/or not having enough sources), it will be used since force_update is set to true when seer.update_prices is called:

        fn update_prices(ref self: ContractState) {
            self.access_control.assert_has_role(seer_roles::UPDATE_PRICES);
            self.update_prices_internal(true);
        }

• This will risk the health of the users troves (positions) if the updated assets prices are invalid/zero, since users positions might be liquidated based on an invalid/stale asset price as there's no way to ensure that the price used to calculate the collateral value is up-to-date/fresh/has enough sources because it is enforced regardless of its validity.

Proof of Concept

seer.update_prices function

        fn update_prices(ref self: ContractState) {
            self.access_control.assert_has_role(seer_roles::UPDATE_PRICES);
            self.update_prices_internal(true);
        }

seer.update_prices_internal function

fn update_prices_internal(ref self: ContractState, force_update: bool) {
            let shrine: IShrineDispatcher = self.shrine.read();
            let sentinel: ISentinelDispatcher = self.sentinel.read();

            // loop through all yangs
            // for each yang, loop through all oracles until a
            // valid price update is fetched, in which case, call shrine.advance()
            // the expectation is that the primary oracle will provide a
            // valid price in most cases, but if not, we can fallback to other oracles
            let mut yangs: Span<ContractAddress> = sentinel.get_yang_addresses();
            loop {
                match yangs.pop_front() {
                    Option::Some(yang) => {
                        let mut oracle_index: u32 = LOOP_START;
                        loop {
                            let oracle: IOracleDispatcher = self.oracles.read(oracle_index);
                            if oracle.contract_address.is_zero() {
                                // if branch happens, it means no oracle was able to
                                // fetch a price for yang, i.e. we're missing a price update
                                self.emit(PriceUpdateMissed { yang: *yang });
                                break;
                            }

                            // TODO: when possible in Cairo, fetch_price should be wrapped
                            //       in a try-catch block so that an exception does not
                            //       prevent all other price updates

                            match oracle.fetch_price(*yang, force_update) {
                                Result::Ok(oracle_price) => {
                                    let asset_amt_per_yang: Wad = sentinel.get_asset_amt_per_yang(*yang);
                                    let price: Wad = oracle_price * asset_amt_per_yang;
                                    shrine.advance(*yang, price);
                                    self.emit(PriceUpdate { oracle: oracle.contract_address, yang: *yang, price });
                                    break;
                                },
                                // try next oracle for this yang
                                Result::Err(_) => { oracle_index += 1; }
                            }
                        };
                    },
                    Option::None => { break; }
                };
            };

            self.last_update_prices_call_timestamp.write(get_block_timestamp());
            self.emit(UpdatePricesDone { forced: force_update });
        }

pragma.fetch_price function

fn fetch_price(ref self: ContractState, yang: ContractAddress, force_update: bool) -> Result<Wad, felt252> {
            let pair_id: felt252 = self.yang_pair_ids.read(yang);
            assert(pair_id.is_non_zero(), 'PGM: Unknown yang');

            let response: PragmaPricesResponse = self.oracle.read().get_data_median(DataType::SpotEntry(pair_id));

            // convert price value to Wad
            let price: Wad = fixed_point_to_wad(response.price, response.decimals.try_into().unwrap());

            // if we receive what we consider a valid price from the oracle,
            // return it back, otherwise emit an event about the update being invalid
            // the check can be overridden with the `force_update` flag
            if force_update || self.is_valid_price_update(response) {
                return Result::Ok(price);
            }

            self
                .emit(
                    InvalidPriceUpdate {
                        yang,
                        price,
                        pragma_last_updated_ts: response.last_updated_timestamp,
                        pragma_num_sources: response.num_sources_aggregated,
                    }
                );
            Result::Err('PGM: Invalid price update')
        }

Tools Used

Manual Review.

Recommended Mitigation Steps

Avoid enforcing price updates if the oracle returns invalid/stale values.

Assessed type

Context

[c4-pre-sort]

bytes032 marked the issue as duplicate of #13

[c4-pre-sort]

bytes032 marked the issue as sufficient quality report

[c4-judge]

alex-ppg marked the issue as unsatisfactory: Invalid