ADM - Lender is able to steal borrowers collateral by calling rollLoan with unfavourable terms on behalf of the borrower.

[sherlock-admin]

ADM

high

Lender is able to steal borrowers collateral by calling rollLoan with unfavourable terms on behalf of the borrower.

Summary

A Lender is able to call provideNewTermsForRoll with whatever terms they want and then can call rollLoan on behalf of the borrower forcing them to roll the loan with the terms they provided. They can abuse this to make the loan so unfavourable for the borrower to repay that they must forfeit their collateral to the lender.

Vulnerability Detail

Say a user has 100 collateral tokens valued at $1,500 and they wish to borrow 1,000 debt tokens valued at $1,000 they would would call: (values have simplified for ease of math)

requestLoan("1,000 debt tokens", "5% interest", "10 loan tokens for each collateral", "1 year")

If a lender then clears the request the borrower would expect to have 1 year to payback 1,050 debt tokens to be able to receive their collateral back.

However a lender is able to call provideNewTermsForRoll with whatever terms they wish: i.e.

provideNewTermsForRoll("loanID", "10000000% interest", "1000 loan tokens for each collateral" , "1 year")

They can then follow this up with a call to rollLoan(loanID):
During the rollLoan function the interest is recalculated using:

    function interestFor(uint256 amount_, uint256 rate_, uint256 duration_) public pure returns (uint256) {
        uint256 interest = (rate_ * duration_) / 365 days;
        return (amount_ * interest) / DECIMALS_INTEREST;
    }

As rate_ & duration_ are controllable by the borrower when they call provideNewTermsForRoll they can input a large number that the amount returned is much larger then the value of the collateral. i.e. input a rate_ of amount * 3 and duration of 365 days so that the interestFor returns 3,000.

This amount gets added to the existing loan.amount and would make it too costly to ever repay as the borrower would have to spend more then the collateral is worth to get it back. i.e. borrower now would now need to send 4,050 debt tokens to receive their $1,500 worth of collateral back instead of the expected 1050.

The extra amount should result in more collateral needing to be sent however it is calculated using loan.request.loanToCollateral which is also controlled by the lender when they call provideNewTermsForRoll, allowing them to input a value that will result in newCollateralFor returning 0 and no new collateral needing to be sent.

    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);  
        // Lender can force neededCollateral to always be less than loan.collateral

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

As a result a borrower who was expecting to have repay 1050 tokens to get back their collateral may now need to spend many multiples more of that and will just be forced to just forfeit their collateral to the lender.

Impact

Borrower will be forced to payback the loan at unfavourable terms or forfeit their collateral.

Code Snippet

Cooler.sol#L192-L217
Cooler.sol#L282-L300

Tool used

Manual Review

Recommendation

Add a check restricting rollLoan to only be callable by the owner. i.e.:

function rollLoan(uint256 loanID_) external {
        Loan memory loan = loans[loanID_];
        
        if (msg.sender != owner()) revert OnlyApproved();

Note: unrelated but rollLoan is also missing its event should add:

factory().newEvent(reqID_, CoolerFactory.Events.RollLoan, 0);

[jkoppel]

Whether this is medium or high depends on how likely borrowers are to make massively over-collateralized loans

[0xrusowsky]

imo a Medium

[Oot2k]

escalate
split frontrunning and access control into own issues

[sherlock-admin2]

escalate
split frontrunning and access control into own issues

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[0xrusowsky]

• fix: Extend loan instead of roll + small cleanup ohmzeus/Cooler#54
• fix: enforce non-zero extension ohmzeus/Cooler#60
• fix: NO full interest on partially repaid extensions ohmzeus/Cooler#61

[Oot2k]

Following issues are not duplicates of 26 and should be grouped together and treaded as another issue:
16 (#16)
18 (#18)
72 (#72)
99 (#99)
130 (#130)
137 (#137)
150 (#150)
204 (#204)
221 (#221)
243 (#243)
271 (#271)

226 -> Invalid

[Oot2k]

Addition:
226 shows attack path and root cause, mentions tokens that are not supported -> sherlock has to decide if valid/invalid
231 is not duplicate of this issue and should be grouped with the other ones mentioned above

[hrishibhat]

Result:
Medium
Has duplicates
The respective set of issues has been separated

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• Oot2k: accepted

[jkoppel]

Fix confirmed. Sponsor agreed to accept some economic concerns with the fix, but no security concerns were identified.