detectiveking - Borrower collateral that they are owed can get stuck in Vault and not sent back to them after calling repay

[sherlock-admin2]

detectiveking

high

Borrower collateral that they are owed can get stuck in Vault and not sent back to them after calling repay

Summary

There's a case where a borrower calls borrow, perhaps does a bunch of intermediate actions like calling increaseCollateralBalance, and then calls repay a short while later (so fees haven't had a time to increase), but the collateral they are owed is stuck in the Vault instead of being sent back to them after they repay.

Vulnerability Detail

First, let's say that a borrower called borrow in LiquidityBorrowingManager. Then, they call increase increaseCollateralBalance with a large collateral amount. A short time later, they decide they want to repay so they call repay.

In repay, we have the following code:

            if (
                collateralBalance > 0 &&
                (currentFees + borrowing.feesOwed) / Constants.COLLATERAL_BALANCE_PRECISION >
                Constants.MINIMUM_AMOUNT
            ) {
                liquidationBonus +=
                    uint256(collateralBalance) /
                    Constants.COLLATERAL_BALANCE_PRECISION;
            } else {
                currentFees = borrowing.dailyRateCollateralBalance;
            }

Notice that if we have collateralBalance > 0 BUT !((currentFees + borrowing.feesOwed) / Constants.COLLATERAL_BALANCE_PRECISION > Constants.MINIMUM_AMOUNT) (i.e. the first part of the if condition is fine but the second is not. It makes sense the second part is not fine because the borrower is repaying not long after they borrowed, so fees haven't had a long time to accumulate), then we will still go to currentFees = borrowing.dailyRateCollateralBalance; but we will not do:

                liquidationBonus +=
                    uint256(collateralBalance) /
                    Constants.COLLATERAL_BALANCE_PRECISION;

However, later on in the code, we have:

            Vault(VAULT_ADDRESS).transferToken(
                borrowing.holdToken,
                address(this),
                borrowing.borrowedAmount + liquidationBonus
            );

So, the borrower's collateral will actually not even be sent back to the LiquidityBorrowingManager from the Vault (since we never incremented liquidationBonus). We later do:

            _pay(borrowing.holdToken, address(this), msg.sender, holdTokenBalance);
            _pay(borrowing.saleToken, address(this), msg.sender, saleTokenBalance);

So clearly the user will not receive their collateral back.

Impact

User's collateral will be stuck in Vault when it should be sent back to them. This could be a large amount of funds if for example increaseCollateralBalance is called first.

Code Snippet

https://github.com/sherlock-audit/2023-10-real-wagmi/blob/main/wagmi-leverage/contracts/LiquidityBorrowingManager.sol#L565-L575

https://github.com/sherlock-audit/2023-10-real-wagmi/blob/main/wagmi-leverage/contracts/LiquidityBorrowingManager.sol#L632-L670

Tool used

Manual Review

Recommendation

You should separate:

            if (
                collateralBalance > 0 &&
                (currentFees + borrowing.feesOwed) / Constants.COLLATERAL_BALANCE_PRECISION >
                Constants.MINIMUM_AMOUNT
            ) {
                liquidationBonus +=
                    uint256(collateralBalance) /
                    Constants.COLLATERAL_BALANCE_PRECISION;
            } else {
                currentFees = borrowing.dailyRateCollateralBalance;
            }

Into two separate if statements. One should check if collateralBalance > 0, and if so, increment liquidationBonus. The other should check (currentFees + borrowing.feesOwed) / Constants.COLLATERAL_BALANCE_PRECISION > Constants.MINIMUM_AMOUNT and if not, set currentFees = borrowing.dailyRateCollateralBalance;.

[fann95]

Fixed: RealWagmi/wagmi-leverage@96ad6c1

[IAm0x52]

Escalate

This is a valid issue but it should be medium rather than high as it only occurs under very specific circumstances. For most collaterals this would only occur of the user borrows and repays in the same block or if the borrow amount is exceptionally low.

[sherlock-admin2]

Escalate

This is a valid issue but it should be medium rather than high as it only occurs under very specific circumstances. For most collaterals this would only occur of the user borrows and repays in the same block or if the borrow amount is exceptionally low.

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.

[detectiveking123]

Disagree with the escalation.

To name a few things,

1. User could lose a large amount of funds if increaseCollateralBalance is called first, as my report mentions. The amount of funds lost here could be quite high.
2. The escalation says: "For most collaterals this would only occur of the user borrows and repays in the same block or if the borrow amount is exceptionally low." <- This seems incorrect, you could easily wait a few minutes before returning on a decently sized collateral and still lose some funds. But for sure the worst case is waiting a few seconds to return on a higher amount of funds. I argue that this is actually quite a common use case. This brings me to my next point.
3. Remember that this is a trading protocol at the end of the day. Borrowing and repaying is a trade. Many traders employ high frequency strategies, and might open a position and attempt to close a short time afterwards (even in the next block, for example). This is extremely common if you look at well known on-chain perp protocols. I maintain that a large number of traders would run into this issue and lose funds.

[IAm0x52]

A single 18 dp token is 10^18. At the minimum interest of 0.1% per day the interest per second is:

10^18 * 0.001 / 86400 = 11.574 * 10^9

This is such higher than the MINIMUM_AMOUNT of 1000. This means that any 18 dp token (unless each token was valued at $11,500,000 which currently doesn't exist) would only suffer from this if closed in the same block.

For 6 dp tokens:

10^6 * 0.001 / 86400 = 1.1574 * 10^-2

This means that 6 dp positions over 86400 would not suffer from this unless closed in the same block.

For ETH with a block time of 12 seconds, a position of 7200 could be closed by the next block and not suffer from this.

As stated above this only affects limited number of tokens/positions and should therefore be medium not high.

[detectiveking123]

For the dp point, I think USDC and USDT are tokens that constitute an extremely large portion of the market share here, so I disagree with your "limited number of tokens" point. Also, as you kind of alluded to, each (10^6) token can be worth more than $1.

Nonetheless, I personally think $86,400 per position is a lot of money (and the total number of funds at risk is much higher, since this bug can be repeated for every position. This isn't even taking a potential call to increaseCollateralBalance into account.). Much faster chains, like FTM, also exist in scope, where block times are much lower and the probability of a new block coming out in one second or less after the previous is nontrivial.

The bigger point is that, even if increaseCollateralBalance is not called, this bug completely bricks the HFT use case. If you look at the top perp protocols, a pretty high percentage of volume is just bots flipping around futures in short time frames. I don't think a use case that many institutional investors rely on and probably constitutes over 50% of volume on existing top perp protocols should be considered "limited".

Here is Sherlock's criteria for High validity:

High: This vulnerability would result in a material loss of funds, and the cost of the attack is low (relative to the amount of funds lost). The attack path is possible with reasonable assumptions that mimic on-chain conditions. The vulnerability must be something that is not considered an acceptable risk by a reasonable protocol team.

"This vulnerability would result in a material loss of funds, and the cost of the attack is low (relative to the amount of funds lost)" -> There is indeed a material loss of funds.

"The attack path is possible with reasonable assumptions that mimic on-chain conditions." -> Yes, the HFT use case is pretty reasonable and common. The HFT use case is completely bricked and will lead to a material loss of funds. I also think that starting off with a lower amount of collateral, calling increase collateral balance (you don't want to be afraid of liquidation), and then attempting to close your position a few minutes later (e.g. if price moves against you and you change your mind), is a reasonable use case that will lead to a large material loss of funds.

"The vulnerability must be something that is not considered an acceptable risk by a reasonable protocol team." -> Sponsor has confirmed that this is not an acceptable risk.

[Czar102]

It seems that the crux of the escalation argument is that this bug affects only a subset of use cases. I think the use cases affected are not "theoretical" enough not to deserve a high severity, they seem sensible. Even repaying in the same block should be allowed, for example could be used by a protocol utilizing this system that allows everyone to contribute.

The protocol team doesn't seem to have implemented considered if clause because of the formal correctness, rather real use cases.

Also, I don't follow this:

For 6 dp tokens:

10^6 * 0.001 / 86400 = 1.1574 * 10^-2

This means that 6 dp positions over 86400 would not suffer from this unless closed in the same block.

It seems that the interest from an Ethereum block would be below 1 wei, certainly below Constants.MINIMUM_AMOUNT. But there is a good chance I am misunderstanding something.

@IAm0x52 please let me know if my approach makes sense.

[IAm0x52]

My argument stems from this set of judging rules which states that the issue is medium if it requires certain states/external conditions which it does. Also this isn't an attack. No one can repay or close your loan for you (besides liquidation which is irrelevant here). You would have to cause this loss for yourself.

By 86400 above I mean 86400 full 6 dp tokens i.e. 86400e6

[Czar102]

I am aware of Sherlock's rules. The conditionality in this issue is not of a state or external factors, but the use case of the protocol. In other words, this bug is not triggered if external conditions are met, or a specific state is achieved, which may be a reason to decrease severity to medium. As you said, triggering of this bug only depends on the use case of the protocol.

By executing actions that should normally work (and those actions have real, reasonable use cases), the user irreversibly loses funds which are locked in the contract forever.

I am planning to reject the escalation and leave this a high severity issue.

[IAm0x52]

I see. This seems to be a much different interpretation of Sherlock's rule than has been previously used. My comments have been based on how I have seen this judged previously. We are all on the same page it seems from the technical side of if/when this issue happens. If @Evert0x agrees with this interpretation of the high risk criteria based on potentially reasonable use cases then there is nothing more to talk about here. If this is the case, I would also appreciate a change to judging rules to reflect this new line of logic.

I personally don't see opening/adding collateral and closing in the exact same block as a common use case. I know HFT was mentioned above but since a single block is one specific point in time the only utility I see for that type of action would be arbitrage within the block. Given the large number of other more efficient ways to accomplish this it seems unlikely. Outside of single block closure the amount of time required between open and close to avoid this is a very short time (less than a minute for all but the smallest 6 dp token positions) I also find it highly unlikely that this will occur often at all.

[detectiveking123]

"Also this isn't an attack. No one can repay or close your loan for you (besides liquidation which is irrelevant here). You would have to cause this loss for yourself."

Historically many issues that aren't an attack, but cause a loss of funds, have been classified as high.

sherlock-audit/2023-08-cooler-judging#119 is one such example

[Czar102]

Discussed this issue with @Evert0x. The high severity was designed to distinguish issues that would very likely cause protocol failure if in production. This bug is triggered only in scenarios that are somewhat extreme (execution in the same block, e.g. after gas price normalizes while the frontend allows that, some very specific protocol integrations, etc.)

The use cases presented are potentially reasonable, but very specific. Decided to consider this a medium severity issue. Escalation will be accepted and severity will be changed to medium.

[detectiveking123]

It's not exactly execution in the same block though -- you can execute multiple blocks later and still lose a decent amount of money (e.g. thousands at stake 10 blocks later). HFT is also a use case that will constitute a large amount, if not majority, of the volume on the platform.

Will respect the outcome though, it is probably a borderline case.

[Czar102]

@detectiveking123 could you work out an example of a large loss in the longest timeframe possible? (most likely show payoff parameters for a worst case scenario)

[Evert0x]

@detectiveking123 will accept escalation and make Medium in 24 hours unless requested argument is provided.

[HHK-ETH]

@detectiveking123 could you work out an example of a large loss in the longest timeframe possible? (most likely show payoff parameters for a worst case scenario)

Because MINIMUM_AMOUNT doesn't scale with the token's decimals like I mentionned in this issue, if you were to use a token with even lower decimals than 1e6 you could end up with a much longer time frame.

If you want to long GUSD (2 decimals) against USDC (360k+ TVL on Uniswap v3), you can open a position with 5k GUSD with 0.1% fee and close it a day later and still be under MINIMUM_AMOUNT thus loosing your collateral balance.

5k GUSD -> 500000 wei
MINIMUM_AMOUNT -> 1000

500000 * 0.1 / 100 = 500 < MINIMUM_AMOUNT

Amount lost will be very small most of the time as it will be the collateral for a day so between 0.05% and 1% according to current protocol's parameters.

Exception is if the user called increaseCollateralBalance() before changing his mind and closing the position in which case it can be way higher.
An example using the case above is a user betting on a USDC depeg and so open a position to short it and long GUSD and put collateral for a few days by calling increaseCollateralBalance(), then a news comes in that makes the depeg unlikely anymore and so our user close his positions but looses his whole collateral.

So I guess the question is how likely is this kind of scenario to know if it falls in "Causes a loss of funds but requires certain external conditions or specific states".

[Czar102]

Amount lost will be very small most of the time as it will be the collateral for a day so between 0.05% and 1% according to current protocol's parameters.

From my understanding, it was a loss of the whole collateral. So in the above case, it's $5k. Can you confirm @HHK-ETH @detectiveking123?

[HHK-ETH]

Amount lost will be very small most of the time as it will be the collateral for a day so between 0.05% and 1% according to current protocol's parameters.

From my understanding, it was a loss of the whole collateral. So in the above case, it's $5k. Can you confirm @HHK-ETH @detectiveking123?

Yes it's a loss of the whole collateral but no collateral is only a percentage of the borrowing position since you don't control the tokens borrowed this protocol doesn't need you to have an overcollaterized position.

[HHK-ETH]

Updated the calculation in #122 (comment) as I made an error yesterday. New example is a day long.

[Evert0x]

Based on my interpretation this is a clear case for Medium severity. But if I understand correctly, you are advocating for high severity with your comment? @HHK-ETH

• Loss is between 0.05% and 1%
• Requires 2 decimal tokens (GUSD)
• User needs to call increaseCollateralBalance() before to increase the loss %

So I guess the question is how likely is this kind of scenario to know if it falls in "Causes a loss of funds but requires certain external conditions or specific states".

You are also quoting the "How to identify a medium issue" here, so I'm kind of confused what your comment is intending to clarify. As medium was agreed upon earlier.

[HHK-ETH]

I was mostly trying to give extra examples that could help judges define the severity.

Although I have a duplicate of this finding and would benefit from it accepted as high, it seems that it fits better as a medium severity indeed.

[Evert0x]

Result:
Medium
Has Duplicates

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• IAm0x52: accepted

[IAm0x52]

Fix looks good. Minimum calculations have been moved to _calcFeeCompensationUpToMin which charges the user the minimum fee if the currently accrued fee is lower than the minimum.