Inconsistency in users Guild rewards after new mintRatio is set

[c4-bot-7]

Lines of code

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

Vulnerability details

Impact

When mintRatio is changed, a staker's guild amount should increase or decrease accordingly. However, this adjustment only occurs if the staker or someone else calls the updateMintRatio() function after the Governor set a new ratio. If a user unstakes or claims rewards after the mintRatio is updated but updateMintRatio() hasn't been called for them, they will receive rewards based on the old ratio, leading to a potential inconsistency.

The protocol cannot depend on the assumption that all users will update their guild amounts after the governor sets a new mintRatio. This reliance could result in future rewards being based on inconsistent guild amounts.

Proof of Concept

1. A user stakes 1000 credits at a 3:1 mintRatio.
2. Rewards accumulate.
3. The Governor changes the ratio to 2:1.
4. No one calls the updateMintRatio() for the user.
5. Staker claims old rewards based on 3:1 ratio
6. Rewards accumulate again.
7. When the user unstakes or claims rewards again, their rewards will be based on 3000 guild again, not on 2000 guild.

Coded PoC

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

Place the 3 tests in SurplusGuildMinter.t.sol

Run them with:

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

function testGuildRewardsAfterChangeMintRatio() public {
    vm.startPrank(governor);
    sgm.setRewardRatio(0.1e18);
    sgm.setMintRatio(3e18);
    vm.stopPrank();

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

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

    vm.warp(block.timestamp + 150 * 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("-------------------Rewards accumulation-------------------");

    SurplusGuildMinter.UserStake memory userStakeBeforeUpdate = sgm.getUserStake(address(this), term);
    console.log("Staker guild amount before update: ", userStakeBeforeUpdate.guild);

    vm.prank(governor);
    sgm.setMintRatio(2e18);

    SurplusGuildMinter.UserStake memory userStakeAfterUpdate = sgm.getUserStake(address(this), term);
    console.log("Staker guild amount after update:  ", userStakeAfterUpdate.guild);

    sgm.getRewards(address(this), term);

    vm.warp(block.timestamp + 150 * 13);
    console.log("-----------------New rewards accumulation-----------------");
    credit.mint(address(profitManager), 1e18);
    profitManager.notifyPnL(term, 1e18);
    
    sgm.getRewards(address(this), term);
    SurplusGuildMinter.UserStake memory userStakeAfterUpdate2 = sgm.getUserStake(address(this), term);
    console.log("Staker guild amount after update 2:", userStakeAfterUpdate2.guild);
}

Logs:
  -------------------Rewards accumulation-------------------
  Staker guild amount before update:  3000000000000000000000
  Staker guild amount after update:   3000000000000000000000
  -----------------New rewards accumulation-----------------
  Staker guild amount after update 2: 3000000000000000000000

Tools Used

Manual Review

Recommended Mitigation Steps

Make sure to update all users' guild amounts right after setting a new mintRatio. While we can't provide specific code advice due to issues in other parts of the contract, here's the main idea:
When the governor changes the mintRatio, users should accumulate rewards based on their old guild amounts up to that moment. From the moment of the mintRatio update, the calculation of new rewards should be based on the new mint ratio. It's essential to distribute these rewards after the update and apply the logic in updateMintRatio() to ensure that new rewards reflect the updated guild amounts.

Assessed type

Context

[c4-pre-sort]

0xSorryNotSorry marked the issue as sufficient quality report

[c4-pre-sort]

0xSorryNotSorry marked the issue as duplicate of #937

[c4-judge]

Trumpero changed the severity to QA (Quality Assurance)

[c4-judge]

Trumpero marked the issue as grade-b

[c4-judge]

Trumpero marked the issue as grade-a

[Slavchew]

Hey, @Trumpero

This issue would need to be reconsidered.

eswak:

Somewhat similar to #1026 so I'm going to comment something along the same lines :

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

This is the expected behavior, users are supposed to check each other and if the mintRatio go down, updateMintRatio of others so that they are not earning more rewards unduly. And if mintRatio is going up, users are expected to update their position to benefit from the new ratio. 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, and individually they are expected to do the actions needed to maximize their rewards.

#937 (comment)

The sponsor suggests that users should monitor if the mintRatio changes and then execute updateMintRatio for themselves and all other users in each term within the current market.

If the mintRatio changes and there are 100 stakers, according to the suggestion, the subsequent 100 transactions would require all stakers to execute updateMintRatio, which is deemed impractical and hard to achieve.

Also, as we've pointed out, who will prevent me or any user from claiming their reward right after the mintRatio update, before anyone else updates their Guild supply?

Claiming rewards at a better mint ratio can even happen unintentionally because of external factors, such as low gas provided or network congestion.

[Trumpero]

Commented here