#37354 [SC-Low] Single below MCR trove temporarily blocks redemptions

Submitted on Dec 2nd 2024 at 18:46:28 UTC by @SeveritySquad for IOP | Fluid Protocol

Report ID: #37354
Report Type: Smart Contract
Report severity: Low
Target: https://github.com/Hydrogen-Labs/fluid-protocol/tree/main/contracts/protocol-manager-contract/src/main.sw
Impacts:
- Smart contract unable to operate due to lack of token funds
- Contract fails to deliver promised returns, but doesn't lose value

Description

Brief/Intro

If Trove Manager contract contains a single trove and that trove is below its MCR all redemption attempts will revert until the the Trove is liquidated.

Vulnerability Details

The redemption method redeem_collateral() callable from the Protocol Manager contract attempts to redeem collateral from the lowest collateralized troves (those with lowest CR). It calls get_all_assets_info() which gets lowest CR trove from each asset. Then if that trove is below CR it will try to get the next one until it finds the lowest CR, but still higher than MCR:

        while (current_borrower != null_identity_address() && current_cr < MCR) {
            current_borrower = sorted_troves.get_prev(current_borrower, asset);
            current_cr = trove_manager.get_current_icr(current_borrower, price);
        }

The problem is when there is no more troves available and the get_prev() returns a null identity. The get_current_icr() will then revert because the get_current_icr() calls internal_get_current_icr(), which calls internal_get_entire_debt_and_coll(), which looks like this:

fn internal_get_entire_debt_and_coll(borrower: Identity) -> EntireTroveDebtAndColl {
    let trove = storage.troves.get(borrower).try_read().unwrap_or(Trove::default()); // <- condition handled
    let coll = trove.coll;
    let debt = trove.debt;
    let pending_coll_rewards = internal_get_pending_asset_reward(borrower); // <- issue here
    let pending_debt_rewards = internal_get_pending_usdf_reward(borrower);
    /// [...]
}

The revert will happen on an optimistic attmpt to get a reward_snapshot using a null identity here:

fn internal_get_pending_asset_reward(address: Identity) -> u64 {
    let snapshot_asset = storage.reward_snapshots.get(address).read().asset; // <- reverts here!
    /// [...]
}

Solution Proposal

Add a condition to check if get_prev() returns a null Identity here:

        while (current_borrower != null_identity_address() && current_cr < MCR) {
            current_borrower = sorted_troves.get_prev(current_borrower, asset);
            // if current_borrow is null, break.
            current_cr = trove_manager.get_current_icr(current_borrower, price);
        }

Impact Details

This situation is rare as we can expect that the liquidation will eventually proceed and there will be no troves in that Trove Manager. Hence the situation is only short lived, and the condition for it to happen is very specific. Yet it looks like the condition's logic was not programmed correctly and the system could revert where it shouldn't. As the conditions for it to happen are rare and temporary we give it a Low impact.

References

Reference: https://github.com/Hydrogen-Labs/fluid-protocol/blob/78ab7bdd243b414b424fca6e1eb144218f36a18a/contracts/protocol-manager-contract/src/main.sw#L327

Proof of Concept

Try the following test to see the redemption failure.

#[tokio::test]
async fn redemption_fail_with_single_trove_below_cr() {
    let (contracts, _admin, mut wallets) = setup_protocol(5, true, false).await;

    let healthy_wallet1 = wallets.pop().unwrap();
    let healthy_wallet2 = wallets.pop().unwrap();
    let healthy_wallet3 = wallets.pop().unwrap();

    let balance = 10_000 * PRECISION;

    token_abi::mint_to_id(
        &contracts.asset_contracts[0].asset,
        balance,
        Identity::Address(healthy_wallet1.address().into()),
    )
    .await;

    token_abi::mint_to_id(
        &contracts.asset_contracts[0].asset,
        balance,
        Identity::Address(healthy_wallet2.address().into()),
    )
    .await;

    token_abi::mint_to_id(
        &contracts.asset_contracts[0].asset,
        balance,
        Identity::Address(healthy_wallet3.address().into()),
    )
    .await;

    let borrow_operations_healthy_wallet1 = ContractInstance::new(
        BorrowOperations::new(
            contracts.borrow_operations.contract.contract_id().clone(),
            healthy_wallet1.clone(),
        ),
        contracts.borrow_operations.implementation_id.clone(),
    );

    oracle_abi::set_debug_timestamp(&contracts.asset_contracts[0].oracle, PYTH_TIMESTAMP).await;
    pyth_oracle_abi::update_price_feeds(
        &contracts.asset_contracts[0].mock_pyth_oracle,
        pyth_price_feed(1),
    )
    .await;

    borrow_operations_abi::open_trove(
        &borrow_operations_healthy_wallet1,
        &contracts.asset_contracts[0].oracle,
        &contracts.asset_contracts[0].mock_pyth_oracle,
        &contracts.asset_contracts[0].mock_redstone_oracle,
        &contracts.asset_contracts[