Guild Holders can frontrun notifyPnL to escape losing GUILD when a loss in a lending term occurs

[c4-bot-9]

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/tokens/GuildToken.sol#L137-L140
https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/tokens/GuildToken.sol#L214-L217
https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/tokens/GuildToken.sol#L252-L255

Vulnerability details

Description

When a loss occurs in a lending term (loan generates bad debt), all the weight allocated to that term by Guild holders before the loss occurred must be burnt. To know if a Guild holder's weight is to be burnt, the timestamp when the loss occurs lastGaugeLoss[gauge], is compared with the timestamp of the first time the holder voted for the term lastGaugeLossApplied[gauge][user]. The check below is done when applyGaugeLoss is called in GuildToken to apply the loss of a term to a Guild holder.

    require(
        _lastGaugeLoss != 0 && _lastGaugeLossApplied < _lastGaugeLoss,
        "GuildToken: no loss to apply"
    );

If _lastGaugeLossApplied < _lastGaugeLoss it means the holder voted before the loss occurred and the weight he allocated must be burnt. A similar check is done in incrementGauge and decrementGauge to prevent the user from increasing or decreasing his voting weight and updating lastGaugeLossApplied[gauge][user] to escape having his weight burnt. Here's the check in _incrementGaugeWeight and _decrmentGaugeWeight functions which are called by the incrementGauge and decrementGauge functions respectively.

            require(
                _lastGaugeLossApplied >= _lastGaugeLoss,
                "GuildToken: pending loss"
            );

The two checks above protect the weight of a holder who votes for a term in the same block where a loss occurs from being burnt. This is logical since the new weight didn't support the loan the loss occurred. But this also allows weights that supported the loan from being burnt if they can update lastGaugeLossApplied[gauge][user] in the same block where the loss occurs. The holder can update it if he calls incrementGauge. It's updated in this if statement in incrementGauge.

    if (getUserGaugeWeight[user][gauge] == 0) {
        lastGaugeLossApplied[gauge][user] = block.timestamp;
    } else {
        require(
            _lastGaugeLossApplied >= _lastGaugeLoss,
            "GuildToken: pending loss"
        );
    }

The first section of the if statement in incrementGauge will set lastGaugeLossApplied[gauge][user] to block.timestamp which is what he needs. But his weight will have to be empty. To do that he can call decrementGauge to remove his weight before calling incrementWeight to add the weight back and reset lastGaugeLossApplied[gauge][user] to block.timestamp. This allows him to remain in the pool and escape burning. He can do this provided the issuance doesn't become greater than the debt ceiling of the term when he decrements else the decrementGauge call will revert.

To do this he can frontrun any transaction that calls notifyPnL on the term with a transaction that decrements and increments his weight on the same block before the notifyPnL transaction is processed.

He may also decide to frontrun the call with a call to decrementGauge to allow him to leave the pool.

Stakers in SurplusGuildMinter will also escape getting slashed if they can unstake and stake in the same block the loss occurs before the loss transaction is processed. They can also just completely unstake their position and abandon the term.

Impact

This allows a holder to escape getting his weight burnt despite supporting a loan that produces a bad debt.

Proof of Concept

A scenario this can occur is:

1. Alice votes for gauge1 using her Guild balance.
2. Bob takes a loan two days later.
3. Bob fails to partially repay on time and the loan is called and will generate bad debt.
4. But before Bob's loan could be called (step 3), Alice notices the transaction and frontruns it with a transaction that calls decrementGauge and incrementGauge.
5. Alice escapes having her weight being burnt and can remove her weight from the gauge (term) at anytime.

Here's a POC in code. The code can be run in GuildToken.t.sol.

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

    // setup
    token.setMaxGauges(3);
    token.addGauge(1, gauge1);
    token.mint(alice, 100e18);
    vm.prank(alice); 
    token.incrementGauge(gauge1, 40e18);

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

    // Bad girl Alice frontrun notifyPnL call with a call to
    // decrease and increase weight
    vm.startPrank(alice);
    token.decrementGauge(gauge1, 40e18);
    token.incrementGauge(gauge1, 40e18);
    vm.stopPrank();

    // loss is applied by governance
    profitManager.notifyPnL(gauge1, -100);

    // loss can't be applied to Bad Girl Alice
    vm.expectRevert("GuildToken: no loss to apply");
    token.applyGaugeLoss(gauge1, alice);
    
    // Bad girl Alice can still decrement her weight
    vm.prank(alice); 
    token.decrementGauge(gauge1, 40e18);
}

Tools Used

Manual Analysis

Recommended Mitigation Steps

From the code, notifyPnl is called with debt when a loan is forgiven, and from the AuctionHouse when the total debt of a loan is not covered.
Calls to decrementGauge can be reverted if the associated lending term currently has a loan in the auction. This prevents holders from calling it to remove all their weight when they notice a bid in the auction will produce bad debt.
It's difficult to suggest a fix for forgiving a loan since it will pass through governance and holders can see that they'll be burnt even before it is done.

Assessed type

Invalid Validation

[c4-pre-sort]

0xSorryNotSorry marked the issue as sufficient quality report

[c4-pre-sort]

0xSorryNotSorry marked the issue as duplicate of #906

[c4-pre-sort]

0xSorryNotSorry marked the issue as duplicate of #877

[c4-judge]

Trumpero marked the issue as unsatisfactory:
Invalid

[c4-judge]

Trumpero marked the issue as unsatisfactory:
Invalid

[c4-judge]

Trumpero marked the issue as unsatisfactory:
Invalid