Ethereum Credit Guild - jasonxiale's results

Lines of code

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

Vulnerability details

Impact

SurplusGuildMinter.getRewards is used to update a user's position info(including credit, guild), but there is an error in current implementation that if GuildToken(guild).lastGaugeLoss(term) is larger than zero, all users' UserStake can be cleared, which will cause all stakers lose tokens/rewards. Since SurplusGuildMinter.getRewards can be called by anyone, a malicious user can abuse this function to clear all stakeers' UserStake

Proof of Concept

In SurplusGuildMinter.getRewards, slashed is set depends on term's lastGaugeLoss and userStake.lastGaugeLoss, the issue is userStake is a memory variable and hasn't been initialized yet, in such case userStake.lastGaugeLoss will be 0.

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)) { <<<--- Here userStake hasn't been initialized yet
230             slashed = true;
231         }
232
    ...
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({  <<<---- Here stake info will be cleared
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     }

So if lastGaugeLoss is larger than zero, slashed will be true And if slashed variable is true, userStake will be cleared

Tools Used

VIM

Recommended Mitigation Steps

diff --git a/src/loan/SurplusGuildMinter.sol b/src/loan/SurplusGuildMinter.sol
index f68e9ff..95c0f5c 100644
--- a/src/loan/SurplusGuildMinter.sol
+++ b/src/loan/SurplusGuildMinter.sol
@@ -226,12 +226,12 @@ contract SurplusGuildMinter is CoreRef {
     {
         bool updateState;
         lastGaugeLoss = GuildToken(guild).lastGaugeLoss(term);
+        userStake = _stakes[user][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);
 

Assessed type

Error