There is no way to liquidate a position if it breaches maxDebtPerCollateralToken value creating bad debt.

[c4-bot-4]

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/LendingTerm.sol#L634-L675

Vulnerability details

Impact

Liquidations in the system are done via LendingTerm._call(), which will auction the loan's collateral to repay outstanding debt. A loan can be called only if the term has been offboarded or if a loan missed a periodic partialRepay.
To understand the issue, we must understand how the loan is created and how it can be called.
First, when a loan is created through a call to _borrow(), the contract checks if the borrowAmount is lesser than the maxBorrow:

uint256 maxBorrow = (collateralAmount * params.maxDebtPerCollateralToken) / creditMultiplier;
 require(
            borrowAmount <= maxBorrow,
            "LendingTerm: not enough collateral"
        );

It calcualates maxBorrow using params.maxDebtPerCollateralToken. For example, if maxDebtPerCollateralToken is 2000e18, then with 15e18 tokens of collateral, a user can borrow up to 30_000 of CREDIT tokens.

Then it's important to understand how liquidations work in the system. If we were to look at _call() function we could notice that it only allows liquidations in two specific cases:

require(
            GuildToken(refs.guildToken).isDeprecatedGauge(address(this)) ||
                partialRepayDelayPassed(loanId),
            "LendingTerm: cannot call"
        );

It will only allow to call a position if a term is depreceated or if the loan missed periodic partial repayment.
This approach creates problems and opens up a griefing attack vector.

There are few ways it can go wrong. Let's firstly discuss an issue that will arise even for a non-malicious user.
In order for a user to not get liquidated, he must call partialRepay() before a specific deadline set in term's parameters. If we look at the _partialRepay() function:

require(
            debtToRepay >= (loanDebt * params.minPartialRepayPercent) / 1e18,
            "LendingTerm: repay below min"
        );

We can see, that it enforces user to repay at least params.minPartialRepayPercent, which may not always be enough for a position to stay "healthy". By "healthy" I mean a position that does not breach maxDebtPerCollateralToken value, which is a parameter of a LendingTerm.
Imagine a scenario:

• Interest rate = 15%
• minPartialRepayPercent = 10%
• maxDebtPerCollateralToken = 2000

User borrows 30_000 CREDIT with 15 TOKENS of collateral. His debtPerCollateral value is 30_000 / 15 = 2000, which is exactly equal to maxDebtPerCollateralToken.
Now a year has passed, the loanDebt (debt + interest) is 34_500, a user is obligated to repay at least 34_500 * 0.1 = 3450. After partial repayment his debtPerCollateral value is (34500 - 3450) / 15 = 2070. While he breached the maxDebtPerCollateralToken value, his position is not callable, because he did not miss a periodic partial repayment.

Also it's worth noting, that currently even if a user missed a periodic partial repayment, he can still make a call to partialRepay() if his position was not yet called. Becuase of this, it will be easire for such situation to occur, since the interest will be accruing and when a user finally calls partialRepay() he is still only obligated to repay at least minPartialRepayPercent for a position that went even deeper underwater.
So currently due to a combination of multiple factors, bad debt can essentially occur and it will be impossible to liquidate such position without offboarding a term.

Now let's talk about a potential malicious behaviour that is encouraged in the current implementation.
Periodic partial repayments are not enforced for every term, they may or may not be enabled, so the only condition for a liquidation in this case is a depreceated term.
This means that basically every position is essentially "unliquitable", because partialRepayDelayPassed() will always return false in such case:

function partialRepayDelayPassed(
        bytes32 loanId
    ) public view returns (bool) {
        // if no periodic partial repays are expected, always return false
        if (params.maxDelayBetweenPartialRepay == 0) return false;

A malicious user can abuse this by not repaying his loan or by not adding collateral to his loan when interest accrues above maxDebtPerCollateralToken.
There will be no way to do anything with such positions, the only possible solution would be to offboard a full term. This will obviously damage the protocol, as offboarding a term means calling every position, which subsequently increases a chance of a loss occuring. Also by offboarding a term, lenders will miss out on interest, because every position is force-closed.

The current plan of the protocol was to have partialRepay of at least interestRate every year, so that positions do not grow into insolvent territory.
It's hard and unreasonable to know every set of parameters that can lead to debtPerCollateral being greater than maxDebtPerCollateralToken even after partialRepay. After a discussion with the sponsor, it was said that ultimately the protocol team will not be the only one to deploy new terms, so it's better to enforce a proper liquidation flow in the contract.

Proof of Concept

Here is the PoC demonstrating this issue, for the sake of simplicity I did not change the protocol's test suite configuration, but I just wanted to show that this is possible with any parameters (even the ones expected by the team), because a loan can still be partially repaid even if it missed partial repay deadline (I mention this earlier in my report, saying that this is the reason that makes this situation even easier to occur).

function testBreakMaxDebtPerCollateralToken() public {
        // prepare
        uint256 borrowAmount = 30_000e18;
        uint256 collateralAmount = 15e18;
        collateral.mint(address(this), collateralAmount);
        collateral.approve(address(term), collateralAmount);
        credit.approve(address(term), type(uint256).max);

        // borrow
        bytes32 loanId = term.borrow(borrowAmount, collateralAmount);
        vm.warp(block.timestamp + (term.YEAR() * 3));
        // 3 years have passed, and now position's debt is 39_000
        uint256 loanDebt = term.getLoanDebt(loanId);
        assertEq(loanDebt, 39_000e18);
        // A user is able to call partialRepays even if he missed partialRepays deadline
        term.partialRepay(
            loanId,
            (loanDebt * _MIN_PARTIAL_REPAY_PERCENT) / 1e18
        );
        // After repaying just minPartialRepayPercent, a debtPerCollateralToken of the position is 2080, which is greater than maxDebtPerCollateral
        uint256 newLoanDebt = term.getLoanDebt(loanId);
        assertEq((newLoanDebt / 15e18) * 1e18, 2080000000000000000000);
        assertGt((newLoanDebt / 15e18) * 1e18, _CREDIT_PER_COLLATERAL_TOKEN);

        // A position cannot be called
        vm.expectRevert("LendingTerm: cannot call");
        term.call(loanId);
    }

Please add this function to LendingTerm.t.sol and run it with forge test --match-test 'testBreakMaxDebtPerCollateralToken' -vv.
In conclusion I want to say that parameters of a LendingTerm are only limited to a logical sort of degree, i.e:

• interest rate can be anything from 0 to 100%
• maxDelayBetweenPartialRepay can be anything from 0 to 1 year
• minPartialRepayPercent can be anything from 0 to 100%

Because of this the situation is bound to occur. It's better to enforce strict rules on the smart contract level.

Tools Used

Manual review

Recommended Mitigation Steps

If periodic partial repays are turned on, the possible solution would be to enforce the maxDebtPerCollateral check in _partialRepay, that will enforce users to repay an amount that would make debtPerCollateralToken value lesser than maxDebtPerCollateralToken value.
Something like this would be sufficent enough:

require(loan.debtPerCollateralToken <= param.maxDebtPerCollateralToken, "Position is not healthy.");

In case if partial repays are turned on it's important to have this check in _partialRepay() rather than in _call(), because otherwise almost every position would immediately go underwater and be liquidatable as soon as it gets opened if a user borrowed up to the maximum amount, but it's fine to have debtPerCollateralToken greater than maxDebtPerCollateralToken until user makes a partialRepay.

In case if periodic partial repays are turned off, the check must be in _call(), since there will be no partial repays. Check if debtPerCollateralToken value is lesser than maxDebtPerCollateral token, otherwise liquidate a position, but that would mean that users won't be able to borrow up to maxDebtPerCollateral, since their position will immediatelly go underwater, but it seems to be a matter of trade-offs. This the potential way to modify _call().

require(
            GuildToken(refs.guildToken).isDeprecatedGauge(address(this)) ||
                partialRepayDelayPassed(loanId) ||
                (params.maxDelayBetweenPartialRepay == 0 &&
                    loan.debtPerCollateralToken > params.maxDebtPerCollateral),
            "LendingTerm: cannot call"
        );

Assessed type

Other

[0xSorryNotSorry]

The issue is very well demonstrated, properly formatted, contains a coded POC.
Marking as HQ.

To the attention of the Judge;

The submission also refers to the submission #153 as maxDebtPerCollateral comparison holds the relative value of the collateral.

[c4-pre-sort]

0xSorryNotSorry marked the issue as high quality report

[c4-pre-sort]

0xSorryNotSorry marked the issue as primary issue

[eswak]

Confirming this, thanks for the high quality of the report!

On one hand, I think it might be considered a governance issue if the chosen term parameters allow loans to grow into unsafe territory, and that the situation is handled properly in the current implementation because GUILD holders can offboard the term if any loan of the term is unsafe, but on the other hand, I don't think it's a large code change to allow loans to be called if they violate the "max debt check that is in _borrow" during their lifetime, it is an elegant addition to the codebase that we'll probably do, so I think it's worth including in the audit report.

[c4-sponsor]

eswak (sponsor) confirmed

[c4-judge]

Trumpero marked the issue as satisfactory

[c4-judge]

Trumpero removed the grade

[c4-judge]

Trumpero marked the issue as satisfactory

[c4-judge]

Trumpero marked the issue as selected for report

[kazantseff]

Hi @Trumpero,
I would like to point out that this issue is about loans violating maxDebtPerCollateral check that is in _borrow() function during their lifetime.
The current implementation of a LendingTerm offers 2 ways a system can work: partial repays may be enabled and may be disabled. In my submission I clearly state two impacts of this issue to the system, one for the case when partial repays are expected and loans cannot be called despite violating aformentioned check and one for the case when partial repays are not expected and in such case loans also cannot be called, despite violating the check. The root cause of this is the lack of health factor check and not maxDelayBetweenPartialRepay being set to 0.
The only duplicate under this primary that correctly idenitifes the root cause is #58. #58 states that all loans cannot be called even when they violate maxBorrow check during their lifetime.
Other issues grouped under this primary do not correctly identify the root cause, do not describe the same vulnerability and only talk about the specific case when maxDelayBetweenPartialRepay = 0, thus the recommened mitigations for such issues also do not address the problem properly.
Given all of this context, I would like to ask you to reevaluate the judging on relevant duplicates under this primary.
Thank you for you time and work!

[Trumpero]

@kazantseff
Firstly, I want to refer to the sponsor's statement and point out that your scenario is also a case of unsafe configs for a lending term.

On one hand, I think it might be considered a governance issue if the chosen term parameters allow loans to grow into unsafe territory, and that the situation is handled properly in the current implementation because GUILD holders can offboard the term if any loan of the term is unsafe

The configs in your scenario requires a large interest rate and repayment duration, resulting in growing interest higher than minPartialRepayPercent. I believe the likelihood of this scenario doesn't differ from the case when maxDelayBetweenPartialRepay == 0, as both cases represent unsafe and risky configs that need careful on-boarding and off-boarding from governance and users.
From the start, I had doubts about the severity of whether these issues should be considered as medium or QA, since everyone will be aware of the term's configs before onboarding. However, I acknowledge that these represent potential risks, and the mitigation is both useful and necessary.

Secondly, there are only 2 cases that can cause a loan to breach maxDebtPerCollateralToken: a lending term with interest growing faster than the minimum repayment (your scenario) and a lending term with non-periodic payment loans (maxDelayBetweenPartialRepay == 0). Both of these unsafe cases lack a liquidation mechanism when the loan exceeds maxDebtPerCollateralToken. Therefore, I believe they share the same root cause and should be marked as duplicates.

However, I understand your concern since your report has a high quality and your recommended mitigation was accepted. I will reevaluate the quality of each duplicate and decrease the partial credit if needed.

[kazantseff]

@Trumpero
Thank you for your time and a thorough response! Even though somewhat I want to disagree, I won't relitigate your position, which you respectully defended. Really appreciate the effort as it helps to see the same issue under a different angle. With that being said, I won't take anymore of your time arguing about this issue. Have a good one!