Beedle - Oracle free perpetual lending

Summary

Multiple vulnerabilities in giveLoan() function, including array length mismatch, improper protocolInterest fee handling, risk of incorrect loanId to poolId mapping, and vulnerabilities related to handling multiple loanIds, etc.

Vulnerability Details

A)
Frontend: Assuming the frontend would ensure that each loanId selected to be given to another pool would be mapped directly to that other pool so that the two arrays are an exact one to one mapping match from loanId to poolId, then all good, otherwise big bad bug.
However, if it's possible for an attacker to make it so that each loanId is not mapped to the correct poolId, then we could potentially have some bug issues.

Recommendation:
Regardless, the contract code is what matters above all else, and therefore we need to add a check above L359 as follows:

	require(loanIds.length == poolIds.length, "Array length mismatch");

or alternatively:

if (loanIds.length != poolIds.length) revert("Array length mismatch");

B)
Additionally, the protocolInterest is actually sent via transfer() to governance, but see below description:
https://github.com/Cyfrin/2023-07-beedle/blob/d9718f2ca6521756624c017c90f3d4d4e80da90c/src/Lender.sol#L398
https://github.com/Cyfrin/2023-07-beedle/blob/d9718f2ca6521756624c017c90f3d4d4e80da90c/src/Lender.sol#L403
https://github.com/Cyfrin/2023-07-beedle/blob/d9718f2ca6521756624c017c90f3d4d4e80da90c/src/Lender.sol#L409

Description:

According to the repay() function the borrower will transfer the protocolInterest fees to the fee receiver. But during the giveLoan() function the loan isnt actually repaid to the protocol, it's just given to another lender, however, the protocolInterest fee is now sent from the Lender contract to the governance, before/without any actual repayments, without involving any transfer of collateral and lending tokens between the borrower and protocol. Where did the Lender contract/old pool get the protocolInterest fee from? The borrower didnt send it. The lender interest wasnt paid yet by the borrower because the loan wasnt repaid yet, just given to another pool. (What is my noobness missing here?)

C)
Risk of one to one mapping between loanIds & poolIds being wrong, regardless of how it happens. Managing this from frontend is not a solid enough mitigation strategy. At the initial/first point of deciding/selecting which loanId should be given to which new lender/pool, at that step it should be ensured that the mapping is 1-to-1 in terms of array positions, i.e. for i, loanIds[i] should always be a correct match to poolIds[i]. Currently no checks to ensure this?

D)
Regarding the use of two arrays during the for loop:

In each of below cases, the following seems to be an issue:
The protocolInterest fee being sent, via transfer(), from Lender contract to governance when it shouldnt be sent at all?

For single loanId case:

a) one to one mapping is correct:
OK.

b) one to one mapping not correct:
b)1) it will revert if at least one token from pair dont match. Damage level = gas.
b)2) if tokens and everything else match, then risk of giving loan to matching but wrong pool. Damage level = low.

For multiple loanIds case:

a) one to one mapping correct:
OK, except it might run out of gas due to transfer() function gas consumption and if array size is too big for available gas, then it will revert, and charge gas to msg.sender.

b) one to one mapping not correct:
similar to single loanId case but might give loans to more unintended pools and pay higher gas fees.

Worst case scenario: DoS of giveLoan() functionality if care not taken to ensure that for each i, loanIds[i] is given to poolIds[i], and not poolIds[n].

Impact

Right now the giveLoan() function seems very buggy and CAN work correctly under controlled conditions(except for protocolInterest fee), but it CAN DoS lender and prevent giving loans to other lenders/pools, or unintentionally give loan to incorrect(but matching) pool.

When it's ensured that the two arrays have equal length, and that the loanIds[i] are mapped to poolIds[i], and not to poolIds[n], for each i, then the worst vulnerabilities it seems are:

• potential gas DoS of giveLoan()

• sending (non-existent) protocolInterest fees from Lender contract to governance via ERC20 transfer(), which will pass even for zero value, but if protocolInterest > 0 and there's funds in contract, it will successfully send it, before the borrower has repaid the loan. Remember, the borrower usually pays this protocolInterest fee directly to the fee receiver, via transferFrom() during loan repayment.

Tools Used

VSC, manual.

Recommendations

1. Array Length Mismatch Check (Mitigation A):
   Add a check at the beginning of the function to ensure that the length of both loanIds and poolIds arrays is the same. If they are not the same length, revert the transaction to prevent potential array-related vulnerabilities.

require(loanIds.length == poolIds.length, "Array length mismatch");

Alternatively:

if (loanIds.length != poolIds.length) revert("Array length mismatch");

2. Ensure One-to-One Mapping (Mitigation C):
   Implement a check to ensure that each loanId is correctly mapped to the corresponding poolId. This can be done in the frontend while selecting which loanId should be given to which new lender/pool. The mapping should be done in a way that loanIds[i] corresponds to poolIds[i] for each i.

3. Handle protocolInterest Fees (Mitigation B):
   Currently, the protocolInterest fee is sent to governance in the giveLoan() function even before any actual repayments. To mitigate this issue, you need to ensure that the protocolInterest fee is only transferred when the loan is repaid by the borrower to the protocol. This can be achieved by making changes to the giveLoan() function to avoid sending protocolInterest at this stage, unless it's intended functionality...

4. Prevent Gas DoS (Mitigation D):
   If the array size is too large, the transfer() function can consume a lot of gas, potentially leading to a DoS scenario. To mitigate this, you may want to consider implementing a limit on the number of loanIds that can be processed in a single transaction.