Chinmay - Borrower can lose fees from Uniswap Positions when an Aloe market is paused

[sherlock-admin]

Chinmay

high

Borrower can lose fees from Uniswap Positions when an Aloe market is paused

Summary

An aloe market can be paused for 30 minutes suspecting manipulation of the associated uniswap pool, and when it is paused a position cannot be modified. Now fees collection for the borrower contract's position is only possible via uniswapWithdraw which is not callable when market is paused. This poses a risk of borrower losing his large amounts of fees(especially when he has large positions).

Vulnerability Detail

Borrowers with large position fees may lose fees when they have not collected fees in a long time and then the aloe market is paused. During the pause window, if the borrower's fees had grown to a large enough value before, it can be near and overflow type(uint128).max during the pause window and the borrower will not be able to collect the fees and hence overflowing will make the accrued fees very small. See overflow condition : See v3-core-Position.sol.

For the fees to grow, it has to be accrued via a mint or burn call. Suppose an attacker is monitoring a borrower contract which has large positions and is anticipated to earn large fees when collected, he accrues the fees in a zero liquidity mint call. Now even if the borrower notices this, he cannot frontrun this to collect his fees because the aloe market is paused and there is no way to collect the fees.

Another problem is that the mechanism forces a borrower to withdraw his liquidity to be able to collect the already accrues fees (See Borrower.sol Line 548) and there is no other way to collect the fees. ( pool.collect : msg.sender is required to call collect and our borrower contract has no other function available other than withdrawing all his liquidity)

The solution to all the above problems is to have a separate collectFees mechanism in place which is callable even if aloe market is paused. This is safe because pool.collect doesn't accrue new pending fees, it just collects the already stored fees. This way we will avoid forcing borrower to withdraw his Liquidity if he wants to collect fees, and he can colelct fees if it is going to overflow soon, even if the market is paused.

Impact

Clear Loss of fees to the borrower. Possibly, It can also impact the lender because fees is included in the aggregate asset balances used in paying the divergence loss and if it overflows while accruing, it will not get added in the aggregate asset balances. It can provide a safety to the repayment assets.

Added to this, the chances of overflowing are even higher because it is forced to burn before collecting, the burn itself adds all underlying liquidity into tokensOwed0 and tokensOwed1. See v3-core/Pool.sol

Code Snippet

https://github.com/sherlock-audit/2023-10-aloe/blob/b60c21af24738d517941f18f7caa8c7272f771c5/aloe-ii/core/src/Borrower.sol#L320

Tool used

Manual Review

Recommendation

Add a function to collect fees for the uniswap position usable anytime even if aloe market is paused.

[sherlock-admin2]

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

panprog commented:

low, because 30 minutes shouldn't be the time window which makes it possible for the user to lose fees due to overflow, if user lets it be this way, it's user's mistake which is at most low or invalid.

tsvetanovv commented:

Information

MohammedRizwan commented:

valid

[chinmay-farkya]

Escalate I believe this is a high severity issue because of the following reasons :

1. The position.tokenOwed0 or position.tokenOwed1 can overflow even during normal operation (so it doesn't require user mistake of not collecting his fees). See v3-core pool.sol : the sum of two uint128 numbers { ie. position.tokensOwed0 {which is accrued fees in this logic flow and uint128(amount0) which is the assets from the burned liquidity } is stored in a uint128 variable {position.tokensOwed0}. This addition can overflow to become a small number if the sum is near type(uint128).max. The problem is that aloe's mechanism forces borrower to burn liquidity to collect fees(that is the only option) which means very high chances of overflow because the burned liquidity amount is being added to tokenOwed0 which already has the accrued fees stored via _modifyPosition in the lines above. In aloe, this forced burn goes into liquidate function too, so if the token0Owed or token1Owed overflows {if burned liquidity assets + fees goes above uint128.max}, this means that the assets withdrawn from the position will be very small as compared to the real assets : which means that the position will have liabilities as it is but very low assets to cover it {ie. the withdrawn liquidity}, causing huge amount of bad debt. This is high severity because it can occur easily in normal operation and causes losses for Lenders.

2. In the case if manipulation has happened, it is equally dangerous because manipulation means heavy price movement which can cause heavy accrual of fees too if the liquidity is in favorable range. According to how TWAP manipulation works, the manipulated price has to be kept at the manipulated price for quite some time to actually influence the final TWAP for the full window. Since the protocol has mechanisms to protect against it, this means that it has fair chances of happening. Now if the manipulation moves asset prices in extreme ends, the amounts calculated for the underlying liquidity will also be extreme (either tokenOwed0 or tokenOwed1 as very big) So in such a condition, the the overflow can happen again in the burn function where the addition of position.tokensOwed0 ie. fees and amount0 (ie. underlying liquidity amounts happens) as linked above. So even in a 30 min twap window, these values can overflow, again leading to undervalued assets causing huge bad debt for the protocol and losses for the lenders.

[sherlock-admin2]

Escalate I believe this is a high severity issue because of the following reasons :

1. The position.tokenOwed0 or position.tokenOwed1 can overflow even during normal operation (so it doesn't require user mistake of not collecting his fees). See v3-core pool.sol : the sum of two uint128 numbers { ie. position.tokensOwed0 {which is accrued fees in this logic flow and uint128(amount0) which is the assets from the burned liquidity } is stored in a uint128 variable {position.tokensOwed0}. This addition can overflow to become a small number if the sum is near type(uint128).max. The problem is that aloe's mechanism forces borrower to burn liquidity to collect fees(that is the only option) which means very high chances of overflow because the burned liquidity amount is being added to tokenOwed0 which already has the accrued fees stored via _modifyPosition in the lines above. In aloe, this forced burn goes into liquidate function too, so if the token0Owed or token1Owed overflows {if burned liquidity assets + fees goes above uint128.max}, this means that the assets withdrawn from the position will be very small as compared to the real assets : which means that the position will have liabilities as it is but very low assets to cover it {ie. the withdrawn liquidity}, causing huge amount of bad debt. This is high severity because it can occur easily in normal operation and causes losses for Lenders.

2. In the case if manipulation has happened, it is equally dangerous because manipulation means heavy price movement which can cause heavy accrual of fees too if the liquidity is in favorable range. According to how TWAP manipulation works, the manipulated price has to be kept at the manipulated price for quite some time to actually influence the final TWAP for the full window. Since the protocol has mechanisms to protect against it, this means that it has fair chances of happening. Now if the manipulation moves asset prices in extreme ends, the amounts calculated for the underlying liquidity will also be extreme (either tokenOwed0 or tokenOwed1 as very big) So in such a condition, the the overflow can happen again in the burn function where the addition of position.tokensOwed0 ie. fees and amount0 (ie. underlying liquidity amounts happens) as linked above. So even in a 30 min twap window, these values can overflow, again leading to undervalued assets causing huge bad debt for the protocol and losses for the lenders.

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.

[haydenshively]

The problem is that aloe's mechanism forces borrower to burn liquidity to collect fees(that is the only option) which means very high chances of overflow because the burned liquidity amount is being added to tokenOwed0 which already has the accrued fees stored via _modifyPosition in the lines above.

Users can call uniswapWithdraw with liquidity set to 0 if they wish to collect fees without burning the rest of their liquidity. Also, overflowing uint128 is incredibly unlikely to begin with.

Now if the manipulation moves asset prices in extreme ends, the amounts calculated for the underlying liquidity will also be extreme (either tokenOwed0 or tokenOwed1 as very big)

Yes the underlying amounts will differ from what's "normal," but the Uniswap pool still must hold at least tokenOwed0 and tokenOwed1 tokens. So for overflow an 18 decimal token, the Uniswap pool would need to hold 3.4e20 tokens.

Lastly, I'd like to add that the Borrower can claim fees when borrowing is paused as long as they repay all borrows by the end of modify

[Trumpero]

Planning to reject escalation and keep the issue invalid based on the sponsor's comment.

The contract already has the uniswapWithdraw function, which can collect fees from Uniswap's position for users even in case of pausing. Additionally, overflowing uint128 for fees amount during only 30 minutes should be considered an impossible case.

[chinmay-farkya]

The problem is that aloe's mechanism forces borrower to burn liquidity to collect fees(that is the only option) which means very high chances of overflow because the burned liquidity amount is being added to tokenOwed0 which already has the accrued fees stored via _modifyPosition in the lines above.

Users can call uniswapWithdraw with liquidity set to 0 if they wish to collect fees without burning the rest of their liquidity. Also, overflowing uint128 is incredibly unlikely to begin with.

Now if the manipulation moves asset prices in extreme ends, the amounts calculated for the underlying liquidity will also be extreme (either tokenOwed0 or tokenOwed1 as very big)

Yes the underlying amounts will differ from what's "normal," but the Uniswap pool still must hold at least tokenOwed0 and tokenOwed1 tokens. So for overflow an 18 decimal token, the Uniswap pool would need to hold 3.4e20 tokens.

Lastly, I'd like to add that the Borrower can claim fees when borrowing is paused as long as they repay all borrows by the end of modify

I agree that the zero withdrawl of liquidity can be used to collect fees. One scenario where this overflow can forcibly happen is when being liquidated.

manipulation happens => large fees or large tokensOwed0 / tokensOwed1 can be accumulated due to extreme price changes altering the units of underlying liquidity => now liquidate calls withdraw full liquidity => this can still lead to an overflow in the addition inside univ3pool's burn function (because of token0 or token1 amounts exaggerated as explained above acc to v3 liquidity formula on withdrawl).

This overflow would mean way lesser assets than deserved to cover the liabilities (especially in cases where users hold positions as the major source of assets => which is most likely almost all users naturally to earn yield) => finally leading to bad debt.

This should be a medium because this scenario is definitely possible and causes bad debt to the protocol / losses to the lenders.

[chinmay-farkya]

Lastly, I'd like to add that the Borrower can claim fees when borrowing is paused as long as they repay all borrows by the end of modify

Also, this is again a forced withdrawl of entire liquidity for most users because most users might use v3 positions as the main portion of assets to cover the liabilities as they would also want to earn yield on their positions.

The contract already has the uniswapWithdraw function, which can collect fees from Uniswap's position for users even in case of pausing. Additionally, overflowing uint128 for fees amount during only 30 minutes should be considered an impossible case.

Yes, but only if he withdraws entire liquidity even if a small portion of it would have covered the liquidity (again using the assumption that most users will use positions instead of raw assets as the major portion). imo there should still be a separate collectFees function. And we are not talking about only the fees overflowing in 30 minutes, but the addition of "withdrawn tokenOwed0 / tokenOwed1 and the fees" overflowing when manipulation happens causing price drift one way and aloe market is paused.

[cvetanovv]

It is difficult for me to judge whether the report is a valid medium. I rather agree with the opinion of the sponsor and the Judge.

[Trumpero]

@chinmay-farkya In my view, the function _uniswapWithdraw in Borrower contract calls UNISWAP_POOL.collect with the parameters amount0Requested and amount1Requested set to type(uint128).max, which means that the function always collects all fees of the specific position from UniswapV3. This is the correct behavior when interacting with UniswapV3's pools. Users will be able to collect all of their fees anytime, even when the Aloe market is paused. The overflow isn't related to Aloe contract, and it is the expectation of UniswapV3 that users should collect fees before the overflow of accumulation. Therefore, I still believe this issue should be considered invalid.

[chinmay-farkya]

Users will be able to collect all of their fees anytime, even when the Aloe market is paused.

No they can't because the uniswapWithdraw can only be called from within the modify callback, and the modify function blocks any action if the market is paused.

The overflow isn't related to Aloe contract, and it is the expectation of UniswapV3 that users should collect fees before the overflow of accumulation

Yes it is expected but aloe forces full withdrawl of liquidity added to fees collection in some cases, and the sum of these two numbers can overflow in the pool's burn function.

Even if fees and burned liquidity are otherwise smaller than uint128, in case of manipulation the burned liquidity can have extreme values which alone or after addition of fees can be > uint128.max

Either there should be a partial withdrawl possible or separate fees collection possible, that needs to be allowed all the time from aloe's end because uniswap says borrower is responsible to prevent this overflow by collecting his fees from time-to-time but the aloe market pausing prevents the borrower contract from abiding to this functionality in this case.

Also, there isn't a guarantee that the paused market will unpause immediately in one window (30 minutes), it can still be paused for many windows ( ie. multiples of 30 minutes) due to longer manipulation and the time it takes for twap to recover to original prices.

[cvetanovv]

haydenshively What do you think about this report?

[haydenshively]

Even if fees and burned liquidity are otherwise smaller than uint128, in case of manipulation the burned liquidity can have extreme values which alone or after addition of fees can be > uint128.max

uint128 should be more than enough to store the totalSupply of any commonly-used ERC20, so I don't see how manipulation could cause the liquidity inside Uniswap to exceed that value. Happy to be shown otherwise if @chinmay-farkya can show an example, but given current information I think this should remain closed.

[chinmay-farkya]

Well I raised this issue because uniswap says here that the user needs to keep an eye on the fees and collect before it overflows, I think there maybe a possibility according to uniswap devs' POV, but I do not have an example.

[Czar102]

I don't think that the 30 min lockup changes much when it comes to liquidity management of such a position. It also seems to me that those uniswap v3 limitations are a part of the design. I think this should be informational (as brings those unobvious limitations up).

[Czar102]

Result: Low Unique

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• chinmay-farkya: rejected