Attacker/guild holders can drain guild rewards(steal guild rewards of others guild holder) from profitmanager contract.

[c4-bot-8]

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/governance/ProfitManager.sol#L430

Vulnerability details

Impact

when the guild token is non transferable, there will always be less reward for the guild holders and if guild tokens become transferable, there may remain 0 rewards for the guild holders.

Proof of Concept

1. Let assume total guild tokens = 416(alice has 200,bob has 50 and john has 166 guild tokens).there is only one gauge which has 250 weight(alice has given 200 weight,bob has given 50 weight,john has not given any weight to the gauge). creditSplit =50%, guildSplit = 50%,surplusBufferSplit = 0%,otherSplit = 0%.
2. Now there are 200 rewards for the gauge,50% i.e 100 goes for creditSplit and other 100 goes to alice and bob for guild voting.
3. Now see the code,
   if (_gaugeWeight != 0) {
   uint256 _gaugeProfitIndex = gaugeProfitIndex[gauge];
   if (_gaugeProfitIndex == 0) {
   _gaugeProfitIndex = 1e18;
   }
   gaugeProfitIndex[gauge] =
   _gaugeProfitIndex +
   (amountForGuild * 1e18) /
   _gaugeWeight;
   }

so gaugeProfitIndex[gauge] = 1e18+(100e18*1e18)/250e18 = 1e18+0.4e18 =1.4e18.
4. Alice calls for rewards and function claimGaugeRewards is called, see the function claimGaugeRewards code,
function claimGaugeRewards(
address user,
address gauge
) public returns (uint256 creditEarned) {
uint256 _userGaugeWeight = uint256(
GuildToken(guild).getUserGaugeWeight(user, gauge)
);
if (_userGaugeWeight == 0) {
return 0;
}
uint256 _gaugeProfitIndex = gaugeProfitIndex[gauge];
uint256 _userGaugeProfitIndex = userGaugeProfitIndex[user][gauge];
if (_gaugeProfitIndex == 0) {
_gaugeProfitIndex = 1e18;
}
if (_userGaugeProfitIndex == 0) {
_userGaugeProfitIndex = 1e18;
}
uint256 deltaIndex = _gaugeProfitIndex - _userGaugeProfitIndex;
if (deltaIndex != 0) {
creditEarned = (_userGaugeWeight * deltaIndex) / 1e18;
userGaugeProfitIndex[user][gauge] = _gaugeProfitIndex;
}
if (creditEarned != 0) {
emit ClaimRewards(block.timestamp, user, gauge, creditEarned);
CreditToken(credit).transfer(user, creditEarned);
}
}

as initially userGaugeProfitIndex[alice][gauge] = 0,so userGaugeProfitIndex[alice][gauge] sets to 1e18. gaugeProfitIndex[gauge] = 1.4e18. So deltaIndex = 1.4e18-1e18 = 0.4e18.alice’s creditearned = (200e180.4e18)/1e18 = 80e18, similarly bob’s creditearned = (50e180.4e18)/1e18 = 20e18.now userGaugeProfitIndex[alice][gauge] = 1.4e18, userGaugeProfitIndex[bob][gauge]= 1.4e18.
5. Now there is 400 rewards, 50% i.e 200 goes for creditSplit and other 200 goes to alice and bob for guild voting. gaugeProfitIndex[gauge] = 1.4e18+(200e18*1e18)/250e18 = 1e18+0.8e18 =2.2e18.
6. Alice and bob before claiming the reward , john increase(add) 166 weight to the gauge,see the code,
function _incrementGaugeWeight(
address user,
address gauge,
uint256 weight
) internal override {
uint256 _lastGaugeLoss = lastGaugeLoss[gauge];
uint256 _lastGaugeLossApplied = lastGaugeLossApplied[gauge][user];
if (getUserGaugeWeight[user][gauge] == 0) {
lastGaugeLossApplied[gauge][user] = block.timestamp;
} else {
require(
_lastGaugeLossApplied >= _lastGaugeLoss,
"GuildToken: pending loss"
);
}

    ProfitManager(profitManager).claimGaugeRewards(user, gauge);

    super._incrementGaugeWeight(user, gauge, weight);
}

function _incrementGaugeWeight is called which call ProfitManager(profitManager).claimGaugeRewards(john, gauge), see claimGaugeRewards function , as GuildToken(guild).getUserGaugeWeight(john, gauge) = 0, creditEarned return 0 and john’s ProfitIndex is not updated(this is the attack vector).now john’s weight to the gauge is 160 i.e GuildToken(guild).getUserGaugeWeight(john, gauge) = 166.
7. Alice and bob before claiming the reward, John calls the function claimGaugeRewards, gaugeProfitIndex[gauge] = 2.2e18. as initially userGaugeProfitIndex[alice][gauge] = 0,so userGaugeProfitIndex[alice][gauge] sets to 1e18.So deltaIndex = 2.2e18-1e18 = 1.2e18. john’s creditearned = (166e18*1.2e18)/1e18 = 199.2e18. Now alice and bob have no rewards to claim.
8. Alice and bob before claiming the reward,john can claim the rewards by calling two functions( function incrementGauge ,function claimGaugeRewards) in a single transaction by creating a smart contract.When guild token will be transferable , attacker/john can transfer guild token to another address and attacker can always steal rewards of other guild holders.

Tools Used

manual review

Recommended Mitigation Steps

when the function incrementGauge is called, if caller weight is 0, set userGaugeProfitIndex[caller][gauge] to gaugeProfitIndex[gauge] in function claimGaugeRewards.

Assessed type

Other

[c4-pre-sort]

0xSorryNotSorry marked the issue as insufficient quality report

[c4-pre-sort]

0xSorryNotSorry marked the issue as duplicate of #1211

[c4-judge]

Trumpero marked the issue as satisfactory