seeques - Incorrect calculations of borrowingCollateral leads to DoS for positions in the current tick range due to underflow #86
Comments
github-actions
bot
added
High
cvetanovv
added
the
Sponsor Confirmed
sherlock-admin2
changed the title
|
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. |
|
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. |
|
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. |
|
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. |
|
But can't we send any token and make it DOS? |
|
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. |
|
Also, I want to remind you that there is an issue that doesn't relate to this bug: #37 |
Evert0x
added
Medium
|
Result: |
sherlock-admin
added
the
Escalation Resolved
|
Escalations have been resolved successfully! Escalation status:
|
|
Fix looks good. Underflow of this calculation is prevented via a ternary operator |
seeques
high
Incorrect calculations of borrowingCollateral leads to DoS for positions in the current tick range due to underflow
Summary
The
borrowingCollateralis 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 theborrowingCollateralcalculation is most likely to underflow.Vulnerability Detail
This calculation is most likely to underflow
The
cache.borrowedAmountis the calculated amount of holdTokens based on the liquidity of a position.cache.holdTokenBalanceis the balance of holdTokens queried after liquidity extraction and tokens transferred to theLiquidityBorrowingManager. If any amounts of the saleToken are transferred as well, these are swapped to holdTokens and added tocache.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 makecache.holdTokenBalance > cache.borrowedAmountsincecache.holdTokenBalance == cache.borrowedAmount + amount of sale token swappedand 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
The text was updated successfully, but these errors were encountered: