amoMinterBorrow() improperly allows borrowing unclaimedPoolCollateral, breaking protocol functionality

Summary

The amoMinterBorrow() does not consider the collateral that has been queued for redeems, which potentially makes users unable to access collateral and cause denial of service in some functions.

Vulnerability Detail

Ubiquity allows AMO (Automatic Market Operations) minters to borrow the collateral deposited in the protocol in order to make yield on external protocols. In order to allow such functionality, UbiquityPoolFacet.sol incorporates an amoMinterBorrow() function, which internally calls LibUbiquityPool.sol's amoMinterBorrow() :

// UbiquityPoolFacet.sol

function amoMinterBorrow(uint256 collateralAmount) external {
    LibUbiquityPool.amoMinterBorrow(collateralAmount);
}

// LibUbiquityPool.sol

function amoMinterBorrow(uint256 collateralAmount) internal onlyAmoMinter {
    UbiquityPoolStorage storage poolStorage = ubiquityPoolStorage();

    // checks the collateral index of the minter as an additional safety check
    uint256 minterCollateralIndex = IDollarAmoMinter(msg.sender)
        .collateralIndex();

    // checks to see if borrowing is paused
    require(
        poolStorage.isBorrowPaused[minterCollateralIndex] == false,
        "Borrowing is paused"
    );

    // ensure collateral is enabled
    require(
        poolStorage.isCollateralEnabled[
            poolStorage.collateralAddresses[minterCollateralIndex]
        ],
        "Collateral disabled"
    );

    // transfer
    IERC20(poolStorage.collateralAddresses[minterCollateralIndex])
        .safeTransfer(msg.sender, collateralAmount);
  }

As shown in the code snippet, amoMinterBorrow() allows any AMO minter to borrow an arbitrary collateralAmount amount from Ubiquity’s total deposited collateral.

On the other hand, Ubiquity redeem process is split in two steps:

Users call redeemDollar(), which queues a desired collateral amount to be redeemed and burns the redeemer’s uAD. The queued collateral amount to be redeemed is tracked individually for the redeemer in the poolStorage.redeemCollateralBalances mapping, and globally in the poolStorage.unclaimedPoolCollateral mapping.
Once the minimum redemption delay blocks have passed, users can claim their queued balance by calling collectRedemption()

Once the borrowing and redeeming mechanics are understood, we can acknowledge an issue regarding the amounts allowed to be borrowed in amoMinterBorrow(). As mentioned before, an arbitrary collateralAmount amount can be borrowed by the AMO minters. The problem with this approach is that borrowing **does not consider the currently queued unclaimed pool collaterals.** This makes two problems arise:

AMO minters are susceptible of leaving the contract without enough funds for redeemers to be able to collect their queued redemptions because no limit is imposed in amoMinterBorrow(). This will prevent users from withdrawing their queued collateral, even if the minimum redemption delay has passed.
If AMO minters borrow a higher amount than the current poolStorage.unclaimedPoolCollateral , minting will be DoS’ed due to using freeCollateralBalance(). Let’s examine the freeCollateralBalance() function, which returns the free collateral balance (i.e the amount that can be borrowed by AMO minters):
```
function freeCollateralBalance(
        uint256 collateralIndex
    ) internal view returns (uint256) {
        UbiquityPoolStorage storage poolStorage = ubiquityPoolStorage();
        return
            IERC20(poolStorage.collateralAddresses[collateralIndex])
                .balanceOf(address(this))
                .sub(poolStorage.unclaimedPoolCollateral[collateralIndex]);
    }
```
As we can see, poolStorage.unclaimedPoolCollateral[collateralIndex] will be substracted from Ubiquity’s current collateral balance (IERC20(poolStorage.collateralAddresses[collateralIndex]).balanceOf(address(this))). Because AMO minters have no restriction when borrowing, they can leave the contract with a balance smaller than the actual amount stored in poolStorage.unclaimedPoolCollateral[collateralIndex] (i. e (IERC20(poolStorage.collateralAddresses[collateralIndex]).balanceOf(address(this)) < poolStorage.unclaimedPoolCollateral[collateralIndex]). If such situation takes place, freeCollateralBalance() will always throw an underflow error due to the substraction performed in the function.

Given that freeCollateralBalance() is used in the mintDollar() function in order to check the pool’s ceiling, if such situation arises, minting will be effectively DoS’ed, rendering a crucial protocol mechanic unusable:

// LibUbiquityPool.sol

function mintDollar(
        uint256 collateralIndex,
        uint256 dollarAmount,
        uint256 dollarOutMin,
        uint256 maxCollateralIn
    )
        internal
        collateralEnabled(collateralIndex)
        returns (uint256 totalDollarMint, uint256 collateralNeeded)
    {
        ...
        // check the pool ceiling
        require(
            freeCollateralBalance(collateralIndex).add(collateralNeeded) <=
                poolStorage.poolCeilings[collateralIndex],
            "Pool ceiling"
        );

        ...
    }

Impact

High. It is true that although users are prevented from obtaining their funds after waiting for the redemption delay period, funds are not stuck forever and can be recovered when AMO minters return funds back to the protocol.

However, as stated in this report, this vulnerability can potentially DoS uAD minting, which is a crucial mechanism to keep uAD’s peg to $1. If such operation is DoS’ed, the main goal of the protocol (keeping uAD’s peg with USD) will not be fulfilled, which is actually of HIGH impact.

Proof of Concept

The following proof of concept illustrates how a redeemer won’t be able to collect the amount borrowed by the amo minter. In order to run it, just enter the ubiquity-dollar/packages/contracts folder in your terminal and add the function showed below into UbiquityPoolFacet.t.sol . Finally, execute the following command to run the PoC: forge test --mt testVuln_amoMinterBorrowAllowsBorrowingUnclaimedPoolCollateral -vv.

// UbiquityPoolFacet.t.sol

function testVuln_amoMinterBorrowAllowsBorrowingUnclaimedPoolCollateral() public {
        // Step 1. Become admin and set mint and redeem thresholds
        vm.prank(admin);
        ubiquityPoolFacet.setPriceThresholds(
            1000000, // mint threshold
            1000000 // redeem threshold
        );

        // Step 2. User sends 100 collateral tokens and gets 99 Dollars (1% mint fee)
        vm.startPrank(user);
        ubiquityPoolFacet.mintDollar(
            0, // collateral index
            100e18, // Dollar amount
            99e18, // min amount of Dollars to mint
            100e18 // max collateral to send
        );

        // Step 3. user queues a redeem of 99 uAD 
        ubiquityPoolFacet.redeemDollar(
            0, // collateral index
            99e18, // Dollar amount
            90e18 // min collateral out
        );

        // Step 4. Wait 3 blocks for collecting redemption to become active
        vm.roll(3);

        // Step 5. Borrow being dollarAmoMinter before user actually collecting redemption
        vm.startPrank(address(dollarAmoMinter));
        ubiquityPoolFacet.amoMinterBorrow(100e18);

        // balances before
        assertEq(collateralToken.balanceOf(address(dollarAmoMinter)), 100e18);
        assertEq(collateralToken.balanceOf(user), 0);

        // Step 6. User won't be able to collect the redemption amount due to AMO minter having borrowed it
        vm.startPrank(address(user));
        vm.expectRevert("ERC20: transfer amount exceeds balance");
        ubiquityPoolFacet.collectRedemption(0);

    }

Code Snippet

https://github.com/sherlock-audit/2023-12-ubiquity/blob/main/ubiquity-dollar/packages/contracts/src/dollar/facets/UbiquityPoolFacet.sol#L124

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

Tool used

Manual Review, foundry

Recommendation

Check the freeCollateralBalance() in amoMinterBorrow() so that AMO minters cannot borrow an amount greater than the actual borrowable amount (amount that considers theunclaimedPoolCollateral):

function amoMinterBorrow(uint256 collateralAmount) internal onlyAmoMinter {
        UbiquityPoolStorage storage poolStorage = ubiquityPoolStorage();

        // checks the collateral index of the minter as an additional safety check
        uint256 minterCollateralIndex = IDollarAmoMinter(msg.sender)
            .collateralIndex();

        // checks to see if borrowing is paused
        require(
            poolStorage.isBorrowPaused[minterCollateralIndex] == false,
            "Borrowing is paused"
        );

        // ensure collateral is enabled
        require(
            poolStorage.isCollateralEnabled[
                poolStorage.collateralAddresses[minterCollateralIndex]
            ],
            "Collateral disabled"
        );

+        // ensure borrowed amount is not greater than free collateral balance
+       require(collateralAmount <= freeCollateralBalance(), "Amount is greater than freeCollateralBalance");

        // transfer
        IERC20(poolStorage.collateralAddresses[minterCollateralIndex])
            .safeTransfer(msg.sender, collateralAmount);
    }

Duplicate of #1

sherlock-audit / 2023-12-ubiquity-judging

0xadrii - amoMinterBorrow() improperly allows borrowing unclaimedPoolCollateral, breaking protocol functionality #127