ArmedGoose - `mintingFee` and `redemptionFee` are not used and are not withdrawable causing protocol to miss profit off it

[sherlock-admin2]

ArmedGoose

medium

mintingFee and redemptionFee are not used and are not withdrawable causing protocol to miss profit off it

Summary

In UbiquityPoolFacet, implemented in LibUbiquityPool, there are variables and some logic for collecting mintingFee and redemptionFee. However this logic is nonfunctional due to a fact, that these fees are not accounted anywhere and there is also no logic allowing for withdrawing / utilizing these fees. In fact, these fees just increase the contract's collateral balance, which causes the protocol to miss any profit that may come off the fees.

Vulnerability Detail

UbiquityPoolFacet allows for core operations of minting and redeeming Ubiquity dollars. Each of those operations includes a fee, which is deducted off the amount user inputs, for mint, the minted amount is decreased by mintingFee, which means that fee part of deposited collateral is received by the protocol without minting dollars in exchange, and in redeem, similarly, the amount redeemed is diminished by the fee amount, which also means, that fee part of the collateral stays in the contract instead of being fully returned for ubiquity dollars. There is also no withdraw() or similar function that could allow for extraction of the user fees - it simply stays on the contract.

As an effect, the protocol gains surplus collateral stored on the contract, but this fee is not utilized or even accounted, and thus protocol cannot benefit off it.

Impact

The protocol misses the revenue from fees which is a financial loss.

Code Snippet

    function mintDollar(
        uint256 collateralIndex,
        uint256 dollarAmount,
        uint256 dollarOutMin,
        uint256 maxCollateralIn
    )
        internal
        collateralEnabled(collateralIndex)
        returns (uint256 totalDollarMint, uint256 collateralNeeded)
    {
        UbiquityPoolStorage storage poolStorage = ubiquityPoolStorage();

        require(
            poolStorage.isMintPaused[collateralIndex] == false,
            "Minting is paused"
        );

        // update Dollar price from Curve's Dollar Metapool
        LibTWAPOracle.update();
        // prevent unnecessary mints
        require(
            getDollarPriceUsd() >= poolStorage.mintPriceThreshold,
            "Dollar price too low"
        );

        // update collateral price
        updateChainLinkCollateralPrice(collateralIndex);

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

        // subtract the minting fee
        totalDollarMint = dollarAmount
            .mul(
                UBIQUITY_POOL_PRICE_PRECISION.sub(
                    poolStorage.mintingFee[collateralIndex] //@audit nothing happens with the fee
                )
            )
            .div(UBIQUITY_POOL_PRICE_PRECISION);
[...]

    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] //@audit nothing happens with the fee
                )
            )
            .div(UBIQUITY_POOL_PRICE_PRECISION);
[...]

Tool used

Manual Review

Recommendation

1) Implement a variable(s) that could track accumulated fees, after each deduction add the newly deducted fees to these variables for accountability and 2) consider implementing a logic to administer these fees e.g. send them to a treasury, allow withdrawal, etc. Make sure to treat them separately from the contract balance so they are not used for minting, borrowing, or lost in any other way.

[sherlock-admin2]

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

auditsea commented:

The goal of fee taken for mint/redeem is to hedge the risk of price deviation

[sherlock-admin2]

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

auditsea commented:

The goal of fee taken for mint/redeem is to hedge the risk of price deviation

[rndquu]

@pavlovcik

There are mint and redeem fees in the Ubiquity pool contract. Initially (as far as I remember) we planned to set those fees to 0. Right now there is no way for admin to withdraw mint and redeem fees so these fees serve (as some sherlock judge already stated) to "hedge the risk of price deviation" (i.e. locked in the pool). Should the admin be able to withdraw mint and redeem fees?

[0x4007]

Should the admin be able to withdraw mint and redeem fees?

I hadn't thought through this before. Following the approach of all of our other contracts, I think that having the flexibility to do this makes sense. However, given that I am just now hearing/thinking about this, perhaps the fee profits are abstracted away by lowering how much the "protocol owes."

A stablecoin is just a balance sheet of assets and liabilities. Assets means collateral in the bank, liabilities means how many Dollar tokens (think of these as just "I owe you" tokens to redeem $1.00 of underlying collateral.)

Example: if we burn 100% of the circulating supply of "I owe you" tokens, then we "profited" 100% of the collateral, as nobody (except for us) can redeem the collateral anymore.

So I am not 100% up-to-speed on how the mint/redeem fees are implemented, but if they burn more Dollars, then technically we are making a profit. In which case, perhaps we don't need to implement "fee withdrawal".

[rndquu]

So I am not 100% up-to-speed on how the mint/redeem fees are implemented, but if they burn more Dollars, then technically we are making a profit. In which case, perhaps we don't need to implement "fee withdrawal".

Technically those fees are not burning anything in terms of ERC20.burn() but "burning" in terms of fees:

1. On mintDollar() user gets less Dollars for provided collateral with mint fee enabled
2. On redeemDollar user gets less collateral for provided Dollars with redeem fee enabled

So both those actions reduce the amount of "I owe you" tokens hence this is equal to "burning" them which lowers the Dollar/USD quote in the end.

Since the contracts are upgradeable and we don't plan to withdraw fees I think we should save time and mark this issue as valid and "won't fix".

@gitcoindev @molecula451 What do you guys think?

[gitcoindev]

So I am not 100% up-to-speed on how the mint/redeem fees are implemented, but if they burn more Dollars, then technically we are making a profit. In which case, perhaps we don't need to implement "fee withdrawal".

Technically those fees are not burning anything in terms of ERC20.burn() but "burning" in terms of fees:

1. On mintDollar() user gets less Dollars for provided collateral with mint fee enabled
2. On redeemDollar user gets less collateral for provided Dollars with redeem fee enabled

So both those actions reduce the amount of "I owe you" tokens hence this is equal to "burning" them which lowers the Dollar/USD quote in the end.

Since the contracts are upgradeable and we don't plan to withdraw fees I think we should save time and mark this issue as valid and "won't fix".

@gitcoindev @molecula451 What do you guys think?

I am fine with that, let's mark as valid but won't fix.

[jingyi2811]

Escalate

Fees can still be borrowed by an AMO Minter; this implies that fees can be collected.

As a trusted role, the AMO Minter has extensive capabilities, including the potential to manage these fees and even transfer them to the fee recipient. So it is withdrawable to the fee recipient.

Modified the existing testCollectRedemption_ShouldCollectRedemption test unit to prove that AMO minter can move the fee. See @audit

function testCollectRedemption_ShouldCollectRedemption() public {
        vm.prank(admin);
        ubiquityPoolFacet.setPriceThresholds(
            1000000, // mint threshold
            1000000 // redeem threshold
        );

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

        // user redeems 99 Dollars for collateral
        vm.prank(user);
        ubiquityPoolFacet.redeemDollar(
            0, // collateral index
            99e18, // Dollar amount
            90e18 // min collateral out
        );

        // wait 3 blocks for collecting redemption to become active
        vm.roll(3);

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

        vm.prank(user);
        uint256 collateralAmount = ubiquityPoolFacet.collectRedemption(0);
        assertEq(collateralAmount, 97.02e18); // $99 - 2% redemption fee

        // balances after
        assertEq(
            collateralToken.balanceOf(address(ubiquityPoolFacet)),
            2.98e18 // @audit this is fee balance
        );

        // @audit Customize code here
        vm.prank(address(dollarAmoMinter));
        ubiquityPoolFacet.amoMinterBorrow(2.98e18); // @audit Amo minter tries to move all the fee balance

        assertEq(
            collateralToken.balanceOf(address(ubiquityPoolFacet)),
            0 // @audit All fee balance is successfully moved
        );
    }

[sherlock-admin2]

Escalate

Fees can still be borrowed by an AMO Minter; this implies that fees can be collected.

As a trusted role, the AMO Minter has extensive capabilities, including the potential to manage these fees and even transfer them to the fee recipient. So it is withdrawable to the fee recipient.

Modified the existing testCollectRedemption_ShouldCollectRedemption test unit to prove that AMO minter can move the fee. See @audit

function testCollectRedemption_ShouldCollectRedemption() public {
        vm.prank(admin);
        ubiquityPoolFacet.setPriceThresholds(
            1000000, // mint threshold
            1000000 // redeem threshold
        );

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

        // user redeems 99 Dollars for collateral
        vm.prank(user);
        ubiquityPoolFacet.redeemDollar(
            0, // collateral index
            99e18, // Dollar amount
            90e18 // min collateral out
        );

        // wait 3 blocks for collecting redemption to become active
        vm.roll(3);

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

        vm.prank(user);
        uint256 collateralAmount = ubiquityPoolFacet.collectRedemption(0);
        assertEq(collateralAmount, 97.02e18); // $99 - 2% redemption fee

        // balances after
        assertEq(
            collateralToken.balanceOf(address(ubiquityPoolFacet)),
            2.98e18 // @audit this is fee balance
        );

        // @audit Customize code here
        vm.prank(address(dollarAmoMinter));
        ubiquityPoolFacet.amoMinterBorrow(2.98e18); // @audit Amo minter tries to move all the fee balance

        assertEq(
            collateralToken.balanceOf(address(ubiquityPoolFacet)),
            0 // @audit All fee balance is successfully moved
        );
    }

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[0xnirlin]

AMO is for generating yield not collecting fees, if something is done through AMO, it doesn't mean it should be. But the bigger problem is the fee is not tracked anywhere either it is just deducted as mentioned in issue #73

[0xadrii]

I discussed this with a team member during the audit. The team was aware of this issue and just assumed that the fees would be withdrawn using the AMO minter. Although I agree with the fact that protocol fees should always be properly tracked, no fees will be lost given that the Ubiquity team will always be able to use the AMO minter to borrow the fees. Moreover, the amount of fees accrued by the protocol does not impact the protocol mechanics in any way.

From Sherlock's Criteria for Issue Validity:

V. How to identify a medium issue: Causes a loss of funds but requires certain external conditions or specific states. Breaks core contract functionality, rendering the contract useless (should not be easily replaced without loss of funds) or >leading to unknown potential exploits/loss of funds. Ex: Unable to remove malicious user/collateral from the contract. A material loss of funds, no/minimal profit for the attacker at a considerable cost

In this case, no funds are lost given that the Ubiquity team is capable of withdrawing them, not complying with Sherlock's criteria for a medium impact.

Because of all of this, I believe that the impact should be Low at most.

[0xnirlin]

How it isn't loss of funds when the fee is not even being tracked?

Secondly above comments by sponsors directly show that they weren't aware of the problem.

[0xadrii]

How it isn't loss of funds when the fee is not even being tracked?

Secondly above comments by sponsors directly show that they weren't aware of the problem.

A loss of funds would mean the protocol is not able to withdraw the fees in ANY WAY (however in this specific situation the AMO minter, which is controlled by the team, can do so). As I said before, I understand that not tracking fees is a completely wrong way of handling the fees. The protocol team was not aware of the fact that doing so is the proper way of handling fees, however they were indeed aware of the fact that the AMO minter is capable of withdrawing them.

At least that is my pov considering Sherlock's rules.

[0xLogos]

Escalate Informational imho Redo escalation because previous escalation do not have verdict (not sure if it's important, but anyway). My point is that collected fees are not useless (as mentioned by auditsea, to hedge the risk of price deviation) and you can't know for sure if it is intended also as protocol revenue or not (no public info during audit about that). If fees also protocol revenue, we can assume that amo minter withdraw funds (deposits + fee) and it's yield is net protocol revenue (you do not have to track fees separately). It's speculaltion, but not worse than the one proposed in the report ("protocol misses the revenue from fees")

[sherlock-admin2]

Escalate Informational imho Redo escalation because previous escalation do not have verdict (not sure if it's important, but anyway). My point is that collected fees are not useless (as mentioned by auditsea, to hedge the risk of price deviation) and you can't know for sure if it is intended also as protocol revenue or not (no public info during audit about that). If fees also protocol revenue, we can assume that amo minter withdraw funds (deposits + fee) and it's yield is net protocol revenue (you do not have to track fees separately). It's speculaltion, but not worse than the one proposed in the report ("protocol misses the revenue from fees")

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[ArmedGoose]

There are multiple arguments from different watsons. Let ma answer each of them - I stand at position this finding is a valid one. There is nothing less to add to the finding description from my side, but I would like to disagree with all the escalations below. Separating per watson because watsons raised different arguments, so I believe its better to answer each concern directly for clarity.

jingyi2811

• Fees can still be borrowed by an AMO Minter; this implies that fees can be collected.

Fees cannot be known, because they are not tracked. But assuming the protocol team spends multiple weeks scripting offchain fee calculating solution (which is a loss of funds due to labour usage), I still do not believe AMO minter should be considered trusted facing that 25 wardens submitted it as insecure and pointed that it's too lenient in its possibility to withdraw funds. Since its going to get redesigned, I do not think relying on it is secure anymore. Also elaborating on AMO minter below.

0xadrii

• The team was aware of this issue and just assumed that the fees would be withdrawn using the AMO minter

This watson in the same time submitted here that the AMO minter utility is insecure, recommending that it allows for borrowing only "borrowable amount", along ~25 other watsons concerned about that utility. Therefore I believe we should not rely on its current state as its likely to get redesigned facing such strong concern. I feel like saying "AMO minter can withdraw anyway" is like saying, for a contract with hypotethical reentrancy issue: "hey we have a reentrancy here, so funds are never locked, because you can use reentrancy to withdraw". Also I allow myself to question the fact the team assumed this as a withdrawal method, because above it's said in the team comments that the team was not really considering this scenario.

• Moreover, the amount of fees accrued by the protocol does not impact the protocol mechanics in any way.

Yes, they do not impact protocol mechanics, but they impact potential team revenue. The finding is about overlooked architecture design, which causes the team to lose potential profit. It's never said any mechanic is flawed; rather it is missing.

0xLogos

• collected fees are not useless (as mentioned by auditsea, to hedge the risk of price deviation)

I do not believe this is a valid argument, first, still the protocol misses profit off the fees. Even if we say "they loose but its not totally in vain", they still don't receive fees. Also the term fees is used in many protocols, in a commonly understandable context: a small portion of volume is deducted to fund the team or other protocol-related goals. We should not redefine defi in the beginning, questioning everything. I think that, if the dusts were intentionally left on the contract, we wouldn't have any fees-related logic here. For me its clear, that fees here are designed with proper variables and a setter mechanism, but they lack accountability, thats all. Moreover, trying to turn it into a hedging logic is trying to find a creative justification to current situation, I don't believe that using fees which usually aren't higher than 1 or 2% MAX of protocol volume can be used as a hedge, it is simply too low amount to hedge anything.

• you can't know for sure if it is intended also as protocol revenue or not (no public info during audit about that)

it is not intended, the protocol spoke above. They acknowledge but they haven't thought about what to do with it yet.

• If fees also protocol revenue, we can assume that amo minter withdraw funds (deposits + fee) and it's yield is net protocol revenue (you do not have to track fees separately)

You have to account, otherwise you will withdraw funds that belong to other protocol task. I discussed AMO minter cases above. The AMO minter reported issue basically says that it should not consider funds that shouldnt be available, and you cannot know which fees are not available unless you account them, in my opinion.

[0xLogos]

it is simply too low amount to hedge anything

I think it can hedge dai and lusd price deviation because it is quite low.

it is not intended, the protocol spoke above. They acknowledge but they haven't thought about what to do with it yet.

There is no loss of funds nor misbehavior. When the developers find a suitable use for these funds, they will simply upgrade the contract.

[ArmedGoose]

Hello. Let me answer below.

I think it can hedge dai and lusd price deviation because it is quite low.

Since I, as a watson, was obligated to give an explanation and prove that the issue exist, I believe, that a counterargument should also be supported by a proof - it is not possible to prove, that this logic is a valid hedging mechanics because nobody designed it to do so. It is just loose declaration of team that they might throw their fees into the protocol, to maybe hedge. Are you able to provide a calculation how effective is such hedge, and confirm with the team, that it is their intended and valid scenario they use in the protocol? There was nothing in the docs about a hedging logic, this is just a creative argument. Otherwise please provide a Poc of working hedging mechanics, how and to what degree it works. I never heard of using a fees as a hedge logic in any defi, there are various remove dust utilities, or fee collectors, but this is first time I heard something like this, this does not sound like a legitimate solution.

There is no loss of funds nor misbehavior. When the developers find a suitable use for these funds, they will simply upgrade the contract.

Upgrading the contract to move these funds will mean that the team took action to recover these fees, because they are wanted, which means there is an issue which requires fixing, and the upgrade to withdraw them will be the fix. If we speak that the team might want to reach these fees, that means, if they cannot do so, they lose them. If they upgrade to do it, that's a fix of this vulnerability, which means, it was an issue that required upgrade action.

[0xLogos]

https://docs.sherlock.xyz/audits/judging/judging#vii.-list-of-issue-categories-that-are-not-considered-valid

p.8

... Funds are temporarily stuck and can be recovered by the administrator or owner. Also, if a submission is a design opinion/suggestion without any clear indications of loss of funds is not a valid issue.

Maybe applicable here, anyway it's better to wait some comments from judges

[ArmedGoose]

Sure lets wait for judge opinion. Anyway I think that's not the case here because I think its about a case where there IS a legitimate withdraw utility, if you assume that this can be done after contract upgrade, then I think, you can say this about everything, but the fact of upgrade in my opinion will be applying a fix. Looking forward for the judge to comment too.

[nevillehuang]

Since fees can be utilized by the protocol via the AMO without being locked, I believe this issue is low severity.

[Evert0x]

It's true that fees can be recovered by the admin using the AMO, so they will not be lost.

However, it's still an accounting issue as the fees are not being tracked.

Will get back to this.

[Czar102]

I believe it doesn't make much sense to introduce a function used to withdraw fees with the logic equivalent to the AMO minter withdraw. AMO minters can watch fees off-chain (including fees due to mint/redeem thresholds) and withdraw additionally that amount, it is not a vulnerability to track these values off-chain by a trusted party. This may have been a design choice, from my understanding.

Planning to accept the escalation and consider this a low/informational issue.

[CergyK]

I believe it doesn't make much sense to introduce a function used to withdraw fees with the logic equivalent to the AMO minter withdraw. AMO minters can watch fees off-chain (including fees due to mint/redeem thresholds) and withdraw additionally that amount, it is not a vulnerability to track these values off-chain by a trusted party. This may have been a design choice, from my understanding.

Planning to accept the escalation and consider this a low/informational issue.

There is an additional problem: Since fees are taken as a piece of collateral, and not accounted for in freeBalance. Theoretically normal users can simply redeem all of the balance of a given collateral and there would be no fees for the protocol (there would need to be a disparity between TWAP price and chainlink price for it to happen but possible)

The fix is quite simple, account for the fees and subtract them from free balance

[Czar102]

Theoretically normal users can simply redeem all of the balance of a given collateral and there would be no fees for the protocol (there would need to be a disparity between TWAP price and chainlink price for it to happen but possible)

@CergyK I'm not following, from my understanding for every nonzero mint/redeem thresholds' spread, the protocol will have earnings.

[CergyK]

Theoretically normal users can simply redeem all of the balance of a given collateral and there would be no fees for the protocol (there would need to be a disparity between TWAP price and chainlink price for it to happen but possible)

@CergyK I'm not following, from my understanding for every nonzero mint/redeem thresholds' spread, the protocol will have earnings.

Yes if TWAP and chainlink are mostly in sync (which is not guaranteed).

But even if we consider TWAP and chainlink are always in-sync to some extent, there may be a disparity between the availability of two collaterals:

• let's say we have 30 uAD minted for Alice, backed by 10 DAI and 25 LUSD, uAD is overcollateralized and everything is fine.

• Protocol has calculated offchain that they have the right to claim 1 DAI as fees and 3 LUSD as fees.

• Alice proceeds to redeem some of her uAD against all of the DAI in the reserve.

The protocol cannot withdraw DAI fees

[Czar102]

So fees are a part of the collateral, I can't see how is this an exploit or a bug. It seems the user has a choice of the collateral withdrawn and that's by design.

[CergyK]

So fees are a part of the collateral, I can't see how is this an exploit or a bug. It seems the user has a choice of the collateral withdrawn and that's by design.

The bug would be that the protocol is unable to withdraw fees

[Czar102]

From my understanding, the fees would have been swapped from DAI to LUSD and the protocol would be able to withdraw them in LUSD. Is that accurate?

[0xadrii]

Hey @CergyK let me know if I'm missing something, but I believe whenever a deposit is performed the fee is always applied, meaning that the full 10 DAI won't be considered as collateral. Instead, the considered amount to mint uAD will actually be the 10 DAI minus the fee. This makes it impossible for Alice to then obtain the 10 DAI back when redeeming, given that the uAD amount she obtained minting was not worth 10 DAI but a bit less (due to the fee). I believe Alice won't be able to obtain the full 10 DAI back (given that the fee was applied when minting), which makes the pool always have an idle amount of collateral that the AMO minter can borrow.

[CergyK]

From my understanding, the fees would have been swapped from DAI to LUSD and the protocol would be able to withdraw them in LUSD. Is that accurate?

Yes agreed

Hey @CergyK let me know if I'm missing something, but I believe whenever a deposit is performed the fee is always applied, meaning that the full 10 DAI won't be considered as collateral. Instead, the considered amount to mint uAD will actually be the 10 DAI minus the fee. This makes it impossible for Alice to then obtain the 10 DAI back when redeeming, given that the uAD amount she obtained minting was not worth 10 DAI but a bit less (due to the fee). I believe Alice won't be able to obtain the full 10 DAI back (given that the fee was applied when minting), which makes the pool always have an idle amount of collateral that the AMO minter can borrow.

Please reread the situation outlined above with two collaterals, to understand how the situation may arise with non-zero minting and redeeming fees.

[Czar102]

Planning to accept the escalation and consider this issue and duplicates informational.

[ubiquibot[bot]]

! No price label has been set. Skipping permit generation.

[Czar102]

Result: Low Has duplicates

Rejecting the second escalation; thank you for adding a verdict but can't penalize the Lead Judge by accepting two escalations – there is a single modification made to the issue and duplicates.

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• jingyi2811: accepted
• 0xLogos: rejected