Unfair distribution of guild and credit tokens in the staking process

[c4-bot-2]

Lines of code

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

Vulnerability details

Summary

When the mintRatio (the ratio how much guild is minted for the given amount of credit tokens staked) is updated by the governance. The changes need to be applied manually to every staking position of every user, which can be done by everyone. As these changes are not automatically applied to rewards on the unstaking process, unfair distribution conditions are created.

Vulnerability Details

The governance is able to update the mintRatio the ratio how much guild is minted for the given amount of credit tokens:

function setMintRatio(uint256 _mintRatio) external onlyCoreRole(CoreRoles.GOVERNOR) {
  mintRatio = _mintRatio;
  emit MintRatioUpdate(block.timestamp, _mintRatio);
}

These changes are not automatically applied but can be manually applied by everyone and needs to be applied to every staking position of every user to distribute rewards fairly. This is done in the updateMintRatio function:

function updateMintRatio(address user, address term) external {
  // apply pending rewards
  (, UserStake memory userStake, bool slashed) = getRewards(user, term);

  // if the user has been slashed or isnt staking, there is nothing to do
  if (userStake.stakeTime == 0 || slashed) return;

  // update amount of GUILD tokens staked
  uint256 guildBefore = uint256(userStake.guild);
  uint256 guildAfter = (mintRatio * uint256(userStake.credit)) / 1e18;
  if (guildAfter > guildBefore) {
    uint256 guildAmount = guildAfter - guildBefore;
    RateLimitedMinter(rlgm).mint(address(this), guildAmount);
    GuildToken(guild).incrementGauge(term, guildAmount);
    _stakes[user][term].guild = SafeCastLib.safeCastTo128(guildAfter);
  } else if (guildAfter < guildBefore) {
    uint256 guildAmount = guildBefore - guildAfter;
    GuildToken(guild).decrementGauge(term, guildAmount);
    RateLimitedMinter(rlgm).replenishBuffer(guildAmount);
    GuildToken(guild).burn(guildAmount);
    _stakes[user][term].guild = SafeCastLib.safeCastTo128(guildAfter);
  }
}

Therefore, this can lead to unfair distributions of guild and credit tokens in the staking process. For example, if the mintRatio is updated, the user can choose to apply it to the own position if the new mintRatio is benificial for the user. Or if the new mintRatio is not benificial for the user, the user can choose to not apply it to the own position and claim rewards and unstake with the old mintRatio before anyone else applies the new mintRatio to the user's position. The user will then receive more guild and credit tokens than other stakers.

The following POC can be implemented in SurplusMinter.t.sol test file:

function testMissingMintRatioUpdate() public {
  address staker1 = address(12435);
  address staker2 = address(54321);

  // both stakers start with the same conditions:
  credit.mint(staker1, 150e18);
  credit.mint(staker2, 150e18);

  vm.startPrank(staker1);
  credit.approve(address(sgm), 150e18);
  sgm.stake(term, 150e18);
  vm.stopPrank();

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

  assertEq(sgm.getUserStake(staker1, term).credit, sgm.getUserStake(staker2, term).credit);
  assertEq(sgm.getUserStake(staker1, term).guild, sgm.getUserStake(staker2, term).guild);

  // mint ratio is halfed
  vm.startPrank(governor);
  sgm.setMintRatio(MINT_RATIO / 2);
  vm.stopPrank();

  // staker1 calls updateMintRatio on staker2
  vm.startPrank(staker1);
  sgm.updateMintRatio(staker2, term);
  vm.stopPrank();

  // staker1 now has double the guild amount than staker2 on providing the same credit
  assertEq(sgm.getUserStake(staker1, term).guild, sgm.getUserStake(staker2, term).guild * 2);

  // term earns interest
  vm.prank(governor);
  profitManager.setProfitSharingConfig(
    0.5e18, // surplusBufferSplit
    0, // creditSplit
    0.5e18, // guildSplit
    0, // otherSplit
    address(0) // otherRecipient
  );
  credit.mint(address(profitManager), 1000e18);
  profitManager.notifyPnL(term, 1000e18);
  vm.warp(block.timestamp + 13);
  vm.roll(block.number + 1);

  // both stakers earn their rewards and unstake
  vm.startPrank(staker1);
  sgm.getRewards(staker1, term);
  sgm.unstake(term, sgm.getUserStake(staker1, term).credit);
  vm.stopPrank();
  vm.startPrank(staker2);
  sgm.getRewards(staker2, term);
  sgm.unstake(term, sgm.getUserStake(staker2, term).credit);
  vm.stopPrank();

  // staker1 now owns 2x the amount of guild and 1.5x the amount of credit tokens
  // in comparison to staker2, but both staked the same amount of credit tokens
  assertEq(credit.balanceOf(staker1), (credit.balanceOf(staker2) * 3) / 2);
  assertEq(guild.balanceOf(staker1), guild.balanceOf(staker2) * 2);
}

Impact

Unfair distribution of guild and credit tokens in the staking process.

Recommendations

Implement a unified reward system by automatically applying the new mintRatio before rewards are claimed or unstaking is done.

Assessed type

Math

[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