c4-bot-2 commented 7 months ago

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/SurplusGuildMinter.sol#L114 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/SurplusGuildMinter.sol#L216 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/ProfitManager.sol#L392 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/ProfitManager.sol#L409

Vulnerability details

Impact

Detailed description of the impact of this finding. If a credit token holder wants to stake the credit tokens so as to earn rewards in terms of credit tokens and Guild tokens,the user transfers the credit tokens to the SurplusGuildMinter contract and the guild tokens are minted to the SuprlusGuildMinter contract which on the behalf of the credit token holder increases the gauge weight of that gauge which user inputs. Now if the user is slashed then its stakes are reduced to zero whereas the gauge weight which the SurplusGuildMinter contract increased is not decreased immediately.Now a user can only be slashed if a gauge is depreciated i.e it is off boarded and it has suffered a loss.Now a depreciated can again be added using on boarding mechanism,so now again new users can stake the tokens and can earn rewards if that gauge earns profits.Issue is if the gauge weight contributed by the surplus minter is not decreased immediately after a user gets slashed it can cause the gauge weight to remain same instead it should decrease because some users were slashed so their wights should be decreased from the gauge weight, if it is not done than it can cause the rewards to the current stakers to be reduced than the actual value.And if the weights increased by the suprlusGuild minter contract are reduced after the gauge has been onbarded again it can cause the weights to reduce by more than the required value and cause more rewards been allocated to the users.

Proof of Concept

Lets start step by step 1.User calls the stake function

function stake(address term, uint256 amount) external whenNotPaused {
        // apply pending rewards
        (uint256 lastGaugeLoss, UserStake memory userStake, ) = getRewards(
            msg.sender,
            term
        );

        require(
            lastGaugeLoss != block.timestamp,
            "SurplusGuildMinter: loss in block"
        );
        require(amount >= MIN_STAKE, "SurplusGuildMinter: min stake");

        // pull CREDIT from user & transfer it to surplus buffer
        CreditToken(credit).transferFrom(msg.sender, address(this), amount);
        CreditToken(credit).approve(address(profitManager), amount);
        ProfitManager(profitManager).donateToTermSurplusBuffer(term, amount);

        // self-mint GUILD tokens
        uint256 _mintRatio = mintRatio;
        uint256 guildAmount = (_mintRatio * amount) / 1e18;
        RateLimitedMinter(rlgm).mint(address(this), guildAmount);
        GuildToken(guild).incrementGauge(term, guildAmount);

        // update state
        userStake = UserStake({
            stakeTime: SafeCastLib.safeCastTo48(block.timestamp),
            lastGaugeLoss: SafeCastLib.safeCastTo48(lastGaugeLoss),
            profitIndex: SafeCastLib.safeCastTo160(
                ProfitManager(profitManager).userGaugeProfitIndex(
                    address(this),
                    term
                )
            ),
            credit: userStake.credit + SafeCastLib.safeCastTo128(amount),
            guild: userStake.guild + SafeCastLib.safeCastTo128(guildAmount)
        });
        _stakes[msg.sender][term] = userStake;

        // emit event
        emit Stake(block.timestamp, term, amount);
    }

get rewards function is also used

 function getRewards(
        address user,
        address term
    )
        public
        returns (
            uint256 lastGaugeLoss, // GuildToken.lastGaugeLoss(term)
            UserStake memory userStake, // stake state after execution of getRewards()
            bool slashed // true if the user has been slashed
        )
    {
        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);

        // compute CREDIT rewards
        ProfitManager(profitManager).claimRewards(address(this)); // this will update profit indexes
        uint256 _profitIndex = ProfitManager(profitManager)
            .userGaugeProfitIndex(address(this), term);
        uint256 _userProfitIndex = uint256(userStake.profitIndex);

        if (_profitIndex == 0) _profitIndex = 1e18;
        if (_userProfitIndex == 0) _userProfitIndex = 1e18;

        uint256 deltaIndex = _profitIndex - _userProfitIndex;

        if (deltaIndex != 0) {
            uint256 creditReward = (uint256(userStake.guild) * deltaIndex) /
                1e18;
            uint256 guildReward = (creditReward * rewardRatio) / 1e18;
            if (slashed) {
                guildReward = 0;
            }

            // forward rewards to user
            if (guildReward != 0) {
                RateLimitedMinter(rlgm).mint(user, guildReward);
                emit GuildReward(block.timestamp, user, guildReward);
            }
            if (creditReward != 0) {
                CreditToken(credit).transfer(user, creditReward);
            }

            // save the updated profitIndex
            userStake.profitIndex = SafeCastLib.safeCastTo160(_profitIndex);
            updateState = true;
        }

        // if a loss occurred while the user was staking, the GuildToken.applyGaugeLoss(address(this))
        // can be called by anyone to slash address(this) and decrement gauge weight etc.
        // The contribution to the surplus buffer is also forfeited.
        if (slashed) {
            emit Unstake(block.timestamp, term, uint256(userStake.credit));
            userStake = UserStake({
                stakeTime: uint48(0),
                lastGaugeLoss: uint48(0),
                profitIndex: uint160(0),
                credit: uint128(0),
                guild: uint128(0)
            });
            updateState = true;
        }

        // store the updated stake, if needed
        if (updateState) {
            _stakes[user][term] = userStake;
        }
    }

If the user has not staked then get rewards returns the following

if (userStake.stakeTime == 0)
        return (lastGaugeLoss, userStake, slashed);

Below you can see the credit tokens are transferred to the SurplusGuildMinter (sgm) contract.

CreditToken(credit).transferFrom(msg.sender, address(this), amount);
    CreditToken(credit).approve(address(profitManager), amount);
    ProfitManager(profitManager).donateToTermSurplusBuffer(term, amount);

NOw guild tokens which should be minted are calculated as follows.Notice the guild tokens are not minted to the user who mints instead there are minted to the sgm contract itself then gauge weight is increased .

// self-mint GUILD tokens
    uint256 _mintRatio = mintRatio;
    uint256 guildAmount = (_mintRatio * amount) / 1e18;
    RateLimitedMinter(rlgm).mint(address(this), guildAmount);
    GuildToken(guild).incrementGauge(term, guildAmount);

IF you see the increment Gauge function it clear _incrementGaugeWeight is called with msg.sender as the sgm contract and weight equal to the guild amount minted

function incrementGauge(
    address gauge,
    uint256 weight
) public virtual returns (uint256 newUserWeight) {
    require(isGauge(gauge), "ERC20Gauges: invalid gauge");
    _incrementGaugeWeight(msg.sender, gauge, weight);
    return _incrementUserAndGlobalWeights(msg.sender, weight);
}

In the below function it is key to note getGaugeWeight is increased by weight(which is equal to the guildAmount minted) and getUserGaugeWeight[user][gauge] is also increased where user is sgm contract.

function _incrementGaugeWeight(
    address user,
    address gauge,
    uint256 weight
) internal virtual {
    bool added = _userGauges[user].add(gauge); // idempotent add
    if (added && _userGauges[user].length() > maxGauges) {
        require(canExceedMaxGauges[user], "ERC20Gauges: exceed max gauges");
    }

    getUserGaugeWeight[user][gauge] += weight;

    getGaugeWeight[gauge] += weight;

    totalTypeWeight[gaugeType[gauge]] += weight;

    emit IncrementGaugeWeight(user, gauge, weight);
}

Now going back to our stake function userstake is now stored in the storage with msg.sender being the original usr who called the staked function you can see guild: userStake.guild + SafeCastLib.safeCastTo128(guildAmount) guildAmount = weight by which the gaugeweight increased and it also denotes how much of the totalVotes in the gauge a user has voted.

 // update state
        userStake = UserStake({
            stakeTime: SafeCastLib.safeCastTo48(block.timestamp),
            lastGaugeLoss: SafeCastLib.safeCastTo48(lastGaugeLoss),
            profitIndex: SafeCastLib.safeCastTo160(
                ProfitManager(profitManager).userGaugeProfitIndex(
                    address(this),
                    term
                )
            ),
            credit: userStake.credit + SafeCastLib.safeCastTo128(amount),
            guild: userStake.guild + SafeCastLib.safeCastTo128(guildAmount)
        });
        _stakes[msg.sender][term] = userStake;

Now we will look how rewards are calculated

ProfitManager(profitManager).claimRewards(address(this)); // this will update profit indexes
        uint256 _profitIndex = ProfitManager(profitManager)
            .userGaugeProfitIndex(address(this), term);
        uint256 _userProfitIndex = uint256(userStake.profitIndex);

        if (_profitIndex == 0) _profitIndex = 1e18;
        if (_userProfitIndex == 0) _userProfitIndex = 1e18;

        uint256 deltaIndex = _profitIndex - _userProfitIndex;

        if (deltaIndex != 0) {
            uint256 creditReward = (uint256(userStake.guild) * deltaIndex) /
                1e18;
            uint256 guildReward = (creditReward * rewardRatio) / 1e18;
            if (slashed) {
                guildReward = 0;
            }

firstly the following is called ProfitManager(profitManager).claimRewards(address(this));

claim rewards function is as follows

  function claimRewards(
        address user
    ) external returns (uint256 creditEarned) {
        address[] memory gauges = GuildToken(guild).userGauges(user);
        for (uint256 i = 0; i < gauges.length; ) {
            creditEarned += claimGaugeRewards(user, gauges[i]);
            unchecked {
                ++i;
            }
        }
    }

from the above you can see it claims the credit earned from all the gauges where the sgm contract has voted i.e it has increased the weight.

claimGaugerewards function is as follows

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);
        }
    }

rewards are caluclated mainly using the following

 uint256 deltaIndex = _gaugeProfitIndex - _userGaugeProfitIndex;
        if (deltaIndex != 0) {
            creditEarned = (_userGaugeWeight * deltaIndex) / 1e18;
            userGaugeProfitIndex[user][gauge] = _gaugeProfitIndex;
        }

_gaugeProfitIndex = gaugeProfitIndex[gauge] is calculated as follows it happens when a profit occurs in a particular gauge

  if (amountForGuild != 0) {
                // update the gauge profit index
                // if the gauge has 0 weight, does not update the profit index, this is unnecessary
                // because the profit index is used to reattribute profit to users voting for the gauge,
                // and if the weigth is 0, there are no users voting for the gauge.
                uint256 _gaugeWeight = uint256(
                    GuildToken(guild).getGaugeWeight(gauge)
                );
                if (_gaugeWeight != 0) {
                    uint256 _gaugeProfitIndex = gaugeProfitIndex[gauge];
                    if (_gaugeProfitIndex == 0) {
                        _gaugeProfitIndex = 1e18;
                    }
                    gaugeProfitIndex[gauge] =
                        _gaugeProfitIndex +
                        (amountForGuild * 1e18) /
                        _gaugeWeight;
                }
            }

you can see from above that the gaugeProfitIndex uses _gaugeWeight = getGaugeWeight(gauge)

Now creditEarned = (_userGaugeWeight * deltaIndex) / 1e18 here _userGaugeWeight = uint256( GuildToken(guild).getUserGaugeWeight(user, gauge) i.e the weight contributed by the sgm contract in our context

if simplified creditEarned = (_userGaugeWeight ((amountForGuild 1e18) / _gaugeWeight;)) / 1e18 i.e percentage of reward for the sgm contract

Now coming back to how rewards are distributed to the stakers.

uint256 _profitIndex = ProfitManager(profitManager)
            .userGaugeProfitIndex(address(this), term);
        uint256 _userProfitIndex = uint256(userStake.profitIndex);

        if (_profitIndex == 0) _profitIndex = 1e18;
        if (_userProfitIndex == 0) _userProfitIndex = 1e18;

        uint256 deltaIndex = _profitIndex - _userProfitIndex;

        if (deltaIndex != 0) {
            uint256 creditReward = (uint256(userStake.guild) * deltaIndex) /
                1e18;
            uint256 guildReward = (creditReward * rewardRatio) / 1e18;
            if (slashed) {
                guildReward = 0;
            }

_profitIndex = ProfitManager(profitManager) .userGaugeProfitIndex(address(this), term); and userGaugeProfitIndex[user][gauge] = _gaugeProfitIndex here user is sgm contract and _gaugeProfitIndex = gaugeProfitIndex[gauge] and gaugeProfitIndex[gauge] is influenced by gaugeweight as follows (amountForGuild * 1e18) / _gaugeWeight;

now uint256 creditReward = (uint256(userStake.guild) deltaIndex) / 1e18; you can see here userStake.guild represent the weights or user which were contributed by the sgm contract on its behalf so credit rewards in other ways is calculated by how much of the total weights og gauge are contributed by this user. uint256 deltaIndex = _profitIndex - _userProfitIndex; = userGaugeProfitIndex[user][gauge]- _userProfitIndex = _gaugeProfitIndex - _userProfitIndex = (amountForGuild 1e18) / _gaugeWeight (equivalent)

so creditReward = (uint256(userStake.guild) ((amountForGuild 1e18) / _gaugeWeight)) / 1e18;

Now if a user gets slashed following is done in the getRewards function

 if (slashed) {
            emit Unstake(block.timestamp, term, uint256(userStake.credit));
            userStake = UserStake({
                stakeTime: uint48(0),
                lastGaugeLoss: uint48(0),
                profitIndex: uint160(0),
                credit: uint128(0),
                guild: uint128(0)
            });
            updateState = true;
        }

The above updates the userStake .It can be seen that guild is reduced to zero for the user but there is no decrement of weights of that particular gauge , where sgm increased the weights on the behalf of user.

/// @notice apply a loss that occurred in a given gauge
    /// anyone can apply the loss on behalf of anyone else
    function applyGaugeLoss(address gauge, address who) external {
        // check preconditions
        uint256 _lastGaugeLoss = lastGaugeLoss[gauge];
        uint256 _lastGaugeLossApplied = lastGaugeLossApplied[gauge][who];
        require(
            _lastGaugeLoss != 0 && _lastGaugeLossApplied < _lastGaugeLoss,
            "GuildToken: no loss to apply"
        );

        // read user weight allocated to the lossy gauge
        uint256 _userGaugeWeight = getUserGaugeWeight[who][gauge];

        // remove gauge weight allocation
        lastGaugeLossApplied[gauge][who] = block.timestamp;
        _decrementGaugeWeight(who, gauge, _userGaugeWeight);
        if (!_deprecatedGauges.contains(gauge)) {
            totalTypeWeight[gaugeType[gauge]] -= _userGaugeWeight;
            totalWeight -= _userGaugeWeight;
        }

        // apply loss
        _burn(who, uint256(_userGaugeWeight));
        emit GaugeLossApply(
            gauge,
            who,
            uint256(_userGaugeWeight),
            block.timestamp
        );
    }

Above is a applyGaugeLoss function for decerasing the weights of the users if a loss has occurred.The following function can be called by anyone.The following function should be called with the who = sgm(contract) whenever someone gets slashed so that the weight of the gauge is removed correctly, because if no one calls the following function the gaugeWeight remains same for the depreciated gauge and once the the gauge gets onboarded and if new users stake and increase the weight of the gauge then they would get less rewards because the weights of the previous users which sgm used on their behalf to increase the weights of the gauge wouldn't be reduced and gaugeProfitIndex[gauge] which is calculated as follows gaugeProfitIndex[gauge] = _gaugeProfitIndex +(amountForGuild * 1e18)/_gaugeWeight; will be less because _gaugeWeight wouldn't be reduced this impacts the deltaIndex which is used while calculating the rewards for the users who staked as shown above when we calculated user rewards.All in all it reduces the rewards for users who staked.

I am labelling it as high because it would impact the rewards for the users who staked.

Tools Used

Manual Review

Recommended Mitigation Steps

Add the following in getRewards when a user is slashed

if (slashed) {
            emit Unstake(block.timestamp, term, uint256(userStake.credit));
            userStake = UserStake({
                stakeTime: uint48(0),
                lastGaugeLoss: uint48(0),
                profitIndex: uint160(0),
                credit: uint128(0),
                guild: uint128(0)
            });
            updateState = true;
        }
GuildToken(guild).applyGaugeLoss(term, address(this));

If applyGaugeLoss is applied on the sgm it gurantees the first rewards are claimed and then the weight is reduced,you can see that in teh applyGaugeLoss function.

Assessed type

Context

c4-pre-sort commented 7 months ago

0xSorryNotSorry marked the issue as sufficient quality report

c4-pre-sort commented 7 months ago

0xSorryNotSorry marked the issue as duplicate of #1211

c4-judge commented 6 months ago

Trumpero marked the issue as not a duplicate

Trumpero commented 6 months ago

Above is a applyGaugeLoss function for decerasing the weights of the users if a loss has occurred.The following function can be called by anyone.The following function should be called with the who = sgm(contract) whenever someone gets slashed so that the weight of the gauge is removed correctly, because if no one calls the following function the gaugeWeight remains same for the depreciated gauge and once the the gauge gets onboarded and if new users stake and increase the weight of the gauge then they would get less rewards because the weights of the previous users which sgm used on their behalf to increase the weights of the gauge wouldn't be reduced and gaugeProfitIndex[gauge] which is calculated as follows

I believe this issue is not a duplicate of #1194, and it should be invalid. The above statement is incorrect because the stake function will call GuildToken.incrementGauge() for the SurplusGuildMinter contract, triggering the GuildToken._incrementGaugeWeight() function. This function will revert if the address has an unapplied loss. Therefore, SurplusGuildMinter is unable to stake as long as it hasn't been applied loss.