Partial Repayment Could Still be Possible Even When the Term's maxDelayBetweenPartialRepay is Zero

[c4-bot-9]

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/LendingTermOnboarding.sol#L105-L178
https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/LendingTerm.sol#L524

Vulnerability details

Impact

From the comment at:
https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/LendingTerm.sol#L130

/// if set to 0, no periodic partial repayments are expected.

From the comment, it's obvious that no periodic partial repayment is expected for a lending term when the maxDelayBetweenPartialRepay is Zero, but LendingTermOnboarding::createTerm function still allows the creation of a Lending term with maxDelayBetweenPartialRepay as zero and minPartialRepayPercent as non zero.
When such terms are onboarded borrowers can still make a partial repayment.
When we look at LendingTermOnboarding::createTerm function:
https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/LendingTermOnboarding.sol#L132-L140
We can observe that these values are allowed to go from [0, x]
so this case is indeed possible

I have added a coded POC below to better show this case:

Proof of Concept

    function test_CanPartialRepay() public {
        LendingTerm term = LendingTerm(
            onboarder.createTerm(
                address(termImplementation),
                LendingTerm.LendingTermParams({
                    collateralToken: address(collateral),
                    maxDebtPerCollateralToken: 2000e18,
                    interestRate: 0.10e18,
                    maxDelayBetweenPartialRepay: 0,
                    minPartialRepayPercent: 0.2e18,
                    openingFee: 0,
                    hardCap: _HARDCAP
                })
            )
        );

        assertEq(onboarder.created(address(term)), block.timestamp);
        vm.label(address(term), "term");

        // mint GUILD & self delegate
        guild.mint(alice, _PROPOSAL_THRESHOLD);
        guild.mint(bob, _QUORUM);
        vm.prank(alice);
        guild.delegate(alice);
        vm.prank(bob);
        guild.incrementDelegation(bob, _QUORUM);
        vm.roll(block.number + 1);
        vm.warp(block.timestamp + 13);

        // propose onboard
        vm.prank(alice);
        uint256 proposalId = onboarder.proposeOnboard(address(term));
        (
            address[] memory targets,
            uint256[] memory values,
            bytes[] memory calldatas,
            string memory description
        ) = onboarder.getOnboardProposeArgs(address(term));

        // check proposal creation
        assertEq(
            uint8(onboarder.state(proposalId)),
            uint8(IGovernor.ProposalState.Pending)
        );
        vm.roll(block.number + 1);
        vm.warp(block.timestamp + 13);
        assertEq(
            uint8(onboarder.state(proposalId)),
            uint8(IGovernor.ProposalState.Active)
        );

        // support & check status
        vm.prank(bob);
        onboarder.castVote(
            proposalId,
            uint8(GovernorCountingSimple.VoteType.For)
        );
        vm.roll(block.number + _VOTING_PERIOD + 1);
        vm.warp(block.timestamp + 13);
        assertEq(
            uint8(onboarder.state(proposalId)),
            uint8(IGovernor.ProposalState.Succeeded)
        );

        // queue
        vm.roll(block.number + 1);
        vm.warp(block.timestamp + 13);
        onboarder.queue(
            targets,
            values,
            calldatas,
            keccak256(bytes(description))
        );
        assertEq(
            uint8(onboarder.state(proposalId)),
            uint8(IGovernor.ProposalState.Queued)
        );

        // execute
        vm.roll(block.number + 1);
        vm.warp(block.timestamp + _TIMELOCK_MIN_DELAY + 13);
        onboarder.execute(
            targets,
            values,
            calldatas,
            keccak256(bytes(description))
        );
        assertEq(
            uint8(onboarder.state(proposalId)),
            uint8(IGovernor.ProposalState.Executed)
        );

        // check execution
        assertEq(guild.isGauge(address(term)), true);

        //deal with debtceiling issue
        guild.mint(address(this), _HARDCAP * 2);
        guild.incrementGauge(address(term), _HARDCAP);

        // prepare & borrow
        uint256 mintAmount = 31_000e18;
        uint256 borrowAmount = 20_000e18;
        uint256 collateralAmount = 15e18;
        collateral.mint(bob, mintAmount);
        vm.startPrank(bob);
        collateral.approve(address(term), mintAmount);
        bytes32 loanId = term.borrow(borrowAmount, collateralAmount);
        assertEq(term.getLoan(loanId).collateralAmount, collateralAmount);

        vm.warp(block.timestamp + term.YEAR());
        vm.roll(block.number + 1);

        assertEq(term.getLoanDebt(loanId), 22_000e18);
        credit.approve(address(term), 11_000e18);
        // test can still make a partial repay
        term.partialRepay(loanId, 11_000e18);

        assertEq(term.partialRepayDelayPassed(loanId), false);

        vm.warp(block.timestamp + term.YEAR() * 2 + 1);
        vm.roll(block.number + 1);
        //this always returns false because maxDelayBetweenPartialRepay is initialized to zero, thereby making
        //partialRepayDelayPassed() to never return a truthy
        assertEq(term.partialRepayDelayPassed(loanId), false);

        //though partial repayment is possible, calling this loanId isn't
        //All call attempts fails
        vm.expectRevert("LendingTerm: cannot call");
        term.call(loanId);
    }

Here are the logs:

[⠃] Compiling...
[⠒] Compiling 1 files with 0.8.13
[⠆] Solc 0.8.13 finished in 12.14s
Compiler run successful!

Running 1 test for test/unit/governance/LendingTermOnboarding.t.sol:LendingTermOnboardingUnitTest
[PASS] test_CanPartialRepay() (gas: 2153118)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 15.78ms
 
Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

Manual Review

Recommended Mitigation Steps

As discussed with a sponsor, I will suggest adding an explicit check in the LendingTermOnboarding::createTerm function that ensures maxDelayBetweenPartialRepay and minPartialRepayPercent are either both zero or non-zero when creating a term

Assessed type

Error

[c4-pre-sort]

0xSorryNotSorry marked the issue as sufficient quality report

[c4-pre-sort]

0xSorryNotSorry marked the issue as duplicate of #1057

[c4-judge]

Trumpero marked the issue as satisfactory