Slashing in SurplusGuildMinter is determined incorrectly leading to user losing his GUILD stake and also the guildReward from his staking Position

[c4-bot-5]

Lines of code

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

Vulnerability details

Impact

When a user stakes his CREDIT token into Surplus Guild Minter, his GUSDC gets donated to the termSurplusBuffer and he gets minted some GUILD tokens that are automatically involved as gauge weights in the user's chosen gauge via the SurplusGuildMinter's position in that gauge. After initial staking, the user can collect any rewards ie. guildSplit (profits distributed from Lendingterms for GUILD voters in that gauge) through getRewards, which also determines if the user has to be slashed (ie. when the term accrues a loss after the user staked his GUILD here in SGM).

The problem is that this slashing is being determined incorrectly, such that every user will be slashed by default whenever he collects rewards or even if anyone else calls getRewards on his behalf. This slashing will lead to zeroing of the user's position in SGM and also deprive him of any additional guildReward he might have earned.

This is because slashing is true if user's previously recorded lastGaugeLoss (which is recorded as the current lastGaugeLoss of that gauge at the time user stakes first) is before the current lastGaugeLoss of the term) : which is correct by design, but in code slashing does not use user's real lastGaugeLoss to compare. Instead slashing compares an uninitialized value of lastGaugeLoss in an empty userStake struct => which will always be zero and hence < gauge's current lastGaugeLoss if the gauge had accrued a loss ever. This is a problem because user should only be slashed if he staked after the gauge accrued loss last time.

See here : lastGaugeLoss is being compared with a value from an empty struct just initialized in the returns part of the function.

We can see that userStake struct has been initialized just after this as userStake = _stakes[user][term];

This incorrect comparison leads to slashing of a user who has staked after the lastGaugeLoss of that gauge, which means all his GUILD stake is gone and also any guildReward he might have deserved is gone.

Proof of Concept

guildReward is zero if slashed https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/SurplusGuildMinter.sol#L253

Staking position goes to zero if slashed : https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/SurplusGuildMinter.sol#L274

Tools Used

Manual review

Recommended Mitigation Steps

Set userStake = _stakes[user][term]; before comapring user's lastGaugeLoss. the userStake struct needs to be correctly initialized first.

Assessed type

Context

[c4-pre-sort]

0xSorryNotSorry marked the issue as sufficient quality report

[c4-pre-sort]

0xSorryNotSorry marked the issue as duplicate of #1164

[c4-judge]

Trumpero marked the issue as satisfactory