HonorLt - Missing or not entirely correct validations

[github-actions]

HonorLt

medium

Missing or not entirely correct validations

Summary

I have noticed several validations missing or not robust enough in the protocol that could impact the behavior and lead to unexpected results. I have grouped all of them together in this one submission.

Vulnerability Detail

1. addCollateralType and queueCollateralChange should validate the _interestPer3Min >= DIVISION_BASE (1e18), otherwise the virtualPrice will always be 0 (or impossible to update):

  collateral.virtualPrice = (collateral.virtualPrice * collateral.interestPer3Min) / DIVISION_BASE;

2. _checkDailyMaxLoans should be <=:

  require( dailyTotal  < dailyMax, "Try again tomorrow loan opening limit hit");

3. closeLoan should be >=:

  require(colInUSD > borrowMargin , "Remaining debt fails to meet minimum margin!");

4. getOraclePrice price boundaries should be inclusive <= and >=:

        require(signedPrice < _maxPrice, "Upper price bound breached");
        require(signedPrice > _minPrice, "Lower price bound breached");

5. getOraclePrice does not validate stale round require(answeredInRound >= roundID, "Stale round");:

        (
            /*uint80 roundID*/,
            int signedPrice,
            /*uint startedAt*/,
            uint timeStamp,
            /*uint80 answeredInRound*/
        ) = _priceFeed.latestRoundData();

6. vaultUpdateVirtualPriceAndTime does not verify the invariant mentioned in the docs:
   "Vaults should only be able to interact with the collateral relating to their AssetType."

    function vaultUpdateVirtualPriceAndTime(
        address _collateralAddress,
        uint256 _virtualPriceUpdate,
        uint256 _updateTime
    ) external onlyVault collateralExists(_collateralAddress){
        _updateVirtualPriceAndTime(_collateralAddress, _virtualPriceUpdate, _updateTime);
    }

should have something like this:

  require(msg.sender == vaults[collateralProps[_collateralAddress].assetType], "...");

7. addCollateralType does not check if liquidityPoolOf[_currencyKey] already exists, it always overrides with a new one:

  liquidityPoolOf[_currencyKey]= _liquidityPool;

8. callLiquidation could have a slippage control parameter where liquidators can specify what minimum reward amount satisfies them.

9. Synths closeLoan does not validate that if not all the loan is repaid, the remaining USD borrowed is >= ONE_HUNDRED_DOLLARS.

10. closeLoan and callLiquidation in Velo vault does not validate that _loanNFTs entries are unique (not repeated).

Impact

All these missing validations could result in a protocol misbehaving if these edge conditions are triggered.

For example, if the virtual price becomes 0, it will be impossible to open new or close existing loans or even liquidate because the interest calculation will revert (division by 0):

  isoUSDLoanAndInterest[_collateralAddress][msg.sender] + ((_USDborrowed * LOAN_SCALE) / virtualPrice);

Code Snippet

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

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

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

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

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

https://github.com/sherlock-audit/2022-11-isomorph/blob/main/contracts/Isomorph/contracts/Vault_Lyra.sol#L234

https://github.com/sherlock-audit/2022-11-isomorph/blob/main/contracts/Isomorph/contracts/Vault_Synths.sol#L226

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

https://github.com/sherlock-audit/2022-11-isomorph/blob/main/contracts/Velo-Deposit-Tokens/contracts/DepositReceipt_Base.sol#L157-L181

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

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

Tool used

Manual Review

Recommendation

The code should be robust enough to handle every possible case that can be anticipated. Consider applying the aforementioned validations and improvements.

Duplicate of #200

[pauliax]

Escalate for 5 USDC.

Number 5 is a duplicate of #200.
As you can see an introduced fix added exactly this line:
kree-dotcom/Velo-Deposit-Tokens@5c656e7

[sherlock-admin]

Escalate for 5 USDC.

Number 5 is a duplicate of #200.
As you can see an introduced fix added exactly this line:
kree-dotcom/Velo-Deposit-Tokens@5c656e7

You've created a valid escalation for 5 USDC!

To remove the escalation from consideration: Delete your comment.
To change the amount you've staked on this escalation: Edit your comment (do not create a new comment).

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[hrishibhat]

Escalation accepted

Although issue 5 is valid, multiple issues should not be combined into a single issue.
Escalations of such nature may not be considered valid in future.

[sherlock-admin]

Escalation accepted

Although issue 5 is valid, multiple issues should not be combined into a single issue.
Escalations of such nature may not be considered valid in future.

This issue's escalations have been accepted!

Contestants' payouts and scores will be updated according to the changes made on this issue.