First-time staker can steal past rewards from SurplusGuildMinter

[c4-bot-4]

Lines of code

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

Vulnerability details

Impact

When someone stakes in the SurplusGuildMinter, or if getRewards is called by anyone, the contract claims all its rewards for all existing gauges where this has some GUILD weight, and updates the userGaugeProfitIndex accordingly. So when someone stakes/ unstakes from the SGM, all rewards for SGM are claimed from ProfitManager and the staker gets their share of these rewards.

userStake.profitIndex is initialized to the SGM's userGaugeprofitIndex for that gauge in the ProfitManager after claiming all pending rewards (so this userGaugeProfitIndex of the SGM will be the current value) ; this userStake.profitIndex is then used to give the staker their share of the rewards earned by the SGM as a whole in that gauge. But the problem is that for the first-time staker, this claiming of rewards (and hence updation of SGM's userGaugeprofitIndex for that gauge in the ProfitManager) is skipped, but the userGaugeprofitIndex is still used for initializing userStake.profitIndex in the stake function.

So now the user gets an old profitIndex even though he has staked just now, and now he can claim his share of rewards from the difference of this profitIndex from the real current userGaugeprofitIndex of SGM here , even though he staked right now.

So user is able to steal past rewards from the SGM contract, and he can do this repeatedly for all gauges and using new addresses to stake everytime he sees that the SGM has some profits pending to claim). He only has to stake 1e18 (MIN_STAKE) and then he can unstake again.

Proof of Concept

stake function calls getRewards : https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/SurplusGuildMinter.sol#L116

If user is a new staker for that gauge, getRewards skips claiming of rewards and updating of current userGaugeProfitIndex : https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/SurplusGuildMinter.sol#L236

Staker gets assigned userGaugeProfitIndex of the SGM (which is outdated) as his own userStake.profitIndex : https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/SurplusGuildMinter.sol#L142

Now he can claimRewards on SGM's behalf, claim his own share of it (which is scaled by his own guild stake), unstake. Rinse and repeat.

Tools Used

Manual review

Recommended Mitigation Steps

stake(new user) => getRewards should call ProfitManager.claimRewards without skipping even if the stake is for a new user.

Assessed type

Context

[c4-pre-sort]

0xSorryNotSorry marked the issue as sufficient quality report

[c4-pre-sort]

0xSorryNotSorry marked the issue as duplicate of #1211

[Trumpero]

This issue is not a duplicate of #1211 since it's about SurplusGuildMinter's rewards, and I believe it's invalid. When a user stakes for the first time, it's the correct behavior for the getRewards() function to skip claiming rewards and return an empty UserStake struct. The stake function will update userStake.profitIndex with the newest gauge weight of this term (code snippet). After that, if a user claims rewards again, they will receive nothing because the delta profit index will be 0 (code snippet). They need to wait for the next profit distribution to receive any rewards.

[c4-judge]

Trumpero marked the issue as not a duplicate

[c4-judge]

Trumpero marked the issue as unsatisfactory: Invalid