Submitted by SBSecurity, also found by grearlake, sl1, alexzoid, Giorgio, NentoR, 0xpiken, 0xanmol, btk, santipu_, TheSchnilch, asui, kaden, and ether_sky
https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/tokens/GuildToken.sol#L41
In GuildToken.sol, theres a mistake where profitManager is set in the constructor. This is problematic because different markets have different ProfitManagers, and the logic was initially designed for only one market (e.g., gUSDC). As a result, calling notifyPnL() with negative value (via forgive()or onBid() in other type of terms (e.g. gWETH)), it triggers GuildToken::notifyGaugeLoss(). However, this always results in a revert for other term types because the caller is the ProfitManager of that type, whereas GuildToken::notifyGaugeLoss() expects the one set in the constructor.
As a result, this means that loans from other markets wont be removed unless users repay them, resulting in bad debt for the protocol.
GuildToken::notifyGaugeLoss()
Note: Its using this profitManager in other parts of GuildToken, but it has nothing to do with the attack, and it doesnt cause any impact. However, we show how to fix it in the recommendation section. Because the profitManager should be removed altogether and always called dynamically based on the passed gauge.
Conditions:
Firstly, you need to add additional variables for other term type and include them in the setUp().
Modify SurplusGuildMinter.t.sol as shown:
Place the test in the same SurplusGuildMinter.t.sol and run with:
In GuildToken.sol, ProfitManager needs to be dynamically called, because there will be different ProfitManager for each market.
Since the caller of the notifyGaugeLoss() needs to be the profitManager of the passed gauge, here is the refactored logic:
You should also rework _decrementGaugeWeight() and _incrementGaugeWeight() as follows:
DoS
eswak (Ethereum Credit Guild) confirmed, but disagreed with severity and commented:
TrungOre (judge) decreased severity to Medium
