Guild rewards are calculated wrong

[c4-bot-5]

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/SurplusGuildMinter.sol#L216-L290 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/SurplusGuildMinter.sol#L328-L333

Vulnerability details

Impact

Every staker in the SurplusGuildMinter accumulates rewards for their Guild tokens received upon staking. The reward calculation relies on the Guild amount for each staker converted to Credit reward and the current reward ratio. However, there is an issue with how rewards are handled after the update of the reward ratio.

When the rewardRatio changes, the guild rewards up to that point should be calculated based on the old ratio. After the change, new rewards should be accumulated based on the updated ratio. However, the current implementation doesn't support this behavior. Instead, regardless of how the rewardRatio changes, it always multiplies it with the staker's guild amount converted to credit reward. This is because, on a rewardRatio update, the system does not store all staker rewards up to that moment.

See: Line 252

File: src/loan/SurplusGuildMinter.sol

216: function getRewards(
217:     address user,
218:     address term
219: )
         // ... (Code)
248:
249:     if (deltaIndex != 0) {
250:         uint256 creditReward = (uint256(userStake.guild) * deltaIndex) /
251:             1e18;
252:  -->    uint256 guildReward = (creditReward * rewardRatio) / 1e18;

         // ... (More Code)

The issue lies in the fact that, irrespective of the rewardRatio being 10%, 8%, or 6%, the computation takes place only when the getRewards() function is called. This poses a problem because if a staker has maintained their stake from the beginning and calls getRewards() later when the ratio is, for instance, 6%, they won't receive the correct amount of rewards but the calculation will calculate it as if it was 6% from the beginning.

Also, if staker A stakes at time 0 and claims rewards at a 6% rate at time 1500, and staker B stakes at time 1499 and claims rewards with staker A at time 1500, both will receive the same amount. This is totally wrong and remove the incentive for staking.

Proof of Concept 1

1. A staker stakes 1000 credit tokens at a 2:1 ratio, receiving 2000 Guild tokens.
2. The RewardRatio decreases twice, first from 10% to 8%, and then to 6%.
3. Despite being a staker during these changes, the staker doesn't claim rewards.
4. When they finally call getRewards(), the rewardRatio is 6%, resulting in an incorrect payout from 2000 guild (in the form of credit rewards) * 6%. This is inaccurate as the staker joined when the reward ratio was 10%.

Results from the diagram provided above and test.

Note: The rewards are calculated based on converting 2000 guild tokens to creditReward and subsequently multiplying the result by the rewardRatio. The table demonstrates how rewards vary when claimed in three different situations after applying a notifyPnL() with 1e18.

It's important to note that these rewards are not added together and getRewards() is called only one time. Refer to the test below for more detailed information.

RewardRatio at called block	Block at which getRewards() is called	Rewards
10%	4	5e15
8%	9	4e15
6%	15	3e15

Coded PoC

Import console from "@forge-std/Test.sol”

Comment out these 2 lines in the setUp() to work on an empty balance and see it more clearly:

74: guild.mint(address(this), 50e18);
75: guild.incrementGauge(term, uint112(50e18));

Place the 3 tests in SurplusGuildMinter.t.sol

Run them with:

forge test --match-contract "SurplusGuildMinterUnitTest" --match-test "testGuildRewardsWrongAccumulated" -vvv

function testGuildRewardsWrongAccumulatedCase1() public {
    vm.prank(governor);
    sgm.setRewardRatio(0.1e18); // From deploy script

    credit.mint(address(this), 100e18);

    vm.startPrank(address(this));
    credit.approve(address(sgm), 100e18);
    sgm.stake(term, 100e18);
    vm.stopPrank();

    vm.warp(block.timestamp + 4 * 13);
    vm.prank(governor);
    profitManager.setProfitSharingConfig(
        0.05e18, // surplusBufferSplit
        0.9e18, // creditSplit
        0.05e18, // guildSplit
        0, // otherSplit
        address(0) // otherRecipient
    );

    credit.mint(address(profitManager), 1e18);
    profitManager.notifyPnL(term, 1e18);

    console.log("Staker guild balance after accumulating rewards:                    ", guild.balanceOf(address(this)));
    sgm.getRewards(address(this), term);
    console.log("Staker guild reward if claim them at 10% rewardRate:  ", guild.balanceOf(address(this)));

    vm.warp(block.timestamp + 5 * 13);
    vm.prank(governor);
    sgm.setRewardRatio(0.08e18);

    vm.warp(block.timestamp + 6 * 13);
    vm.prank(governor);
    sgm.setRewardRatio(0.06e18);
}

function testGuildRewardsWrongAccumulatedCase2() public {
    vm.prank(governor);
    sgm.setRewardRatio(0.1e18); // From deploy script

    credit.mint(address(this), 100e18);

    vm.startPrank(address(this));
    credit.approve(address(sgm), 100e18);
    sgm.stake(term, 100e18);
    vm.stopPrank();

    vm.warp(block.timestamp + 4 * 13);
    vm.prank(governor);
    profitManager.setProfitSharingConfig(
        0.05e18, // surplusBufferSplit
        0.9e18, // creditSplit
        0.05e18, // guildSplit
        0, // otherSplit
        address(0) // otherRecipient
    );

    credit.mint(address(profitManager), 1e18);
    profitManager.notifyPnL(term, 1e18);

    console.log("Staker guild balance after accumulating rewards:                    ", guild.balanceOf(address(this)));

    vm.warp(block.timestamp + 5 * 13);
    vm.prank(governor);
    sgm.setRewardRatio(0.08e18);

    sgm.getRewards(address(this), term);
    console.log("Staker guild reward if claim them at 8% rewardRate:   ", guild.balanceOf(address(this)));

    vm.warp(block.timestamp + 6 * 13);
    vm.prank(governor);
    sgm.setRewardRatio(0.06e18);
}

function testGuildRewardsWrongAccumulatedCase3() public {
    vm.prank(governor);
    sgm.setRewardRatio(0.1e18); // From deploy script

    credit.mint(address(this), 100e18);

    vm.startPrank(address(this));
    credit.approve(address(sgm), 100e18);
    sgm.stake(term, 100e18);
    vm.stopPrank();

    vm.warp(block.timestamp + 4 * 13);
    vm.prank(governor);
    profitManager.setProfitSharingConfig(
        0.05e18, // surplusBufferSplit
        0.9e18, // creditSplit
        0.05e18, // guildSplit
        0, // otherSplit
        address(0) // otherRecipient
    );

    credit.mint(address(profitManager), 1e18);
    profitManager.notifyPnL(term, 1e18);

    console.log("Staker guild balance after accumulating rewards:                    ", guild.balanceOf(address(this)));

    vm.warp(block.timestamp + 5 * 13);
    vm.prank(governor);
    sgm.setRewardRatio(0.08e18);

    vm.warp(block.timestamp + 6 * 13);
    vm.prank(governor);
    sgm.setRewardRatio(0.06e18);

    sgm.getRewards(address(this), term);
    console.log("Staker guild reward if claim them at 6% rewardRate:   ", guild.balanceOf(address(this)));
}

[PASS] testGuildRewardsWrongAccumulatedCase1() (gas: 734911)
Logs:
  Staker guild balance after accumulating rewards:                     0
  Staker guild reward if claim them at 10 rewardRate:   5000000000000000

[PASS] testGuildRewardsWrongAccumulatedCase2() (gas: 734884)
Logs:
  Staker guild balance after accumulating rewards:                     0
  Staker guild reward if claim them at 8 rewardRate:    4000000000000000

[PASS] testGuildRewardsWrongAccumulatedCase3() (gas: 734945)
Logs:
  Staker guild balance after accumulating rewards:                     0
  Staker guild reward if claim them at 6 rewardRate:    3000000000000000

Proof of Concept 2

1. Staker A stakes 1000 Credits on block 0 with a 10% rate.
2. Staker B stakes the same amount on block 1499.
3. Rewards accumulate until block 1500, and both stakers call getRewards().
4. Both end up with the same rewards.

Coded PoC

Import console from "@forge-std/Test.sol”

Comment out these 2 lines in the setUp() to work on an empty balance and see it more clearly:

74: guild.mint(address(this), 50e18);
75: guild.incrementGauge(term, uint112(50e18));

Place the test in SurplusGuildMinter.t.sol

Run with:

forge test --match-contract "SurplusGuildMinterUnitTest" --match-test "testGuildRewardsFor2Stakers" -vvv

function testGuildRewardsFor2Stakers() public {
    vm.prank(governor);
    sgm.setRewardRatio(0.1e18); // From deploy script

    credit.mint(address(this), 100e18);

    vm.startPrank(address(this));
    credit.approve(address(sgm), 100e18);
    sgm.stake(term, 100e18);
    vm.stopPrank();

    vm.warp(block.timestamp + 149 * 13);

    address staker2 = makeAddr("staker2");
    credit.mint(staker2, 100e18);

    vm.startPrank(staker2);
    credit.approve(address(sgm), 100e18);
    sgm.stake(term, 100e18);
    vm.stopPrank();

    vm.warp(block.timestamp + 1 * 13);
    vm.prank(governor);
    profitManager.setProfitSharingConfig(
        0.05e18, // surplusBufferSplit
        0.9e18, // creditSplit
        0.05e18, // guildSplit
        0, // otherSplit
        address(0) // otherRecipient
    );

    credit.mint(address(profitManager), 1e18);
    profitManager.notifyPnL(term, 1e18);

    sgm.getRewards(address(this), term);
    sgm.getRewards(staker2, term);
    console.log("StakerA guild reward if claim them at 10%% rewardRate:   ", guild.balanceOf(address(this)));
    console.log("StakerB guild reward if claim them at 10%% rewardRate:   ", guild.balanceOf(staker2));
}

Logs:
  StakerA guild reward if claim them at 10% rewardRate:    2500000000000000
  StakerB guild reward if claim them at 10% rewardRate:    2500000000000000

Tools Used

Manual Review

Recommended Mitigation Steps

When the Governor calls setRewardRatio(), it should first invoke getRewards() for all stakers. Moreover, it is crucial to implement an elapsed time logic for calculating rewards based on the duration elapsed from one rewardRatio to another. Each update of the rewardRatio should be treated as a distinct phase, and all rewards for these phases should be stored in the UserStake struct. The calculation for phase rewards should be executed each time setRewardRatio() is called, considering the time elapsed since the last update.

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 primary issue

[eswak]

Acknowledging this, disagree with severity (imo it's informational).

This is the expected behavior, users are supposed to check each other and if the rewards go up, claimRewards of others so that they are not earning more rewards unduly. And if rewards are going down (nominal foreseen case), users are expected to claim their rewards prior to the rewardRatio decline. Ultimately the governance is a game of who has relatively more tokens, so the users act as keepers to each other to make sure no undue rewards are earned. Also there is no interpolation of the rewards because the amounts are expected to be small and frequent.

[c4-sponsor]

eswak (sponsor) acknowledged

[c4-sponsor]

eswak marked the issue as disagree with severity

[c4-judge]

Trumpero changed the severity to QA (Quality Assurance)

[c4-judge]

Trumpero marked the issue as grade-a

[Trumpero]

Considering this issue as informational

[Slavchew]

Hey, @Trumpero, thanks for your time,

We believe this issue should be reconsidered because the sponsor explained their approach and how the protocol should function, but we demonstrate that it's completely incorrect.

We highlight two main problems with the rewards.

When the rewardRatio changes, the guild rewards up to that point should be calculated based on the old ratio. After the change, new rewards should be accumulated based on the updated ratio. However, the current implementation doesn't support this behavior.

The sponsor suggested that users should monitor if the rewardRatio changes and then execute claimRewards for themselves and all other users in each term within the current market.

If the rewardRatio changes and there are 100 stakers, according to the suggestion, the subsequent 100 transactions would require all stakers to execute claimRewards, which is deemed impractical and hardly to achieve. Additionally, there's a concern about the user's incentive to claim rewards if the ratio drops, as all their rewards, regardless of the staked amount, will be based on the new ratio.

Looking at the other scenario, it's evident that rewards are solely based on the amount staked, not on the duration of staking. This means if I stake at the beginning and someone stakes 100 blocks after me, during reward distribution, both will receive equal amounts, eliminating the entire incentive for staking early.

Here's a similar problem example: https://solodit.xyz/issues/m-11-should-accrue-before-change-loss-of-rewards-in-case-of-change-of-settings-code4rena-reserve-reserve-contest-git

[Trumpero]

@Slavchew Reward distribution is intended to be a game among users, where users follow and interact with each other. This is the reason why claimRewards and applyGaugeLoss functions are permissionless. They don't need to update all positions, instead, they should consider the completed positions of others and the gas cost to act and optimize their rewards. Additionally, the likelihood of changes in rewardRatio and mintRatio is low, so the cost to update other positions is not too much compared to the long-term reward.