Calculating the total borrowed credit may encounter underflow, potentially preventing any borrowing activity in the market.

[c4-bot-8]

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/ProfitManager.sol#L172-L176

Vulnerability details

Impact

We determine the borrowed credit by subtracting the pegged amount linked to underlying tokens from the total supply of credit tokens.
While the logic is correct, there's a potential for underflow in the totalBorrowedCredit function.
If such an underflow occurs, any borrowing attempt across all terms in the market will be reverted, as this function is invoked whenever someone attempts to borrow from a term.
I marked this as high because if such a case occurs, this market would become unusable.

Proof of Concept

The function responsible for calculating the total borrowed credit is as below:
https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/ProfitManager.sol#L172-L176

function totalBorrowedCredit() external view returns (uint256) {
    return
        CreditToken(credit).targetTotalSupply() -
        SimplePSM(psm).redeemableCredit();
}

When borrowers repay their debts or their debts are called by others, the total borrowed credit is reduced to 0.
However, there is no guarantee that the following statement is always true:

CreditToken(credit).targetTotalSupply() >= SimplePSM(psm).redeemableCredit()

I will illustrate one example to demonstrate such a case.
While this scenario is both simple and uncommon, it serves as an illustration of potential underflow, and there may be other cases leading to similar issues.

A lender initially lends 10^20 credit tokens.
Borrower A borrows 10^20 from a term, followed by borrower B also borrowing 10^20 from the same term.
At this point, the total supply is 3 * 10^20, and the creditMultiplier is 10^18.
Subsequently, the debt for borrower A is either called or forgiven, leading to a new creditMultiplier of 666,666,666,666,666,666.
Afterward, borrower B's debt is similarly called or forgiven, resulting in a new creditMultiplier of 333,333,333,333,333,332.
The target total supply remains at 3 * 10^20, with a redeemableCredit of 300,000,000,000,000,001,200.

X = 10^20, p1 = 10^18;
// first forgive
p1 * (3 * X - X * 10^18 / p1) = p2 * 3 * X; // p2 = 666,666,666,666,666,666
// second forgive
p2 * (3 * X - X * 10^18 / p2) = p3 * 3 * X; // p3 = 333,333,333,333,333,332
SimplePSM(psm).redeemableCredit() = 300,000,000,000,000,001,200;
targetTotalSupply = 300,000,000,000,000,000,000;

The result of the PoC test is as follows:

|   [11310] 0x535B3D7A252fa034Ed71F0C53ec0C6F784cB64E1::borrow(500000000000000000000 [5e20], 100000000000000000000 [1e20]) 
    │   ├─ [11130] LendingTerm::borrow(500000000000000000000 [5e20], 100000000000000000000 [1e20]) [delegatecall]
    │   │   ├─ [394] profitManager::creditMultiplier() [staticcall]
    │   │   │   └─ ← 333333333333333332 [3.333e17]
    │   │   ├─ [608] profitManager::minBorrow() [staticcall]
    │   │   │   └─ ← 300000000000000001200 [3e20]
    │   │   ├─ [3188] profitManager::totalBorrowedCredit() [staticcall]
    │   │   │   ├─ [1361] SimplePSM::redeemableCredit() [staticcall]
    │   │   │   │   ├─ [394] profitManager::creditMultiplier() [staticcall]
    │   │   │   │   │   └─ ← 333333333333333332 [3.333e17]
    │   │   │   │   └─ ← 300000000000000001200 [3e20]
    │   │   │   ├─ [576] credit::targetTotalSupply() [staticcall]
    │   │   │   │   └─ ← 300000000000000000000 [3e20]
    │   │   │   └─ ← "Arithmetic over/underflow"
    │   │   └─ ← "Arithmetic over/underflow"
    │   └─ ← "Arithmetic over/underflow"
    └─ ← "Arithmetic over/underflow"

And the PoC for this is as below:

function testTotalBorrowedCredit() public {
        // create a new lending term
        vm.startPrank(governor);
        LendingTerm newTerm = LendingTerm(Clones.clone(address(new LendingTerm())));
        newTerm.initialize(
            address(core),
            LendingTerm.LendingTermReferences({
                profitManager: address(profitManager),
                guildToken: address(guild),
                auctionHouse: address(auctionHouse),
                creditMinter: address(rlcm),
                creditToken: address(credit)
            }),
            LendingTerm.LendingTermParams({
                collateralToken: address(collateral),
                maxDebtPerCollateralToken: _CREDIT_PER_COLLATERAL_TOKEN,
                interestRate: _INTEREST_RATE,
                maxDelayBetweenPartialRepay: _MAX_DELAY_BETWEEN_PARTIAL_REPAY,
                minPartialRepayPercent: _MIN_PARTIAL_REPAY_PERCENT,
                openingFee: 0,
                hardCap: _HARDCAP
            })
        );
        core.grantRole(CoreRoles.RATE_LIMITED_CREDIT_MINTER, address(newTerm));
        core.grantRole(CoreRoles.GAUGE_PNL_NOTIFIER, address(newTerm));
        vm.stopPrank();

        guild.addGauge(1, address(newTerm));
        guild.incrementGauge(address(newTerm), _HARDCAP / 100);

        uint128 X = 100e18;
        vm.startPrank(governor);
        core.grantRole(CoreRoles.CREDIT_MINTER, address(psm));
        vm.stopPrank();

        collateral.mint(address(this), X);
        collateral.approve(address(psm), X);
        psm.mint(address(this), X);

        collateral.mint(address(this), X);
        collateral.approve(address(term), X);
        bytes32 loanID_1 = term.borrow(X, X);

        vm.warp(block.timestamp + 3 days);
        collateral.mint(address(this), X);
        collateral.approve(address(term), X);
        bytes32 loanID_2 = term.borrow(X, X);

        vm.startPrank(governor);
        term.forgive(loanID_1);
        term.forgive(loanID_2);
        vm.stopPrank();

        // all borrowing requests will be reverted due to arithmetic underflow in totalBorrowedCredit() function.
        newTerm.borrow(5 * X, X);
}

Tools Used

https://www.calculator.net/big-number-calculator.html

Recommended Mitigation Steps

In the totalBorrowedCredit function, please incorporate a check for underflow.
In such a case, return 0.
This adjustment won't impact the protocol, as the value is negligible and can be safely ignored.

function totalBorrowedCredit() external view returns (uint256) {
        + if (SimplePSM(psm).redeemableCredit() > CreditToken(credit).targetTotalSupply()) return 0;
        return
            CreditToken(credit).targetTotalSupply() -
            SimplePSM(psm).redeemableCredit();
}

Assessed type

Under/Overflow

[c4-pre-sort]

0xSorryNotSorry marked the issue as sufficient quality report

[c4-pre-sort]

0xSorryNotSorry marked the issue as duplicate of #1170

[c4-judge]

Trumpero changed the severity to 2 (Med Risk)

[c4-judge]

Trumpero marked the issue as satisfactory