The collateralToken can be duplicated in LibUbiquityPool.

Summary

In LibUbiquityPool.sol#addCollateralToken function, it didn't check duplication of collateral. So the collateralToken can be duplicated and it is impossible to solve this problem.

Vulnerability Detail

LibUbiquityPool.sol#addCollateralToken function is as follows.

    function addCollateralToken(
        address collateralAddress,
        address chainLinkPriceFeedAddress,
        uint256 poolCeiling
    ) internal {
        UbiquityPoolStorage storage poolStorage = ubiquityPoolStorage();

        uint256 collateralIndex = poolStorage.collateralAddresses.length;

        // add collateral address to all collaterals
        poolStorage.collateralAddresses.push(collateralAddress);

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

        // set collateral initially to disabled
        poolStorage.isCollateralEnabled[collateralAddress] = false;

        // add in the missing decimals
        poolStorage.missingDecimals.push(
            uint256(18).sub(ERC20(collateralAddress).decimals())
        );

        // add in the collateral symbols
        poolStorage.collateralSymbols.push(ERC20(collateralAddress).symbol());

        // initialize unclaimed pool collateral
        poolStorage.unclaimedPoolCollateral.push(0);

        // initialize paused prices to $1 as a backup
        poolStorage.collateralPrices.push(UBIQUITY_POOL_PRICE_PRECISION);

        // set fees to 0 by default
        poolStorage.mintingFee.push(0);
        poolStorage.redemptionFee.push(0);

        // handle the pauses
        poolStorage.isMintPaused.push(false);
        poolStorage.isRedeemPaused.push(false);
        poolStorage.isBorrowPaused.push(false);

        // set pool ceiling
        poolStorage.poolCeilings.push(poolCeiling);

        // set price feed address
        poolStorage.collateralPriceFeedAddresses.push(
            chainLinkPriceFeedAddress
        );

        // set price feed staleness threshold in seconds
        poolStorage.collateralPriceFeedStalenessThresholds.push(1 days);
    }

As we can see above, newly added collateralAddress is not checked for duplication. But in L642 it sets collateralIndex of newly added collateralAddress. So if you add same collateral twice, poolStorage.collateralIndex[collateralAddress] indicates secondly added index. Therefore, in LibUbiquityPool.sol#collateralInformation, setCollateralChainLinkPriceFeed it is impossible to operate with collateralIndex added first and we cannot delete that index.

Impact

The collateral can be duplicated so it can cause unexpected errors.

Code Snippet

https://github.com/sherlock-audit/2023-12-ubiquity/blob/main/ubiquity-dollar/packages/contracts/src/dollar/libraries/LibUbiquityPool.sol#L639-L642

Tool used

Manual Review

Recommendation

LibUbiquityPool.sol#addCollateralToken function has to check duplication of collateral as follows.

    function addCollateralToken(
        address collateralAddress,
        address chainLinkPriceFeedAddress,
        uint256 poolCeiling
    ) internal {
        UbiquityPoolStorage storage poolStorage = ubiquityPoolStorage();

+       for(uint256 i; i < poolStorage.collateralAddresses.length; ){
+           require(poolStorage.collateralAddresses[i] != collateralAddress, "Collateral is duplicated");
+           unchecked{
+               i++;
+           }
+       }

        ...
    }

Duplicate of #27

sherlock-admin2 commented 8 months ago

1 comment(s) were left on this issue during the judging contest.

auditsea commented:

This issue describes about collateral duplication by admin function call, not important