HHK - `repay()` is prone to sandwich attacks

[sherlock-admin]

HHK

high

repay() is prone to sandwich attacks

Summary

The repay() function of the LiquidityBorrowingManager contract is used to close a borrowing position.

When called by the borrower itself or by a liquidator bot, the amount of token received by the caller can be less than it should if the transaction is sandwiched. Greatly reducing potential profits for a borrower or liquidator.

Vulnerability Detail

When a position is at profit, the lender can call repay() to close it. If the collateral if negative a liquidator can also call this function to reimburse collateral and profit from the liquidationBonus as well as the position's profits.

To close the position and realize the profits, if a the position is not out of range, a swap is made. Some holdToken are swapped for saleToken to be able to add the liquidity back to the position in the function _restoreLiquidity().

The current issue is that the minimum amount out of the swap is determined onchain which makes sandwich attacks possible as an MEV bot could bundle our transaction with 2 swaps one before and one after to affect the amount we receive on our swap effectively sandwiching out transaction.

The bot would make profits from our unrealized profits leading to the borrower or the liquidator to make less or even 0 profits.

Impact

High. If the repay() transaction is sandwiched the profits will be greatly reduced.

Code Snippet

https://github.com/sherlock-audit/2023-10-real-wagmi/blob/b33752757fd6a9f404b8577c1eae6c5774b3a0db/wagmi-leverage/contracts/LiquidityBorrowingManager.sol#L532 https://github.com/sherlock-audit/2023-10-real-wagmi/blob/b33752757fd6a9f404b8577c1eae6c5774b3a0db/wagmi-leverage/contracts/abstract/LiquidityManager.sol#L223

Tool used

Manual Review

Recommendation

Consider adding an array of minOut to the parameters that will be used for each loan swap, reducing the gas cost as we won't need to get a quote anymore as well as protect the users from sandwich attacks.

Duplicate of #109

[fann95]

duplicate https://github.com/sherlock-audit/2023-10-real-wagmi-judging/issues/109

[cvetanovv]

Escalate

Duplicate with #109

[sherlock-admin2]

Escalate

Duplicate with #109

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.

[Abelaby]

@cvetanovv This is certainly not a duplicate of #109, that issue describes slippage issue arising from using slot0 to calculate SqrtPriceX96 while there is no mention of slot0 or SqrtPriceX96 anywhere in this issue.

[talfao]

If you read correctly #109, the issue describes also usage of dynamic slippage params, they are directly in the name. So yes this issue is clearly a duplicate.

The sandwiching is possible because of slot0 based on that are calculated slippage params.

It is not good to use dynamic slippage params in general but it will work more if twap is used.

[Abelaby]

@talfao I disagree, #109 and this issue talks about 'dynamic slippage parameters', but #109 and its duplicates 'clearly' mention about the issue arising from using 'slot0' to calculate SqrtPriceX96. While this issue and its duplicates are talking about using dynamic variables, it remains same if that was done using slot0 or any other function to dynamically calculate the variable. It's clearer if you read the duplicates of issue no 109. Described root causes are different, one can even argue this issue is vague and don't specify the root cause and is invalid (same has been done in other contests, but that's another story, I leave it to the judge to decide).

As per sherlock docs,

Scenario A: There is a root cause/error/vulnerability A in the code. This vulnerability A -> leads to two attack paths: B -> high severity path C -> medium severity attack path/just identifying the vulnerability. Both B & C would not have been possible if error A did not exist in the first place. In this case, both B & C should be put together as duplicates. In addition to this, there is a submission D which identifies the core issue but does not clearly describe the impact or an attack path. Then D is considered low. Scenario B: In the above example if the root issue A is one of the following generic vulnerabilities: Reentrancy Access control Front-running Then the submissions with valid attack paths and higher vulnerability are considered valid. If the submission is vague or does not identify the attack path with higher severity clearly it will be considered low. B is a valid issue C is low

[talfao]

Leave the decision to judge. I still support Escalation.

[fann95]

Slot0 will continue to be used as it is needed to calculate the proportion of tokens to restore liquidity. The slippage will be calculated based on the sqrt price received off-chain and passed to the function.

[Czar102]

The fix to both issues is the same, hence, despite viewing the issue from other sides, the core issue is the same. Hence those issues should be duplicates. @Abelaby please let me know if the above statement is accurate.

[Abelaby]

@Czar102 My initial comment was based on two factors;

1. This issue and its duplicates don't mention the root cause is from using slot0 , the issue description is 'vague' and some might even argue its invalid according to sherlock criteria, but I leave that to the judge and sponsors.

2. Most of the duplicates of this issue mentions amount0Min and amount1Min which is hardcoded to 0 and not affected by using slot0 or anything else for calculating SqrtPriceX96 so those has another root cause and not duplicates of #109. But this concern is resolved now as @talfao has kindly tagged and escalated those type of issues as duplicate of #19 .

[HHK-ETH]

If you read correctly #109, the issue describes also usage of dynamic slippage params, they are directly in the name. So yes this issue is clearly a duplicate.

The sandwiching is possible because of slot0 based on that are calculated slippage params.

It is not good to use dynamic slippage params in general but it will work more if twap is used.

This. It is to me a duplicate of https://github.com/sherlock-audit/2023-10-real-wagmi-judging/issues/109.

While the finding doesn't specify the usage of slot0 it does explains that the issue lies in calculating the amountOut onchain during _restoreLiquidity() when we try to swap hold token for sale token.

It is rarely a good idea to determine swap outputs onchain doesn't matter how you do it, even with oracles or twaps you can be sandwiched (less impact but still possible) thus my proposition of adding a minOut parameter.

Most of the duplicates of this issue mentions amount0Min and amount1Min which is hardcoded to 0 and not affected by using slot0 or anything else for calculating SqrtPriceX96 so those has another root cause and not duplicates of https://github.com/sherlock-audit/2023-10-real-wagmi-judging/issues/109. But this concern is resolved now as @talfao has kindly tagged and escalated those type of issues as duplicate of https://github.com/sherlock-audit/2023-10-real-wagmi-judging/issues/19 .

Agree with this, some of the duplicates for this finding are not actually duplicates.

[Czar102]

As far as I understand, slot0 information needs to be used to calculate the swap amounts. Those can vary slightly as swaps are made and it's fine to derive them on chain. The exploit happens when there is slippage in this swap or when the price is manipulated.

Some findings viewed this issue from the perspective of a swap – someone can manipulate the AMM price and the swap will be at a very bad rate. Others looked at this in another way – agreeing to whatever slot0 says about the price would cause slippage and potential price manipulation, which is an issue.

I think both are right.

And yes, there are issues within duplicates of both of these that I don't think deserve being rewarded. I'd like to thank @talfao for escalating those issues.

Hopefully, in the future, we will have some better tools to recognize "vague" reports, as @Abelaby mentioned.

Planning to accept the escalation and make these issues duplicates.

[Evert0x]

Result: High Duplicate of #109

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• cvetanovv: accepted