HonorLt - Changes in system parameters might hurt existing users

[github-actions]

HonorLt

medium

Changes in system parameters might hurt existing users

Summary

There are certain problems regarding the system parameters. An admin must be very careful when making parameter changes not to trigger certain problems. I have described a group of several potential problems below.

Vulnerability Detail

1. When the protocol is paused, users can't increase their collateral. And when it is unpaused later, their positions can be liquidated if the collateral value has fallen below the threshold during the pause period. Usually, the liquidators are faster with automated bots leaving practically 0 chances for normal users to top up their collateral before it is too late.
   A suggestion is to add a delay after the unpause when the callLiquidation can be invoked, e.g. 1 hour after unpause.

2. A similar problem is when the collateral is set to not exist or inactive, the users can't manage their existing positions and these positions cannot be liquidated. A big problem would be if, for example, an admin removes existing collateral, users can't increase their collaterals, the market price of collateral shifts not in favor of the user, and later when this collateral is set to active again, the user positions can be liquidated.
   A suggestion is to apply _collateralExists only when opening new loans, but let old loans be closed or liquidated even when collateral is paused.

3. Setting a treasury is a 2-step process. First admin has to propose a new address and after the 3 days delay confirm it:

    /// @notice admin only function to queue treasury address change which must wait the timelock period before being implemented
    function proposeTreasury(address _newTreasury) external onlyAdmin {
        require(_newTreasury != address(0)); 
        pendingTreasury = _newTreasury;
        updateTreasuryTimestamp = block.timestamp + VAULT_TIME_DELAY;
    }

However, the treasury can be changed even when the protocol is paused, leaving a gap to escape this precaution. Admin can pause the vault, then update the settings, wait 3 days, then unpause and profit.
A suggestion is that the treasury might allow change only when the protocol is not paused.

Impact

A change in critical system parameters might unexpectedly hurt legitimate users.

Code Snippet

https://github.com/sherlock-audit/2022-11-isomorph/blob/main/contracts/Isomorph/contracts/Vault_Base_ERC20.sol#L115-L120

https://github.com/sherlock-audit/2022-11-isomorph/blob/main/contracts/Isomorph/contracts/Vault_Velo.sol#L166-L171

https://github.com/sherlock-audit/2022-11-isomorph/blob/main/contracts/Isomorph/contracts/Vault_Base_ERC20.sol#L84-L95

https://github.com/sherlock-audit/2022-11-isomorph/blob/main/contracts/Isomorph/contracts/CollateralBook.sol#L181-L195

Tool used

Manual Review

Recommendation

The code should be hardened to prevent or minimize the chances of such accidents happening. Consider applying the aforementioned suggestions.

Duplicate of #57

[kree-dotcom]

Duplicate of issues #57 and #38 for points 1 and 2. Impact of point 3 about the treasury is disputed, yes the admin address can alter the treasury address, this is intended and has no impact on the rest of the system.