Beedle - Oracle free perpetual lending

Summary

Missing return value checks for all 3 transfer function calls, which could potentially DoS the ability for the specific borrower to repay the same loanId.

If one or more of the 3 transfer calls fail, they will only return false but the repay() function execution will continue as normal, allowing for the delete loans[loanId]; to be executed and therefore the loanId to be deleted. It would seem repayment transaction was successful, but either the borrower didnt receive their collateral back, and/or the lender didnt receive their loan token back, and/or the fee receiver didnt get their fee. The first two cases are obviously much worse.

Vulnerability Details

"PoC" of repayment DoS for specific borrower:

Once the borrower tries to redo the repayment via the repay() function, the loans[loanId] record would have been deleted, and there would be no way for the borrower to repay that specific loanId via the repay() function, effectively DoS-ing his ability to repay that loan. And if liquidation ever becomes a risk for that loanId, the borrower has no control over it and therefore no way to prevent liquidation.

Additionally, we have two subcases here:

• frontend: borrower cannot input loanId manually because frontend would normally have this already available for borrower to select, therefore the frontend wont have any record of this loanId anymore since it was deleted via delete loans[loanId], and the repay() call functionality would be unavailable on the frontend for borrower for this specific loanId.

• smart contract: borrower is able to manually enter the loanId when calling repay() function in Lender.sol, and now it seems we have a most interesting subcase. I manually checked all the functions involved in the repay() call and it seems the non-existent deleted loanId was able to pass through without any revert until the successful end of the function call. It even deleted the already deleted loanId again, because delete loans[loanId]; would not revert, as per my understanding.
  (If I'm completely wrong with my analysis here, please forgive my noobness, I will get better.)

Impact

At the very least, could potentially DoS the ability for the specific borrower to repay the same loanId or prevent liquidation.
Loss of borrower collateral, but retention of borrowed funds, and loss of lender token funds for protocol.
Liquidation becomes almost guaranteed at some point.

Tools Used

VSC, manual.

Recommendations

• Implement Return Value Checks: Add return value checks for all three transfer function calls in the repay function to ensure that the transfers are successful before proceeding with further operations. Revert the transaction if any of the transfers fail.

• Handle Failure Scenarios: In case of failed transfers, revert the transaction and emit appropriate events to indicate the failure to repay the loan. Handle the failure scenarios gracefully to prevent data inconsistency and unexpected behavior.

• Maintain Loan Records Until Fully Repaid: Refrain from deleting loan records until the entire repayment process is successfully completed. Only delete the loan record after all transfer operations have been verified and confirmed.

• Frontend Validation: Implement frontend validation to ensure that the borrower cannot attempt to repay a deleted loanId. Display a meaningful error message or disable the functionality for repaying deleted loanIds.

• Input Validation: Add input validation in the repay function to ensure that the loanId being passed actually exists and has not been deleted before proceeding with the repayment process.

• Use SafeERC20 Library: Consider using a well-audited SafeERC20 library to handle token transfers. This library provides safer implementations of ERC20 transfer functions with built-in return value checks and reverts on failure.