Nihavent - [M-1] Accounts can be liquidated when deposits are paused

[sherlock-admin2]

Nihavent

medium

[M-1] Accounts can be liquidated when deposits are paused

[M-1] Accounts can be liquidated when deposits are paused, resulting in unwarrented liquidations.

Summary

The RegistryGuardian address has the ability to pause deposits and withdrawals. Despite this role being a trusted position, when the protocol is paused users (accounts) are unable to manage their positions by depositing and withdrawing collateral. In the event that deposits are paused, and an accounts' asset valuation decreases below openDebt + minimumMargin, this user becomes liquidatable. Under normal circumstances, some of these users may have deposited more collateral, or closed their open positions before they were liquidatable, which means some of these liquidations are unwarrented.

Vulnerability Detail

When the RegistryGuardian::pause() function is paused, both deposits and withdrawals are paused until the protocol is either unpaused by the guardian or 30 days has elapsed:

https://github.com/sherlock-audit/2023-12-arcadia/blob/main/accounts-v2/src/guardians/RegistryGuardian.sol#L63-L66

When deposits and withdrawals are paused users cannot manage their open positions. These users may be subject to liquidation, even if they intend to deposit funds to protect against liquidation.

Impact

Users may be subject to liquidation if the protocol is paused in unfavourable market conditions. These liquidations may be unwarrented and place user funds at risk.

Code Snippet

Copy the following into a new file in Accounts-V2/test/accounts/AccountV1/

Then run forge test --mt test_startLiquidationWhenDepositsPaused

/**
 * Created by Pragma Labs
 * SPDX-License-Identifier: BUSL-1.1
 */
pragma solidity 0.8.22;

import { StdStorage, stdStorage } from "../../../../lib/forge-std/src/Test.sol";
import { AccountV1_Fuzz_Test, AccountErrors } from "./_AccountV1.fuzz.t.sol";
import { console2 } from "lib/forge-std/src/console2.sol";
import { AccountExtension, AccountV1 } from "../../../utils/Extensions.sol";
import { ICreditor } from "../../../../src/interfaces/ICreditor.sol";
import { AssetValuationLib, AssetValueAndRiskFactors } from "../../../../src/libraries/AssetValuationLib.sol";
import { AssetValuationLibExtension } from "../../../utils/Extensions.sol";
import { RegistryGuardianExtension } from "../../../utils/Extensions.sol";

contract startLiquidationWhenDepositsPaused_AccountV1_Fuzz_Test is AccountV1_Fuzz_Test {
    using stdStorage for StdStorage;

    /* ///////////////////////////////////////////////////////////////
                             VARIABLES
    /////////////////////////////////////////////////////////////// */

    AccountExtension internal accountExtension2;
    AssetValuationLibExtension internal assetValuationLib;
    RegistryGuardianExtension internal registryGuardian;

    struct Flags {
        bool withdrawPaused;
        bool depositPaused;
    }

    /* ///////////////////////////////////////////////////////////////
                              SETUP
    /////////////////////////////////////////////////////////////// */

    function setUp() public override {
        AccountV1_Fuzz_Test.setUp();

        vm.prank(users.accountOwner);
        accountExtension2 = new AccountExtension(address(factory));

        assetValuationLib = new AssetValuationLibExtension();
        vm.stopPrank();

        //Registry guardian setup
        vm.startPrank(users.creatorAddress);
        registryGuardian = new RegistryGuardianExtension();
        registryGuardian.changeGuardian(users.guardian);
        vm.stopPrank();

    }

    /*//////////////////////////////////////////////////////////////
                              TESTS
    //////////////////////////////////////////////////////////////*/

    function test_startLiquidationWhenDepositsPaused() public {

        //Setup parameters so a user can be liquidated
        uint96 minimumMargin = 1;
        uint256 openDebt = 42e32; // arbitrary large number to meet liquidation condition (openDebt + minimumMargin > assetValuationLib.calculateLiquidationValue(assetAndRiskValues))
        uint112 depositAmountToken1 =  type(uint112).max - 1;
        address liquidationInitiator = makeAddr("liquidationInitiator");

        // Pauses are only available 32 days since last pause or deployment
        uint32 time = 32 days + 1;

        AssetValueAndRiskFactors[] memory assetAndRiskValues;
        {
            address[] memory assetAddresses = new address[](1);
            assetAddresses[0] = address(mockERC20.token1);

            uint256[] memory assetIds = new uint256[](1);
            assetIds[0] = 0;

            uint256[] memory assetAmounts = new uint256[](1);
            assetAmounts[0] = depositAmountToken1;

            accountExtension2.initialize(users.accountOwner, address(registryExtension), address(creditorToken1));
            accountExtension2.setMinimumMargin(minimumMargin);
            creditorToken1.setOpenPosition(address(accountExtension2), openDebt);
            stdstore.target(address(factory)).sig(factory.isAccount.selector).with_key(address(accountExtension2))
                .checked_write(true);

            // Update updatedAt to avoid InactiveOracle() reverts.
            vm.startPrank(users.defaultTransmitter);
            mockOracles.token1ToUsd.transmit(int256(rates.token1ToUsd));

            assetAndRiskValues = registryExtension.getValuesInNumeraire(
                accountExtension2.numeraire(), accountExtension2.creditor(), assetAddresses, assetIds, assetAmounts
            );

            // Given : Liquidation value is smaller than used margin
            console2.log(openDebt + minimumMargin);
            console2.log(assetValuationLib.calculateLiquidationValue(assetAndRiskValues));

            // Mint and approve stable1 tokens
            vm.startPrank(users.tokenCreatorAddress);
            mockERC20.token1.mint(users.accountOwner, depositAmountToken1);
            vm.startPrank(users.accountOwner);
            mockERC20.token1.approve(address(accountExtension2), type(uint256).max);

            // Deposit stable1 token in account
            accountExtension2.deposit(assetAddresses, assetIds, assetAmounts);
            vm.stopPrank();
        }

        // Warp time
        vm.warp(time);

        // The registry guardian pauses
        vm.startPrank(users.guardian);
        registryGuardian.pause(); // This necessarily pauses both deposits and withdrawals

        // Update updatedAt to avoid InactiveOracle() reverts.
        vm.startPrank(users.defaultTransmitter);
        mockOracles.token1ToUsd.transmit(int256(rates.token1ToUsd));

        // When: The liquidator initiates a liquidation
        vm.startPrank(accountExtension2.liquidator());
        (
            address[] memory assetAddresses_,
            uint256[] memory assetIds_,
            uint256[] memory assetAmounts_,
            address creditor_,
            uint96 minimumMargin_,
            uint256 totalOpenDebt,
            AssetValueAndRiskFactors[] memory assetAndRiskValues_
        ) = accountExtension2.startLiquidation(liquidationInitiator);
        vm.stopPrank();

        // Then: Account should be liquidatable and return specific values.
        assertEq(accountExtension2.inAuction(), true);
        assertEq(assetAddresses_[0], address(mockERC20.token1));
        assertEq(assetIds_[0], 0);
        assertEq(assetAmounts_[0], mockERC20.token1.balanceOf(address(accountExtension2)));
        assertEq(creditor_, accountExtension2.creditor());
        assertEq(minimumMargin_, minimumMargin);
        assertEq(totalOpenDebt, ICreditor(accountExtension2.creditor()).getOpenPosition(address(accountExtension2)));
        assertEq(assetAndRiskValues_[0].assetValue, assetAndRiskValues[0].assetValue);
        assertEq(assetAndRiskValues_[0].collateralFactor, assetAndRiskValues[0].collateralFactor);
        assertEq(assetAndRiskValues_[0].liquidationFactor, assetAndRiskValues[0].liquidationFactor);

        // And: lastActionTimestamp is updated.
        assertEq(accountExtension2.lastActionTimestamp(), time);
    }
}

Tool used

Manual Review

Recommendation

Allow users to manage positions when protocol is paused. One way to achieve this is by increasing the flexibility of the RegistryGuardian::pause() to pause withdrawals but not pause deposits. This would allow users to ensure they avoid unwanted liquidations in a market downturn.

[sherlock-admin2]

1 comment(s) were left on this issue during the judging contest.

takarez commented:

invalid: the liquidation will also be paused in that case; so invalid

[nevillehuang]

Invalid based on sherlock rules

3. An admin action can break certain assumptions about the functioning of the code. Example: Pausing a collateral causes some users to be unfairly liquidated or any other action causing loss of funds. This is not considered a valid issue.