Ethereum Credit Guild - cccz's results

Lines of code

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

Vulnerability details

Impact

In SurplusGuildMinter, the user stakes credit tokens to mint guild tokens and increase gauge, and when bad debt arises, the user's stake is slashed. In getRewards(), when lastGaugeLoss > userStake.lastGaugeLoss, slashed is true, indicating that the user will be slashed.

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

The problem here is that the userStake is uninitialized when getting userStake.lastGaugeLoss, which results in slashed being true when getRewards() is called again even though the user has already been slashed, and if the user stakes credit tokens after it has been slashed, the user will be slashed again. The POC is as follows.

    function testpoc() public {
        // setup
        credit.mint(address(this), 150e18);
        credit.approve(address(sgm), 150e18);
        sgm.stake(term, 99e18);
        assertEq(sgm.getUserStake(address(this), term).credit, 99e18);
        
        // the guild token earn interests
        vm.prank(governor);
        profitManager.setProfitSharingConfig(
            0.5e18, // surplusBufferSplit
            0, // creditSplit
            0.5e18, // guildSplit
            0, // otherSplit
            address(0) // otherRecipient
        );

        // next block
        vm.warp(block.timestamp + 13);
        vm.roll(block.number + 1);
        profitManager.notifyPnL(term, -27.5e18);
        guild.applyGaugeLoss(term, address(sgm));

        // next block
        vm.warp(block.timestamp + 13);
        vm.roll(block.number + 1);
        sgm.stake(term, 9e18);
        assertEq(sgm.getUserStake(address(this), term).credit, 9e18);

        // next block
        vm.warp(block.timestamp + 13);
        vm.roll(block.number + 1);
        sgm.stake(term, 10e18);
        assertEq(sgm.getUserStake(address(this), term).credit, 10e18); //@audit: should be 19e18

    }

Proof of Concept

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

Tools Used

none

Recommended Mitigation Steps

Change to

    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);
+       userStake = _stakes[user][term];
        if (lastGaugeLoss > uint256(userStake.lastGaugeLoss)) {
            slashed = true;
        }

        // if the user is not staking, do nothing
-       userStake = _stakes[user][term];

Assessed type

Error

Lines of code

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

Vulnerability details

Impact

ProfitManager.notifyPnL is used to distribute rewards or notify gauge loss. When amount is less than 0, it will notify gauge losse and users will lose the guild tokens incremented to the gauge. And in SurplusGuildMinter, users also lose pledged credit tokens.

    function notifyPnL(
        address gauge,
        int256 amount
    ) external onlyCoreRole(CoreRoles.GAUGE_PNL_NOTIFIER) {
        uint256 _surplusBuffer = surplusBuffer;
        uint256 _termSurplusBuffer = termSurplusBuffer[gauge];
        address _credit = credit;

        // handling loss
        if (amount < 0) {
            uint256 loss = uint256(-amount);

            // save gauge loss
            GuildToken(guild).notifyGaugeLoss(gauge);

When amount is greater than 0, the reward is distributed.

        else if (amount > 0) {
            ...
            if (amountForCredit != 0) {
                CreditToken(_credit).distribute(amountForCredit);
            }

            // distribute to the guild
            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;
                }
            }

And since the protocol does not implement a locking mechanism, users can add or remove tokens at any time. This allows users to frontrun ProfitManager.notifyPnL to avoid losing or claiming rewards.

For example, since the auction is linear and bad debts are only generated after the midpoint which makes pnl negative, users can avoid losses by unstaking tokens from SurplusGuildMinter in advance of the midpoint.

In addition, when interest is incurred due to user repays or bids, the user can frontrun the transaction to increment to the gauge and decrement as soon as getting the reward.

Proof of Concept

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

Tools Used

None

Recommended Mitigation Steps

It is recommended that instead of allowing the user to make immediate exit, the user would be required to make a exit request and would be allowed to exit after a period, and then the exit would only be allowed within a limited time frame.

Assessed type

Context

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/LendingTerm.sol#L270-L281

Vulnerability details

Impact

In _decrementGaugeWeight, debtCeiling is called to get the debt ceiling after decrementation and requires the issuance to be less than it.

    function _decrementGaugeWeight(
        address user,
        address gauge,
        uint256 weight
    ) internal override {
        uint256 _lastGaugeLoss = lastGaugeLoss[gauge];
        uint256 _lastGaugeLossApplied = lastGaugeLossApplied[gauge][user];
        require(
            _lastGaugeLossApplied >= _lastGaugeLoss,
            "GuildToken: pending loss"
        );

        // update the user profit index and claim rewards
        ProfitManager(profitManager).claimGaugeRewards(user, gauge);

        // check if gauge is currently using its allocated debt ceiling.
        // To decrement gauge weight, guild holders might have to call loans if the debt ceiling is used.
        uint256 issuance = LendingTerm(gauge).issuance();
        if (issuance != 0) {
            uint256 debtCeilingAfterDecrement = LendingTerm(gauge).debtCeiling(-int256(weight));
            require(
                issuance <= debtCeilingAfterDecrement,
                "GuildToken: debt ceiling used"
            );
        }

In debtCeiling, gaugeWeight is added to gaugeWeightDelta as the latest gaugeWeight. The problem here is that totalWeight does not have gaugeWeightDelta added to it, and since the totalWeight in the real post-decrease debtCeiling has gaugeWeightDelta added to it, this results in a larger totalWeight than it really is (gaugeWeightDelta is negative), the returned debtCeiling may be smaller than the actual, the user's decrease is smaller than actual.

    function debtCeiling(
        int256 gaugeWeightDelta
    ) public view returns (uint256) {
        address _guildToken = refs.guildToken; // cached SLOAD
        uint256 gaugeWeight = GuildToken(_guildToken).getGaugeWeight(
            address(this)
        );
        gaugeWeight = uint256(int256(gaugeWeight) + gaugeWeightDelta);
        uint256 gaugeType = GuildToken(_guildToken).gaugeType(address(this));
        uint256 totalWeight = GuildToken(_guildToken).totalTypeWeight(
            gaugeType
        );

Proof of Concept

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/LendingTerm.sol#L270-L281 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/tokens/GuildToken.sol#L207-L231

Tools Used

None

Recommended Mitigation Steps

Change to

    function debtCeiling(
        int256 gaugeWeightDelta
    ) public view returns (uint256) {
        address _guildToken = refs.guildToken; // cached SLOAD
        uint256 gaugeWeight = GuildToken(_guildToken).getGaugeWeight(
            address(this)
        );
        gaugeWeight = uint256(int256(gaugeWeight) + gaugeWeightDelta);
        uint256 gaugeType = GuildToken(_guildToken).gaugeType(address(this));
        uint256 totalWeight = GuildToken(_guildToken).totalTypeWeight(
            gaugeType
        );
+       if (!GuildToken(refs.guildToken).isDeprecatedGauge(address(this))) 
+          totalWeight += uint256(int256(totalWeight) + gaugeWeightDelta)

Assessed type

Math