Borrower can still add collateral, even after he can get liquidated because lending term is deprecated

[c4-bot-6]

Lines of code

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

Vulnerability details

Vulnerability details

According to the protocol logic once a lending term(gauge) has been deprecated all loans in it can be immediately called for liquidation by executing the call method of LendingTerm.

function _call(
        address caller,
        bytes32 loanId,
        address _auctionHouse
    ) internal {
        ....

        // check that the loan can be called
        require(
            //@audit - if gauge is deprecated I can get called even if I'm a good debtor
            GuildToken(refs.guildToken).isDeprecatedGauge(address(this)) ||
                partialRepayDelayPassed(loanId),
            "LendingTerm: cannot call"
        );
}

In the same contract there is also an addCollateral function - its purpose is to allow the borrower to keep his debt in good standing:

     /// @notice add collateral on an open loan.
    /// a borrower might want to add collateral so that his position does not go underwater due to
    /// interests growing up over time.
    function _addCollateral(
        address borrower,
        bytes32 loanId,
        uint256 collateralToAdd
    ) internal {...}

However no check is made in addCollateral if the term has been deprecated (it can be called anytime). This means that an unsuspecting user might add collateral to his loan, that will get called anyways. At the same the purpose of adding collateral in the system is to prevent exactly that from happening(as described in the docs and in function natspec). A malicious party might be monitoring all open debts on deprecated terms and take advantage by calling them and getting a higher collateral amount.

Impact

Advantageous users/bots might take advantage of the situation and wait for users to increase their callable debts collateral amount and take it for themselves.

This would also be a bad experience for the borrowers and would hurt badly the trust in the protocol, because borrowers would be adding collateral thinking it will keep their positions healthy and safe from being auctioned.

Proof of Concept

A POC added to LendingTerm.t.sol

function testAddCollateralToDeprecatedTerm() public {
        uint256 borrowAmount = 20_000e18;
        uint256 collateralAmount = 12e18;
        collateral.mint(address(this), collateralAmount * 2);
        collateral.approve(address(term), collateralAmount);

        // borrow
        bytes32 loanId = term.borrow(borrowAmount, collateralAmount);

        // verify borrow
        assertEq(term.getLoan(loanId).borrower, address(this));
        assertEq(term.getLoan(loanId).borrowTime, block.timestamp);
        assertEq(term.getLoan(loanId).borrowAmount, borrowAmount);
        assertEq(term.getLoan(loanId).collateralAmount, collateralAmount);
        assertEq(term.getLoan(loanId).caller, address(0));
        assertEq(term.getLoan(loanId).callTime, 0);
        assertEq(term.getLoan(loanId).closeTime, 0);

        // time passes
        vm.warp(block.timestamp + 1 weeks);

        // gauge is deprecated
        guild.removeGauge(address(term));

        // time passes
        vm.warp(block.timestamp + 1 days);

        // borrower increases 30% of the initial collateral amount
        collateral.approve(address(term), collateral.balanceOf(address(this)));
        term.addCollateral(loanId, (collateralAmount * 30) / 100);

        //bob is monitoring for the trx and calls the loan
        vm.prank(bob);
        term.call(loanId);

        //repayment not possible -even though I'm a good borrower
        vm.expectRevert("LendingTerm: loan called");
        term.repay(loanId);
    }

Tools Used

Foundry

Recommended Mitigation Steps

Add a check to addCollateral that prevents execution if gauge has been deprecated

require(
            !GuildToken(refs.guildToken).isDeprecatedGauge(address(this))
            "LendingTerm: cannot add collateral"
        );

Assessed type

Invalid Validation

[0xSorryNotSorry]

QA - User Error

[c4-pre-sort]

0xSorryNotSorry marked the issue as insufficient quality report

[c4-judge]

Trumpero changed the severity to QA (Quality Assurance)

[c4-judge]

Trumpero marked the issue as grade-c