totalBorrowedCredit can revert, breaking gauges.

[c4-bot-3]

Lines of code

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

Vulnerability details

Impact

The function totalBorrowedCredit is used to get an estimate of the amount of credit borrowed out by the system. The issue is that changes in creditMultiplier affect this value and can even lead to a revert due to underflow.

The function totalBorrowedCredit is defined as follows:

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

The first term is the total supply of the credit tokens including rebasing rewards. The second part is the amount of credit tokens that can be redeemed, and is defined as shown below:

uint256 creditMultiplier = ProfitManager(profitManager)
    .creditMultiplier();
return (amountIn * decimalCorrection * 1e18) / creditMultiplier;

If creditMultiplier decreases due to a realized loss, the value returned by redeemableCredit goes up. If this value exceeds the targetTotalSupply() value, this function call will revert.

Assume a fresh market deployment. There are no loans at all. Alice now mints 1e18 credit tokens via the PSM, so the targetTotalSupply() is 1e18 and redeemableCredit is also 1e18. If the same situation is realized after the market has incurred a loss, the creditMultiplier will be less than 1e18, and the redeemableCredit will be greater than 1e18. This will cause the function to revert. A malicious borrower can also burn some of their credit tokens to help brick this function.

The totalBorrowedCredit function is used in debtCeiling calculations, and thus when it reverts it will also break term borrow operations, breaking functionality.

Proof of Concept

In this POC the following steps are demonstrated:

1. User mints via PSM module
2. A loss is realized, causing the creditMultiplier to decrease
3. User burns up a bunch of their credit tokens, causing the totalSupply to drop
4. The totalBorrowedCredit function call reverts.

Put this in the ProfitManager.t.sol file.

function testAttackRevert() public {
  // grant roles to test contract
  vm.startPrank(governor);
  core.grantRole(CoreRoles.GAUGE_PNL_NOTIFIER, address(this));
  core.grantRole(CoreRoles.CREDIT_MINTER, address(this));
  vm.stopPrank();

  emit log_named_uint('TBC 1', profitManager.totalBorrowedCredit());
  // psm mint 100 CREDIT
  pegToken.mint(address(this), 100e6);
  pegToken.approve(address(psm), 100e6);
  psm.mint(address(this), 100e6);

  emit log_named_uint('TBC 2', profitManager.totalBorrowedCredit());

  // apply a loss
  // 50 CREDIT of loans completely default (50 USD loss)
  profitManager.notifyPnL(address(this), -50e18);
  emit log_named_uint('TBC 3', profitManager.totalBorrowedCredit());

  // burn tokens to throw off the ratio
  credit.burn(70e18);
  vm.expectRevert();
  emit log_named_uint('TBC 4', profitManager.totalBorrowedCredit());
}

The expectRevert in the end shows the revert. Consequences due to this failure is evident since this function is used in the debtCeiling calculations in lending terms.

Since this breaks lending terms, it is a critical issue.

Tools Used

Foundry

Recommended Mitigation Steps

The totalBorrowedCredit function should never fail. Either cap it to 0 to prevent underflows. Or a better way to is to track the total tokens minted by the PSM module with a variable which is incremented every mint and decremented every burn. This removes the dependence on creditMultiplier which can change over time even if PSM module users haven't minted or burnt any excess tokens.

Assessed type

Under/Overflow

[c4-pre-sort]

0xSorryNotSorry marked the issue as primary issue

[c4-pre-sort]

0xSorryNotSorry marked the issue as sufficient quality report

[c4-sponsor]

eswak (sponsor) confirmed

[eswak]

Confirming this issue.

I don't know what rules are used to determine high/medium, but this would only prevent new borrows in a market, and doesn't prevent a safe wind down of a market, and does not prevent users from withdrawing their funds, so I'd qualify as Medium, even though it definitely needs a fix.

Thanks for the quality of the report.

[c4-sponsor]

eswak marked the issue as disagree with severity

[Trumpero]

I agree that this should be a medium based on both its impact and likelihood.

[c4-judge]

Trumpero changed the severity to 2 (Med Risk)

[c4-judge]

Trumpero marked the issue as satisfactory

[c4-judge]

Trumpero marked the issue as selected for report