Opus - minhquanym's results

Lines of code

https://github.com/code-423n4/2024-01-opus/blob/04583e0411dbf8027952d668a8678fda0cb5b160/src/core/shrine.cairo#L1379-L1397

Vulnerability details

Impact

The withdraw_helper() function is an internal function called by the external withdraw() function to handle trove accounting. This function reads the current trove_balance from storage and calculates the new_trove_balance and new_total values. However, these calculations occur before the charge() function is called.

The charge() function is used to accrue any interest from the last interacted timestamp to the current timestamp. It also pulls any redistribution debts and yangs. During this process, the yang balance of troves might be updated due to exceptional redistribution.

After calling charge(), new_total and new_trove_balance are written to storage. But, these values may be outdated if the trove balance was updated in the charge() function. As a result, the stored values could be incorrect, potentially causing user losses.

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

    let new_trove_balance: Wad = trove_balance - amount;
    let new_total: Wad = self.yang_total.read(yang_id) - amount;
  
    // @audit Need to charge first, `trove_balance` might change after `charge()` 
    self.charge(trove_id); 
  
    // @audit `new_trove_balance` and `new_total` is calculated before `charge()`, it is outdated but still written to storage.
    self.yang_total.write(yang_id, new_total);
    self.deposits.write((yang_id, trove_id), new_trove_balance);
    ...
}

Proof of Concept

Consider the following scenario:

1. Only Alice and Bob deposited yang X into Shrine. Alice's trove has MIN_RECIPIENT_POOL_YANG - 1 yang X.
2. There is a redistribution of yang X from Bob's trove. Since only Alice and Bob deposited X, the redistributed_yang_recipient_pool will be MIN_RECIPIENT_POOL_YANG - 1, resulting in an exceptional redistribution.
3. Alice calls withdraw to remove amount = MIN_RECIPIENT_POOL_YANG - 1 X that she deposited into her pool.
4. The withdraw_helper function calculates new_trove_balance = 0. The charge() function then pulls the exceptional distribution and increases Alice's yang X trove balance. However, after that, new_trove_balance = 0 is still written to storage, meaning the amount of X redistributed to Alice's trove is lost forever.

Tools Used

Manual Review

Recommended Mitigation Steps

Call the charge() function before any calculation in the withdraw_helper() function.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-01-opus/blob/04583e0411dbf8027952d668a8678fda0cb5b160/src/core/shrine.cairo#L1752-L1755

Vulnerability details

The function redistribute_helper() in the Opus codebase may skip over an empty yang during the redistribution process. This can cause an incorrect update to the yang balances within a trove.

Impact

The redistribute_helper() function iterates over each yang in a trove for redistribution. During this process, it maintains two lists: new_yang_totals and updated_trove_yang_balances. These lists record the new total yang and new trove yang balance after each distribution, respectively. Both lists should have the same index, meaning index i in each list should refer to the same yang.

At the end of the function, the values in these two lists are used to write to storage:

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; },
    };
};

As you can see, each time new_yang_totals pops an element, updated_trove_yang_balances also pops one element to write to storage. If new_yang_totals.len() < updated_trove_yang_balances.len(), some of the last updated_trove_yang_balances won't be updated. This can cause incorrect accounting for the trove following liquidation.

Specifically, if there are three yangs A, B, and C, and the balance of B is zero, new_yang_totals will only contain values for [A, C], while updated_trove_yang_balances contains all values for all [A, B, C]. During the writing to storage, updated_trove_yang_balances of C won't be processed.

Proof of Concept

The scenario described in the Impact section can occur when skipping an empty yang:

// Skip over this yang if it has not been deposited in the trove
// @audit `new_yang_totals` is not appended
if trove_yang_amt.is_zero() { 
    updated_trove_yang_balances.append(*yang_balance);
    continue;
}

During the loop through the yangs of the trove, if trove_yang_amt is zero, the function will skip over it and append the same trove balance to updated_trove_yang_balances. However, nothing is added to new_yang_totals. This can cause a mismatch between these two lists. In the end, the length of new_yang_totals would be smaller than updated_trove_yang_balances, causing some of the last values in updated_trove_yang_balances not to be written to storage.

Tools Used

Manual Review

Recommended Mitigation Steps

To mitigate this issue, append to new_yang_totals when skipping over a yang.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-01-opus/blob/04583e0411dbf8027952d668a8678fda0cb5b160/src/core/absorber.cairo#L441

Vulnerability details

Impact

In the Opus protocol, the absorber is a stability pool that permits yin holders to contribute their yin and participate in liquidations (also known as absorptions) as a consolidated pool. Provided yin from users could be absorbed during an absorption. In return, users receive yang, which usually has a higher value than the absorbed yin. However, in the event of bad debt, it could be less. This situation could lead to risk-free yield frontrunning tactics, where an attacker only provides yin when the absorption brings profit then removing all yin right after that.

The Opus team is aware of this attack vector and has therefore implemented a request-withdraw procedure. Users must first request a withdrawal and wait for a certain period before executing the withdrawal.

However, the provide() method does not reset these request timestamps, which allows an attacker to bypass this safeguard.

Proof of Concept

The attacker's tactics will be:

1. Deposit a small amount of yin into 100 of his accounts. Note that the total value of these small amounts is negligible. Also please note that the value 100 is just arbitrary value to describe the idea. It could be higher or lower depending on the REQUEST_BASE_TIMELOCK and REQUEST_VALIDITY_PERIOD.
2. Regularly request withdrawals for these accounts to ensure that at least one account has a valid request at any given time. This can be easily achieved by requesting a withdrawal from the 1st account at timestamp X, the 2nd at X + REQUEST_VALIDITY_PERIOD, the 3rd at X + 2 * REQUEST_VALIDITY_PERIOD, and so on. The 1st account request will expire at X + REQUEST_BASE_TIMELOCK + REQUEST_VALIDITY_PERIOD, but now 2nd request account become valid because it has requested at X + REQUEST_VALIDITY_PERIOD.
3. Since the provide() function does not reset the request, the attacker can simply select one account with a valid removing request and perform the "risk-free yield tactics". He does this by providing a large amount of yin to this account to earn yield and then immediately calling remove().

Tools Used

Manual Review

Recommended Mitigation Steps

Reset the withdrawal request in the provide() function.

Assessed type

MEV

Lines of code

https://github.com/code-423n4/2024-01-opus/blob/04583e0411dbf8027952d668a8678fda0cb5b160/src/core/abbot.cairo#L191-L200

Vulnerability details

Impact

In a redistribution, the unhealthy trove's collateral and debt is distributed among troves proportionally to its collateral composition. If no other troves has deposited a yang that is to be redistributed, then the debt and value attributed to that yang will be redistributed among all other yangs in the system according to their value proportionally to the total value of all remaining yangs in the system.

However, the Opus protocol allows anyone to deposit into any trove, even if they are not the trove's owner, as seen in the function abbot.deposit(). This feature could potentially be exploited by an attacker. For instance, if a yang is only deposited in one trove that's being redistributed, the attacker could deposit a small amount into a victim's trove. This could result in all bad debt being redistributed to the victim's trove instead of exceptional redistribution, even if the victim didn't want this (i.e., they didn't deposit this yang into their trove).

fn deposit(ref self: ContractState, trove_id: u64, yang_asset: AssetBalance) {
    // There is no need to check the yang address is non-zero because the
    // Sentinel does not allow a zero address yang to be added.

    assert(trove_id != 0, 'ABB: Trove ID cannot be 0');
    assert(trove_id <= self.troves_count.read(), 'ABB: Non-existent trove');
    // note that caller does not need to be the trove's owner to deposit
    // @audit Attacker could deposit for any trove to make it the only trove deposited this yang and being redistributed when bad debt happen
    self.deposit_helper(trove_id, get_caller_address(), yang_asset);
}

Proof of Concept

Consider the following scenario:

1. Assume there is a highly volatile yang X added to Opus. Alice, aware of the risk of X, does not deposit any amount of X into her trove.
2. Bob, the attacker, has deposited some X into his trove.
3. When Bob's trove is about to be redistributed due to bad debt, since no other trove has deposited X, the debt should be redistributed among all other yangs (exceptional redistribution). This means everyone would share the loss of the bad debt. However, Bob could bypass this by depositing some X into Alice's trove. Since the deposit function doesn't require the caller to be the trove's owner, Alice's trove now holds some X. As a result, a regular redistribution will occur, and all of Bob's bad debt will be redistributed to Alice's trove.

Tools Used

Manual Review

Recommended Mitigation Steps

Limit deposits to only the trove's owner.

Assessed type

Other