Curve reentrancy check for tokens being borrowed is missing

[hats-bug-reporter[bot]]

Github username: @bahurum Twitter username: bahurum Submission hash (on-chain): 0x445cf137ddf578bc3c58b45f02850886f981c27fc76282da35a904c6b95d4c82 Severity: high

Description: Description\ The function MainHelper._curveSecurityChecks() will check for curve lp tokens price manipulation via read-only-reentrancy for all _lendTokens and _borrowTokens used by the nftId.

The issue is that in WiseCore._coreBorrowTokens() the function _prepareAssociatedTokens() is called before _addPositionTokenData(), meaning that _poolToken isn't included in the borrowTokens array and the curve reentrancy check is not performed on it.

With read-only-reentrancy, the curve pool's virtual_price can be manipulated downwards, so the health check will allow to borrow more of the lp token than it should be allowed, at the profit of an attacker.

Attack Scenario\ Consider this simplified scenario:

1. There are 1000 ETH worth of stETH/ETH lp token available for borrow.
2. Attacker takes flashloan of 500 ETH from some other protocol
3. Attacker deposits 500 ETH
4. Attacker manipulates curve lp virtual price to half its value and during the reentrant call:
   1. Attacker borrows 1000 ETH worth of stETH/ETH lp token
   2. No reentrancy check is performed because of the issue
   3. Because of the manipulation, during health check the value borrowed seen by the protocol is 500 ETH instead of 1000 ETH. Health check passes.
5. Attacker repays flash loans of 500 ETH and profits 500 ETH.
6. 500 ETH of bad debt is left in the protocol.

Impact\ Borrowable curve lp tokens with manipulatable virtual price can be drained from the protocol at a profit, leaving bad debt in the protocol.

Recommendation\ In _coreBorrowTokens(), add _poolToken to the borrowTokens array before passing it into _curveSecurityChecks().

Attachments

1. Proof of Concept (PoC) File\ The issue is quite simple. I can provide a PoC later if requested.

[Foon256]

Hey,

I am not sure why you think that the curve security check is related to _prepareAssociatedTokens(). This function only adds the acquired tokens from the borrow rate since last interaction to have the latest pseudo amounts for follow up compuation of all token the user has at this point. Afterwards the borrow process begins and in the end of the function, after the borrow token is added to the token list of the user, we perfrome the _curveSecurityChecks(). Just look at line 397 in WiseCore.sol.

As you can see in the above picture, _addPositionTokenData() is performed before _curveSecurityChecks() and therefore the above attack is not possible. Do you agree with that?

[bahurum]

Because the lendTokens and borrowTokens which are passed as arguments to _curveSecurityChecks() are the arrays returned by _prepareAssociatedTokens(). So _curveSecurityChecks() performs the check on lendTokens and borrowTokens and not the updated token list of the user.

[vm06007]

allowborrow: false -> doesnt matter curve pools not intended to be used for borrowing, only as collateral.

@vonMangoldt can confirm.

[bahurum]

Thanks you for your reponse. As the reentrancy check is performed on borrowTokens as well one would assume that curve lp tokens could be borrowed.

[Foon256]

Thank you for your effort. You are right with the above statement, that the performed borrowTokens check is redundant and can be removed. Despite that this is oos, we take this change in our code and will discuss internally how/if we can reward you with a litte amount for that finding.