Unset Variable in validation in getRewards in SurplusGuildMinter leads to incorrect slashing of users

[c4-bot-6]

Lines of code

https://github.com/volt-protocol/ethereum-credit-guild/blob/4d33abf95fee69391af0652e3cbe5e0cffa25f9f/src/loan/SurplusGuildMinter.sol#L216-L231

Vulnerability details

Impact

In SurplusGuildMinter, users can stake their CREDIT tokens and set weight on a gauge and thus receive rewards. There is the function getRewards for this:

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)) { //@audit: Variable userStake has not yet been set but is used for validation
230:            slashed = true;
231:        }
232:
233:        // if the user is not staking, do nothing
234:        userStake = _stakes[user][term]; //@audit: Variable is set

Since userStruct is not yet set during validation, userStake.lastGaugeLoss is always 0. Since the check takes place here to see whether the user has already been slashed for the latest loss, this if statement always becomes true after the gauge for which getRewards was called, once in loss because then lastGaugeLoss is always larger than userStake.lastGaugeLoss which is zero. So the user is always slashed and loses everything he staked, even if he was already slashed for the last loss.

Proof of Concept

Here is a POC which can be added in the file test/unit/loan/SurplusGuildMinter.t.sol and can be started with this command: forge test --match-test testAuditPOC2

    function testAuditPOC2() public {
        //Setup
        address user1 = address(1337);
        credit.mint(user1, 1000e18);

        //POC
        vm.startPrank(user1);
        credit.approve(address(sgm), 100e18);
        sgm.stake(address(term), 100e18); //user1 stakes in SurplusGuildMinter
        vm.stopPrank();

        profitManager.notifyPnL(term, -100e18); //loss occurs

        uint256 balanceBeforeUnstake = credit.balanceOf(user1);
        vm.startPrank(user1);
        sgm.unstake(address(term), 100e18); //user1 tries to unstake
        assert(balanceBeforeUnstake == credit.balanceOf(user1)); //user1 was slashed correctly and does not get his credit tokens back

        vm.warp(block.timestamp + 100); //a little time passes after the loss

        credit.approve(address(sgm), 900e18);
        sgm.stake(address(term), 900e18); //user1 stakes again
        sgm.unstake(address(term), 900e18); //and then unstakes again
        assert(credit.balanceOf(user1) == 0); //but since the validation doesn't work properly, user1 is incorrectly slashed again and doesn't get any tokens back
        vm.stopPrank();
    }

Tools Used

VSCode, Foundry

Recommended Mitigation Steps

userStake should be set before the validation:

+       userStake = _stakes[user][term];
        bool updateState;
        lastGaugeLoss = GuildToken(guild).lastGaugeLoss(term);
        if (lastGaugeLoss > uint256(userStake.lastGaugeLoss)) {
            slashed = true;
        }

        // if the user is not staking, do nothing
-       userStake = _stakes[user][term];

Assessed type

Invalid Validation

[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