fix: spoke fixes (#664)

* fix: skip isCollateral branch when cf is 0

* fix: split debt in base currency

* chore: retain current behaviour

* test: hf calc does not include zero cf collateral

* chore: zero addr check for authority

* feat: finalise

* chore: rename

* chore: fix test name

Author: DhairyaSethi

snapshots/Spoke.Getters.json

@@ -1,7 +1,7 @@
 {
   "getUserAccountData: supplies: 0, borrows: 0": "9239",
-  "getUserAccountData: supplies: 1, borrows: 0": "44949",
-  "getUserAccountData: supplies: 2, borrows: 0": "75566",
-  "getUserAccountData: supplies: 2, borrows: 1": "98873",
-  "getUserAccountData: supplies: 2, borrows: 2": "120687"
+  "getUserAccountData: supplies: 1, borrows: 0": "44980",
+  "getUserAccountData: supplies: 2, borrows: 0": "75628",
+  "getUserAccountData: supplies: 2, borrows: 1": "99115",
+  "getUserAccountData: supplies: 2, borrows: 2": "121137"
 }

snapshots/Spoke.Operations.json

@@ -1,26 +1,26 @@
 {
-  "borrow: first": "250206",
-  "borrow: second action, same reserve": "213225",
-  "liquidationCall: full": "309118",
-  "liquidationCall: partial": "383728",
-  "permitReserve + repay (multicall)": "270499",
+  "borrow: first": "250417",
+  "borrow: second action, same reserve": "213436",
+  "liquidationCall: full": "309540",
+  "liquidationCall: partial": "384150",
+  "permitReserve + repay (multicall)": "270710",
   "permitReserve + supply (multicall)": "140611",
   "permitReserve + supply + enable collateral (multicall)": "173849",
-  "repay: full": "174561",
-  "repay: partial": "229281",
+  "repay: full": "174592",
+  "repay: partial": "229492",
   "supply + enable collateral (multicall)": "152282",
   "supply: 0 borrows, collateral disabled": "116445",
   "supply: 0 borrows, collateral enabled": "119441",
   "supply: 1 borrow": "119430",
   "supply: second action, same reserve": "102341",
   "updateUserDynamicConfig: 1 collateral": "53113",
   "updateUserDynamicConfig: 2 collaterals": "58377",
-  "updateUserRiskPremium: 1 borrow": "122002",
-  "updateUserRiskPremium: 2 borrows": "157802",
+  "updateUserRiskPremium: 1 borrow": "122213",
+  "updateUserRiskPremium: 2 borrows": "158221",
   "usingAsCollateral: 0 borrows, enable": "53930",
-  "usingAsCollateral: 1 borrow, disable": "132204",
+  "usingAsCollateral: 1 borrow, disable": "132415",
   "usingAsCollateral: 1 borrow, enable": "24831",
-  "withdraw: 0 borrows, full": "136205",
-  "withdraw: 0 borrows, partial": "137732",
-  "withdraw: 1 borrow, partial": "239439"
+  "withdraw: 0 borrows, full": "136236",
+  "withdraw: 0 borrows, partial": "137763",
+  "withdraw: 1 borrow, partial": "239650"
 }

src/contracts/Spoke.sol

@@ -60,6 +60,7 @@ contract Spoke is ISpoke, Multicall, AccessManaged, EIP712 {
    * @param authority_ The address of the authority contract which manages permissions.
    */
   constructor(address authority_) AccessManaged(authority_) {
+    require(authority_ != address(0), InvalidAddress());
     _liquidationConfig.closeFactor = Constants.HEALTH_FACTOR_LIQUIDATION_THRESHOLD;
     emit LiquidationConfigUpdate(_liquidationConfig);
   }
@@ -68,10 +69,13 @@ contract Spoke is ISpoke, Multicall, AccessManaged, EIP712 {
   // Governance
   // /////
 
+  /// @inheritdoc ISpoke
   function updateOracle(address newOracle) external restricted {
-    require(newOracle != address(0), InvalidOracle());
     oracle = IAaveOracle(newOracle);
-    require(oracle.DECIMALS() == 8, InvalidOracle());
+    require(
+      newOracle != address(0) && oracle.DECIMALS() == Constants.ORACLE_DECIMALS,
+      InvalidOracle()
+    );
     emit OracleUpdate(newOracle);
   }
 
@@ -95,10 +99,11 @@ contract Spoke is ISpoke, Multicall, AccessManaged, EIP712 {
     DataTypes.ReserveConfig calldata config,
     DataTypes.DynamicReserveConfig calldata dynamicConfig
   ) external restricted returns (uint256) {
-    require(hub != address(0), InvalidHubAddress());
+    require(hub != address(0), InvalidAddress());
     require(!_reserveExists[hub][assetId], ReserveExists());
 
     _validateReserveConfig(config);
+    _validateDynamicReserveConfig(dynamicConfig);
     uint256 reserveId = _reserveCount++;
     uint16 dynamicConfigKey; // 0 as first key to use
 
@@ -222,7 +227,6 @@ contract Spoke is ISpoke, Multicall, AccessManaged, EIP712 {
 
     userPosition.suppliedShares -= withdrawnShares.toUint128();
 
-    // calc needs new user position, just updating drawn debt is enough
     uint256 newUserRiskPremium = _refreshAndValidateUserPosition(onBehalfOf); // validates HF
     _notifyRiskPremiumUpdate(onBehalfOf, newUserRiskPremium);
 
@@ -250,7 +254,6 @@ contract Spoke is ISpoke, Multicall, AccessManaged, EIP712 {
       positionStatus.setBorrowing(reserveId, true);
     }
 
-    // calc needs new user position, just updating drawn debt is enough
     uint256 newUserRiskPremium = _refreshAndValidateUserPosition(onBehalfOf); // validates HF
     _notifyRiskPremiumUpdate(onBehalfOf, newUserRiskPremium);
 
@@ -503,13 +506,11 @@ contract Spoke is ISpoke, Multicall, AccessManaged, EIP712 {
     return drawnDebt + premiumDebt;
   }
 
-  /// @dev We do not differentiate between duplicate reserves (assetId) on the same hub
   function getReserveSuppliedAmount(uint256 reserveId) external view returns (uint256) {
     DataTypes.Reserve storage reserve = _reserves[reserveId];
     return reserve.hub.getSpokeAddedAmount(reserve.assetId, address(this));
   }
 
-  /// @dev We do not differentiate between duplicate reserves (assetId) on the same hub
   function getReserveSuppliedShares(uint256 reserveId) external view returns (uint256) {
     DataTypes.Reserve storage reserve = _reserves[reserveId];
     return reserve.hub.getSpokeAddedShares(reserve.assetId, address(this));
@@ -676,15 +677,10 @@ contract Spoke is ISpoke, Multicall, AccessManaged, EIP712 {
 
   /**
    * @dev Calculates the user's premium debt offset in assets amount from a given share amount.
-   * @dev Rounds down to the nearest assets amount.
-   * @dev Uses the opposite rounding direction of the debt shares-to-assets conversion to prevent underflow
-   * in premium debt.
-   * @param hub The liquidity hub of the reserve.
-   * @param assetId The identifier of the asset.
-   * @param shares The amount of shares to convert to assets amount.
-   * @return The amount of assets converted corresponding to user's premium offset.
+   * @dev Rounds down to the nearest assets amount. Uses the opposite rounding direction of the
+   * debt shares-to-assets conversion to prevent underflow in premium debt.
    */
-  function _previewOffset(
+  function _previewPremiumOffset(
     IHub hub,
     uint256 assetId,
     uint256 shares
@@ -728,7 +724,10 @@ contract Spoke is ISpoke, Multicall, AccessManaged, EIP712 {
   }
 
   function _validateLiquidationConfig(DataTypes.LiquidationConfig calldata config) internal pure {
-    _validateCloseFactor(config.closeFactor);
+    require(
+      config.closeFactor >= Constants.HEALTH_FACTOR_LIQUIDATION_THRESHOLD,
+      InvalidCloseFactor()
+    );
     require(
       config.liquidationBonusFactor <= PercentageMath.PERCENTAGE_FACTOR,
       InvalidLiquidationBonusFactor()
@@ -739,10 +738,6 @@ contract Spoke is ISpoke, Multicall, AccessManaged, EIP712 {
     );
   }
 
-  function _validateCloseFactor(uint256 closeFactor) internal pure {
-    require(closeFactor >= Constants.HEALTH_FACTOR_LIQUIDATION_THRESHOLD, InvalidCloseFactor());
-  }
-
   /**
    * @dev Validates the reserve can be set as collateral.
    * @dev Collateral can be disabled if the reserve is frozen.
@@ -847,21 +842,23 @@ contract Spoke is ISpoke, Multicall, AccessManaged, EIP712 {
             ? (userPosition.configKey = reserve.dynamicConfigKey)
             : userPosition.configKey
         ];
-
-        vars.userCollateralInBaseCurrency = _getUserBalanceInBaseCurrency(
-          userPosition,
-          hub,
-          vars.assetId,
-          vars.assetPrice,
-          vars.assetUnit
-        );
-
-        vars.totalCollateralInBaseCurrency += vars.userCollateralInBaseCurrency;
-        list.add(vars.i, reserve.collateralRisk, vars.userCollateralInBaseCurrency);
-        vars.avgCollateralFactor += vars.userCollateralInBaseCurrency * dynConfig.collateralFactor;
-
-        unchecked {
-          ++vars.i;
+        uint256 collateralFactor = dynConfig.collateralFactor;
+        if (collateralFactor != 0) {
+          vars.userCollateralInBaseCurrency = _getUserBalanceInBaseCurrency(
+            userPosition,
+            hub,
+            vars.assetId,
+            vars.assetPrice,
+            vars.assetUnit
+          );
+
+          vars.totalCollateralInBaseCurrency += vars.userCollateralInBaseCurrency;
+          list.add(vars.i, reserve.collateralRisk, vars.userCollateralInBaseCurrency);
+          vars.avgCollateralFactor += vars.userCollateralInBaseCurrency * collateralFactor;
+
+          unchecked {
+            ++vars.i;
+          }
         }
       }
 
@@ -899,7 +896,6 @@ contract Spoke is ISpoke, Multicall, AccessManaged, EIP712 {
     // @dev from this point onwards, `collateralCounterInBaseCurrency` represents running collateral
     // value used in risk premium, `debtCounterInBaseCurrency` represents running outstanding debt
     while (vars.i < list.length() && vars.debtCounterInBaseCurrency > 0) {
-      if (vars.debtCounterInBaseCurrency == 0) break;
       (vars.collateralRisk, vars.userCollateralInBaseCurrency) = list.get(vars.i);
       if (vars.userCollateralInBaseCurrency > vars.debtCounterInBaseCurrency) {
         vars.userCollateralInBaseCurrency = vars.debtCounterInBaseCurrency;
@@ -933,7 +929,8 @@ contract Spoke is ISpoke, Multicall, AccessManaged, EIP712 {
     uint256 assetUnit
   ) internal view returns (uint256) {
     (uint256 drawnDebt, uint256 premiumDebt, ) = _getUserDebt(hub, assetId, userPosition);
-    return ((drawnDebt + premiumDebt) * assetPrice).wadDivUp(assetUnit);
+    return
+      (drawnDebt * assetPrice).wadDivUp(assetUnit) + (premiumDebt * assetPrice).wadDivUp(assetUnit);
   }
 
   function _getUserBalanceInBaseCurrency(
@@ -996,7 +993,7 @@ contract Spoke is ISpoke, Multicall, AccessManaged, EIP712 {
         .drawnShares
         .percentMulUp(newUserRiskPremium)
         .toUint128());
-      uint256 newPremiumOffset = (userPosition.premiumOffset = _previewOffset(
+      uint256 newPremiumOffset = (userPosition.premiumOffset = _previewPremiumOffset(
         vars.hub,
         vars.assetId,
         userPosition.premiumShares

src/contracts/TreasurySpoke.sol

@@ -27,7 +27,7 @@ contract TreasurySpoke is ITreasurySpoke, Ownable {
    * @param hub_ The address of the Hub
    */
   constructor(address owner_, address hub_) Ownable(owner_) {
-    require(hub_ != address(0), InvalidHubAddress());
+    require(hub_ != address(0), InvalidAddress());
 
     HUB = IHub(hub_);
   }

src/interfaces/ISpoke.sol

@@ -127,10 +127,15 @@ interface ISpoke is ISpokeBase, IMulticall, IAccessManaged {
   error Unauthorized();
   error ConfigKeyUninitialized();
   error InactivePositionManager();
+  error InvalidAddress();
   error InvalidSignature();
 
   function updateLiquidationConfig(DataTypes.LiquidationConfig calldata config) external;
 
+  /**
+   * @notice Allows governance to update the spoke oracle.
+   * @dev Does not validate all existing reserves are supported on `newOracle`.
+   */
   function updateOracle(address newOracle) external;
 
   function updateReservePriceSource(uint256 reserveId, address priceSource) external;

src/interfaces/ISpokeBase.sol

@@ -10,11 +10,6 @@ import {DataTypes} from 'src/libraries/types/DataTypes.sol';
  * @notice Minimal interface for Spoke
  */
 interface ISpokeBase {
-  /**
-   * @notice Error thrown when the hub address is invalid.
-   */
-  error InvalidHubAddress();
-
   /**
    * @notice Emitted on the supply action.
    * @param reserveId The reserve identifier of the underlying asset as registered on the spoke.

src/interfaces/ITreasurySpoke.sol

@@ -10,6 +10,7 @@ import {ISpokeBase} from 'src/interfaces/ISpokeBase.sol';
  */
 interface ITreasurySpoke is ISpokeBase {
   error UnsupportedAction();
+  error InvalidAddress();
 
   /**
    * @notice Supplies a specified amount of the underlying asset to a given reserve.

src/libraries/helpers/Constants.sol

@@ -7,6 +7,7 @@ library Constants {
   uint8 public constant MAX_ALLOWED_ASSET_DECIMALS = 18;
   uint56 internal constant MAX_CAP = type(uint56).max;
   /// @dev Spoke Constants
+  uint8 public constant ORACLE_DECIMALS = 8;
   uint64 public constant HEALTH_FACTOR_LIQUIDATION_THRESHOLD = 1e18;
   uint24 public constant MAX_COLLATERAL_RISK = 1000_00; // 1000.00%
   // keccak256('SetUserPositionManager(address positionManager,address user,bool approve,uint256 nonce,uint256 deadline)')

tests/Base.t.sol

@@ -1067,6 +1067,23 @@ abstract contract Base is Test {
     return configKey;
   }
 
+  function updateCollateralFactorAtKey(
+    ISpoke spoke,
+    uint256 reserveId,
+    uint16 configKey,
+    uint256 newCollateralFactor
+  ) internal pausePrank {
+    DataTypes.DynamicReserveConfig memory config = spoke.getDynamicReserveConfig(
+      reserveId,
+      configKey
+    );
+    config.collateralFactor = newCollateralFactor.toUint16();
+    vm.prank(SPOKE_ADMIN);
+    spoke.updateDynamicReserveConfig(reserveId, configKey, config);
+
+    assertEq(spoke.getDynamicReserveConfig(reserveId), config);
+  }
+
   function updateReserveBorrowableFlag(
     ISpoke spoke,
     uint256 reserveId,
@@ -1302,8 +1319,23 @@ abstract contract Base is Test {
     IPriceOracle oracle = spoke.oracle();
     uint256 assetId = spoke.getReserve(reserveId).assetId;
     return
-      (amount * oracle.getReservePrice(reserveId).toWad()) /
-      (10 ** spoke.getReserve(reserveId).hub.getAsset(assetId).decimals);
+      (amount * oracle.getReservePrice(reserveId)).wadDivDown(
+        10 ** spoke.getReserve(reserveId).hub.getAsset(assetId).decimals
+      );
+  }
+
+  /// returns the USD value of the reserve normalized by it's decimals, in terms of WAD
+  function _getDebtValueInBaseCurrency(
+    ISpoke spoke,
+    uint256 reserveId,
+    uint256 amount
+  ) internal view returns (uint256) {
+    IPriceOracle oracle = spoke.oracle();
+    uint256 assetId = spoke.getReserve(reserveId).assetId;
+    return
+      (amount * oracle.getReservePrice(reserveId)).wadDivUp(
+        10 ** spoke.getReserve(reserveId).hub.getAsset(assetId).decimals
+      );
   }
 
   /// @notice Convert 1 asset amount to equivalent amount in another asset.
@@ -1869,6 +1901,13 @@ abstract contract Base is Test {
     return spoke.getDynamicReserveConfig(reserveId).collateralFactor;
   }
 
+  function _getCollateralFactor(
+    ISpoke spoke,
+    function(ISpoke) internal view returns (uint256) reserveId
+  ) internal view returns (uint16) {
+    return spoke.getDynamicReserveConfig(reserveId(spoke)).collateralFactor;
+  }
+
   function _hasRole(
     IAccessManager authority,
     uint64 role,

tests/unit/Spoke/Spoke.Borrow.Scenario.t.sol

@@ -576,4 +576,36 @@ contract SpokeBorrowScenarioTest is SpokeBase {
       label: 'bob dai data after'
     });
   }
+
+  function test_userAccountData_does_not_include_zero_cf_collateral() public {
+    uint256 coll1ReserveId = _daiReserveId(spoke1);
+    uint256 coll1Amount = 1000e18;
+    uint256 coll2ReserveId = _wethReserveId(spoke1);
+    uint256 coll2Amount = 1e18;
+    uint256 debtReserveId = _usdxReserveId(spoke1);
+    uint256 debtBorrowAmount = 500e6;
+
+    assertNotEq(_getCollateralFactor(spoke1, coll1ReserveId), 0);
+    assertNotEq(_getCollateralFactor(spoke1, coll2ReserveId), 0);
+
+    uint256 coll1InBaseCurrency = _getValueInBaseCurrency(spoke1, coll1ReserveId, coll1Amount);
+    uint256 coll2InBaseCurrency = _getValueInBaseCurrency(spoke1, coll2ReserveId, coll2Amount);
+
+    Utils.supplyCollateral(spoke1, coll1ReserveId, alice, coll1Amount, alice);
+    Utils.supplyCollateral(spoke1, coll2ReserveId, alice, coll2Amount, alice);
+    _openSupplyPosition(spoke1, debtReserveId, debtBorrowAmount);
+    Utils.borrow(spoke1, debtReserveId, alice, debtBorrowAmount, alice);
+
+    (uint256 userRiskPremium, , , uint256 totalCollateralInBaseCurrency, ) = spoke1
+      .getUserAccountData(alice);
+    assertEq(_calculateExpectedUserRP(alice, spoke1), userRiskPremium);
+    assertEq(coll1InBaseCurrency + coll2InBaseCurrency, totalCollateralInBaseCurrency);
+
+    uint16 configKey = spoke1.getUserPosition(coll1ReserveId, alice).configKey;
+    updateCollateralFactorAtKey(spoke1, coll1ReserveId, configKey, 0);
+
+    (userRiskPremium, , , totalCollateralInBaseCurrency, ) = spoke1.getUserAccountData(alice);
+    assertEq(_calculateExpectedUserRP(alice, spoke1), userRiskPremium);
+    assertEq(coll2InBaseCurrency, totalCollateralInBaseCurrency); // coll1 is not included
+  }
 }