search

sherlock-audit / 2023-10-real-wagmi-judging

16 stars 14 forks source link

pinalikefruit - Unuseful external swap when trying to execute `repay()` function by liquidator/lender #113

Closed sherlock-admin closed 11 months ago

sherlock-admin commented 11 months ago

pinalikefruit

medium

Unuseful external swap when trying to execute repay() function by liquidator/lender

Summary

The protocol has two ways to perform a swap (Uniswap and external swap), but the external swap option will throw an error every time the liquidator/lender try to repay a position.

Vulnerability Detail

The trader has two options to exchange their tokens: they can use Uniswap or a third-party exchange. The regular flow is that a borrower call borrow() and if for any reason the position needs to be liquidated, they (lender/liquidator) has the same options to exchange the tokens.

But, the external swap cannot be used when they call the repay() function, it never worked, because the calculation of amountOut when subtracting liquidity is poorly designed.

amountOut = _getBalance(tokenOut) - balanceOutBefore;

contracts/abstract/ApproveSwapAndPay.sol#169

balanceOutBefore will always be greater than _getBalance(tokenOut) because the balance goes down when the swap is successful. If this is attempted it will throw an error, as the result will always be negative and amountOut is declared as uint256.

Impact

Code Snippet

https://github.com/sherlock-audit/2023-10-real-wagmi/blob/main/wagmi-leverage/contracts/abstract/ApproveSwapAndPay.sol#L169

Tool used

Recommendation

Ensure correct calculation of amountOut within the _patchAmountsAndCallSwap() function. When any user calls the borrow() and repay() functions for an external swap.

sherlock-admin2 commented 11 months ago

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

tsvetanovv commented:

I think the external swap is expected to increase the balance of tokenOut

pinalikefruit commented 11 months ago

ESCALED

External swap is expected work for both sides to increase and decrease the balance of tokenOut. Let's break down.

In the first instance this function can be called when the user wants to swap saleToken to holdToken when he is borrowing.

And this sentence amountOut = _getBalance(tokenOut) - balanceOutBefore; since the balanceOutBefore was calculated prior to the swap as uint256 balanceOutBefore = _getBalance(tokenOut); and the initial one is 0 or if you want to increment (borrow) the current balance will always be greater than the old balance of the same token _getBalance(tokenOut) - balanceOutBefore > 0. So, the proposed problem is not here.

We see that this comment to the judge works well I think the external exchange is expected to increase the balance of tokenOut

But what happens when it is the opposite case, the user wants to repay? This fuction_patchAmountsAndCallSwap( can also be called in _restoreLiquidity() and in this case the calculation of uint256 balanceOutBefore = _getBalance(tokenOut); will always be higher when the swap is done since it decrements the token balance and this sentence amountOut = _getBalance (tokenOut) - balanceOutBefore; will throw an overflow error.

In my report I mentioned that the protocol has two options to perform a swap, if you look at the second option using Uniswap, this flow is the implementation is correct for both parties, borrow and repay.

HHK-ETH commented 11 months ago

When restoring liquidity we pass the sale token as token out as we do want to swap for sale token so how would the balance before the swap be greater unless the user didn't pass the right swap params, he is expected to swap hold tokens for sale tokens (but then it's a user issue) ?

I think this is what the judge means as it's expected that the user passed the right swap params and thus the balance can only increase or be the same as we sell token in (hold token) for more token out (sale token).