harisnabeel - rollLoan() can be front-run by malicious Lender

[sherlock-admin2]

harisnabeel

high

rollLoan() can be front-run by malicious Lender

Summary

Lender can front run Borrowers's rollLoan() transaction by monitoring mem-pool that will cause loss to Borrower unknowingly.

Vulnerability Detail

1 - Borrower had taken a loan of 1000 DAI with loanToCollateral = 300 * 1e18 locking 3.33 Collateral tokens.
2 - Lender calls provideNewTermsForRoll() with old loanToCollateral = 300 * 1e18 so that Borrower will not need to lock additional collateral in order to rollLoan().
3 - Borrower decides to call rollLoan() and submits transaction.
4 - Malicious lender who was monitoring mem-pool for Borrower's rollLoan() transaction see the rollLoan() transaction.
5 - Lender submits provideNewTermsForRoll() with loanToCollateral = 100 * 1e18 with higher gas fees so that his transaction executes before Borrower's rollLoan().
6 - Now in rollLoan(), newCollateralFor() with new loanToCollateral value will return (10 * 1e18 - 3.33 * 1e18) = 6.67 * 1e18 meaning Borrower has to lock 6.67 * 1e18 collateral tokens in order to rollLoan().

    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;
    }

7 - Assuming that Borrower back in some time had max approved collateral tokens to Cooler, the Cooler can pull 6.67 * 1e18 collateral tokens from Borrower, causing loss to Borrower.

        if (newCollateral > 0) {
            collateral().safeTransferFrom(msg.sender, address(this), newCollateral);
        }

References:
https://github.com/sherlock-audit/2023-08-cooler/blob/main/Cooler/src/Cooler.sol#L282C17-L282C17
https://github.com/sherlock-audit/2023-08-cooler/blob/main/Cooler/src/Cooler.sol#L192
https://github.com/sherlock-audit/2023-08-cooler/blob/main/Cooler/src/Cooler.sol#L199
https://github.com/sherlock-audit/2023-08-cooler/blob/main/Cooler/src/Cooler.sol#L211

Impact

A rollLoan() operation that required no new collateral to roll the loan will end up pulling X-amount of tokens depending on loanToCollateral value causing Borrower to lose collateral tokens unknowingly.

Code Snippet

  function provideNewTermsForRoll(
        uint256 loanID_,
        uint256 interest_,
        uint256 loanToCollateral_,
        uint256 duration_
    ) external {
        Loan storage loan = loans[loanID_];

        if (msg.sender != loan.lender) revert OnlyApproved();

        loan.request = Request(
            loan.amount,
            interest_,
            loanToCollateral_
            duration_,
            true
        );
    }

    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;
    }

Tool used

Manual Review

Recommendation

Take input the parameters of new terms from Borrower and validate that it matches with loan.request params set by Lender like below:

function rollLoan(uint256 loanID_, 
        uint256 interest_,
        uint256 loanToCollateral_,
        uint256 duration_) external {
        Loan memory loan = loans[loanID_];
        require(loanToCollateral_ == loan.request.loanToCollateral, "LTC Miss-match");
        require(interest_ == loan.request.interest, "Interest Miss-match");
        require(duration_ == loan.request.duration, "Duration Miss-match");
}

Duplicate of #243