HHK - No check on liquidation and daily rates update while borrowing

[sherlock-admin]

HHK

medium

No check on liquidation and daily rates update while borrowing

Summary

When a user calls the borrow() function of the LiquidityBorrowingManager contract. He expect that the currentDailyRate and liquidationBonusForToken are the same as what was displayed by the UI.

But given asynchronous nature of blockchains, an admin could update these values for the tokens the borrower is about to borrow and result in more token charged than expected.

Vulnerability Detail

During the borrow() function, the token to be sent by the borrower are computed using multiple state variables that define the rates and default amounts for the tokens to borrow.

These variables can be modified by the owner/dailyRateOperator. While the function check the deadline and maxCollateral given by the borrower. It doesn't check that the currentDailyRate nor liquidationBonusForToken changes.

If these values are changed while the transaction is being confirmed, it could result in more tokens charged to the user than he expected.

Impact

Medium. If some state parameters are changed while a borrower's transaction is being confirmed it could result in more token charged and a different daily fee than expected.

Code Snippet

https://github.com/sherlock-audit/2023-10-real-wagmi/blob/b33752757fd6a9f404b8577c1eae6c5774b3a0db/wagmi-leverage/contracts/LiquidityBorrowingManager.sol#L465 https://github.com/sherlock-audit/2023-10-real-wagmi/blob/b33752757fd6a9f404b8577c1eae6c5774b3a0db/wagmi-leverage/contracts/LiquidityBorrowingManager.sol#L211 https://github.com/sherlock-audit/2023-10-real-wagmi/blob/b33752757fd6a9f404b8577c1eae6c5774b3a0db/wagmi-leverage/contracts/abstract/OwnerSettings.sol#L65

Tool used

Manual Review

Recommendation

Consider adding maxDailyRate and maxLiquidationBonus parameters and check them.

[nevillehuang]

Escalate

Should be invalid. Admin changes is assumed to be trusted, so it is assumed that it will be made known before the changes.

Are the admins of the protocols your contracts integrate with (if any) TRUSTED or RESTRICTED? Trusted

[sherlock-admin2]

Escalate

Should be invalid. Admin changes is assumed to be trusted, so it is assumed that it will be made known before the changes.

Are the admins of the protocols your contracts integrate with (if any) TRUSTED or RESTRICTED? Trusted

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.

[HHK-ETH]

Escalate

Should be invalid.

Are the admins of the protocols your contracts integrate with (if any) TRUSTED or RESTRICTED? Trusted

I believe the issue is not if admins are trusted or not but if the execution of a transaction matchs what's the user expects. Admins will most likely not check the MEMPOOL before updating the parameters of the protocol. So if this update happens during a user's transaction is being mined the user would get charged a higher collateral and liquidation bonus and might not understand what happened or find that it doesn't fit the long/short scenario he wanted to execute as the protocol is charging to many tokens.

At the end of the day it's not very likely to happen and impact is moderate so would understand if this gets lowered although it was validated by the sponsor.

[Oot2k]

In the past these issues have never been valid, yes I think its a valid concern but changes from admin side are most likely well known in advance.

Valid Low

[fann95]

I don’t see the point here in controlling the liquidation bonus, but at the same time, the dailyRate variable will change dynamically, and we intend to check it.

[fann95]

Fixed: https://github.com/RealWagmi/wagmi-leverage/commit/c5770402d617a6efc4154c6d191ebb6d86938f14 In addition, some variables associated with checks in this function have been renamed to improve code readability.

[Czar102]

It seems users shouldn't assume that the interest rates or liquidation bonus won't change. If this assumption is mentioned in docs/code/comments/readme, this issue should be valid. If interest rates can change during the borrow, I don't see the impact difference between them changing within the period the transaction was in the mempool and the moment shortly after the position has been created. If that's the case, the interest rate check doesn't make much sense to me. Anyway, unless there is an explicit mention in the docs/code/comments/readme of the assumption that is said to be broken, this issue should be considered either invalid or low.

@HHK-ETH can you which option of the above is the case?

[HHK-ETH]

From my understanding the team is going for the second option. My concern was that compared to other protocol here you send a certain amount to the contract that is determined by the rate and liquidation bonus. If you sign the transaction expecting to send 5 tokens but end up sending 10 because parameters changed it can be confusing and not match initial expectations.

Thinking back about it, seems more of a UX problem so I would tend to agree that this should be lowered.

[Czar102]

Sorry, which one is the second option? If the deposit amount would change, I'd agree that this issue requires more consideration. But if there is no liquidation, the issue causes more interest to be charged, right? And no more funds are pulled at the time of the deposit?

[HHK-ETH]

If interest rates can change during the borrow

I believe interest rate can change any time.

If the deposit amount would change, I'd agree that this issue requires more consideration. But if there is no liquidation, the issue causes more interest to be charged, right? And no more funds are pulled at the time of the deposit?

When borrowing you deposit the liquidation bonus amount as well as collateral for a day. These two amounts are determined by the daily rate and liquidation bonus percentages. So funds being pulled from your wallet on borrowing can differ from what you expected if these percentages are updated while your transaction is being processed.

[fann95]

I have made a fix to control changes in the daily rate, becouse it may change dynamically. For the liquidation bonus, I did not make such changes as it will not change often..in any case, the liquidation bonus is returned to the trader when the position is closed.

[Czar102]

I see. According to Sherlock rules:

When external-admin=trusted, issues related to these external admins affecting a protocol (being audited) by updating the external protocol parameters is a valid issue (...) as the bug can occur even when the external admin is well intended

In this case, even a well-intended change of interest rates or liquidation bonus can unfairly affect users by taking more collateral. This protocol should work with any frontend, and custom ones don't have a protocol-wide method of predicting those changes. The protocol also didn't specify that they will inform about such changes (which would invalidate this issue).

The question is, are users fair to assume that this collateral transfer will be of a predefined amount? I think yes, especially because of the existence of maxCollateral. Also, the caller may be surprised with interest rates, limits, etc., but they should have total control over how much funds are they letting the contract pull after an approval. Hence, they may expect it to pull an amount predicted in an offchain simulation.

So, despite the external admin is not acting maliciously, more tokens may be taken from the user than intended by the user. I will consider this a medium severity issue, hence planning to reject the escalation.

[Oot2k]

I see. According to Sherlock rules:

When external-admin=trusted, issues related to these external admins affecting a protocol (being audited) by updating the external protocol parameters is a valid issue (...) as the bug can occur even when the external admin is well intended

In this case, even a well-intended change of interest rates or liquidation bonus can unfairly affect users by taking more collateral. This protocol should work with any frontend, and custom ones don't have a protocol-wide method of predicting those changes. The protocol also didn't specify that they will inform about such changes (which would invalidate this issue).

The question is, are users fair to assume that this collateral transfer will be of a predefined amount? I think yes, especially because of the existence of maxCollateral. Also, the caller may be surprised with interest rates, limits, etc., but they should have total control over how much funds are they letting the contract pull after an approval. Hence, they may expect it to pull an amount predicted in an offchain simulation.

So, despite the external admin is not acting maliciously, more tokens may be taken from the user than intended by the user. I will consider this a medium severity issue, hence planning to reject the escalation.

I have to add that we are in this case not talking about an external admin, but an admin issue. This is a different category in sherlock rules!!!

According to Sherlock rules: "An admin action can break certain assumptions about the functioning of the code. Example: Pausing a collateral causes some users to be unfairly liquidated or any other action causing loss of funds. This is not considered a valid issue. "

For this reason in the past these issues have always been considered low.

[nevillehuang]

Leaning with @Oot2k here, based on rules here. In this case, we are specifically mentioning an internal admin parameter change, not an external one.

An admin action can break certain assumptions about the functioning of the code. Example: Pausing a collateral causes some users to be unfairly liquidated or any other action causing loss of funds. This is not considered a valid issue.

[Czar102]

Sorry, my bad. Thank you for correcting me. The point I quoted does concern external admins. I'll rejudge this issue.

[Czar102]

Planning to accept the escalation and change severity to low after a discussion with @Evert0x.

[Evert0x]

Result: Invalid Unique

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• nevillehuang: accepted

[IAm0x52]

Fix looks good. maxDailyRate has been added to BorrowParams struct which protects against rate changes