codehawks-bot commented 1 year ago

Refinancing in the same pool requires it to have a higher balance than needed because of a flawed check

Severity

Medium Risk

Relevant GitHub Links

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

Summary

Refinancing a loan into the same pool it is in requires the pool to have the original loan amount +- the difference because it doesn't check whether the pool already has the original amount as outstanding debt.

Vulnerability Details

Lender.sol's refinance() has a check, which makes sure the pool we are refinancing into has enough liquidity to cover the new debt of the loan.

if (pool.poolBalance < debt) revert LoanTooLarge();

The issue arises when the loan is getting refinanced in the same pool it is already in. Then it will require the pool to have the amount of the loan again instead of just the difference between the old debt and the new debt. This makes it harder for a loan to be refinanced in the same pool as the pool may not have enough liquidity to pass this check.

Impact

Some loans will not be able to be properly refinanced.

Tools Used

Manual Review

Recommendations

Consider checking whether the loan's old pool is the new pool as well first and doing additional checks accordingly.

PatrickAlphaC commented 1 year ago

I think this would cause it to revert. Will check with sponsor.

} else if (debtToPay < debt) {
                // we have excess loan tokens so we give some back to the borrower
                // first we take our borrower fee
                uint256 fee = (borrowerFee * (debt - debtToPay)) / 10000;
                IERC20(loan.loanToken).transfer(feeReceiver, fee);
                // transfer the loan tokens from the contract to the borrower
                IERC20(loan.loanToken).transfer(msg.sender, debt - debtToPay - fee);
            }

PatrickAlphaC commented 1 year ago

Moved to LOW since the impact is LOW and likelihood is MEDIUM.

To explain to me for later:

Borrow borrows 100% of pool
Lender updates pool to better interest rate
Borrow goes to refinance to same pool with better rate, but can't because
```
if (pool.poolBalance < debt) revert LoanTooLarge();
```
The debt is == pool.poolBalance

KristianApostolov commented 1 year ago

I believe the issue should be of medium severity as it demonstrates how protocol functionality and availability for a user get disrupted, which is directly how medium severity issues are explained in the documentation.

https://docs.codehawks.com/rewards-and-judging#What-is-a-finding?

Medium:
- Funds are indirectly at risk
- Disruption of protocol functionality or availability

PatrickAlphaC commented 1 year ago

I think it's still a low, because they can refinance to another pool, or just repay their debt. at risk is a bit subjective.

If my funds are in my wallet, they are at risk of someone breaking into my house and keylogging my computer to get the password to my wallet.

A user who wants to refinance isn't really at that much of a risk of their funds being taken.

I think you're right though, we should update the docs to be more clear, and talk about the matrix of likelihood vs impact instead of a hard "funds at risk" or "not at risk".

If we take a hard line of "funds are at risk" then one could argue anytime someone has to make a transaction that's at least a medium severity vulnerability since they have to pay gas fees.

Keeping it as a low. Thank you for your escalation.

Cyfrin / 2023-07-beedle