VaultLiquidationFacet Manual Review Findings

VLF-01M: Unknown Integration Point

Type	Severity	Location
External Call Validation		VaultLiquidationFacet.sol:L26

Description:

The referenced statement represents an integration point with an IBalancer contract that is not in scope of this engagement.

Example:

Copy
26_s.userInfo[user].deposit += _s.balancer.fullWithdraw(asset, user);

Recommendation:

We advise the 0xPhase team to adequately validate the referenced integration point and to potentially include the contract in scope of the audit if it comprises part of the 0xPhase protocol's internal architecture.

Alleviation (3dd3d7bf0c2693b2f9c23bacedfa420393f7ea84):

The implementation of the balancer has been audited in full, rendering this exhibit alleviated.

VLF-02M: Accuracy-Loss Prone Convoluted Mathematical Operations

Type	Severity	Location
Mathematical Operations		VaultLiquidationFacet.sol:L176-L182

Description:

The referenced mathematical calculations are overly convoluted at no benefit as they utilize multiple value literals and perform a complex sequence of calculations in one statement, rendering the calculations prone to arithmetic overflows.

Impact:

The current calculations do not appear to be correct and suffer from potential interim overflows. The liquidation amount that should be performed must be equal to the amount necessary to bring back a user to their health factor. To achieve this, the collateral ratio as well as the target health factor need to be considered and the delta to be used as a debtChange. The _s.liquidationFee should be applied after the calculation concludes.

Example:

Copy
176debtChange = ((1 ether *
177  (debt *
178    1 ether ** uint256(2) -
179    (collateral * targetHealth * maxCollateralRatio))) /
180  (1 ether ** uint256(3) -
181    (targetHealth * maxCollateralRatio * 1 ether) -
182    (_s.liquidationFee * targetHealth * maxCollateralRatio)));

Recommendation:

We advise the calculations to be broken down to multiple statements, greatly enhancing the legibility of the codebase while minimizing the numerical accuracy that interim values should possess.

Alleviation (3dd3d7bf0c2693b2f9c23bacedfa420393f7ea84):

The 0xPhase team evaluated this exhibit and while stating that the calculation performed as expected, they did acknowledge that it was getting close to the maximum value of a uint256.

They have since refactored the calculation and split it into multiple segments that remain in more acceptable value ranges and thus are performed safely. As a result, we consider this exhibit fully alleviated.

VLF-03M: Loss of Arithmetic Accuracy

Type	Severity	Location
Mathematical Operations		VaultLiquidationFacet.sol:L98, L201-L205

Description:

The VaultLiquidationFacet::_withoutFee function does not represent the inverse of VaultLiquidationFacet::_withFee as Solidity suffers from mathematical truncation in divisions. As a result, the VaultLiquidationFacet::liquidationInfo function which assumes the return of VaultLiquidationFacet::_withoutFee will be equivalent to the original amount can potentially be incorrect and yield a lower amount than expected.

Impact:

A deviation of the pureChange value even by one unit may cause a fee to be applied that exceeds the actual one the liquidation should apply. As such, it can cause the whole liquidation to not execute which can cause significant complications in the health of the vault.

Example:

Copy
80/// @inheritdoc IVaultLiquidation
81function liquidationInfo(
82  uint256 user
83) public view override returns (LiquidationInfo memory) {
84  if (_isSolvent(user)) {
85    return LiquidationInfo(true, 0, 0, 0, 0);
86  }
87
88  (uint256 borrowChange, uint256 collateralChange) = _liquidationAmount(user);
89
90  if (borrowChange == 0 || collateralChange == 0) {
91    return LiquidationInfo(true, 0, 0, 0, 0);
92  }
93
94  uint256 tPrice = _price();
95  uint256 userDeposit = _deposit(user);
96  uint256 collateralValue = _scaleFromAsset(userDeposit * tPrice) / 1 ether;
97  uint256 cappedChange = MathLib.min(collateralValue, collateralChange);
98  uint256 pureChange = _withoutFee(cappedChange);
99  uint256 totalFee = cappedChange - pureChange;
100  uint256 protocolFee = _scaleToAsset(
101    (totalFee * _treasuryLiquidationFee()) / 1 ether
102  );
103
104  uint256 realTokens = MathLib.min(
105    userDeposit,
106    _scaleToAsset((cappedChange * 1 ether) / (tPrice))
107  );
108
109  uint256 rebate = 0;
110
111  if (pureChange > collateralValue) {
112    rebate = MathLib.min(
113      _s.treasury.tokenBalance(VaultConstants.REBATE_CAUSE, address(_s.cash)),
114      pureChange - collateralValue
115    );
116  }
117
118  return
119    LiquidationInfo(false, borrowChange, realTokens, protocolFee, rebate);
120}

Recommendation:

We advise the actual fee to be calculated and subtracted from the amount that is set as input to the VaultLiquidationFacet::_withoutFee function, guaranteeing that the amount without fee represents the correct truncation-safe amount.

Alleviation (3dd3d7bf0c2693b2f9c23bacedfa420393f7ea84):

The liquidation calculations within VaultLiquidationFacet::liquidationInfo were reworked to ensure truncation is accounted for, preventing edge conditions from rendering a liquidation inoperable. As such, we consider this exhibit fully alleviated albeit with a different approach than the one we recommended.

VLF-04M: Incorrect Rebate Calculation Mechanism

Type	Severity	Location
Logical Fault		VaultLiquidationFacet.sol:L111-L116

Description:

The rebate mechanism will fail to execute as expected under all liquidation scenarios as it calculates the pureChange incorrectly. In detail, the pureChange is calculated from the cappedChange minus the fee (i.e. guaranteeing that pureChange <= cappedChange) and the cappedChange is in turn calculated as the minimum between collateralValue and collateralChange (i.e. guaranteeing that cappedChange <= collateralValue).

Impact:

The rebate mechanism will fail to execute as expected, causing unhealthy positions that are expected to be liquidated with a rebate to not acquire any rebate at all further magnifying the protocol's low health factor.

Example:

Copy
80/// @inheritdoc IVaultLiquidation
81function liquidationInfo(
82  uint256 user
83) public view override returns (LiquidationInfo memory) {
84  if (_isSolvent(user)) {
85    return LiquidationInfo(true, 0, 0, 0, 0);
86  }
87
88  (uint256 borrowChange, uint256 collateralChange) = _liquidationAmount(user);
89
90  if (borrowChange == 0 || collateralChange == 0) {
91    return LiquidationInfo(true, 0, 0, 0, 0);
92  }
93
94  uint256 tPrice = _price();
95  uint256 userDeposit = _deposit(user);
96  uint256 collateralValue = _scaleFromAsset(userDeposit * tPrice) / 1 ether;
97  uint256 cappedChange = MathLib.min(collateralValue, collateralChange);
98  uint256 pureChange = _withoutFee(cappedChange);
99  uint256 totalFee = cappedChange - pureChange;
100  uint256 protocolFee = _scaleToAsset(
101    (totalFee * _treasuryLiquidationFee()) / 1 ether
102  );
103
104  uint256 realTokens = MathLib.min(
105    userDeposit,
106    _scaleToAsset((cappedChange * 1 ether) / (tPrice))
107  );
108
109  uint256 rebate = 0;
110
111  if (pureChange > collateralValue) {
112    rebate = MathLib.min(
113      _s.treasury.tokenBalance(VaultConstants.REBATE_CAUSE, address(_s.cash)),
114      pureChange - collateralValue
115    );
116  }
117
118  return
119    LiquidationInfo(false, borrowChange, realTokens, protocolFee, rebate);
120}

Recommendation:

We advise the pureChange to be calculated as a result of the collateralChange rather than the collateralValue, ensuring that the actual desired change is larger than what the collateral value that can be extracted from the faulty position is.

Alleviation (3dd3d7bf0c2693b2f9c23bacedfa420393f7ea84):

The rebate mechanism correctly utilizes the targetCollateralChange rather than the pureChange it was utilizing previously, ensuring rebate amounts are properly calculated.