Updating MintRatio can lead to out of sync reward values

[c4-bot-3]

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/loan/SurplusGuildMinter.sol#L293-L315 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/loan/SurplusGuildMinter.sol#L250-L251

Vulnerability details

Impact

When the governor updates the mintRatio, all the users who stake are not required to call updateMintRatio This can result in lost rewards for users.

Proof of Concept

The following code can be added to the SurplusGuildMinterUnitTest.sol

struct UserStake {
    uint48 stakeTime;
    uint48 lastGaugeLoss;
    uint160 profitIndex;
    uint128 credit;
    uint128 guild;
}

function test_unstakeUpdateMintRatio() public {
    credit.mint(alice, 100e18);
    credit.mint(bob, 100e18);

    assertEq(sgm.mintRatio(), MINT_RATIO); // 1:2

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

    UserStake memory userStakeBefore = sgm.getUserStake(alice, term);
    asserEq(userStakeBefore.credit, 100e18);
    asserEq(userStakeBefore.guild, 200e18);

    vm.prank(governor);
    sgm.setMintRatio(MINT_RATIO * 2); // 1:4

    // still the same after update
    asserEq(userStakeBefore.credit, 100e18);
    asserEq(userStakeBefore.guild, 200e18);

    // update Ratio for user
    sgm.updateMintRatio(alice, term); // double alice's stake

    // guild amount in user Stake would have been changed.
    UserStake memory userStakeAfter = sgm.getUserStake(alice, term);
    asserEq(userStakeBefore.credit, 100e18);
    asserEq(userStakeBefore.guild, 400e18);

    // when the stake would have rewards
    // without updating mint ratio for user:
    // uint256 creditReward = (uint256(200e18) * 0.2e18) / 1e18;
    // uint256 guildReward = (40e18 * 5e18) / 1e18;
            // creditReward would be: 40e18
    // guildReward would be: 200e18

    // when updateMintRatio would be required when mintUpdate change
    // this would result in higher rewards for user:
    // uint256 creditReward = (uint256(400e18) * 0.2e18) / 1e18;
    // uint256 guildReward = (80e18 * 5e18) / 1e18;
            // creditReward would be: 80e18
    // guildReward would be: 400e18
}

Tools Used

Foundry

Recommended Mitigation Steps

Add a modifier to the getRewards can help fix this issue. This way when the user is calling getRewards and the mintRatio has changed, the user will get updated rewards based on the mintRatio

modifier syncMintRatio(address user, address term) {
    updateMintRatio(user, term)
    _;
}

function getRewards(
  address user,
  address term
) public
+ syncMintRatio(user, term)
returns (
  uint256 lastGaugeLoss, // GuildToken.lastGaugeLoss(term)
  UserStake memory userStake, // stake state after execution of getRewards()
  bool slashed // true if the user has been slashed
) {
    // get rewards logic
}

Assessed type

Timing

[c4-pre-sort]

0xSorryNotSorry marked the issue as sufficient quality report

[c4-pre-sort]

0xSorryNotSorry marked the issue as primary issue

[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.

[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 based on the sponsor's comment

[c4-judge]

Trumpero marked the issue as grade-c