c4-bot-3 commented 8 months ago

Lines of code

https://github.com/code-423n4/2024-01-opus/blob/4720e9481a4fb20f4ab4140f9cc391a23ede3817/src/core/shrine.cairo#L1753

Vulnerability details

in redistribute_helper()

Two important arrays (updated_trove_yang_balances[], new_yang_totals[]) are calculated , and then updated based on these two arrays.

updated_trove_yang_balances[] is used to update the remaining quantity of trove.
new_yang_totals[] is used to update the total remaining quantity of yang.

For example, Assume there are ETH, BTC and trove[1] trove[1].ETH = 0 trove[1].BTC = 200 total_yang_ETH = 1000 total_yang_BTC = 2000

Execute redistribute(trove[1]), redistribute eth = 0, redistribute btc = 20

After calculation: updated_trove_yang_balances = [0,180] new_yang_totals = [1000,1980]

Then update the corresponding values based on these two arrays. An important point is that these two arrays should be of the same length and correspond one-to-one.

The main code is as follows:

        fn redistribute_helper(
            ref self: ContractState,
            redistribution_id: u32,
            trove_id: u64,
            debt_to_redistribute: Wad,
            pct_value_to_redistribute: Ray,
            current_interval: u64
        ) {
...
            loop {
                match trove_yang_balances_copy.pop_front() {
                    Option::Some(yang_balance) => {
                        let trove_yang_amt: Wad = (*yang_balance).amount;
                        let yang_id_to_redistribute = (*yang_balance).yang_id;
                        // Skip over this yang if it has not been deposited in the trove
                        if trove_yang_amt.is_zero() {
@>                           updated_trove_yang_balances.append(*yang_balance);
@>                           continue;
                        }
...

                    if is_ordinary_redistribution {
...
                            new_yang_totals
                                .append(
                                    YangBalance {
                                        yang_id: yang_id_to_redistribute,
                                        amount: redistributed_yang_total_supply - yang_amt_to_redistribute - yang_offset
                                    }
                                );
                      }else {
....
                             new_yang_totals
                                  .append(
                                      YangBalance {
                                          yang_id: yang_id_to_redistribute,
                                          amount: redistributed_yang_total_supply - yang_error
                                      }
                                  );
                        }
....        }//end of loop
            let mut new_yang_totals: Span<YangBalance> = new_yang_totals.span();
            let mut updated_trove_yang_balances: Span<YangBalance> = updated_trove_yang_balances.span();
@>          loop {
                match new_yang_totals.pop_front() {
                    Option::Some(total_yang_balance) => {
                        let updated_trove_yang_balance: YangBalance = *updated_trove_yang_balances.pop_front().unwrap();
                        self
                            .deposits
                            .write((updated_trove_yang_balance.yang_id, trove_id), updated_trove_yang_balance.amount);

                        self.yang_total.write(*total_yang_balance.yang_id, *total_yang_balance.amount);
                    },
                    Option::None => { break; },
                };
            };

The problem is that if trove_yang_amt.is_zero(), it will only append updated_trove_yang_balances[] and will not append new_yang_totals[].

                    if trove_yang_amt.is_zero() {
                      updated_trove_yang_balances.append(*yang_balance);  //only updated_trove_yang_balances , no  new_yang_totals
                      continue;
                  }

This leads to the above example becoming After calculation: updated_trove_yang_balances = [0,180] new_yang_totals = [1980] (Correct should be [1000,1980])

Updating through these two arrays of different lengths will cause some not to be updated.

Modification: trove[1].eth = 0
trove[1].btc = 200 (not change , because only loop 1 time , correct should be 180)

total_yang_BTC = 1980 total_yang_ETH = 1000 (not change, new_yang_totals only have btc ,not eth)

Impact

The total balance of trove and yang is not updated correctly.

Recommended Mitigation

        fn redistribute_helper(
            ref self: ContractState,
            redistribution_id: u32,
            trove_id: u64,
            debt_to_redistribute: Wad,
            pct_value_to_redistribute: Ray,
            current_interval: u64
        ) {
...

            // Iterate over the yangs deposited in the trove to be redistributed
            loop {
                match trove_yang_balances_copy.pop_front() {
                    Option::Some(yang_balance) => {
                        let trove_yang_amt: Wad = (*yang_balance).amount;
                        let yang_id_to_redistribute = (*yang_balance).yang_id;
                        // Skip over this yang if it has not been deposited in the trove
                        if trove_yang_amt.is_zero() {
-                           updated_trove_yang_balances.append(*yang_balance);
                            continue;
                        }