codehawks-bot commented 1 year ago

Lenders can force other pools to take loans.

Severity

High Risk

Relevant GitHub Links

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

https://github.com/Cyfrin/2023-07-beedle/blob/658e046bda8b010a5b82d2d85e824f3823602d27/src/Lender.sol#L443

https://github.com/Cyfrin/2023-07-beedle/blob/658e046bda8b010a5b82d2d85e824f3823602d27/src/Lender.sol#L365

Summary

Due to no check in buyLoan() a user can buy a loan on behalf of another pool lender and as they will be set as the loan.lender the pool lender will be unable to remove the loan with giveLoan or startAuction.

Vulnerability Details

In buyLoan there is no check that the msg.sender is the new pools lender and poolId can be input as a function argument, as a result any one can call it on behalf of another pool, buy the loan and have themselves set as the new loan.lender. If the pool lender then wishes to give the loan away or start an auction they will be unable to as they were never set as the loan.lender, the user who called buyLoan was.

Impact

A user is able to force a pool lender to take on a loan that they are unable to remove.

Tools Used

Manual Review

Recommendations

Add a check to buyLoan() confirming msg.sender is pool.lender.

require(pools[poolId].lender == msg.sender);

or if you wish the buyLoan to be callable by anyone have the loan.lender set to the pool lender not msg.sender.

loans[loanId].lender = pools[poolId].lender;

asendz commented 1 year ago

Escalate.

How is this different from #1166? The issue is exactly the same and the solution is exactly the same. Should be duplicates.

Being able to buy loans on behalf of other lenders pools opens up a lot of different variations and possible attack vectors. I don't see a reason why every variation should be considered a separate issue when just fixing the root cause fixes all of them.

devdacian commented 1 year ago

Escalate.

How is this different from #1166? The issue is exactly the same and the solution is exactly the same. Should be duplicates.

Being able to buy loans on behalf of other lenders pools opens up a lot of different variations and possible attack vectors. I don't see a reason why every variation should be considered a separate issue when just fixing the root cause fixes all of them.

FYI, my understanding is that being able to buy loans on behalf of other lenders pools is by design, as this is a design decision listed on the contest details which reads: "Auctioning A Loan: When a lender no longer wants to be in a loan, but there is no lending pool available to give the loan to, lenders are able to put the loan up for auction. This is a Dutch Auction where the variable changing over time is the interest rate and it is increasing linearly. Anyone is able to match an active pool with a live auction when the parameters of that pool match that of the auction or are more favorable to the borrower. This is called buying the loan. If the auction finishes without anyone buying the loan, the loan is liquidated. Then the lender is able to withdraw the borrowers collateral and the loan is closed."

The project has chosen as a design decision to allow anyone to buyLoan() for another pool. Hence the appropriate solution is the 2nd one listed:

loans[loanId].lender = pools[poolId].lender;

And other vulnerabilities that work within this specified design should be considered separate vulnerability classes.

Cyfrin / 2023-07-beedle