Felix

Several critical upgrades to the Liquity v2 codebase have not been integrated into the Felix codebase.

Below is a partial list of missing commits:

• commit d5bd31b (October 23rd) "fix: Apply SP error fix to scale updates”
• commit 87198e9 (October 31st) "fix: SP previous error should be applied to newProductFactor, not P”
• commit 71026e6 (Nov 1st) "fix: Make sure a fully redeemed trove ends up with zero shares in a batch”
• commit b89ef84 (Nov 1st) "fix: Revert if trying to apply debt to empty trove”
• commit 4dfe520 (November 18th) "feat: Add Zappers Hybrid exchange helpers”
• commit afc2464 (Nov 25th) "fix: Receiver usage should be constrained to remove manager”
• commit a689378 (December 4th) "fix: Make sure safeTransfer is used for collateral tokens”

The HLPriceFeed::fetchPrice function reverts if the oracle call returns an erroneous 0 value as the price of the asset. This behavior differs from the Liquity v2 implementation, which would automatically shut down the market and use the last known good price to prevent disruption. In the current setup, however, the market can only be shut down manually, adding a layer of reliance on human intervention. As a result, if oracle calls fail during periods of significant market volatility, protocol users may be unable to react in time, potentially resulting in substantial losses.

The HLPriceFeed contract does not define a fetchRedemptionPrice function, unlike the standard Liquity v2 oracles. As a result, the price calculation does not differ between redemptions and other protocol operations. Although the asset prices provided by the HyperEVM system contract are sourced from major CEXs, the lack of a redemption-specific price may leave Felix vulnerable to unwanted redemption arbitrage if someone is able to manipulate the oracles.

In the assembly block of LiquityBaseInit::initialize, the mask computation is incorrect. The expression shl(20, 1) shifts 1 to the left by 20 bits, whereas the intention is to shift it by 20 bytes (160 bits).

assembly {
  /// @dev Storage variables are written from right to left
  /// Thus the active pool address is located at 2 bytes offset
  /// from the right in the slot
  let ptr := sload(ACTIVE_POOL_SLOT)

  // Dedaub:
  // computed mask:
  // 0xfffffffffffffffffffffffffffffffffffffffffffffffffffffff00000ffff
  // intended mask:
  // 0xffffffffffffffffffff0000000000000000000000000000000000000000ffff
  let mask := not(shl(mul(ACTIVE_POOL_OFFSET, 8), sub(shl(20, 1), 1)))
  ptr := and(ptr, mask)
  ptr := or(ptr, shl(mul(ACTIVE_POOL_OFFSET, 8), activePool))

  sstore(ACTIVE_POOL_SLOT, ptr)
  sstore(DEFAULT_POOL_SLOT, defaultPool)
  sstore(PRICE_FEED_SLOT, priceFeed)
}

Fortunately, the final ptr value remains unaffected by the incorrect mask, assuming the 20 slot bytes to be masked are set to 0 during contract initialization.

The function AdminController::batchAddAddressRegistry loops over the _addressRegistries array and adds each registry to the addressesRegistries storage array after checking that _doesIndexMatch(_addressRegistries[i], index += i) is true.

function batchAddAddressRegistry
  IAddressesRegistry[] memory _addressRegistries
) external onlyOwner {
  if(address(collateralRegistry) == address(0)) revert
    AdminController__CollateralRegistryNotSet();
  uint256 index = addressesRegistries.length;

  for(uint256 i = 0; i < _addressRegistries.length; i++) {
    // Dedaub: should be index + i instead of index += i
    if(!_doesIndexMatch(_addressRegistries[i], index += i))
      revert AdminController__IndexMismatch();

    addressesRegistries.push(_addressRegistries[i]);
    emit AddressRegistryAdded(address(_addressRegistries[i]));
  }
}

The second argument of _doesIndexMatch is used as an index for the tokens array of the CollateralRegistry contract. Therefore, its starting value should equal addressesRegistries.length to account for the existing tokens and address registries, and it should increment by 1 with each subsequent loop iteration. However, the current implementation index += i accumulates i to index on every iteration, whereas index + i would achieve the intended behavior.

The AdminController::proposeNewCollateral function does not verify the number of decimals for the _newCollateral token, even though the current protocol version supports only tokens with 18 decimals. This is a known limitation of the current version; however, a token with a different number of decimals could still be added by mistake.

The event NewCMRSet, which is defined in the TroveManager contract, should be renamed to NewMCRSet.

When applying an AdminController proposal to change the MCR or CCR parameters, it should be checked to ensure that they do not break a critical invariant of the protocol, i.e., the system’s critical collateralization ratio is not set to a value smaller than the maintenance collateralization ratio of individual troves.

When applying an AdminController proposal to change the CCR parameter, it should be checked to ensure that CCR (critical collateralization ratio) is greater than the SCR (shutdown collateralization ratio).

The Felix protocol, unlike Liquity v2, permits contract upgrades while maintaining the existing state through the use of transparent proxies. This design enables essential upgrades to the contracts' logic but also introduces risks. Malicious actors, whether protocol owners or adversaries compromising the owners' accounts, could exploit this feature and introduce malicious code. Additionally, protocol owners might unintentionally introduce buggy functionality during upgrades. The AdminController contract, which facilitates updates, enforces a timelock period (currently set to 3 days) for each contract upgrade. While this provides users with a reasonable window to react, it could still be easily overlooked by a significant number of users.

The Felix protocol introduces mutability for the following critical branch parameters:

• MCR
• CCR
• SPYield
• InterestRouter
• PriceFeed
• MaxDebtCap

Incorrect or malicious parameter settings can negatively affect the health of a branch or the entire system due to the accrual of excessive bad debt, particularly in inefficient markets. Protocol owners should set these parameters defensively to ensure the stability, security, and resilience of the protocol.

The owners of the protocol can add new collaterals/branches. We believe that choosing collaterals requires careful consideration, as the collapse of a branch (whether due to collateral collapses or branch oracle fails) could create bad debt that is shared across all branches. This, in turn, could lead to bank runs, as described in the known Liquity v2 issues guide.

In the AdminController::applyNewCollateral function, the _troveManager parameter is passed directly, though it could be retrieved from the _addressRegistry contract to reduce the risk of errors.

The getToken and getTroveManager functions of CollateralRegistry return address(0) when the index argument is greater than or equal to the number of tokens and trove managers, respectively. We believe that reverting with a clear error in such cases would be a more robust and fail-safe approach.

The AdminController__NotZeroValue error defined in AdminController is never used.

Dependencies such as LiquityBase, AddRemoveManageres, and Ownable should be marked as abstract.

The AddressesRegistry contract defines bounds for CCR and MCR (lower = 1e18, higher = 2e18) that are not consistent with those defined by AdminController (lower = 1.1e18, higher = 4.5e18). This difference might be reasonable if we consider that the AdminController bounds will be applied after initialization and can therefore be more relaxed.

The functions addAddressRegistry and batchAddAddressRegistry in the AdminController contract do not verify that the provided _addressRegistry and _addressRegistries[i] are not equal to address(0).

Oracle constants are undefined for assets other than ETH, likely because the potential collateral types have yet to be finalized.

The @notice code comment for AdminController::setCollateralRegistry is incorrect, as there is a single CollateralRegistry contract, not one per branch.

The code is compiled with Solidity 0.8.24. Version 0.8.24, in particular, has no known bugs..