Bauchibred - Slippage check in `redeemDollar` is wrongly applied

[sherlock-admin]

Bauchibred

medium

Slippage check in redeemDollar is wrongly applied

Proof of Concept

Here is how an attempt on redemption need to be queried: UbiquityPoolFacet.sol#L92-L104

Now, take a look at LibUbiquityPool.sol#L399-L466

    function redeemDollar(
        uint256 collateralIndex,
        uint256 dollarAmount,
        uint256 collateralOutMin
    )
        internal
        collateralEnabled(collateralIndex)
        returns (uint256 collateralOut)
    {
        UbiquityPoolStorage storage poolStorage = ubiquityPoolStorage();

        require(
            poolStorage.isRedeemPaused[collateralIndex] == false,
            "Redeeming is paused"
        );

        // update Dollar price from Curve's Dollar Metapool
        LibTWAPOracle.update();
        // prevent unnecessary redemptions that could adversely affect the Dollar price
        require(
            getDollarPriceUsd() <= poolStorage.redeemPriceThreshold,
            "Dollar price too high"
        );

        uint256 dollarAfterFee = dollarAmount
            .mul(
                UBIQUITY_POOL_PRICE_PRECISION.sub(
                    poolStorage.redemptionFee[collateralIndex]
                )
            )
            .div(UBIQUITY_POOL_PRICE_PRECISION);

        // update collateral price
        updateChainLinkCollateralPrice(collateralIndex);

        // get collateral output for incoming Dollars
        collateralOut = getDollarInCollateral(collateralIndex, dollarAfterFee);

        // checks
        //@audit-issue
        require(
            collateralOut <=
                (IERC20(poolStorage.collateralAddresses[collateralIndex]))
                    .balanceOf(address(this))
                    .sub(poolStorage.unclaimedPoolCollateral[collateralIndex]),
            "Insufficient pool collateral"
        );
        require(collateralOut >= collateralOutMin, "Collateral slippage");

        // account for the redeem delay
        poolStorage.redeemCollateralBalances[msg.sender][
            collateralIndex
        ] = poolStorage
        .redeemCollateralBalances[msg.sender][collateralIndex].add(
                collateralOut
            );
        poolStorage.unclaimedPoolCollateral[collateralIndex] = poolStorage
            .unclaimedPoolCollateral[collateralIndex]
            .add(collateralOut);

        poolStorage.lastRedeemedBlock[msg.sender] = block.number;

        // burn Dollars
        IERC20Ubiquity ubiquityDollarToken = IERC20Ubiquity(
            LibAppStorage.appStorage().dollarTokenAddress
        );
        ubiquityDollarToken.burnFrom(msg.sender, dollarAmount);
    }

Evidently, this function is used to burn the redeemable Ubiquity Dollars and send back the collateral token for every Ubiquity Dollar burned, now as tagged by the "@audit", during the execution there is a slippage check

Now, issue is that right before this line there is a different slippage check (or a check to ensure that enough collateral exists), but this check uses the wrong value for collaterals, i.e collateralOut instead of collateralOutMin, what this essentially leads to is that whenever collateralOutMin <= (IERC20(poolStorage.collateralAddresses[collateralIndex])).balanceOf(address(this)).sub(poolStorage.unclaimedPoolCollateral[collateralIndex]) < collateralOut the transaction reverts, where as it shouldn't since there is available collateral to redeem as much as collateralOutMin which is user's accepted slippage value

Impact

Asides what's been explained in Proof of Concept Slippage is not user provided value, so in the case where the collateral token is dropping in USD price and user wants to sell off their tokens, they can't do that cause the execution would revert leading to users to lose assets in US$ value.

Code Snippet

https://github.com/sherlock-audit/2023-12-ubiquity/blob/d9c39e8dfd5601e7e8db2e4b3390e7d8dff42a8e/ubiquity-dollar/packages/contracts/src/dollar/libraries/LibUbiquityPool.sol#L399-L466

Recommended Mitigation Steps

Modify the checks, first check that collateralOut >= collateralOutMin then check to see if there is enough pool collateral for collateralOut, if no, then check if there is enough pool collateral for collateralOutMin

[sherlock-admin2]

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

auditsea commented:

Makes no difference

[sherlock-admin2]

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

auditsea commented:

Makes no difference

[nevillehuang]

Invalid, checks are correct, no issue here