codehawks-bot commented 1 year ago

A pool lender can fully drain another user's pool by abusing `buyLoan`

Severity

High Risk

Relevant GitHub Links

https://github.com/Cyfrin/2023-07-beedle/blob/main/src/Lender.sol#L465

Summary

A pool lender can fully drain another user's pool by abusing buyLoan function.

Vulnerability Details

Upon buying a loan, all a pool.lender passes as arguments in the function is the id of the loan they want to buy. This could be problematic as the loan could be subject to heavy changes. Consider the following scenario:

User A creates a pool with max interest rate.
From another wallet, the same user takes a borrow for 1000 USDC and gives 3000 USDT as collateral
User A calls startAuction for this loan
User B sees the loan on auction and since it has high interest rate and is well-collateralized, decides to buy it.
User B calls buyLoan and passes to corresponding loanId.
User A sees the pending transaction and front-runs it and changes his pool.maxLoanRatio to type(uint256).max (basically allowing for loans not sufficiently backed up by collateral)
After changing the maxLoanRatio, user A calls refinance and sets the loan debt to user B's pool balance and changes the collateral to 1 USDT.
After doing so, user A changes maxLoanRatio back to its original value and once again calls startAuction with the same loanId.
Considering steps 6-8 all executed before user B's buyLoan, user B's call now executes and they've bought a borrow for all of their pool balance, backed up by pretty much no collateral.
User A successfully executed the attack and has now taken a huge borrow which they have no incentive to repay as there isn't any valuable collateral attached to it.
User A can then either withdraw their funds from their pool or repeat the attack.

Impact

Pool lender can drain other pools.

Tools Used

Manual review

Recommendations

Add the following check to buyLoan

            if (loanRatio > pool.maxLoanRatio) revert RatioTooHigh();

PatrickAlphaC commented 1 year ago

Sik finding. I strongly disagree with your recommendation. I think the recommendation should be to pass in an loanRatio parameter to the buyLoan function and check that the loan's ratio is acceptable.

Maybe that's what you were trying to recommend? But it's not clear here.

devdacian commented 1 year ago

This finding in the report is H-23 currently unique but very similar to findings with the label "finding-buyLoan-steal-collateral-pool-drain" many which use the same maxLoanRatio trick, eg:

https://github.com/Cyfrin/2023-07-beedle/issues/1581 https://github.com/Cyfrin/2023-07-beedle/issues/1483 https://github.com/Cyfrin/2023-07-beedle/issues/1252 https://github.com/Cyfrin/2023-07-beedle/issues/1155

So H-23 shouldn't be a unique finding but should be labelled with the others in "finding-buyLoan-steal-collateral-pool-drain"

qckhp96565463 commented 1 year ago

This finding in the report is H-23 currently unique but very similar to findings with the label "finding-buyLoan-steal-collateral-pool-drain" many which use the same maxLoanRatio trick, eg:

1581 #1483 #1252 #1155

So H-23 shouldn't be a unique finding but should be labelled with the others in "finding-buyLoan-steal-collateral-pool-drain"

OR even https://github.com/Cyfrin/2023-07-beedle/issues/675 finding-buyloan-collateral-ratio-bypassed See both recommendation for fix.

if (loanRatio > pool.maxLoanRatio) revert RatioTooHigh();

deadrosesxyz commented 1 year ago

The issues mentioned above are completely different. They talk about lack of access control and buying on behalf on another pool.

The issue described above describes a scenario where the pool lender buys a seemingly alright loan and completely loses all assets, despite not making any mistake on their behalf.

Adding access control to buyLoan would fix the issues mentioned in the escalation, but wouldn’t make any difference in my issue.

kosedogus commented 1 year ago

This issue is same as #675 (H5) and this issue's impact is much powerful than H5 and its duplicates, I recommend choosing this as selected for H5. Since Codehawks don't have proper documentation regarding duplicate rules yet, I would like to provide some from other contest platforms: Sherlock:

Issues identifying a core vulnerability can be considered duplicates.
Scenario 1:
There is a root cause/error/vulnerability A in the code. This vulnerability A -> leads to two attacks 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.

Code4rena:

All issues which identify the same functional vulnerability will be considered duplicates regardless of effective rationalization of severity or exploit path.
However, any submissions which do not identify or effectively rationalize the top identified severity case may be judged as “partial credit” and may have their shares in that finding’s pie divided by 2 or 4 at judge’s sole discretion (e.g. 50% or 25% of the shares of a satisfactory submission in the duplicate set).

Cyfrin / 2023-07-beedle