Systemic Vulnerability Due to Rigid Asset Dependency in AssetRegistry

[c4-bot-3]

Lines of code

https://github.com/code-423n4/2024-07-reserve/blob/3f133997e186465f4904553b0f8e86ecb7bbacbf/contracts/p1/AssetRegistry.sol#L58-L70

Vulnerability details

Vulnerability Description

The AssetRegistry contract lacks robust mechanisms to handle scenarios where registered collateral assets become compromised or malfunction. This vulnerability stems from the assumption that all registered assets will consistently behave as expected throughout their lifecycle. However, various events such as token upgrades, hacks, or contract self-destructs can invalidate this assumption, potentially leading to a system-wide failure.

The core of this vulnerability lies in three main areas:

• Asset Registration Process: The current implementation lacks comprehensive validation and contingency measures for newly registered assets.
• Refresh Mechanism: The system-wide refresh function fails to isolate assets with individual issues, potentially causing system-wide failures.
• Unregistration Process: The ability to remove compromised assets is paradoxically dependent on the functionality of those same assets. This vulnerability exposes the system to risks from various scenarios such as token contract upgrades, hacks, rug pulls, or even temporary malfunctions in collateral assets.

Impact

• System-Wide Paralysis: A single compromised asset could render core functionalities of the entire protocol inoperable. This includes critical operations like RToken issuance, redemption, and recollateralization.
• Permanent Fund Lockup: In a scenario where a small portion of collateral (e.g., 1%) becomes non-functional, it could lead to the permanent locking of all other assets in the system (e.g., the remaining 99%). This represents a significant financial risk to users and could severely damage the protocol's reputation.
• Irreversible System State: The inability to unregister compromised assets means the system could become stuck in a broken state with no clear path to recovery.

Proof of Concept

Let's examine the vulnerable parts of the AssetRegistry contract in detail:

• Asset Registration Process:

  function _registerIgnoringCollisions(IAsset asset) private returns (bool swapped) {
  if (asset.isCollateral()) {
      require(
          ICollateral(address(asset)).status() == CollateralStatus.SOUND,
          "collateral not sound"
      );
  }
  // ... (other checks)
  
  IERC20Metadata erc20 = asset.erc20();
  if (_erc20s.contains(address(erc20))) {
      if (assets[erc20] == asset) return false;
      else emit AssetUnregistered(erc20, assets[erc20]);
  } else {
      _erc20s.add(address(erc20));
  }
  
  assets[erc20] = asset;
  emit AssetRegistered(erc20, asset);
  
  // Refresh to ensure it does not revert, and to save a recent lastPrice
  asset.refresh();
  return true;
  }

  This function lacks comprehensive checks for long-term asset viability. The SOUND status check is insufficient as it only captures the current state and doesn't account for future potential issues.

• Refresh Functionality: https://github.com/code-423n4/2024-07-reserve/blob/3f133997e186465f4904553b0f8e86ecb7bbacbf/contracts/p1/AssetRegistry.sol#L58-69

  function refresh() public {
  uint256 length = _erc20s.length();
  for (uint256 i = 0; i < length; ++i) {
      assets[IERC20(_erc20s.at(i))].refresh();
  }
  }

  This function iterates through all assets and calls their refresh function. If any single asset's refresh call fails, it will cause the entire operation to revert, potentially breaking critical system functionalities.

• Unregistering Mechanism: https://github.com/code-423n4/2024-07-reserve/blob/3f133997e186465f4904553b0f8e86ecb7bbacbf/contracts/p1/AssetRegistry.sol#L108-121 function unregister(IAsset asset) external governance { require(_erc20s.contains(address(asset.erc20())), "no asset to unregister"); require(assets[asset.erc20()] == asset, "asset not found");

  try basketHandler.quantity{ gas: _reserveGas() }(asset.erc20()) returns (uint192 quantity) { if (quantity != 0) basketHandler.disableBasket(); // not an interaction } catch { basketHandler.disableBasket(); }

  _erc20s.remove(address(asset.erc20())); assets[asset.erc20()] = IAsset(address(0)); emit AssetUnregistered(asset.erc20(), asset); } This function depends on the basketHandler.quantity() call, which could fail if the asset is compromised. This creates a paradoxical situation where a compromised asset cannot be removed from the system.

  Tools Used

  Manual code review

Recommended Mitigation Steps

• Modify the refresh function to handle failures of individual assets gracefully
• Create a function that allows governance to forcefully remove an asset without relying on its functionality

Assessed type

Other