Borrowers can pay as lower as they want if minPartialRepayPercent set as zero and cannot be called for auction.

[c4-bot-10]

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/governance/LendingTermOnboarding.sol#L128

Vulnerability details

Summary

Below we can see code of there is no restriction to set minPartialRepayPercent as zero but maxDelayBetweenPartialRepay as can be set below one year.

        require(
            params.interestRate < 1e18, // interest rate [0, 100[% APR
            "LendingTermOnboarding: invalid interestRate"
        );

        require(
            // 31557601 comes from the constant LendingTerm.YEAR() + 1
            params.maxDelayBetweenPartialRepay < 31557601, // periodic payment every [0, 1 year]
            "LendingTermOnboarding: invalid maxDelayBetweenPartialRepay"
        );

Take a scenario :-
minPartialRepayPercent set as zero

maxDelayBetweenPartialRepay set as 31557600

1. Borrower takes the 20_000e18 credit tokesn by giving the 12+ collateral tokens.

2. Now borrower can call partialReapy() with as lower amount 0.5e18.

3. This will extends the partialRepayDelayedPassed() function means it will always return false then borrower can pay his debt more than 2-3 years but governor can call forgive() function on this loan but it make bad debt for ECG protocol this makes permanent loss for protocol.

4. This loan cannot called for auction because _call() function have below check

        // check that the loan can be called
        require(
            GuildToken(refs.guildToken).isDeprecatedGauge(address(this)) ||
                partialRepayDelayPassed(loanId),
            "LendingTerm: cannot call"
        );

The above check means only gauge(term) off-boarded or loan missed partial repayment.

5.Loan hasn't missed partial payment so cannot be _call().

Impact

This mechanism cause permanent loss for ECG and loyal borrowers also if one or more borrower makes partial payment as much as lower amount. That loan cannot be called for auction also it can be only after gauge is off-boarded.

Proof of Concept

    function test_payPartial() public {

    // create a similar term but with 5% opening fee
        LendingTerm term2 = LendingTerm(
            Clones.clone(address(new LendingTerm()))
        );
        uint256 MAX_PARTIAL_REAPYDELAY = 3_15_57_600;
        term2.initialize(
            address(core),
            term.getReferences(),
            LendingTerm.LendingTermParams({
                collateralToken: address(collateral),
                maxDebtPerCollateralToken: _CREDIT_PER_COLLATERAL_TOKEN,
                interestRate: _INTEREST_RATE,
                maxDelayBetweenPartialRepay: MAX_PARTIAL_REAPYDELAY,
                minPartialRepayPercent: 0,
                openingFee: 0.05e18,
                hardCap: _HARDCAP
            })
        );
        vm.label(address(term2), "term2");
        guild.addGauge(1, address(term2));
        guild.decrementGauge(address(term), _HARDCAP);
        guild.incrementGauge(address(term2), _HARDCAP);
        vm.startPrank(governor);
        core.grantRole(CoreRoles.RATE_LIMITED_CREDIT_MINTER, address(term2));
        core.grantRole(CoreRoles.GAUGE_PNL_NOTIFIER, address(term2));
        vm.stopPrank();

        // prepare
        uint256 borrowAmount = 20_000e18;
        uint256 collateralAmount = 12e18;
        collateral.mint(address(this), collateralAmount);
        collateral.approve(address(term2), collateralAmount);

        // borrow
        bytes32 loanId = term2.borrow(borrowAmount, collateralAmount);
        console.log("Loan debt before partial repay" , term2.getLoanDebt(loanId));

        vm.warp(block.timestamp + term2.YEAR());
        vm.roll(block.number + 1);
        credit.mint(address(this), 1e18);
        credit.approve(address(term2) , 1e18);
        term2.partialRepay(loanId, 0.5e18);
        console.log("Loan debt after First partial repay" , term2.getLoanDebt(loanId));
        console.log("Is partial repay delay passed on 1st year" , term2.partialRepayDelayPassed(loanId));

        vm.warp(block.timestamp + term2.YEAR());
        vm.roll(block.number + 1);
        credit.mint(address(this), 1e18);
        credit.approve(address(term2) , 1e18);
        term2.partialRepay(loanId, 0.5e18);
        console.log("Loan debt after  partial repay 2nd year" , term2.getLoanDebt(loanId));
        console.log("Is partial repay delay passed on 3rd year" , term2.partialRepayDelayPassed(loanId));

        vm.warp(block.timestamp + term2.YEAR());
        vm.roll(block.number + 1);
        credit.mint(address(this), 1e18);
        credit.approve(address(term2) , 1e18);
        term2.partialRepay(loanId, 0.5e18);
        console.log("Loan debt after  partial repay 3rd year" , term2.getLoanDebt(loanId));
        console.log("Is partial repay delay passed on 3rd year" , term2.partialRepayDelayPassed(loanId));

    }

output :-

[PASS] test_payPartial() (gas: 4669410)
Logs:
  Loan debt before partial repay 21000000000000000000000
  Loan debt after First partial repay 22999500000000000014000
  Is partial repay delay passed on 1st year false
  Loan debt after  partial repay 2nd year 24998956521739130467422
  Is partial repay delay passed on 3rd year false
  Loan debt after  partial repay 3rd year 26998373043478260929071
  Is partial repay delay passed on 3rd year false

Tools Used

Manual view , foundry

Recommended Mitigation Steps

Implement the mechaism before initialising the lending term if term wish to have partial repayment then minimum value has to be set.

1.Add boolean type to LendingTermParams struct bool hasPartialRepayment.

2.Add check in createTerm() fucntion on LendingOnboard contract

    struct LendingTermParams {
       
        address collateralToken;
       
        uint256 maxDebtPerCollateralToken;
       
        uint256 interestRate;
        
        uint256 maxDelayBetweenPartialRepay;
       
        uint256 minPartialRepayPercent;

        uint256 openingFee;
        
        uint256 hardCap;
        
        bool hasPartialRepayment;
    }
    
     function createTerm(
        address implementation,
        LendingTerm.LendingTermParams calldata params
    ) external returns (address) {
    
    ...
    
    if(hasPartialRepayment) {
     params.minPartialRepayPercent >= MINIMUM_REPAY_RATE
    }
    ...
 }

Assessed type

Context

[c4-pre-sort]

0xSorryNotSorry marked the issue as sufficient quality report

[c4-pre-sort]

0xSorryNotSorry marked the issue as duplicate of #1174

[c4-pre-sort]

0xSorryNotSorry marked the issue as duplicate of #1057

[c4-judge]

Trumpero marked the issue as not a duplicate

[c4-judge]

Trumpero marked the issue as unsatisfactory:
Invalid

[c4-judge]

Trumpero marked the issue as duplicate of #1057

[c4-judge]

Trumpero marked the issue as satisfactory

[c4-judge]

Trumpero marked the issue as not a duplicate

[Trumpero]

After reviewing it again, I believe this issue isn't a duplicate of #1057, as it reflects the correct behavior of a lending term with minPartialRepayment == 0. This configuration is intended for such cases where maxDelayBetweenPartialRepay shouldn't be too long. So this scenario represents an admin mistake or a governance issue during the configuration of a lending term, making it an invalid issue due to C4's criteria.

[c4-judge]

Trumpero removed the grade

[c4-judge]

Trumpero marked the issue as unsatisfactory:
Invalid

[pavankv241]

Hi @c4-judge, @Trumpero

I hope you are doing well.

I agree this report is not duplicate of #1057 and in our report we didn't mean that minPartialRepayPercent can be set as 0 by admin or governance mistakes as we know it is intended and we got comment from developers also says that if minPartialRepayPercent is set as zero borrowers can as lower as they want.

We highlighted that if minPartialRepayPercent set as 0, borrowers can pay any small amounts and that loan couldn't called and auction also. The only way is that after the whole term off-boarded. There is no threshold days that loan has to pay. This will allows the borrowers to extend their own loan tenure, it causes the loss for lenders and we recommend either introducing a boolean value during term creation to specify if partial repayment is allowed and defining a minimum amount, or implementing a term in years within which borrowers must make their payments.

I appreciate your consideration and look forward to any further feedback.

[Trumpero]

@pavankv241
I still don't see any vulnerability here. In the case where a lending term is intended to have minPartialRepayPercent set to 0, that lending term is expected to be under the control of governance/Guild holders. Since governance and lenders should be aware of lending term's configs and their impacts before on-boarding, this lending term should be handled specially. For instance, the gauge weight delegated to this term shouldn't be too large (to limit debt ceiling), and governance needs to promptly act on off-boarding. It's just intended behavior of that special lending term.