Opus - kodyvim's results

Lines of code

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

Vulnerability details

Impact

This would lead to users not having to pay for redistributed debt when they withdraw and also major discrepancies between the total debts of the trove and its deposits balance.

Proof of Concept

When users perform actions that changes the balance of there trove, interest is accrued or accumulated on the trove as debt, The issue is that when a user tries to withdraw the interest is accrued after reading their balance. https://github.com/code-423n4/2024-01-opus/blob/main/src/core/shrine.cairo#L1379

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);//@audit charge trove after reading balance

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

within charge() the interest accumulated is added as debt to the trove and the yang balance is also updated when their a redistribution but its immediately overridden by the cache variable defined before the call to charge() within withdraw_helper(). https://github.com/code-423n4/2024-01-opus/blob/main/src/core/shrine.cairo#L1400

fn charge(ref self: ContractState, trove_id: u64) {
            
            ...SNIP
            // Pull undistributed debt and update state
            let trove_yang_balances: Span<YangBalance> = self.get_trove_deposits(trove_id);
            let (updated_trove_yang_balances, compounded_trove_debt_with_redistributed_debt) = self
                .pull_redistributed_debt_and_yangs(trove_id, trove_yang_balances, compounded_trove_debt);

            // 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);//@audit users deposits are updated when any redistributed debt
                        },
                        Option::None => { break; },
                    };
                };
            } 
		...SNIP
		// Add the interest charged on the trove's debt to the total troves' debt and
            // budget only if there is a change in the trove's debt. This should not include
            // redistributed debt, as that is already included in the total.
            if charged.is_non_zero() {
                let new_total_troves_debt: Wad = self.total_troves_debt.read() + charged;
                self.total_troves_debt.write(new_total_troves_debt);<@
                self.adjust_budget_helper(charged.into());
                self.emit(TotalTrovesDebtUpdated { total: new_total_troves_debt });
            }
	...SNIP       
        }

if there's any redistribution of debt the yang balances are updated also the total_debts of the trove is also updated but this updated balance would be overridden and not accounted for, because this line within withdraw would override the balance with the cached balance before the call to charge() is made.

Tools Used

Manual Review

Recommended Mitigation Steps

call to self.charge() should be made before reading users deposits during withdraw.

fn withdraw_helper(ref self: ContractState, yang: ContractAddress, trove_id: u64, amount: Wad) {
+           self.charge(trove_id);
            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 });
        }

Assessed type

Context