Ethereum Credit Guild - carrotsmuggler's results

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/SurplusGuildMinter.sol#L223-L234

Vulnerability details

Impact

The function getReward in surplusGuildMinter.sol contract is used to calculate pending rewards for the Surplus minter contract. It also calculates if a term has had negative profit, and thus if a gauge has been slashed. This is done in the following snippet.

function getRewards(
    address user,
    address term
)
    public
    returns (
        uint256 lastGaugeLoss, // GuildToken.lastGaugeLoss(term)
        UserStake memory userStake, // stake state after execution of getRewards()
        bool slashed // true if the user has been slashed
    )
{
    bool updateState;
    lastGaugeLoss = GuildToken(guild).lastGaugeLoss(term);
    if (lastGaugeLoss > uint256(userStake.lastGaugeLoss)) {
        slashed = true;
    }

The issue is that userStake is being used without being assigned. userStake is actually allocated in the return part of the function prototype, and is an empty struct, thus having userStake.lastGaugeLoss return 0. This means if a slashing has happened in the past at any point, every user will be flagged as slashed=true even if they had staked after the slashing.

The correct assignment actually takes place below this code block.

// if the user is not staking, do nothing
userStake = _stakes[user][term];
if (userStake.stakeTime == 0)
    return (lastGaugeLoss, userStake, slashed);

Thus the value is used before assignment, leading to incorrect results. This leads to direct loss, since in the unstake function, the user is not give any tokens back if they are slashed.

if (slashed) return;

Proof of Concept

The test testUnstakeWithoutLoss in SurplusGuildMinter.t.sol has been modified to show the issue. A loss is realized, and then a user stakes on to the system. The getRewards function shows that the user has been slashed, even though they have not.

function testAttackSGM() public {
  // setup
  credit.mint(address(this), 150e18);
  credit.approve(address(sgm), 150e18);
  sgm.stake(term, 150e18);
  assertEq(credit.balanceOf(address(this)), 0);
  assertEq(profitManager.surplusBuffer(), 0);
  assertEq(profitManager.termSurplusBuffer(term), 150e18);
  assertEq(guild.balanceOf(address(sgm)), 300e18);
  assertEq(guild.getGaugeWeight(term), 350e18);
  assertEq(sgm.getUserStake(address(this), term).credit, 150e18);

  // the guild token earn interests
  vm.prank(governor);
  profitManager.setProfitSharingConfig(
    0.5e18, // surplusBufferSplit
    0, // creditSplit
    0.5e18, // guildSplit
    0, // otherSplit
    address(0) // otherRecipient
  );
  credit.mint(address(profitManager), 35e18);
  profitManager.notifyPnL(term, 35e18);
  assertEq(profitManager.surplusBuffer(), 17.5e18);
  assertEq(profitManager.termSurplusBuffer(term), 150e18);
  (, , uint256 rewardsThis) = profitManager.getPendingRewards(address(this));
  (, , uint256 rewardsSgm) = profitManager.getPendingRewards(address(sgm));
  assertEq(rewardsThis, 2.5e18);
  assertEq(rewardsSgm, 15e18);

  // next block
  vm.warp(block.timestamp + 13);
  vm.roll(block.number + 1);

  // loss in gauge
  profitManager.notifyPnL(term, -27.5e18);
  assertEq(profitManager.surplusBuffer(), 17.5e18 + 150e18 - 27.5e18); // 140e18
  assertEq(profitManager.termSurplusBuffer(term), 0);

  // cannot stake if there was just a loss
  vm.expectRevert('SurplusGuildMinter: loss in block');
  sgm.stake(term, 123);

  // unstake (sgm)
  sgm.unstake(term, 123);
  assertEq(credit.balanceOf(address(this)), rewardsSgm); // lost 150 credit principal but earn the 15 credit of dividends
  assertEq(guild.balanceOf(address(this)), 50e18 + 0); // no guild reward because position is slashed
  assertEq(credit.balanceOf(address(sgm)), 0); // did not withdraw from surplus buffer
  assertEq(guild.balanceOf(address(sgm)), 300e18); // still not slashed
  assertEq(guild.getGaugeWeight(term), 350e18); // did not decrementWeight
  assertEq(sgm.getUserStake(address(this), term).credit, 0); // position slashed

  // slash sgm
  guild.applyGaugeLoss(term, address(sgm));
  assertEq(guild.balanceOf(address(sgm)), 0); // slashed
  assertEq(guild.getGaugeWeight(term), 50e18); // weight decremented

  // POC STARTS HERE
  // stake again
  vm.warp(block.timestamp + 13);
  credit.mint(address(this), 150e18);
  credit.approve(address(sgm), 150e18);
  sgm.stake(term, 150e18);
  //check slash
  (, , bool slash) = sgm.getRewards(address(this), term);
  assertEq(slash, true);
}

Only the last few lines after the // POC STARTS HERE statement are new. The rest is from the provided test.

Tools Used

Foundry

Recommended Mitigation Steps

Shift the assignment statement of userStake to the top of the function.

Assessed type

Context

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

Lines of code

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

Vulnerability details

Impact

The function notifyPnL in ProfitManager.sol is responsible for accounting the profits gained by the system, and also the losses, slashing and recovery of bad loans. When a loss is encountered, gauge votes are slashed and if there is a loss to the system, the creditMultiplier is decreased to reflect the loss.

The creditMultiplier is updated in case of a loss in the following snippet.

// update the CREDIT multiplier
uint256 creditTotalSupply = CreditToken(_credit).totalSupply();
uint256 newCreditMultiplier = (creditMultiplier *
    (creditTotalSupply - loss)) / creditTotalSupply;
creditMultiplier = newCreditMultiplier;

Here the term creditTotalSupply - loss is calculated to calculate the new credit multiplier. The issue is that the loss can be larger than the creditTotalSupply for large loans. This will lead to a revert, and block the function notifyPnL breaking the gauge slashing and voting systems as well.

This scenario can happen in the following manner:

1. Alice deposits USDC and mints 1e18 gUSDC. Say she is the largest participator in the market.
2. Alice goes to the SurplusGuildMinter contract and stakes the some of the minted gUSDC tokens (say 0.8e18) on her own term.
3. Alice defaults due to some depeg event. System enters a loss.

Now, the code will enter the notifyPnL function with some large loss amount. Since Alice provided only a part of the tokens to the surplusbuffer in step 2, we can assume loss > _surplusBuffer. This executes the part of the code which updates the creditMultiplier.

else {
// empty the surplus buffer
loss -= _surplusBuffer;
surplusBuffer = 0;
CreditToken(_credit).burn(_surplusBuffer);
emit SurplusBufferUpdate(block.timestamp, 0);

// update the CREDIT multiplier
uint256 creditTotalSupply = CreditToken(_credit).totalSupply();
uint256 newCreditMultiplier = (creditMultiplier *
    (creditTotalSupply - loss)) / creditTotalSupply;
creditMultiplier = newCreditMultiplier;
emit CreditMultiplierUpdate(
    block.timestamp,
    newCreditMultiplier
);

As we can see above, the code first calls burn on the surplusbuffer. This burns up all the tokens supplied by alice in there, drastically reducing the totalSupply of the credit tokens. In this scenario, Alice's loss is 1e18, but the totalSupply just got nuked to 0.2e18. This will lead to a revert in the next line, as creditTotalSupply - loss will be negative. This will break the notifyPnL function, and the gauge slashing and voting systems as well.

This also pushes the system towards more bad debt, since the auction process cannot be completed since the onBid function also calls notifyPnL to update the credit multiplier.

The impact is high, and the likelihood is medium since most DeFi projects have whales who take out large loans and farm with them, making this a likely scenario. Thus the overall severity is high.

Proof of Concept

Attached is a POC loosely following the attack path described above. It is a modification of the testBidSuccessBadDebt test and should be added to the AuctionHouse.t.sol test file.

function testAttackBid() public {
  bytes32 loanId = _setupAndCallLoan();
  uint256 PHASE_1_DURATION = auctionHouse.midPoint();
  uint256 PHASE_2_DURATION = auctionHouse.auctionDuration() - auctionHouse.midPoint();
  vm.roll(block.number + 1);
  vm.warp(block.timestamp + PHASE_1_DURATION + (PHASE_2_DURATION * 2) / 3);

  // At this time, get full collateral, repay half debt
  (uint256 collateralReceived, uint256 creditAsked) = auctionHouse.getBidDetail(loanId);

  emit log_named_uint('collateralReceived', collateralReceived);
  emit log_named_uint('creditAsked', creditAsked);

  vm.startPrank(borrower);
  credit.burn(20_000e18);
  vm.stopPrank();

  // bid
  credit.mint(bidder, creditAsked);
  vm.startPrank(bidder);
  credit.approve(address(term), creditAsked);
  vm.expectRevert();
  auctionHouse.bid(loanId);
  vm.stopPrank();
}

In this POC, Alice mints tokens and then her loan gets sent to the auction house. Once Alice burns her tokens, Bob can no longer bid on her loan. Alice burning her tokens is the same as her supplying them to surplusguildminter since that contract will also burn her tokens.

Tools Used

Manual Review, Foundry

Recommended Mitigation Steps

If the loss is greater than the creditTotalSupply, the creditMultiplier should be set to 0. This will prevent the system from breaking.

Assessed type

Under/Overflow

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/LendingTerm.sol#L652-L656

Vulnerability details

Impact

Liquidations are an important mechanism to keep a lending platform solvent. This platform enables liquidations using an auction mechanism to ensure best prices for defaulted accounts. This is handled via the call function in LendingTerm.sol contract which checks if the loan is indeed able to be auctioned off in the following snippet.

require(
    GuildToken(refs.guildToken).isDeprecatedGauge(address(this)) ||
        partialRepayDelayPassed(loanId),
    "LendingTerm: cannot call"
);

The issue is that the partialRepayDelayPassed always returns false for non-periodic payment loans. The system tracks the value maxDelayBetweenPartialRepay which ensures regular payments are made. The system also shows intention of supporting loans which dont have periodic payments, by setting this value to 0 as described in the comments.

/// @notice maximum delay, in seconds, between partial debt repayments.
/// if set to 0, no periodic partial repayments are expected.
/// if a partial repayment is missed (delay has passed), the loan
/// can be called.
uint256 maxDelayBetweenPartialRepay;

The issue is that if maxDelayPartialRepay is set to 0, the partialRepayDelayPassed function will always return false as seen in the following snippet.

function partialRepayDelayPassed(
    bytes32 loanId
) public view returns (bool) {
    // if no periodic partial repays are expected, always return false
    if (params.maxDelayBetweenPartialRepay == 0) return false;

So the only way to liquidate loans without payment plans is to deprecate the entire gauge. This is not ideal as it would liquidate all loans in the gauge. This is a design issue that can lead to insolvency of the protocol.

Proof of Concept

This is a design issue and basically has missing code to handle loans which the documentation in the code claims to support. No POC is provided since the functionality is entirely missing.

Tools Used

Manual review

Recommended Mitigation Steps

Implement a due date mechanism for loans without payment plans. If the due date passes, the loan will be eligible for liquidation.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/SurplusGuildMinter.sol#L158-L212 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/tokens/GuildToken.sol#L207-L234

Vulnerability details

Impact

The SurplusGuildMinter contract is used to stake credit tokens and partake in a portion of the profits generated by the system. Users can mint guild tokens against their credit tokens which are then automatically staked in gauges. In exchange, the user receives a portion of the profits generated by the system via interest payments.

The user can also directly put their own guild tokens in gauges to claim profits.

The issue is that there are no restrictions for unstaking / removing gaugeweights, meaning that users can take a flashloan of credit tokens and guild tokens, stake them, make a repayment, and then immediately unstake them for profit.

Lets assume user Alice has taken out a large borrow of 1e6 ether gUSDC tokens. When she repays, she will have to pay a 10% interest amounting to 1e5 ether gUSDC tokens.

Alice takes 2 flashloans. First, Alice takes a lashloan of credit tokens, and goes to SurplusGuildMinter.sol and stakes them on her loan term. She then takes a flashloan of guild tokens and stakes them on the gauges on Guildtoken.sol. Flashloans are easily available via flashswaps on modt DEXes, like uniV2 and uniV3. She then pays off her borrow, unstakes both her credit and guild positions, and repays the flashloans.

Since the protocol made a profit, the profit is distributed as shown int he snippet below.

// distribute to surplus buffer
if (amountForSurplusBuffer != 0) {
    surplusBuffer = _surplusBuffer + amountForSurplusBuffer;
    emit SurplusBufferUpdate(
        block.timestamp,
        _surplusBuffer + amountForSurplusBuffer
    );
}

// distribute to other
if (amountForOther != 0) {
    CreditToken(_credit).transfer(
        _profitSharingConfig.otherRecipient,
        amountForOther
    );
}

// distribute to lenders
if (amountForCredit != 0) {
    CreditToken(_credit).distribute(amountForCredit);
}

// distribute to the guild
if (amountForGuild != 0) {

Thus Alice can collect her share from the amountForGuild part through her direct stake in gauges, as well as indirect stake via SurplusGuildMinter.sol. Also, SurplusGuildMinter.sol mints extra guild tokens to stakers when they incur a profit as shown, which Alice can also collect.

if (deltaIndex != 0) {
    uint256 creditReward = (uint256(userStake.guild) * deltaIndex) /
        1e18;
    uint256 guildReward = (creditReward * rewardRatio) / 1e18;
    if (slashed) {
        guildReward = 0;
    }

    // forward rewards to user
    if (guildReward != 0) {
        RateLimitedMinter(rlgm).mint(user, guildReward);
        emit GuildReward(block.timestamp, user, guildReward);
    }
    if (creditReward != 0) {
        CreditToken(credit).transfer(user, creditReward);
    }

    // save the updated profitIndex
    userStake.profitIndex = SafeCastLib.safeCastTo160(_profitIndex);
    updateState = true;
}

So Alice has collected back part of her interest payment, collected extra guild rewards from the SurplusGuildMinter.sol contract, and denied other users of these staking mechanisms any profit while having 0 risk. She can do all these interactions in the same transaction by employing a smart contract.

Since this denies user rewards, games the guild rewards from the SurplusGuildMinter.sol contract, and also happens to be the most optimum way to interact with the protocol, this is a high severity issue.

Proof of Concept

All tests in the test suite show that SurplusGuildMinter.sol and GuildToken.sol do not restrict deposits and withdrawals within the same block in any way.

The attack is carried out in the following steps.

1. Have a large borrow up for repayment
2. Flashloan Credit tokens and Guild tokens.
3. Stake guild tokens in the gauge, and credit tokens in surplusGuildMinter
4. Repay loan
5. Unstake from the 2 contracts and repay the flashloans

Tools Used

Manual Review

Recommended Mitigation Steps

Add either a timelock or a fee for unstaking on SurplusGuildMinter.sol and GuildToken.sol to prevent this attack vector.

Assessed type

MEV

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/LendingTerm.sol#L270-L331

Vulnerability details

Impact

The function applyGaugeLoss in Guild.sol is used to apply a loss to a user's gauge position. It removes the gauge weight and burns up the freed guild tokens associated with it. The function calls the _decrementGaugeWeight function which does certain checks.

// check if gauge is currently using its allocated debt ceiling.
// To decrement gauge weight, guild holders might have to call loans if the debt ceiling is used.
uint256 issuance = LendingTerm(gauge).issuance();
if (issuance != 0) {
    uint256 debtCeilingAfterDecrement = LendingTerm(gauge).debtCeiling(
        -int256(weight)
    );
    require(
        issuance <= debtCeilingAfterDecrement,
        "GuildToken: debt ceiling used"
    );
}

This code check the issuance of the term. If non-zero, it makes sure that the debtCeiling of that term is above the current issuance. Otherwise, it will revert. We look into the debtCeiling function to check what kind of values it returns.

function debtCeiling(int256 gaugeWeightDelta) public view returns (uint256) {
  uint256 creditMinterBuffer = RateLimitedMinter(refs.creditMinter).buffer();
  //...
  return _hardCap < creditMinterBuffer ? _hardCap : creditMinterBuffer;
  //...
  return _hardCap < creditMinterBuffer ? _hardCap : creditMinterBuffer;
  //...
  return _hardCap < creditMinterBuffer ? _hardCap : creditMinterBuffer;
  //...
  if (creditMinterBuffer < _debtCeiling) {
    return creditMinterBuffer;
  }
}

This function is full of this statement, where if creditMinterBuffer is low, it will return that value skipping all other calculations. So we check buffer() on the RateLimitedV2 contract.

function buffer() public view returns (uint256) {
  uint256 elapsed = block.timestamp.safeCastTo32() - lastBufferUsedTime;
  return Math.min(bufferStored + (rateLimitPerSecond * elapsed), bufferCap);
}

This is basically a function whose values grows with time. Another function depleteBuffer can reduce this functions return value by using up the free buffer.

The issue is that if buffer() is low due to a recent borrow or any mint of guild tokens, then the debtCeiling function will return the very low buffer() value. If there is any pending loans on that term, the statement issuance <= debtCeilingAfterDecrement will most likely return false, casuing a revert.

This prevents calls to applyGaugeLoss from working. This will further prevent gauge weights from being unallocated from that gauge. This also prevents the owner of the guild tokens from transferring their guild tokens. Say Alice has 100 guild tokens in various gauges. Gauge 1 got a bad loan and the buffer is low. Alice cannot de-allocate from gauge 2 or send around any of her other guild tokens due to the one stuck position on gauge 1. Bad actors can use this to lock up positions of users for limited amounts of time.

This can also happen under other circumstances as written in a comment in the code.

/// Note that the debt ceiling can be lower than the current issuance under 4 conditions :
/// - params.hardCap is lower than since last borrow happened
/// - gauge votes are fewer than when last borrow happened
/// - profitManager.totalBorrowedCredit() decreased since last borrow
/// - creditMinter.buffer() is close to being depleted

Since this freezes tokens in user accounts depending on conditions outside of the user's control and gives wrong gauge weights, this is a critical issue.

Proof of Concept

The return values of debtCeiling() function is checked in the test testBorrowFailDebtCeilingReached in LendingTerm.sol which shows that this function will return low values if the buffer is low. Since this is the only criteria for this function to revert, it shows that our assumption about the behaviour of debtCeiling is correct. Similarly, the test testDecrementGaugeDebtCeilingUsed in GaugeToken.sol shows that decrementgaugeweight will revert if the debt ceiling is used. No POC is provided since these two tests prove the case.

Tools Used

Foundry

Recommended Mitigation Steps

Add a special permission to applyGaugeLoss so that it can decrement weight even if the debt ceiling is used.

Assessed type

Under/Overflow

Judge has assessed an item in Issue #1212 as 2 risk. The relevant finding follows:

2. _partialRepay in LendingTerm.sol does not support 0 interest loans

The function _partialRepay is used to make partial repayments of a loan. The issue is that during repayment, the interestRepaid is always required to be above 0.

uint256 interestRepaid = debtToRepay - principalRepaid;
require(
    principalRepaid != 0 && interestRepaid != 0,
    "LendingTerm: repay too small"
);