seeques - Incorrect calculations of borrowingCollateral leads to DoS for positions in the current tick range due to underflow

[sherlock-admin2]

seeques

high

Incorrect calculations of borrowingCollateral leads to DoS for positions in the current tick range due to underflow

Summary

The borrowingCollateral is the amount of collateral a borrower needs to pay for his leverage. It should be calculated as the difference of holdTokenBalance (the amount borrowed + holdTokens received after saleTokens are swapped) and the amount borrowed and checked against user-specified maxCollateral amount which is the maximum the borrower wishes to pay. However, in the current implementation the borrowingCollateral calculation is most likely to underflow.

Vulnerability Detail

This calculation is most likely to underflow

uint256 borrowingCollateral = cache.borrowedAmount - cache.holdTokenBalance;

The cache.borrowedAmount is the calculated amount of holdTokens based on the liquidity of a position. cache.holdTokenBalance is the balance of holdTokens queried after liquidity extraction and tokens transferred to the LiquidityBorrowingManager. If any amounts of the saleToken are transferred as well, these are swapped to holdTokens and added to cache.holdTokenBalance.

So in case when liquidity of a position is in the current tick range, both tokens would be transferred to the contract and saleToken would be swapped for holdToken and then added to cache.holdTokenBalance. This would make cache.holdTokenBalance > cache.borrowedAmount since cache.holdTokenBalance == cache.borrowedAmount + amount of sale token swapped and would make the tx revert due to underflow.

Impact

Many positions would be unavailable to borrowers. For non-volatile positions like that which provide liquidity to stablecoin pools the DoS could last for very long period. For volatile positions that provide liquidity in a wide range this could also be for more than 1 year.

Code Snippet

https://github.com/sherlock-audit/2023-10-real-wagmi/blob/main/wagmi-leverage/contracts/LiquidityBorrowingManager.sol#L492-L503
https://github.com/sherlock-audit/2023-10-real-wagmi/blob/main/wagmi-leverage/contracts/LiquidityBorrowingManager.sol#L470
https://github.com/sherlock-audit/2023-10-real-wagmi/blob/main/wagmi-leverage/contracts/LiquidityBorrowingManager.sol#L848-L896

Tool used

Manual Review

Recommendation

The borrowedAmount should be subtracted from holdTokenBalance

uint256 borrowingCollateral = cache.holdTokenBalance - cache.borrowedAmount;

[Ali-Shehab]

Escalate

First issue has nothing to do with the other ones. It must not be duplicate

[peanuts - Max collateral check is not done when increasing collateral balance] #37

[sherlock-admin2]

Escalate

First issue has nothing to do with the other ones. It must not be duplicate

[peanuts - Max collateral check is not done when increasing collateral balance] #37

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.

[fann95]

Fixed: RealWagmi/wagmi-leverage@7937e25

[IAm0x52]

Escalate

This is a valid issue but it should be medium rather than high. Impact is broken functionality and DOS which doesn't qualify as high.

[sherlock-admin2]

Escalate

This is a valid issue but it should be medium rather than high. Impact is broken functionality and DOS which doesn't qualify as high.

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.

[Ali-Shehab]

Some reports were accepted as high with similar impact: https://solodit.xyz/issues/h-2-possible-dos-in-rollerperiphery-approve-function-sherlock-sense-sense-git I don't know if am missing something.

[IAm0x52]

While I acknowledge the similarity of the issues, I have made my escalation based on current Sherlock rules which overrides previous judgments. Loss of functionality is a medium issue. The DOS could be over a year but only for some ranges/LP tokens which is why it is also medium.

[Ali-Shehab]

But can't we send any token and make it DOS?

[Czar102]

As I understand, users wouldn't be able to create a borrow position for borrows with the active tick within the borrow tick range. This is a partial loss of the core functionality of the protocol, but doesn't lock any funds for >1 year. There also doesn't seem to be any loss of funds.

Current interpretation of the Sherlock rules imply that when there is no loss of funds or funds locked, it is not a high issue. I think it should be downgraded to medium, so will be accepting the escalation if there are not counterarguments.

[Ali-Shehab]

Also, I want to remind you that there is an issue that doesn't relate to this bug: #37

[Czar102]

The first escalation here concerns another issue, #37.

Will be accepting both escalations. This will be downgraded to medium and #37 is not a duplicate. It is invalid.

[Evert0x]

Result:
Medium
Has Duplicates

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• ali-shehab: accepted
• IAm0x52: accepted

[IAm0x52]

Fix looks good. Underflow of this calculation is prevented via a ternary operator