Opus - bin2chen's results

Lines of code

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

Vulnerability details

Vulnerability details

when user call abbot.withdraw() -> shrine.withdraw() ->shrine.withdraw_helper()

The main logic is withdraw_helper()

        fn withdraw_helper(ref self: ContractState, yang: ContractAddress, trove_id: u64, amount: Wad) {
            let yang_id: u32 = self.get_valid_yang_id(yang);

            // Fails if amount > amount of yang deposited in the given trove
@>          let trove_balance: Wad = self.deposits.read((yang_id, trove_id));
            assert(trove_balance >= amount, 'SH: Insufficient yang balance');

            let new_trove_balance: Wad = trove_balance - amount;
            let new_total: Wad = self.yang_total.read(yang_id) - amount;

@>          self.charge(trove_id);

            self.yang_total.write(yang_id, new_total);
@>          self.deposits.write((yang_id, trove_id), new_trove_balance);

            // Emit events
            self.emit(YangTotalUpdated { yang, total: new_total });
            self.emit(DepositUpdated { yang, trove_id, amount: new_trove_balance });
        }

Execution steps:

1. Calculate let new_trove_balance = trove_balance - amount;
2. Execute self.charge(trove_id);
3. self.deposits.write((yang_id, trove_id), new_trove_balance);

However, self.charge() may execute pull_redistributed_debt_and_yangs() -> self.deposits.write(), retrieving redistributed yang. These retrieved redistributed yang will be overwritten by the temporary variable new_trove_balance in the third step. This part of the redistributed yang will be lost.

Impact

redistributed yang will lost

Recommended Mitigation

charge first

        fn withdraw_helper(ref self: ContractState, yang: ContractAddress, trove_id: u64, amount: Wad) {
            let yang_id: u32 = self.get_valid_yang_id(yang);
+            self.charge(trove_id);
            // Fails if amount > amount of yang deposited in the given trove
            let trove_balance: Wad = self.deposits.read((yang_id, trove_id));
            assert(trove_balance >= amount, 'SH: Insufficient yang balance');

            let new_trove_balance: Wad = trove_balance - amount;
            let new_total: Wad = self.yang_total.read(yang_id) - amount;

-            self.charge(trove_id);

            self.yang_total.write(yang_id, new_total);
            self.deposits.write((yang_id, trove_id), new_trove_balance);

            // Emit events
            self.emit(YangTotalUpdated { yang, total: new_total });
            self.emit(DepositUpdated { yang, trove_id, amount: new_trove_balance });
        }

Assessed type

Error

Lines of code

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

Vulnerability details

Vulnerability details

in gate.cairo When the user calls deposit(), it calculates the corresponding shares through convert_to_yang_helper(). The code is as follows:

        fn convert_to_yang_helper(self: @ContractState, asset_amt: u128) -> Wad {
            let asset: IERC20Dispatcher = self.asset.read();
            let total_yang: Wad = self.get_total_yang_helper(asset.contract_address);

            if total_yang.is_zero() {
                let decimals: u8 = asset.decimals();
                // Otherwise, scale `asset_amt` up by the difference to match `Wad`
                // precision of yang. If asset is of `Wad` precision, then the same
                // value is returned
                fixed_point_to_wad(asset_amt, decimals)
            } else {
@>              (asset_amt.into() * total_yang) / get_total_assets_helper(asset).into()
            }
        }

The calculation formula is: (asset_amt.into() * total_yang) / get_total_assets_helper(asset).into()

The actual calculation of converting Wad to pure numbers is: (asset_amt * total_yang / 1e18) * 1e18 / total_assets

The above formula (asset_amt * total_yang / 1e18) will lose precision, especially when the asset's decimals are less than 18.

Assume btc as an example, decimals = 8 after add_yang(btc) INITIAL_DEPOSIT_AMT = 1000 so: total_assets = 1000 total_yang = 1000e10 = 1e13

If the user deposits 0.0009e8 BTC, according to the formula = (asset_amt * total_yang / 1e18) = 0.0009e8 * 1e13 /1e18 = 0.9e5 * 1e13 /1e18 = 0

With BTC's price at 40,000 USD, 0.0009e8 = 36 USD

The user will lose 36 USD

We should cancel dividing by 1e18 and then multiplying by 1e18, and calculate directly shares = asset_amt.into() * total_yang.into() / total_assets.into() shares = 0.0009e8 * 1e13 / 1000 = 0.0009e18 = 900000000000000

Note: In order to successfully deposit should be > 0.0009e8 such as 0.0019e8, which is simplified and convenient to explain.

Impact

Due to the premature division by 1e18, precision is lost, and the user loses a portion of their funds.

Proof of Concept

add to test_abbot.cairo

    #[test]
    fn test_wad() {
        let INITIAL_DEPOSIT_AMT: u128 = 1000;
        let decimals:u8 = 8;
        let asset_amt:u128 = 90_000;
        let total_yang:Wad = fixed_point_to_wad(INITIAL_DEPOSIT_AMT, decimals);
        let total_assets:Wad = INITIAL_DEPOSIT_AMT.into();
        let result:Wad = asset_amt.into() * total_yang / total_assets;
        assert(result.into() == 0,' no zero');
        let result2_u:u256 = asset_amt.into() * total_yang.into() / total_assets.into();
        let result2:Wad = Wad { val:result2_u.try_into().expect('u128')};
        assert(result2.into() == 900000000000000,' result2 no zero');
    }

$ scarb test -vvv test_wad 

Running 1 test(s) from src/
[PASS] opus::tests::abbot::test_abbot::test_abbot::test_wad (gas: ~17)
Tests: 1 passed, 0 failed, 0 skipped, 0 ignored, 390 filtered out

Recommended Mitigation

        fn convert_to_yang_helper(self: @ContractState, asset_amt: u128) -> Wad {
            let asset: IERC20Dispatcher = self.asset.read();
            let total_yang: Wad = self.get_total_yang_helper(asset.contract_address);

            if total_yang.is_zero() {
                let decimals: u8 = asset.decimals();
                // Otherwise, scale `asset_amt` up by the difference to match `Wad`
                // precision of yang. If asset is of `Wad` precision, then the same
                // value is returned
                fixed_point_to_wad(asset_amt, decimals)
            } else {
-               (asset_amt.into() * total_yang) / get_total_assets_helper(asset).into()
+               let result:u256 = asset_amt.into() * total_yang.into() / total_assets.into();
+               Wad { val:result.try_into().expect('u128')};
            }
        }

Assessed type

Decimal

Lines of code

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

Vulnerability details

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.

1. updated_trove_yang_balances[] is used to update the remaining quantity of trove.
2. 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;
                        }

Assessed type

Error

Lines of code

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

Vulnerability details

Vulnerability details

in caretaker.release() We can release the remaining yang.

        fn release(ref self: ContractState, trove_id: u64) -> Span<AssetBalance> {
            let shrine: IShrineDispatcher = self.shrine.read();
...
            loop {
                match yangs_copy.pop_front() {
                    Option::Some(yang) => {
@>                      let deposited_yang: Wad = shrine.get_deposit(*yang, trove_id);
                        let asset_amt: u128 = if deposited_yang.is_zero() {
                            0
                        } else {
                            let exit_amt: u128 = sentinel.exit(*yang, trove_owner, trove_id, deposited_yang);
                            // Seize the collateral only after assets have been
                            // transferred so that the asset amount per yang in Gate
                            // does not change and user receives the correct amount
                            shrine.seize(*yang, trove_id, deposited_yang);
                            exit_amt
                        };
                        released_assets.append(AssetBalance { address: *yang, amount: asset_amt });
                    },
                    Option::None => { break; },
                };
            };

As above, we can only release the yang that already exists in the trove: shrine.get_deposit(*yang, trove_id);

When shire.get_live() == false, we can no longer perform shire.pull_redistributed_debt_and_yangs(). So if there is any yang that hasn't been pull, it can't be retrieved through caretaker.release() This part of the yang will be locked in the contract.

Impact

After shut(), the redistributed yang that hasn't been pull will be locked in the contract.

Recommended Mitigation

add pull_when_shut()

        fn pull_when_shut(ref self: ContractState, trove_id: u64) {
            if self.is_live.read() {
                return;
            }
           let trove: Trove = self.troves.read(trove_id);

            let current_interval: u64 = now();
            let trove_yang_balances: Span<YangBalance> = self.get_trove_deposits(trove_id);
            let (updated_trove_yang_balances, _) = self
                .pull_redistributed_debt_and_yangs(trove_id, trove_yang_balances,Wad { val: 0 });

            // If there was any exceptional redistribution, write updated yang amounts to trove
            if updated_trove_yang_balances.is_some() {
                let mut updated_trove_yang_balances = updated_trove_yang_balances.unwrap();
                loop {
                    match updated_trove_yang_balances.pop_front() {
                        Option::Some(yang_balance) => {
                            self.deposits.write((*yang_balance.yang_id, trove_id), *yang_balance.amount);
                        },
                        Option::None => { break; },
                    };
                };
            }
            self.trove_redistribution_id.write(trove_id, self.redistributions_count.read());
         }

        fn release(ref self: ContractState, trove_id: u64) -> Span<AssetBalance> {
            let shrine: IShrineDispatcher = self.shrine.read();
...
+           shrine.pull_when_shut(trove_id);
            loop {
                match yangs_copy.pop_front() {
                    Option::Some(yang) => {
                        let deposited_yang: Wad = shrine.get_deposit(*yang, trove_id);

Assessed type

Error

Lines of code

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

Vulnerability details

Vulnerability details

In sentinel.add_yang(), INITIAL_DEPOSIT_AMT was added to prevent attacks.

Require an initial deposit when adding a yang to prevent first depositor from front-running

However, this does not prevent malicious users from donating to artificially increase the price, leading to an underflow in yang_prices.cumulative_price.

For example: after add_yang(eth): totalShares: totalAsset : = 1000:1000

A malicious user donates an asset to the gate, suppose the donation is (1e18-1000 ETH): totalShares: totalAsset : = 1000 : 1e18 exchangeRate = 1e15

Assuming eth = 2000 USD, then the price of yang "sentinel.get_asset_amt_per_yang()" becomes:

get_asset_amt_per_yang() -> convert_to_assets_helper(WAD_ONE.into())

get_asset_amt_per_yang = WAD_ONE * totalAsset / totalShares = 1e18 * 1e15 = 1e33

price = 2000 USD * get_asset_amt_per_yang() = 2e36

cumulative_price is Wad (uint128) and accumulates every TIME_INTERVAL = 30 minutes:

type(uint128).max / 2e36 = 170 (TIME_INTERVAL) = 170 * 30 minutes = 3.5 days

As stated above, if a malicious user maliciously donates to increase the price eth = 1e18 overflow = 3.5 days eth = 0.1e18 overflow = 35 days eth = 0.01e18 overflow = 350 days

Impact

Malicious donations will not cause errors at the beginning, but after a few days, when updating the price, shrine.advance() will fail, making it impossible to update the price, which will cause many problems.

Recommended Mitigation

INITIAL_DEPOSIT_AMT should not be fixed at 1000, it should be passed in by add_yang(), and different values should be specified for different assets to increase the cost of donation.

        fn add_yang(
            ref self: ContractState,
            yang: ContractAddress,
            yang_asset_max: u128,
            yang_threshold: Ray,
            yang_price: Wad,
            yang_rate: Ray,
            gate: ContractAddress,
+          initial_deposit_amt:u128
        ) {

Assessed type

DoS

Findings Summary

[L-01] absorber.provide() needs to clear provider_request to avoid flash loan attacks

The protocol adds a request->remove mechanism to prevent attacks. However, absorber.provide() does not clear request. This allows malicious users to execute a flash loan attack within a single transaction. 1.request() 2. when remove() can execute

• flash loan
• execute absorber.provide() Amplify shares
• trigger equalizer.allocate() to distribute blesser
• execute absorber.remove() get rewards
• repay flash loan

suggest:

        fn provide(ref self: ContractState, amount: Wad) {
            self.assert_live();
+           //force remove request
+           let mut request: Request = self.provider_request.read(provider);
+           request.has_removed = true;
+           self.provider_request.write(provider, request);

[L-02] absorber.convert_to_yin() may lose precision

in absorber.convert_to_yin()

        fn convert_to_yin(self: @ContractState, shares_amt: Wad) -> Wad {
            let total_shares: Wad = self.total_shares.read();

            // If no shares are issued yet, then it is a new epoch and absorber is emptied.
            if total_shares.is_zero() {
                return WadZeroable::zero();
            }

            let absorber: ContractAddress = get_contract_address();
            let yin_balance: Wad = self.yin_erc20().balance_of(absorber).try_into().unwrap();
@>          (shares_amt * yin_balance) / total_shares
        }

Using the formula: (shares_amt * yin_balance) / total_shares = shares_amt.val * yin_balance.val / 1e18 * 1e18 / total_shares.val. Dividing by 1e18 first can lead to precision loss. It is recommended to modify it to not perform / 1e18 * 1e18.

        fn convert_to_yin(self: @ContractState, shares_amt: Wad) -> Wad {
            let total_shares: Wad = self.total_shares.read();

            // If no shares are issued yet, then it is a new epoch and absorber is emptied.
            if total_shares.is_zero() {
                return WadZeroable::zero();
            }

            let absorber: ContractAddress = get_contract_address();
            let yin_balance: Wad = self.yin_erc20().balance_of(absorber).try_into().unwrap();
-           (shares_amt * yin_balance) / total_shares
+           let   result:u256 = (shares_amt.val * yin_balance.val) / total_shares.val
+           Wad { val:result.try_into().expect('u128')}
        }

[L-03] absorber.transfer_assets() lacks a check for whether the return value is true

in absorber.transfer_assets()

        fn transfer_assets(ref self: ContractState, to: ContractAddress, mut asset_balances: Span<AssetBalance>) {
            loop {
                match asset_balances.pop_front() {
                    Option::Some(asset_balance) => {
                        if (*asset_balance.amount).is_non_zero() {
@>                          IERC20Dispatcher { contract_address: *asset_balance.address }
                                .transfer(to, (*asset_balance.amount).into());
                        }
                    },
                    Option::None => { break; },
                };
            };
        }

The above method does not check if the return value is == true. It is recommended to add this check.

        fn transfer_assets(ref self: ContractState, to: ContractAddress, mut asset_balances: Span<AssetBalance>) {
            loop {
                match asset_balances.pop_front() {
                    Option::Some(asset_balance) => {
                        if (*asset_balance.amount).is_non_zero() {
-                            IERC20Dispatcher { contract_address: *asset_balance.address }
-                               .transfer(to, (*asset_balance.amount).into());
+                            let success:bool = IERC20Dispatcher { contract_address: *asset_balance.address }
+                              .transfer(to, (*asset_balance.amount).into());
+                           asset(success,"not success");
                        }
                    },
                    Option::None => { break; },
                };
            };
        }

[L-04] flash_loan().eject does not include FLASH_FEE

Currently, since FLASH_FEE defaults to 0, shrine.eject() does not include it. It is recommended to add it to avoid errors in the future.

        fn flash_loan(
            ref self: ContractState,
            receiver: ContractAddress,
            token: ContractAddress,
            amount: u256,
            call_data: Span<felt252>
        ) -> bool {
..
-           shrine.eject(receiver, amount_wad);
+          shrine.eject(receiver, amount_wad + FLASH_FEE.into());

            if adjust_ceiling {
                shrine.set_debt_ceiling(ceiling);
            }

            self.emit(FlashMint { initiator, receiver, token, amount });

            self.reentrancy_guard.end();

            true
        }

[L-05] update_prices_internal() If force_update == true, PriceUpdateMissed should not occur

If the oracle is cancelled, it may cause force_update == true to not actually update, purger.absorb()->seer.update_prices(). Not forcibly updating prices can cause major problems.

        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
+                              assert(force_update == false, 'force_update is true ,can't miss');
                                self.emit(PriceUpdateMissed { yang: *yang });
                                break;
                            }

[L-06] set_reward() should restrict the asset from being ying

In absorber.cairo, ying is used as an asset. If the reward is also this, it may be mistakenly taken away as an asset. It is recommended to restrict the reward from being ying.

        fn set_reward(ref self: ContractState, asset: ContractAddress, blesser: ContractAddress, is_active: bool) {
            self.access_control.assert_has_role(absorber_roles::SET_REWARD);

            assert(asset.is_non_zero() && blesser.is_non_zero(), 'ABS: Address cannot be 0');
+           assert(asset != self.shrine.read().contract_address,'not ying');