detectiveking - Malicious lender abusing rollLoan to capture collateral through Cooler

[sherlock-admin2]

detectiveking

high

Malicious lender abusing rollLoan to capture collateral through Cooler

Summary

Malicious peer-to-peer (non ClearingHouse) lender can first call provideNewTermsForRoll with a very high loanToCollateral_ value then call rollLoan (both in Cooler.sol) to trap the borrower's collateral, and then claimDefaulted after the expiration time.

Vulnerability Detail

Assume first that the borrower (same as owner()) has created a loan request directly through the Cooler with a lower than market rate loanToCollateral_ value (so the value of the collateral is worth more than the value of the debt, which is a fair use case to incentivize lenders to fill the request). Or, assume that the price of gOHM has gone up, so the value of the collateral is higher than the value of the debt -- same effect.

A lender (not the ClearingHouse) fills the request through the Cooler. The lender then calls provideNewTermsForRoll with a very high loanToCollateral_ value and a somewhat high interest value (high enough so that the amount of interest is never worth repaying by the borrower, but not high enough to cause overflow). The lender then immediately calls rollLoan. Because the loanToCollateral_ value was extremely high (let's say, UINT_MAX), newCollateralFor will return 0 (or something close to 0, doesn't really matter):

This is because newCollateralFor uses loan.request.loanToCollateral, which was set by the lender in provideNewTermsForRoll.

    function newCollateralFor(uint256 loanID_) public view returns (uint256) {
        Loan memory loan = loans[loanID_];
        // Accounts for all outstanding debt (borrowed amount + interest).
        uint256 neededCollateral = collateralFor(
            loan.amount,
            loan.request.loanToCollateral
        );

        return
            neededCollateral > loan.collateral ?
            neededCollateral - loan.collateral :
            0;
    }

Because the lender passed in a somewhat high interest value to provideNewTermsForRoll, the amount of debt (request.amount) is now higher than the amount of collateral, so the borrower should never repay. The lender will thus just take the higher value collateral out after loan expiration time has passed, making a big profit.

Note that the malicious lender can actually achieve this same effect without manipulating loanToCollateral_ at all; because newCollateralFor doesn't consider the interest rate at all, the malicious lender can just pass in a very high interest rate to provideNewTermsForRoll and then call rollLoan. They will have to pay some amount of collateral into the cooler (but it won't account for the interest rate), but the amount of debt will increase much more (because the debt accounts for the interest rate but not the cooler). Thus the borrower won't be able to pay back the amount borrowed and the lender will get access to all the collateral in the end (which is presumably higher valued).

Note that this is not an issue for users taking loans directly from the ClearingHouse, since the clearing house will always be honest. This only affects the peer to peer case when a borrower interacts directly with the Cooler, calls requestLoan, and an external lender fills the request.

Impact

Malicious lender stealing collateral from borrower

Code Snippet

https://github.com/sherlock-audit/2023-08-cooler/blob/main/Cooler/src/Cooler.sol#L192-L217

Tool used

Manual Review

Recommendation

Make rollLoan only callable by owner() or the clearing house.

Duplicate of #26