detectiveking - Frontrun protection on rollLoan

[sherlock-admin]

detectiveking

high

Frontrun protection on rollLoan

Summary

Currently, the peer to peer flow (not through the ClearingHouse) for rolling is for a lender to call provideNewTermsForRoll and then for the borrower to call rollLoan. The lender can initially call provideNewTermsForRoll with nice parameters, trick the borrower into calling rollLoan, and then front-run their call to rollLoan with a call to provideNewTermsForRoll with bad parameters.

Vulnerability Detail

Note that the collateral().safeTransferFrom(msg.sender, address(this), newCollateral); doesn't actually cause a revert in the case of front-running (e.g., raising the interest rate to something really high in the front-run transaction wouldn't change the amount of additional collateral required).

Note that this is not an issue for users taking and rolling loans directly from the ClearingHouse, since the clearing house will provide the roll terms with the governance issued parameters and then call rollLoan for you. This only affects the peer to peer case where an external, non-ClearingHouse lender and borrower want to agree to rollLoan.

Impact

Borrower could be stuck with a bad loan (worst case, their loan defaults and they lose their collateral)

Code Snippet

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

Tool used

Manual Review

Recommendation

Recommendation is to add a req_id field to Request that is incremented every time the request is changed. Then have that passed to rollLoan as well and check it to make sure there was no front-running.

Duplicate of #243