Admin can break the protocol logic irreversibly by adding the same collateral twice

Summary

UbiquityPoolFacet.sol:addCollateralToken or more specifically the library function it uses LibUbiquityPool:addCollateralToken allows the admin to add the same collateral token twice. This action would break view functions logic, allow users to mint Dollars with misspriced collateral, deposit more collateral than the specified pool ceiling and being able to redeem dollars even if there is no free collateral in the contract.

Vulnerability Detail

Adding a collateral token 2 times with addCollateralToken affects the protocol the following way:

collateralUsdBalance

addCollateralToken updates the following storage variables:

  function addCollateralToken(
      address collateralAddress,
      address chainLinkPriceFeedAddress,
      uint256 poolCeiling
  ) internal {
    ...
    // add collateral address to all collaterals
@>  poolStorage.collateralAddresses.push(collateralAddress);

    // for fast collateral address -> collateral idx lookups later
@>  poolStorage.collateralIndex[collateralAddress] = collateralIndex;
    ...
  }

collateralUsdBalance should return the sum of all collateral value the protocol holds in USD by looping through all poolStorage.collateralAddresses.

Since the collateral addresses can be duplicated inside the array and since inside the loop the function freeCollateralBalance uses IERC20(collateralAddress).balanceOf(address(this)) (can be seen here) this would make collateralUsdBalance return significantly higher sum of all collateral USD value than there is in reality.

    function collateralUsdBalance()
        internal
        view
        returns (uint256 balanceTally)
    {
        UbiquityPoolStorage storage poolStorage = ubiquityPoolStorage();
        // Example poolStorage.collateralAddresses = [LUSD, DAI, LUSD]
        // LUSD balance = 100 LUSD => 100 USD
        // DAI balance = 100 DAI => 100 USD
        // Returned Sum Collateral Value = 300 USD
        // In reality the sum Collateral Value must be = 200 USD
        uint256 collateralTokensCount = poolStorage.collateralAddresses.length;
        balanceTally = 0;
        for (uint256 i = 0; i < collateralTokensCount; i++) {
@>          balanceTally += freeCollateralBalance(i)
                .mul(10 ** poolStorage.missingDecimals[i])
                .mul(poolStorage.collateralPrices[i])
                .div(UBIQUITY_POOL_PRICE_PRECISION);
        }
    }

    function freeCollateralBalance(
        uint256 collateralIndex
    ) internal view returns (uint256) {
        UbiquityPoolStorage storage poolStorage = ubiquityPoolStorage();
        return
            IERC20(poolStorage.collateralAddresses[collateralIndex])
@>              .balanceOf(address(this))
                .sub(poolStorage.unclaimedPoolCollateral[collateralIndex]);
    }

External services could rely on this information and the function would not be able to be fixed by the admin unless the pool facet implementation is upgraded.

collateralInformation

This function (link) would be able to return the data only of the lastly added duplicated collateral. The old collateral index data would not be accessible anymore because of the way the collateral is selected (link):

  ...
  // get the index
  uint256 index = poolStorage.collateralIndex[collateralAddress]; // returns the new index because addCollateralToken updated collateralIndex[collateralAddress]

  returnData = CollateralInformation(
      index,
      poolStorage.collateralSymbols[index],
      collateralAddress,
      poolStorage.collateralPriceFeedAddresses[index],
      poolStorage.collateralPriceFeedStalenessThresholds[index],
      poolStorage.isCollateralEnabled[collateralAddress],
      poolStorage.missingDecimals[index],
      poolStorage.collateralPrices[index],
      poolStorage.poolCeilings[index],
      poolStorage.isMintPaused[index],
      poolStorage.isRedeemPaused[index],
      poolStorage.isBorrowPaused[index],
      poolStorage.mintingFee[index],
      poolStorage.redemptionFee[index]
  );
  ...

setCollateralChainLinkPriceFeed

This function will be able to update the oracle address and staleness threshold only for the newly added duplicated collateral address. If the oracle the collateral uses for pricing needs to be updated this would not be possible for the initial collateralIndex thus allowing the collateral to be valued at incorrect price or to thow error when this collateralIndex is used.

  function setCollateralChainLinkPriceFeed(
@>    address collateralAddress,
      address chainLinkPriceFeedAddress,
      uint256 stalenessThreshold
  ) internal {
      UbiquityPoolStorage storage poolStorage = ubiquityPoolStorage();

@>    uint256 collateralIndex = poolStorage.collateralIndex[ // returns the last collateral index (the old one is not accessible)
          collateralAddress
      ];

      // set price feed address
@>    poolStorage.collateralPriceFeedAddresses[
          collateralIndex
      ] = chainLinkPriceFeedAddress;

      // set staleness threshold in seconds when chainlink answer should be considered stale
@>    poolStorage.collateralPriceFeedStalenessThresholds[
          collateralIndex
      ] = stalenessThreshold;
  ...

toggleCollateral

toggleCollateral toggles collaterals by index, by 2 of the same collateral addresses would mean that when we disable collateral address at index A, this would also mean that we have disabled the usage of the collateral at index B. This is bad because after the admin have added a collateral twice he will not be able to stop users from using whichever version of the collateral they please. The decision would be to not use this collateral forever or to let users use whichever version they would like.

What the user can do if the contract is in this state (the same collateral address is added 2 times as collateral)?

The user can use mintDollar to provide collateral at possibly incorrect price by using the initial collateral index of the duplicated collateral asset (since updateChainLinkCollateralPrice that is used to update the price of the collateral before the minting uses the specified oracle for the collateral at the specified index link).

    ...
    // update collateral price
@>  updateChainLinkCollateralPrice(collateralIndex);

    // get amount of collateral for minting Dollars
    collateralNeeded = getDollarInCollateral(collateralIndex, dollarAmount);
    ...

This is possible since, as mentioned above in setCollateralChainLinkPriceFeed, we cannot update the oracle address or threshold at the old index if such change is necessary at any point. This means that the user can either harm himself or the protocol based on the price of the oracle by minting more or less Dollars he is supposed to. (getDollarInCollateral decides how much collateral must be provided based on the specified Dollar amount and the collateral price link)

The pool ceiling check inside mintDollar (the one here) can be bypassed since freeCollateralBalance returns a value that is the balance of the collateral that the contract holds minus the unclaimedPoolCollateral for the specific collateral index. Meaning if the unclaimedPoolCollateral for the duplicated collateral at index A is low and this does not allow the pool ceiling check to pass, the user can decide to mintDollar by using index B where the unclaimedPoolCollateral could be higher. This way the user would be able to mint Dollar tokens despite the collateral ceiling limit.
The user is able to use redeemDollar even if there is not enough collateral for withdrawing (contract collateral balance minus unclaimedAmount - the unclaimed amount is from users that have used redeemDollar before but still have not used collectRedemption to take out their collateral) because of being able to bypass the Insufficient pool collateral check by using collateral index of the duplicated collateral where the unclaimed amount is lower compared with the other index (this is the same problem like the check inside mintDollar from point 2 explained above due to both checks using poolStorage.unclaimedPoolCollateral[collateralIndex]). This means that a user that have already used redeemDollar but still haven't collected his collateral with collectRedemption could not be able to withdraw his collateral if another user does that before him by using the method described above. Having one real underlying balance of a collateral token by using IERC20(token).balanceOf(address(this)) and 2 separate unclaimedCollateral variables made for each collateral token index makes this issue possible.

Impact

Break view functions logic, allow users to mint Dollars with miss-priced collateral, deposit more collateral than the specified pool ceiling and being able to redeem dollars even if there is no free collateral in the contract.

Code Snippet

Tool used

Manual Review

Recommendation

A mechanism must be used to not allow the adding of duplicated collateral addresses.

The simplest thing to do is to introduce a mapping which tracks if a collateral address have already been added in the array:

  mapping(address collateralAddress => uint256 collateralAddressesIndex) collateralPosition;

+ function _isCollateralAddressDuplicated(address collateralAddress) internal returns (bool) {
+   bool isArrayEmpty = poolStorage.collateralAddresses.length == 0;
+   if (isArrayEmpty) return false;

+   uint256 collateralIndex = collateralPosition[collateralAddress];
+   if (collateralIndex > 0) return true;

+   return poolStorage.collateralAddresses[collateralIndex] == collateralAddress;
+ }

  function addCollateralToken(
      address collateralAddress,
      address chainLinkPriceFeedAddress,
      uint256 poolCeiling
  ) internal {
      UbiquityPoolStorage storage poolStorage = ubiquityPoolStorage();
+     if (_isCollateralAddressDuplicated(collateralAddress)) revert DuplicatedCollateralAddress();

      uint256 collateralIndex = poolStorage.collateralAddresses.length;
+     collateralPosition[collateralAddress] = collateralIndex;
      ...
  }

Duplicate of #27

sherlock-audit / 2023-12-ubiquity-judging

ilchovski - Admin can break the protocol logic irreversibly by adding the same collateral twice #145