A large enough loss will brick the protocol

[c4-bot-3]

Lines of code

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

Vulnerability details

Impact

The totalBorrowedCredit function in the ProfitManager contract may revert if the creditMultiplier is low. This is because redeemableCredit is scaled by the creditMultiplier, but targetTotalSupply isn't. This can potentially brick core protocol functionality as no one would be able to unstake or borrow.

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

    /// @notice returns the sum of all borrowed CREDIT, not including unpaid interests
    /// and creditMultiplier changes that could make debt amounts higher than the initial
    /// borrowed CREDIT amounts.
    function totalBorrowedCredit() external view returns (uint256) {
        return
            CreditToken(credit).targetTotalSupply() -
            SimplePSM(psm).redeemableCredit();
    }

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/loan/SimplePSM.sol

   function redeemableCredit() public view returns (uint256) {
        return getMintAmountOut(pegTokenBalance);
    }

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

The totalBorrowedCredit function is reachable from several entry points including borrow(), unstake(), updateMintRatio(), and applyGaugeLoss(). If a large enough loss occurs, all these functions will revert, effectively bricking the protocol.

The root cause of this issue is the way totalBorrowedCredit is calculated. It subtracts redeemableCredit from targetTotalSupply. However, redeemableCredit is calculated using the creditMultiplier, which can decrease in the event of a loss. This is incorrect and was not the intention of the developers, as can be understood from totalBorrowedCredit()'s documentation. If the creditMultiplier is low enough, redeemableCredit can exceed targetTotalSupply, causing the subtraction to underflow and the function to revert.

Proof of Concept

Consider the following scenario:

1. The protocol has 100 CREDIT minted on deployment.
2. Alice mints 200 CREDIT through the PSM, resulting in a targetTotalSupply of 300 CREDIT and a pegTokenBalance of 200.
3. A loss of 150 CREDIT is applied, causing the creditMultiplier to decrease to 0.5.
4. Now, redeemableCredit is 400 (≈pegTokenBalance / creditMultiplier), which is greater than targetTotalSupply.
5. Calling totalBorrowedCredit now causes an underflow and reverts.

This scenario is illustrated in the testTotalBorrowedCredit_breaks test below:

        function testTotalBorrowedCredit_breaks() public {
        assertEq(profitManager.totalBorrowedCredit(), 100e18);

        // psm mint 200 CREDIT
        pegToken.mint(address(this), 200e6);
        pegToken.approve(address(psm), 200e6);
        psm.mint(address(this), 200e6);

        assertEq(pegToken.balanceOf(address(this)), 0);
        assertEq(pegToken.balanceOf(address(psm)), 200e6);
        assertEq(credit.balanceOf(address(this)), 300e18);
        assertEq(profitManager.totalBorrowedCredit(), 100e18);

        vm.startPrank(governor);
        core.grantRole(CoreRoles.GAUGE_PNL_NOTIFIER, address(this));
        vm.stopPrank();

        // apply a loss
        // 150 CREDIT of loans completely default (~150 USD loss)
        profitManager.notifyPnL(address(this), -150e18);
        assertEq(profitManager.creditMultiplier(), 0.5e18); // 50% discounted

        // totalBorrowedCredit() reverts since targetTotalSupply() is still 300
        // but redeemableCredit() is 400 (= pegTokenBalance / creditMultiplier)
        vm.expectRevert();
        profitManager.totalBorrowedCredit();
    }

Tools Used

Foundry

Recommended Mitigation Steps

Do not use the value returned by redeemableCredit to calculate the totalBorrowedCredit. Instead, use the amount that has been minted through the PSM without scaling.

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)

[Trumpero]

This issue has a valid description and root cause of an underflow vulnerability in the totalBorrowedCredit() function. However, the scenario of the PoC cannot happen in an operational context: for a loss of 150 CREDIT to occur, a loan of 150 CREDIT has to be opened. But if totalSupply is 300 and psm mints are 200, the maximum loan size and loss that can happen is 100. So I believe 75% partial credit is appropriate for this issue.

[c4-judge]

Trumpero marked the issue as satisfactory

[c4-judge]

Trumpero marked the issue as partial-75