search

code-423n4 / 2023-12-ethereumcreditguild-findings

9 stars 5 forks source link

User can manipulate his own userGaugeWeight to steal others' rewards from ProfitManager #1222

Closed c4-bot-3 closed 5 months ago

c4-bot-3 commented 6 months ago

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/ProfitManager.sol#L414

Vulnerability details

Impact

In ProfitManager.claimGaugeRewards function, user gets rewards proportional to the difference in gaugeProfitIndex and userGaugeProfitIndex and also proportional to the existing userGaugeWeight which is the GUILD weight user has posted in a gauge. But this userGaugeWeight is the current weight which can be manipulated in real-time very easily.

Suppose a user has contributed 1e18 GUILD to a gauge and never claimed any rewards. So his userGaugeProfitIndex will be 0.

Now he sees that much of the profit has accumulated => gaugeProfitIndex = 10e18. Now he increases his userGaugeWeight for this particular gauge using incrementGauge. This increases the userGaugeWeight without claiming the pending rewards for the already existing userGaugeWeight.

This way now his userGaugeWeight = 100e18. Now when he calls ProfitManager.claimGaugeRewards, he gets creditEarned = 100e18 * (gaugeProfitIndex - userGaugeProfitIndex) = 900e18. But he only deserved 9e18 credit rewards based on his previous userGaugeWeight that was posted for that much time duration. This amount is huge and might even be greater than the actual credit rewards earned by the whole gauge as a share of the guildSplit => which means credit balance accrued in ProfitManager from other gauges' profits as well as the surplusBuffer and termSurplusBuffers may get used up in this rewards transfer.

The user can do this repeatedly for all gauges whenever he wants. High risk because it steals other users' rewards for all gauges + also depletes the insurance fund(surplusBuffer) which was meant to cover bad debt, thus causing more harm to lenders.

Proof of Concept

ERC20Gauges.incrementGauge function does not accrue pending rewards so userGaugeWeight can be freely manipulated : https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/tokens/ERC20Gauges.sol#L219

credit rewards for user are calculated using the current userGaugeWeight which has been manipulated. https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/ProfitManager.sol#L414

Tools Used

Manual review

Recommended Mitigation Steps

Add a call to claimGaugeRewards in the incrementGauge function so that pending rewards are claimed first and userGaugeProfitIndex is updated before adding new weight to the userGaugeWeight, just like in SurplusGuildMinter.

Assessed type

Context

c4-pre-sort commented 6 months ago

0xSorryNotSorry marked the issue as sufficient quality report

c4-pre-sort commented 6 months ago

0xSorryNotSorry marked the issue as duplicate of #1211

c4-judge commented 5 months ago

Trumpero marked the issue as not a duplicate

Trumpero commented 5 months ago

It's not a duplicate of #1194 or #1211, and it's an invalid issue because the _incrementGaugeWeight function of the GuildToken contract already claims the existing rewards before updating the weight. https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/tokens/GuildToken.sol#L258

c4-judge commented 5 months ago

Trumpero marked the issue as unsatisfactory: Invalid