Any stake will be reverted until the SurplusGuildMinter applies the loss.

[c4-bot-4]

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/SurplusGuildMinter.sol#L136
https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/tokens/GuildToken.sol#L252-L255

Vulnerability details

Impact

Multiple users can stake in one term through SurplusGuildMinter.
If that term experiences any loss, no users can stake in it until the SurplusGuildMinter applies the loss.
Applying loss, in this context, means reducing the gauge weight – the sum of guilds the users staked through SurplusGuildMinter – to 0.
It's obvious that interest can still be assigned to the loss-experienced gauge.
As a result, some users may wish to receive interest through that term, while others want to stake again.
This inconsistency poses a challenge, and user stakes will revert until the loss is applied.
This could lead to fund loss due to gas fees.
I marked this as medium because it may result in fund loss and violate the expectations of users who still wish to receive rewards.

Proof of Concept

When a user attempts to stake in a term, it increases the gauge weight of SurplusGuildMinter for that term.
https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/SurplusGuildMinter.sol#L136

function stake(address term, uint256 amount) external whenNotPaused {
    GuildToken(guild).incrementGauge(term, guildAmount);
}

In the _incrementGaugeWeight function, we check whether this user has applied loss in the event that the gauge has undergone some loss.
If not, it will revert.
https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/tokens/GuildToken.sol#L252-L255

function _incrementGaugeWeight(
    address user,
    address gauge,
    uint256 weight
) internal override {
    uint256 _lastGaugeLoss = lastGaugeLoss[gauge];
    uint256 _lastGaugeLossApplied = lastGaugeLossApplied[gauge][user];
    if (getUserGaugeWeight[user][gauge] == 0) {
    } else {
        require(
            _lastGaugeLossApplied >= _lastGaugeLoss,
            "GuildToken: pending loss"
        );
    }
}

For instance, some users staked in a term T through SurplusGuildMinter.
After some time, that term experienced a loss.
Some users can attempt to stake in term T, but their efforts will revert because SurplusGuildMinter has not yet applied the loss.

The PoC for this is as below:

function testApplyStakeAfterLoss() public {
        ProfitManager newProfitManager;
        MockERC20 newCollateral;
        CreditToken newCredit;
        GuildToken newGuild;
        RateLimitedMinter newRlcm;
        AuctionHouse newAuctionHouse;
        LendingTerm newTerm;
        SimplePSM newPsm;
        RateLimitedMinter newRlgm;
        SurplusGuildMinter newSgm;

        newProfitManager = new ProfitManager(address(core));
        newCollateral = new MockERC20();
        newCredit = new CreditToken(address(core), "name", "symbol");
        newGuild = new GuildToken(
            address(core),
            address(newProfitManager)
        );
        newRlcm = new RateLimitedMinter(
            address(core) /*_core*/,
            address(newCredit) /*_token*/,
            CoreRoles.RATE_LIMITED_CREDIT_MINTER /*_role*/,
            type(uint256).max /*_maxRateLimitPerSecond*/,
            type(uint128).max /*_rateLimitPerSecond*/,
            type(uint128).max /*_bufferCap*/
        );
        newAuctionHouse = new AuctionHouse(address(core), 650, 1800);
        newTerm = LendingTerm(Clones.clone(address(new LendingTerm())));
        newTerm.initialize(
            address(core),
            LendingTerm.LendingTermReferences({
                profitManager: address(newProfitManager),
                guildToken: address(newGuild),
                auctionHouse: address(newAuctionHouse),
                creditMinter: address(newRlcm),
                creditToken: address(newCredit)
            }),
            LendingTerm.LendingTermParams({
                collateralToken: address(newCollateral),
                maxDebtPerCollateralToken: 2000e18,
                interestRate: 0.10e18,
                maxDelayBetweenPartialRepay: 63115200, // 2 years
                minPartialRepayPercent: 0.2e18,
                openingFee: 0,
                hardCap: 20_000_000e18
            })
        );
        newPsm = new SimplePSM(
            address(core),
            address(newProfitManager),
            address(newCredit),
            address(newCollateral)
        );
        newRlgm = new RateLimitedMinter(
            address(core), /*_core*/
            address(newGuild), /*_token*/
            CoreRoles.RATE_LIMITED_GUILD_MINTER, /*_role*/
            type(uint256).max, /*_maxRateLimitPerSecond*/
            type(uint128).max, /*_rateLimitPerSecond*/
            type(uint128).max /*_bufferCap*/
        );
        newSgm = new SurplusGuildMinter(
            address(core),
            address(newProfitManager),
            address(newCredit),
            address(newGuild),
            address(newRlgm),
            MINT_RATIO,
            REWARD_RATIO
        );

        vm.startPrank(governor);
        core.grantRole(CoreRoles.CREDIT_MINTER, address(newPsm));
        core.grantRole(CoreRoles.CREDIT_MINTER, address(newRlcm));
        core.grantRole(CoreRoles.RATE_LIMITED_CREDIT_MINTER, address(newTerm));
        core.grantRole(CoreRoles.GAUGE_PNL_NOTIFIER, address(newTerm));
        core.grantRole(CoreRoles.GUILD_MINTER, address(newRlgm));
        core.grantRole(CoreRoles.RATE_LIMITED_GUILD_MINTER, address(newSgm));
        core.grantRole(CoreRoles.GUILD_SURPLUS_BUFFER_WITHDRAW, address(newSgm));
        newProfitManager.initializeReferences(address(newCredit), address(newGuild), address(newPsm));
        vm.stopPrank();

        // add gauge and vote for it
        newGuild.setMaxGauges(10);
        newGuild.addGauge(1, address(newTerm));

        uint128 X = 100e18;

        // stake
        newCredit.mint(address(this), X);
        newCredit.approve(address(newSgm), X);
        newSgm.stake(address(newTerm), X);

        newCollateral.mint(address(this), X);
        newCollateral.approve(address(newPsm), X);
        newPsm.mint(address(this), X);

        // borrow from this term
        newCollateral.mint(address(this), X);
        newCollateral.approve(address(newTerm), X);
        bytes32 loanId = newTerm.borrow(X, X);

        vm.warp(block.timestamp + 3 days);
        // loss happens
        vm.startPrank(governor);
        newTerm.forgive(loanId);
        vm.stopPrank();

        // another stake
        vm.warp(block.timestamp + 3 days);
        newCredit.mint(address(this), X);
        newCredit.approve(address(newSgm), X);

        // The stake will revert because the SurplusGuildMinter has not applied the loss yet
        vm.expectRevert("GuildToken: pending loss");
        newSgm.stake(address(newTerm), X);
}

Tools Used

Recommended Mitigation Steps

We can call the applyGaugeLoss function for the first user who tries to stake in a loss-experienced gauge.
This can resolve the above problem.
However, it's important to note that other users won't receive rewards anymore because the gauge weight becomes 0.

Assessed type

DoS

[c4-pre-sort]

0xSorryNotSorry marked the issue as sufficient quality report

[c4-pre-sort]

0xSorryNotSorry marked the issue as duplicate of #262

[c4-judge]

Trumpero marked the issue as not a duplicate

[Trumpero]

This's not a dup of #262 and I believe it's invalid as it represents the intended behavior. Gas loss due to user mistakes isn't considered to be an issue.

[c4-judge]

Trumpero marked the issue as unsatisfactory:
Invalid