Underflow causes DOS of borrow function across all terms in a market

[c4-bot-5]

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/ProfitManager.sol#L172-L176 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/SimplePSM.sol#L86-L99 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/tokens/ERC20RebaseDistributor.sol#L451-L453

Vulnerability details

In ProfitManager.sol; the function totalBorrowedCredit() can underflow due to an inconsistent adjustment of the parameters used to calculate it. with the creditMultipler().

totalBorrowedCredit() is calculated by subtracting targetTotalSupply() from redeemableCredit(); where targetTotalSupply() is the sum of all minted CREDIT tokens and redeemableCredit() is the sum of CREDIT issued to user's who deposit funds via SimplePSM.deposit()

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

The issue stems from the creditMutiplier() being applied to redeemableCredit() but not to targetTotalSupply(). This means that where a loss is absorbed by the system, reducing the creditMuliplier(), the value forredeemableCredit()` increases as a proportion of total supply of CREDIT.

    function getRedeemAmountOut(
        uint256 amountIn
    ) public view returns (uint256) {
        uint256 creditMultiplier = ProfitManager(profitManager)
            .creditMultiplier();
        return (amountIn * creditMultiplier) / 1e18 / decimalCorrection;
    }

    /// @notice calculate the total number of CREDIT that can be redeemed
    /// at the moment, based on the pegTokenBalance.
    function redeemableCredit() public view returns (uint256) {
        return getMintAmountOut(pegTokenBalance);
    }

It should not be possible for redeemableCredit() to be greater than the total supply of CREDIT and given that it can be, it leads to an underflow where redeemableCredit() is larger than targetTotalSupply().

Impact

The impact will be a DOS of LendingTerm::borrow() across all Lending Terms in a market as the borrow() function will continue to revert. This means that although risk should be siloed per Lending Term; in actuality a large failing loan in one term can shutdown all the others in a market. There would be no straightforward way to fix the DOS except for adjusting the CREDIT supply by minting or waiting for the market to adjust.

Proof of Concept

Add the following lines of code to LendingTerm.t.sol. First add the following to set-up to add a new gauge:

+   LendingTerm termTwo;

    function setUp() public {
        vm.warp(1679067867);
        vm.roll(16848497);
        core = new Core();

        profitManager = new ProfitManager(address(core));
        collateral = new MockERC20();
        credit = new CreditToken(address(core), "name", "symbol");
        guild = new GuildToken(
            address(core),
            address(profitManager)
        );
        rlcm = new RateLimitedMinter(
           /// SOME CODE ///
        );
        auctionHouse = new AuctionHouse(address(core), 650, 1800);
        term = LendingTerm(Clones.clone(address(new LendingTerm())));
        term.initialize(
           /// SOME CODE ///
        );

+        termTwo = LendingTerm(Clones.clone(address(new LendingTerm())));
+        termTwo.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
+            })
+        );

        psm = new SimplePSM(
            address(core),
            address(profitManager),
            address(credit),
            address(collateral)
        );
        profitManager.initializeReferences(address(credit), address(guild), address(psm));

        // roles
        core.grantRole(CoreRoles.GOVERNOR, governor);
        core.grantRole(CoreRoles.GUARDIAN, guardian);
        core.grantRole(CoreRoles.CREDIT_MINTER, address(this));
        core.grantRole(CoreRoles.GUILD_MINTER, address(this));
        core.grantRole(CoreRoles.GAUGE_ADD, address(this));
        core.grantRole(CoreRoles.GAUGE_REMOVE, address(this));
        core.grantRole(CoreRoles.GAUGE_PARAMETERS, address(this));
+       core.grantRole(CoreRoles.CREDIT_MINTER, address(psm));
        core.grantRole(CoreRoles.CREDIT_MINTER, address(rlcm));
        core.grantRole(CoreRoles.RATE_LIMITED_CREDIT_MINTER, address(term));
        core.grantRole(CoreRoles.GAUGE_PNL_NOTIFIER, address(term));
        core.renounceRole(CoreRoles.GOVERNOR, address(this));

        // add gauge and vote for it
        guild.setMaxGauges(10);
        guild.addGauge(1, address(term));
+        guild.addGauge(1, address(termTwo)); 
        guild.mint(address(this), _HARDCAP * 2);
        guild.incrementGauge(address(term), _HARDCAP);
+        guild.incrementGauge(address(termTwo), _HARDCAP); 

Then add the test test_BorrowRevert to LendingTerm.t.sol.

    function test_BorrowRevert() public {

        uint256 mintAmount = 100000e18;
        uint256 psmDeposit = 100e18;
        uint256 loanAmount = 100e18;
        uint256 collateralAmount = 100e18;
        int256 loss = -50e18;

        // Mint collateral to user
        collateral.mint(address(this), mintAmount);
        collateral.approve(address(psm), mintAmount);
        collateral.approve(address(term), mintAmount);
        collateral.approve(address(termTwo), mintAmount);

        // Deposit collateral for equal amount of CREDIT
        psm.mint(address(this), psmDeposit);
        assertEq(credit.balanceOf(address(this)), psmDeposit);

        // Simulate large loan amount; which will increase the targetTotalSupply 
        term.borrow(loanAmount, collateralAmount);
        assertEq(credit.balanceOf(address(this)), psmDeposit + loanAmount);

        // Artificially decrease creditMultiplier by notifying ProfitManager of a loss and burning tokens
        vm.prank(address(term));
        profitManager.notifyPnL(address(term), loss);
        assertEq(profitManager.creditMultiplier(), 0.75e18);
        credit.burn(100e18); 

        // All terms revert due to totalBorrowedCredit underflow
        vm.expectRevert();
        term.borrow(1e18, collateralAmount);

        vm.expectRevert();
        termTwo.borrow(1e18, collateralAmount);
    }

Tools Used

Manual Review Foundry

Recommended Mitigation Steps

The protocol should adjusts the debt of borrowers as it already adjusts the debt of lenders prior to performing the subtraction. targetTotalSupply() should be adjusted by the creditMultiplier so that underflow is no longer possible before performing the subtraction where the underflow occurs.

Assessed type

DoS

[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 marked the issue as satisfactory