User can't unstake their position after being incorrectly marked as slashed after staking in a gauge/term that suffered a loss

[c4-bot-9]

Lines of code

https://github.com/volt-protocol/ethereum-credit-guild/blob/4d33abf95fee69391af0652e3cbe5e0cffa25f9f/src/loan/SurplusGuildMinter.sol#L229-L234 https://github.com/volt-protocol/ethereum-credit-guild/blob/4d33abf95fee69391af0652e3cbe5e0cffa25f9f/src/loan/SurplusGuildMinter.sol#L160-L166 https://github.com/volt-protocol/ethereum-credit-guild/blob/4d33abf95fee69391af0652e3cbe5e0cffa25f9f/src/loan/SurplusGuildMinter.sol#L293-L298

Vulnerability details

Impact

When user stakes a gauge/term that has suffered loss, the user’s stake is saved with lastGaugeLoss equal to the time of gauge loss. Later, if the user wants to unstake his position, he is incorrectly marked as slashed because in SurplusGuildMinter.getRewards(…) L229 userStake.lastGaugeLoss is checked, but userStake at this point is unassigned, thus always defaulting to 0 → incorrectly marked as slashed. The user will not be able to unstake his credit tokens, resulting in loss/blockage of assets. The user’s mint ratio will also be unable to be updated in SurplusGuildMinter.updateMintRatio(…), because the function also relies on the incorrect “slashed” mark from getRewards(). (SurplusGuildMinter .sol L299)

Proof of Concept

Bug location - https://github.com/volt-protocol/ethereum-credit-guild/blob/4d33abf95fee69391af0652e3cbe5e0cffa25f9f/src/loan/SurplusGuildMinter.sol#L229-L234

Compromised mint ratio for given user - https://github.com/volt-protocol/ethereum-credit-guild/blob/4d33abf95fee69391af0652e3cbe5e0cffa25f9f/src/loan/SurplusGuildMinter.sol#L293-L298

Compromised unstaking function - https://github.com/volt-protocol/ethereum-credit-guild/blob/4d33abf95fee69391af0652e3cbe5e0cffa25f9f/src/loan/SurplusGuildMinter.sol#L160-L166

UnitTest - testIncorrectlySlashedUserBlockedFromUnstaking(): https://gist.github.com/ivasilev93/ab73627570f155f774fbfaa31bc37417

diff --git a/SurplusGuildMinter.t.sol.orig b/SurplusGuildMinter.t.sol
index aefc3f8..2b28113 100644
--- a/SurplusGuildMinter.t.sol.orig
+++ b/SurplusGuildMinter.t.sol
@@ -444,5 +444,84 @@ contract SurplusGuildMinterUnitTest is Test {
         assertEq(profitManager.termSurplusBuffer(term), 150e18);
         assertEq(guild.balanceOf(address(sgm)), 600e18);
         assertEq(guild.getGaugeWeight(term), 50e18 + 600e18);
-    }    
+    }
+
+    function testIncorrectlySlashedUserBlockedFromUnstaking() public {
+        //If a user(user2 in this scenario) stakes CREDIT tokens in gauge(term)
+        // that suffered loss, then he will not be able to unstake
+        // his CREDIT tokens...
+        
+        address term1 = address(new MockLendingTerm(address(core)));
+        guild.addGauge(1, term1);
+        guild.mint(address(this), 100e18);
+        guild.incrementGauge(term1, 50e18);
+
+        address user1 = address(19028109281092);
+        address user2 = address(88120812019200);
+
+        // setup 2 users with CREDIT and each voting through the sgm for
+        // half each gauge
+        credit.mint(user1, 100e18);
+        credit.mint(user2, 200e18);
+
+        vm.startPrank(user1);
+        credit.approve(address(sgm), 100e18);
+        sgm.stake(term1, 50e18);
+        vm.stopPrank();
+
+        assertEq(profitManager.surplusBuffer(), 0);
+        assertEq(profitManager.termSurplusBuffer(term1), 50e18);
+
+        // gauge earn interest
+        vm.prank(governor);
+        profitManager.setProfitSharingConfig(
+            0.5e18, // surplusBufferSplit
+            0, // creditSplit
+            0.5e18, // guildSplit
+            0, // otherSplit
+            address(0) // otherRecipient
+        );
+        credit.mint(address(profitManager), 140e18);
+        profitManager.notifyPnL(term1, 140e18);
+
+        assertEq(profitManager.surplusBuffer(), 70e18);
+        assertEq(profitManager.termSurplusBuffer(term1), 50e18);
+
+        // next block
+        vm.warp(block.timestamp + 13);
+        vm.roll(block.number + 1);
+
+        // loss in term1 + slash sgm
+        profitManager.notifyPnL(term1, -20e18);
+        guild.applyGaugeLoss(term1, address(sgm));
+
+        // next block
+        vm.warp(block.timestamp + 13);
+        vm.roll(block.number + 1);
+
+        //user2 stakes a gauge(term1) that has suffered loss
+        vm.startPrank(user2);
+        credit.approve(address(sgm), 150e18);
+        sgm.stake(term1, 150e18);
+        vm.stopPrank();
+
+        //verify user2 has only 50 balanse of credit token left
+        assertTrue(credit.balanceOf(user2) == 50e18);
+
+        // pass some time again wihtout gauge suffering loss
+        // next block
+        vm.warp(block.timestamp + 13);
+        vm.roll(block.number + 1);
+
+        //try unstake user2, but it will fail
+        vm.startPrank(user2);
+        sgm.unstake(term1, 150e18);
+        vm.stopPrank(); 
+
+        //user2 balance should be 200, but is not
+        assertTrue(credit.balanceOf(user2) == 50e18);
+        
+        //comment above live and uncomment below line to see the test fail
+        //assertTrue(credit.balanceOf(user2) == 200e18);
+    }
 }

Tools Used

Manual review

Recommended Mitigation Steps

Move assignment of userStake and the check for stakeTime (L234-L236) right below L228 (lastGaugeLoss assignment) and above L229(the check for userStake.lastGaugeLoss)

Update getRewards() as shown: https://gist.github.com/ivasilev93/fcdc8fd56344118ea8c0d63e8492360c

diff --git a/SurplusGuildMinter.sol.orig b/SurplusGuildMinter.sol
index f68e9ff..77456ad 100644
--- a/SurplusGuildMinter.sol.orig
+++ b/SurplusGuildMinter.sol
@@ -226,14 +226,15 @@ contract SurplusGuildMinter is CoreRef {
     {
         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];
         if (userStake.stakeTime == 0)
             return (lastGaugeLoss, userStake, slashed);
+            
+        if (lastGaugeLoss > uint256(userStake.lastGaugeLoss)) {
+            slashed = true;
+        }

         // compute CREDIT rewards
         ProfitManager(profitManager).claimRewards(address(this)); // this will update profit indexes

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