The user guild amount is not updated if the mintRatio is updated, causing users to get more rewards in the SurplusGuildMinter contract

[c4-bot-4]

Lines of code

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

Vulnerability details

Impact

The user can stake credit tokens using the SurplusGuildMinter contract. The guildAmount to mint is calculated using the mintRatio and the credit tokens amount:

File: SurplusGuildMinter.sol
114:     function stake(address term, uint256 amount) external whenNotPaused {
...
...
132:         // self-mint GUILD tokens
133:         uint256 _mintRatio = mintRatio;
134:         uint256 guildAmount = (_mintRatio * amount) / 1e18;
135:         RateLimitedMinter(rlgm).mint(address(this), guildAmount);
136:         GuildToken(guild).incrementGauge(term, guildAmount);
...
...

Then based on the calculated guildAmount, the corresponding rewards can be obtained for the user code line 250:

File: SurplusGuildMinter.sol
216:     function getRewards(
217:         address user,
218:         address term
219:     )
220:         public
221:         returns (
222:             uint256 lastGaugeLoss, // GuildToken.lastGaugeLoss(term)
223:             UserStake memory userStake, // stake state after execution of getRewards()
224:             bool slashed // true if the user has been slashed
225:         )
226:     {
...
...
250:             uint256 creditReward = (uint256(userStake.guild) * deltaIndex) /
251:                 1e18;
252:             uint256 guildReward = (creditReward * rewardRatio) / 1e18;
253:             if (slashed) {
254:                 guildReward = 0;
255:             }
...
...

The issue arises when the mintRatio is updated, causing the user's rewards to not be calculated correctly because the guildAmount is not updated based on the new mintRatio. The SurplusGuildMinter::updateMintRatio() function helps to update the new user's guildAmount based on the new mintRatio however this function is not required to be called by the user and there are not comments/doc which mention that updateMintRatio() will be called to all users after a mintRatio change.

In the other hand there is a comment that mentions the following:

/// @dev note that any update to the `rewardRatio` (through `setRewardRatio`) will
/// change the rewards of all pending unclaimed rewards. Before a proposal to update
/// the reward ratio execute, this contract should be pinged with `getRewards` for
/// all users that have pending rewards.

That is, when rewardRatio is changed with the function SurplusGuildMinter::setRewardRatio(), it is necessary to call the rewards of all users via the SurplusGuildMinter::getRewards() function, however nothing is mentioned when mintRatio is updated.

Proof of Concept

Please consider the next scenario where the mintRatio increases from 2e18 to 3e18:

1. The userA stakes 10e18 creditTokens and 20e18 guild tokens are minted guildAmount = (_mintRatio * amount) / 1e18; = (2e18 * 10e18) / 1e18 = 20e18
2. The mintRatio is updated via setMintRatio() to mintRatio=3e18.
3. The userA stakes another 10e18 creditTokens and 30e18 guild tokens are minted, based on the formula guildAmount = (_mintRatio * amount) / 1e18; = (3e18 * 10e18) / 1e18 = 30e18. Now he has 20e18 + 30e18 = 50e18 guildTokens.

That is incorrect because userA should have 60e18 guildTokens insted of 50e18 guildTokens. Since the new mintRatio is 3e18 and the user has staked 20e18 creditTokens, the guild amount should be guildAmount = (mintRatio * amount) / 1e18; = (3e18 * 20e18) / 1e18 = 60e18. I created the following test where the correct guild amount is updated once the SurplusGuildMinter::updateMintRatio() is called the problem is that the updateMintRatio() function is not forced to be called and the user can get more rewards than it should be.

// File: test/unit/loan/SurplusGuildMinter.t.sol:SurplusGuildMinterUnitTest
// $ forge test --match-test "testUpdateMintRatioDoesNotUpdateTheGuildAmount" -vvv
//
    function testUpdateMintRatioDoesNotUpdateTheGuildAmount() public {
        credit.mint(address(this), 20e18);
        credit.approve(address(sgm), 20e18);
        //
        // 1. User stakes 10e18 creditTokens and get 20e18 guild tokens
        sgm.stake(term, 10e18);
        SurplusGuildMinter.UserStake memory stake = sgm.getUserStake(address(this), term);
        assertEq(stake.guild, 20e18); // mintRatio = 2e18
        //
        // 2. adjust to mintRatio=3e18
        vm.prank(governor);
        sgm.setMintRatio(MINT_RATIO + 1e18);
        //
        // 3. The user stakes 10e18 creditTokens and the guildAmount is 50e18 which is incorrect because
        // the user should have 60e18 guild tokens amount.
        sgm.stake(term, 10e18);
        stake = sgm.getUserStake(address(this), term);
        assertEq(stake.guild, 50e18);
        //
        // 4. If the `updateMintRatio` is called, now the guild amount is updated to the correct value (60e18)
        sgm.updateMintRatio(address(this), term);
        stake = sgm.getUserStake(address(this), term);
        assertEq(stake.guild, 60e18);
    }

Consider the next scenario where the mintRatio decreases from 3e18 to 2e18:

1. The userA stakes 10e18 creditTokens and 30e18 guild tokens are minted. guildAmount = (_mintRatio * amount) / 1e18; = (3e18 * 10e18) / 1e18 = 30e18
2. The mintRatio is updated to mintRatio=2e18.
3. The userA claims rewards via the SurplusGuildMinter::getRewards() function using a not updated guildAmount=30e18

In the above scenario, userA will get more rewards because rewards will be calculated using guildAmount=30e18, that's is incorrect because the new mintRatio is 2e18 so guildAmount = (_mintRatio * amount) / 1e18; = (2e18 * 10e18) / 1e18 = 20e18

Tools used

Manual review

Recommended Mitigation Steps

The SurplusGuildMinter::updateMintRatio() function is not enforced to be called once the mintRatio is changed, so the recommendation is to update the guildAmount if the mintRatio has changed at the end of the SurplusGuildMinter::getRewards() function, so now the user is forced to update the guildAmount and mintRatio and get fair rewards:

File: SurplusGuildMinter.sol
216:     function getRewards(
217:         address user,
218:         address term
219:     )
220:         public
221:         returns (
222:             uint256 lastGaugeLoss, // GuildToken.lastGaugeLoss(term)
223:             UserStake memory userStake, // stake state after execution of getRewards()
224:             bool slashed // true if the user has been slashed
225:         )
226:     {
227:         bool updateState;
228:         lastGaugeLoss = GuildToken(guild).lastGaugeLoss(term);
229:         if (lastGaugeLoss > uint256(userStake.lastGaugeLoss)) {
230:             slashed = true;
231:         }
232: 
233:         // if the user is not staking, do nothing
234:         userStake = _stakes[user][term];
235:         if (userStake.stakeTime == 0)
236:             return (lastGaugeLoss, userStake, slashed);
237: 
238:         // compute CREDIT rewards
239:         ProfitManager(profitManager).claimRewards(address(this)); // this will update profit indexes
240:         uint256 _profitIndex = ProfitManager(profitManager)
241:             .userGaugeProfitIndex(address(this), term);
242:         uint256 _userProfitIndex = uint256(userStake.profitIndex);
243: 
244:         if (_profitIndex == 0) _profitIndex = 1e18;
245:         if (_userProfitIndex == 0) _userProfitIndex = 1e18;
246: 
247:         uint256 deltaIndex = _profitIndex - _userProfitIndex;
248: 
249:         if (deltaIndex != 0) {
250:             uint256 creditReward = (uint256(userStake.guild) * deltaIndex) /
251:                 1e18;
252:             uint256 guildReward = (creditReward * rewardRatio) / 1e18;
253:             if (slashed) {
254:                 guildReward = 0;
255:             }
256: 
257:             // forward rewards to user
258:             if (guildReward != 0) {
259:                 RateLimitedMinter(rlgm).mint(user, guildReward);
260:                 emit GuildReward(block.timestamp, user, guildReward);
261:             }
262:             if (creditReward != 0) {
263:                 CreditToken(credit).transfer(user, creditReward);
264:             }
265: 
266:             // save the updated profitIndex
267:             userStake.profitIndex = SafeCastLib.safeCastTo160(_profitIndex);
268:             updateState = true;
269:         }
270: 
271:         // if a loss occurred while the user was staking, the GuildToken.applyGaugeLoss(address(this))
272:         // can be called by anyone to slash address(this) and decrement gauge weight etc.
273:         // The contribution to the surplus buffer is also forfeited.
274:         if (slashed) {
275:             emit Unstake(block.timestamp, term, uint256(userStake.credit));
276:             userStake = UserStake({
277:                 stakeTime: uint48(0),
278:                 lastGaugeLoss: uint48(0),
279:                 profitIndex: uint160(0),
280:                 credit: uint128(0),
281:                 guild: uint128(0)
282:             });
283:             updateState = true;
284:         }
285: 
286:         // store the updated stake, if needed
287:         if (updateState) {
288:             _stakes[user][term] = userStake;
289:         }
290:     }
...
++      if (user.mintRatio != mintRatio) updateMintRatio(user, term);
    }

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

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