Updating asset collateral params can lead to liquidate borrowers arbitrarily

[c4-bot-3]

Lines of code

https://github.com/code-423n4/2024-07-benddao/blob/main/src/modules/Configurator.sol#L147-L163

Vulnerability details

Description

One of the key concepts in this protocol is Cross Lending when the contract will calculate the health-factor of the account, If it is unhealthy the liquidator can repay the debt on behalf of the borrower and take their collateral assets at a certain discount price.

The protocol has two main factors to calculate the health-factor for users, The collateralFactor and liquidationThreshold values are unique for each pool and asset. Also, The Pool Admin can update them at any time by triggering Configurator.sol#setAssetCollateralParams()

The responsibility for computing health-factor is GenericLogic.sol#calculateUserAccountData(), which Calculates the user data across the reserves.

However, the current logic uses liquidationThreshold value also to check if the asset will not be used as collateral (but still, users can supply/lend it to be borrowed and earn interest)

File: GenericLogic.sol#calculateUserAccountData()

      if (currentAssetData.liquidationThreshold != 0) {
       /***/
        result.totalCollateralInBaseCurrency += vars.userBalanceInBaseCurrency;
       /***/
      }

The issue is the PoolAdmin can call Configurator.sol#setAssetCollateralParams() at any time to disable accepting an asset as collateral by updating collateralFactor and liquidationThreshold to zero value. So, users suddenly will get liquidated directly by MEV bots.

Impact

• The Pool Admin can update the asset collateral params, this will leave borrowers with bad debt and allow the liquidators to liquidate all the loans.
• MEV bots will just back-run the setAssetCollateralParams() transaction to liquidate users, They have no chance to update their positions.

Proof of Concept

Foundry PoC:

• Please copy the following POC in TestIntCrossLiquidateERC20.t.sol

  
  function test_Should_LiquidateUSDT_executeSetAssetCollateralParams() public {
  prepareUSDT(tsDepositor1);
  //? 1- borrower supply tsWETH as collateral
  prepareWETH(tsBorrower1);
  
  TestUserAccountData memory accountDataBeforeBorrow = getUserAccountData(address(tsBorrower1), tsCommonPoolId);
  
  // borrow some eth
  uint8[] memory borrowGroups = new uint8[](1);
  borrowGroups[0] = tsLowRateGroupId;
  
  uint256[] memory borrowAmounts = new uint256[](1);
  uint256 usdtCurPrice = tsPriceOracle.getAssetPrice(address(tsUSDT));
  borrowAmounts[0] = (accountDataBeforeBorrow.availableBorrowInBase * (10 ** tsUSDT.decimals())) / usdtCurPrice;
  
  //? 2- the borrower call `crossBorrowERC20()` to borrow `tsUSDT`
  actionCrossBorrowERC20(
    address(tsBorrower1),
    tsCommonPoolId,
    address(tsUSDT),
    borrowGroups,
    borrowAmounts,
    new bytes(0)
  );
  
  // make some interest
  advanceTimes(1 days);
  
  //? 3- set `liquidationThreshold` to zero
  tsHEVM.startPrank(tsPoolAdmin);
  tsConfigurator.setAssetCollateralParams(tsCommonPoolId, address(tsWETH), 0, 0, 0);
  tsHEVM.stopPrank();
  
  //? 4- liquidator call `crossLiquidateERC20()` with `collateralAsset == tsWETH`
  tsLiquidator1.approveERC20(address(tsUSDT), type(uint256).max);
  uint daiBlanceBefore = ERC20(address(tsWETH)).balanceOf(address(tsLiquidator1));
  
  tsHEVM.prank(address(tsLiquidator1));
  tsCrossLiquidation.crossLiquidateERC20(
    tsCommonPoolId,
    address(tsBorrower1),
    address(tsWETH),
    address(tsUSDT),
    borrowAmounts[0],
    false
  );
  
  uint daiBlanceAfter = ERC20(address(tsWETH)).balanceOf(address(tsLiquidator1));
  
  assertGt(daiBlanceAfter, daiBlanceBefore);
  }

## Tools Used

Manual Review

## Recommended Mitigation Steps

In case the Pool Admin decides to no longer accept an asset as collateral, Users who are using this asset as collateral should have a period of time to update their loans.
You can use a buffer period or lock time to do this critical update 

## Assessed type

Other

[c4-judge]

MarioPoneder marked the issue as primary issue

[MarioPoneder]

Seems like a valid concern, @thorseldon could you please provide more context concerning you dispute?

[MarioPoneder]

After a second thought, this seems to fall below Centralisation Risk on Contracts which has Owner or Administrator. according to the README.

[c4-judge]

MarioPoneder marked the issue as unsatisfactory: Invalid

[MarioPoneder]

After a third thought, this is exceeding centralization risks, since it's never a good time to change asset collateral params. The params should be cached for ongoing borrows.

[c4-judge]

MarioPoneder marked the issue as satisfactory

[c4-judge]

MarioPoneder marked the issue as selected for report

[thorseldon]

This a parameter configuration which controlled by the DAO governance, any parameters like liquidation threshold should be carefully discussed and reviewed the community and dev team.

We will use timelock to schedule the configuration updating if the proposal voting is passed.

And this finding looks like same with https://github.com/code-423n4/2024-07-benddao-findings/issues/25.

[MarioPoneder]

I understand the centralization concern of this issue is out of scope and changes are carefully reviewed.
However, on a contract level:

• Even carefully considered changes do directly affect open borrow positions. Therefore, there is never/seldom a good time to change these values.
• It is not ensured on a contract level that e.g. liquidation threshold changes can only happen in favor of the users and not against them.

For these reasons, Medium severity seems justified.