claimGaugeRewards - users who have not yet apply loss still can receive new rewards

[c4-bot-3]

Lines of code

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#L409

Vulnerability details

Impact

Users who potentially lost weight due to the gauge loss is able to claim newly minted rewards after the loss occurs.

Proof of Concept

If a user has a loss on a gauge they have invested in, they will not be able to interact with that gauge until the loss has been applied by applyGaugeLoss.

function _decrementGaugeWeight(
    address user,
    address gauge,
    uint256 weight
) internal override {
    uint256 _lastGaugeLoss = lastGaugeLoss[gauge];
    uint256 _lastGaugeLossApplied = lastGaugeLossApplied[gauge][user];
    require(
@>      _lastGaugeLossApplied >= _lastGaugeLoss,
        "GuildToken: pending loss"
    );
    ...
}

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

If there is a loss, user needs to call applyGaugeLoss to reset the weight assigned to that gauge, and burn as many Guild tokens as the weight assigned. This means that the loss is not immediately reflected in, and someone needs to call applyGaugeLoss to apply it.

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

    // read user weight allocated to the lossy gauge
@>  uint256 _userGaugeWeight = getUserGaugeWeight[who][gauge];

    // remove gauge weight allocation
    lastGaugeLossApplied[gauge][who] = block.timestamp;
@>  _decrementGaugeWeight(who, gauge, _userGaugeWeight);
    if (!_deprecatedGauges.contains(gauge)) {
        totalTypeWeight[gaugeType[gauge]] -= _userGaugeWeight;
        totalWeight -= _userGaugeWeight;
    }

    // apply loss
@>  _burn(who, uint256(_userGaugeWeight));
    emit GaugeLossApply(
        gauge,
        who,
        uint256(_userGaugeWeight),
        block.timestamp
    );
}

However, ProfitManager.claimGaugeRewards does not take into account the loss to calculate how much reward the user can receive.

Therefore, if a loss occurs, user can call the ProfitManager's claimRewards or claimGaugeRewards function directly without calling applyGaugeLoss to receive the new reward using the old weight.

This is PoC. Add it to GuildToken.t.sol.

function testPoCGetNewRewardWithoutApplyloss() public {
  // setting
  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));
  
  profitManager.setProfitSharingConfig(
      0.25e18, // surplusBufferSplit
      0.50e18, // creditSplit
      0.25e18, // guildSplit
      0, // otherSplit
      address(0) // otherRecipient
  );

  vm.stopPrank();

  // setup
  token.setMaxGauges(3);
  token.addGauge(1, gauge1);
  token.addGauge(1, gauge2);
  token.addGauge(1, gauge3);
  token.mint(alice, 100e18);

  vm.startPrank(alice);
  token.incrementGauge(gauge1, 40e18);
  token.incrementGauge(gauge2, 40e18);
  vm.stopPrank();

  assertEq(token.userUnusedWeight(alice), 20e18);
  assertEq(token.getUserWeight(alice), 80e18);

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

  // Profit in gauge 1
  // simulate 100 profit on gauge1
  // 25 goes to alice (guild voting)
  // 50 goes to test (rebasing credit)
  // 25 goes to surplus buffer
  credit.mint(address(profitManager), 100e18);
  profitManager.notifyPnL(gauge1, 100e18);

  // Loss in gauge 1
  profitManager.notifyPnL(gauge1, -100);

  // but alice does not applyGaugeLoss. alice can get future interest with old weight

  // user still can claim old reward without applyGaugeLoss (This is expected behavior)
  assertEq(profitManager.claimRewards(alice), 25e18);

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

  // After some times, new profit generated in gauge 1
  // simulate 100 profit on gauge1
  // 25 goes to alice (guild voting)
  // 50 goes to test (rebasing credit)
  // 25 goes to surplus buffer
  credit.mint(address(profitManager), 100e18);
  profitManager.notifyPnL(gauge1, 100e18);

  // user can claim new profit with old weight (alice did not applyGaugeLoss)
  assertEq(profitManager.claimRewards(alice), 25e18);

  // realize loss in gauge 1
  assertEq(token.lastGaugeLossApplied(gauge1, alice), block.timestamp - 26); // this set when incrementGauge
  assertEq(token.balanceOf(alice), 100e18); // no guild token loss
  assertEq(token.userUnusedWeight(alice), 20e18);
  assertEq(token.getUserWeight(alice), 80e18); // no weight loss
  assertEq(token.getUserGaugeWeight(alice, gauge1), 40e18); // still have weight
  assertEq(token.getUserGaugeWeight(alice, gauge2), 40e18);
}

Tools Used

Manual Review

Recommended Mitigation Steps

Users should be able to claim their rewards up to the time of the loss, but not any new rewards earned after the loss. Each change in reward and loss should recorded as checkpoint-style, and users in a loss state can only claim rewards for last received~closest loss.

Assessed type

Other

[c4-pre-sort]

0xSorryNotSorry marked the issue as sufficient quality report

[c4-pre-sort]

0xSorryNotSorry marked the issue as primary issue

[c4-sponsor]

eswak (sponsor) acknowledged

[c4-sponsor]

eswak marked the issue as disagree with severity

[eswak]

The call to applyGaugeLoss is permissionless and GUILD stakers who did not suffer from the loss are incentivized to call it to increase their relative governance power in the gauge system. Therefore, it is very unlikely that users will continue to earn rewards for a long time after a loss has occurred, the SGM will be slashed and their individual positions will be slashed, by other users. Moreover, it is unlikely that a term suffers a loss and then records profit in a time horizon long enough such that no-one performs the slashing. A term is also likely having all loans in a loss position or all loans in a healthy position, but a mix of the two would be a governance mismanagement.

This likely won't be mitigated, and I'd qualify this as a Low given the low impact to users (minor rewards difference) and the likeliness of the situation (unlikely that no slashing is performed by other users given that the calls are permissionless).

[c4-judge]

Trumpero changed the severity to QA (Quality Assurance)

[c4-judge]

Trumpero marked the issue as grade-a

[c4-judge]

Trumpero marked the issue as grade-b

[Slavchew]

Hello, @Trumpero,

We would like to dispute the statement of the judge and reconsideration of the validity:

The call to applyGaugeLoss is permissionless and GUILD stakers who did not suffer from the loss are incentivized to call it to increase their relative governance power in the gauge system. Therefore, it is very unlikely that users will continue to earn rewards for a long time after a loss has occurred, the SGM will be slashed and their individual positions will be slashed, by other users.

This won’t be the case because there is a direct reward for the user who slashed another user:

• Not financially incentivized, unless they hold a big portion of the staking pool.
• When there are 100 stakers, how I will be sure that I’m not wasting my money on gas fees just to slash a user who has been already called by someone?

Even if we take the scenario when a user is being slashed days by someone after bad debt occurred in the system and then profitIndex of the SurplusGuildMinter contract increased due to repayments made by lenders, the same slashed user will be eligible to claim the rewards, effectively stealing from the other non-slashed users.

[Trumpero]

@Slavchew Regarding the sponsor's comment, reward distribution of protocol is intended to be a game between users. They can follow and slash each other, so I believe it's a fair situation. New stakers will have motivation to slash the old stakers, and slashed users won't receive rewards (because their gauge weight become 0) until they increment their gauge back. Therefore, I don't consider this issue as a loss of yield or loss of rewards, it should be a QA/info issue.