Ethereum Credit Guild - TheSchnilch's results

Lines of code

https://github.com/volt-protocol/ethereum-credit-guild/blob/4d33abf95fee69391af0652e3cbe5e0cffa25f9f/src/governance/ProfitManager.sol#L416-L427

Vulnerability details

Description

If user weight sets on gauges then they can get a part of the profit if the gauge makes profit. The profits are distributed via the ProfitManager with notifyPnL. Each gauge has a profit index that is stored in the mapping gaugeProfitIndex and increased in the event of a profit. Each user also has a Profit Index. Whenever a user collects his profit, the profit index is set to that of the gauge. Both profit indexes begin at 0. The problem is that when a user sets weight to a gauge for the first time, his profit index remains at 0. This leads to a problem if the gauge the user is settting weight on has already made a profit and therefore the profit index of the gauge is already higher. Since the amount of profit a user gets is determined with _gaugeProfitIndex - _userGaugeProfitIndex, the user would get more profit than he should actually get: File: src/governance/ProfitManager.sol

427:        uint256 deltaIndex = _gaugeProfitIndex - _userGaugeProfitIndex;
428:        if (deltaIndex != 0) {
429:            creditEarned = (_userGaugeWeight * deltaIndex) / 1e18;

In this way, all profits that existed before the user even put weight on the gauge are calculated for the user.

Impact

User gets part of the profits that were already distributed before he put weight on the gauge. Since these rewards have normally already been distributed to all users who should correctly receive the profits, the new user accidentally gets a part of the surplus buffer without the ProfitManager accounting for it. So it may be that some losses can no longer be modified because there is no longer enough surplus buffer.

Proof of Concept

Here is a POC:

    function testPOC1() public {
        //Setup
        address user1 = address(1337);
        address user2 = address(1338);
        uint256 guildBalanceUser1 = 20e18;
        uint256 guildBalanceUser2 = 20e18;
        uint256 profit = 10e18;
        int256 loss = -10e18;

        vm.startPrank(governor);
        core.grantRole(CoreRoles.GAUGE_PNL_NOTIFIER, address(this));
        core.grantRole(CoreRoles.CREDIT_MINTER, address(this));
        core.grantRole(CoreRoles.GUILD_MINTER, address(this));
        core.grantRole(CoreRoles.GAUGE_ADD, address(this));
        core.grantRole(CoreRoles.GAUGE_PARAMETERS, address(this));
        core.grantRole(CoreRoles.GUILD_GOVERNANCE_PARAMETERS, address(this));
        profitManager.setProfitSharingConfig(
            0.05e18, // surplusBufferSplit
            0.8e18, // creditSplit
            0.05e18, // guildSplit
            0.1e18, // otherSplit
            address(this) // otherRecipient
        );
        vm.stopPrank();
        guild.mint(user1, guildBalanceUser1);
        guild.mint(user2, guildBalanceUser2);
        guild.setMaxDelegates(4);
        guild.setMaxGauges(4);
        guild.addGauge(1, address(gauge1));

        //POC
        vm.prank(user1);
        guild.incrementGauge(address(gauge1), guildBalanceUser1); //user sets his balance for gauge1

        uint256 surplusBufferSplit = (profit * 0.05e18) / 1e18;
        uint256 guildSplit = (profit * 0.05e18) / 1e18;
        credit.transfer(address(profitManager), profit);
        profitManager.notifyPnL(gauge1, int256(profit)); //profit is distributed for the users who have weight on gauge1
        
        uint256 creditBalancePM = credit.balanceOf(address(profitManager));
        assert(creditBalancePM == surplusBufferSplit + guildSplit); //this shows that the profit manager has the right share of credit tokens after profit

        vm.prank(user1);
        profitManager.claimGaugeRewards(user1, address(gauge1)); //user1 who has weight on gauge1 gets his rewards

        creditBalancePM = credit.balanceOf(address(profitManager));
        assert(creditBalancePM == surplusBufferSplit); //since there is only one user, the entire guild split goes to user1 and the rest remains as a surplus buffer

        vm.startPrank(user2);
        guild.incrementGauge(address(gauge1), guildBalanceUser2); //user2 now also sets weight to gauge1
        profitManager.claimGaugeRewards(user2, address(gauge1)); //user2 gets rewards even though he only sets weight to gauge1 after profit has been notified. 
        //However, since everything has actually already been distributed to the rightful beneficiaries, User2 incorrectly receives what should be available as 
        //surplus buffer.
        vm.stopPrank();

        creditBalancePM = credit.balanceOf(address(profitManager));
        assert(creditBalancePM == 0); //this shows that the profit manager no longer has a surplus buffer
        
        vm.expectRevert("ERC20: burn amount exceeds balance"); //no loss can be notified anymore because it would revert because the surplus buffer is at user2
        profitManager.notifyPnL(gauge1, loss);
    }

This test can be added to the file test/unit/governance/ProfitManager.t.sol and started with this command: forge test --match-test testPOC1

Tools Used

VSCode, Foundry

Recommended Mitigation Steps

When a user sets weight to a gauge for the first time, their profit index should be set to that of the gauge so as not to mistakenly get any of the previous profits. File: src/governance/ProfitManager.sol

    function claimGaugeRewards(
        address user,
        address gauge
    ) public returns (uint256 creditEarned) {
        uint256 _userGaugeWeight = uint256(
            GuildToken(guild).getUserGaugeWeight(user, gauge)
        );
        if (_userGaugeWeight == 0) {
+           userGaugeProfitIndex[user][gauge] = gaugeProfitIndex[gauge];
            return 0;
        }

Assessed type

Other

Lines of code

https://github.com/volt-protocol/ethereum-credit-guild/blob/4d33abf95fee69391af0652e3cbe5e0cffa25f9f/src/loan/SurplusGuildMinter.sol#L216-L231

Vulnerability details

Impact

In SurplusGuildMinter, users can stake their CREDIT tokens and set weight on a gauge and thus receive rewards. There is the function getRewards for this:

216:    function getRewards(
217:        address user,
218:        address term
219:    )
220:        public
221:        returns (
222:            uint256 lastGaugeLoss, // GuildToken.lastGaugeLoss(term)
223:            UserStake memory userStake, // stake state after execution of getRewards()
224:            bool slashed // true if the user has been slashed
225:        )
226:    {
227:        bool updateState;
228:        lastGaugeLoss = GuildToken(guild).lastGaugeLoss(term);
229:        if (lastGaugeLoss > uint256(userStake.lastGaugeLoss)) { //@audit: Variable userStake has not yet been set but is used for validation
230:            slashed = true;
231:        }
232:
233:        // if the user is not staking, do nothing
234:        userStake = _stakes[user][term]; //@audit: Variable is set

Since userStruct is not yet set during validation, userStake.lastGaugeLoss is always 0. Since the check takes place here to see whether the user has already been slashed for the latest loss, this if statement always becomes true after the gauge for which getRewards was called, once in loss because then lastGaugeLoss is always larger than userStake.lastGaugeLoss which is zero. So the user is always slashed and loses everything he staked, even if he was already slashed for the last loss.

Proof of Concept

Here is a POC which can be added in the file test/unit/loan/SurplusGuildMinter.t.sol and can be started with this command: forge test --match-test testAuditPOC2

    function testAuditPOC2() public {
        //Setup
        address user1 = address(1337);
        credit.mint(user1, 1000e18);

        //POC
        vm.startPrank(user1);
        credit.approve(address(sgm), 100e18);
        sgm.stake(address(term), 100e18); //user1 stakes in SurplusGuildMinter
        vm.stopPrank();

        profitManager.notifyPnL(term, -100e18); //loss occurs

        uint256 balanceBeforeUnstake = credit.balanceOf(user1);
        vm.startPrank(user1);
        sgm.unstake(address(term), 100e18); //user1 tries to unstake
        assert(balanceBeforeUnstake == credit.balanceOf(user1)); //user1 was slashed correctly and does not get his credit tokens back

        vm.warp(block.timestamp + 100); //a little time passes after the loss

        credit.approve(address(sgm), 900e18);
        sgm.stake(address(term), 900e18); //user1 stakes again
        sgm.unstake(address(term), 900e18); //and then unstakes again
        assert(credit.balanceOf(user1) == 0); //but since the validation doesn't work properly, user1 is incorrectly slashed again and doesn't get any tokens back
        vm.stopPrank();
    }

Tools Used

VSCode, Foundry

Recommended Mitigation Steps

userStake should be set before the validation:

+       userStake = _stakes[user][term];
        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];

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/ProfitManager.sol#L172-L176 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/SimplePSM.sol#L97-L99 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/SimplePSM.sol#L80-L84

Vulnerability details

Impact

The function totalBorrowedCredit in the ProfitManager calculates how much CREDIT is borrowed by deducting from the totalSupply the tokens that were not borrowed and were minted in PSM. The functions redeemableCredit and getMintAmountOut are used to calculate how many tokens were minted in PSM. However, it is assumed that for all underlying tokens in the PSM CREDIT were minted, but the credit that was minted could also have been burned (e.g. as a surplus buffer), which was not taken into account. This results in the amount returned by redeemableCredit being larger than it actually should be. It could lead to redeemableCredit being greater than totalSupply. This results in totalBorrowedCredit being reverted due to an underflow as you can see in the function. The function debtCeiling can no longer be used in any LendingTerm, as it also uses totalBorrowedCredit. This in turn leads to the _decrementGaugeWeight being reverted. This means you can no longer decrement weight on the gauge and applyGaugeLoss no longer works. Borrow also no longer works on any LendingTerm because totalBorrowedCredit is also used there. But even if this case does not occur and totalBorrowedCredit is simply more than it should be, debtCeiling will be more than it should be, which allows you to decrement more weight than you should actually be able to.

Proof of Concept

Here is a POC that can be inserted into the file test/unit/loan/LendingTerm.t.sol and can be started with the following command: forge test --match-test testPOC6

function testPOC6() public {
        //Setup
        address user1 = address(1337);
        address user2 = address(1338);

        collateral.mint(user1, 10_000e18);
        collateral.mint(user2, 10_000e18);

        //POC
        vm.startPrank(user1);
        collateral.approve(address(term), 1000e18);
        term.borrow(1000e18, 1000e18); //user1 borrows 1000e18 so the totalBorrowedCredit becomes 1000e18
        vm.stopPrank();

        vm.startPrank(user2);
        collateral.approve(address(psm), 3000e18);
        psm.mint(user2, 3000e18);   //user2 mints CREDIT tokens

        credit.approve(address(profitManager), 3000e18);
        profitManager.donateToTermSurplusBuffer(address(term), 3000e18); //The minted tokens are donated as surplus buffer. Normally a user 
        //would do this through SGM but for this POC it has the same effect
        vm.stopPrank();

        assert(profitManager.totalBorrowedCredit() == 1000e18); //Shows that totalBorrowedCredit is 1000e18 even after minting in PSM

        profitManager.notifyPnL(address(term), -1100e18); //Large loss is notified, for example, because of forgiving a loan
        vm.expectRevert();  //This catches the underflow error caused by the miscalculation of totalBorrowedCredit
        profitManager.totalBorrowedCredit();

        vm.warp(block.timestamp + 100);

        vm.expectRevert(); //This also catches the underflow error because of revert in totalBorrowedCredit
        guild.decrementGauge(address(term), 100e18);
    }

These 3 lines should be added to the setup function in the role assignments so that the POC works correctly:

core.grantRole(CoreRoles.GAUGE_PNL_NOTIFIER, address(this));
core.grantRole(CoreRoles.CREDIT_MINTER, address(psm));
core.grantRole(CoreRoles.CREDIT_REBASE_PARAMETERS, address(psm));

Tools Used

VSCode, Foundry

Recommendation

There should be a variable in the ProfitManager that can be increased by the LendingTerms when CREDIT is borrowed and decreased when CREDIT is repaid. This variable should then replace totalBorrowedCredit

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/tokens/GuildToken.sol#L40-L41

Vulnerability details

Impact

The docs say that there should be several markets, but they should all have the same GUILD token:

In the future, it is expected that multiple markets will be deployed, each with their own credit token, but the GUILD token is the same for all markets.

Each market has its own profit manager, the problem is that the Guild token can only store one profit manager, as can be seen in the Github link. This means that only one market can exist for the Guild token, which is a severe limitation since the plan was to deploy several.

Tools Used

VSCode

Recommended Mitigation Steps

The Guild token should have a mapping with profit managers that could look like this: mapping(uint256 type => address profitManager) profitManagers; There would then have to be a function that allows the governance to add new profit managers and remove them again when they are no longer needed. All places in the code in GuildToken.sol where profitManager is used must then be repaired so that the correct profitManager is used.

Assessed type

Other

Judge has assessed an item in Issue #496 as 2 risk. The relevant finding follows:

debtCeiling() does not determine the correct minimum which results in debtCeiling being higher than it should be

Description

At the end of the function debtCeiling in LendingTerm.sol the minimum of creditMinterBuffer, _debtCeiling and _hardCap will be determined like this:

        // return min(creditMinterBuffer, hardCap, debtCeiling)
        if (creditMinterBuffer < _debtCeiling) {
            return creditMinterBuffer;
        }
        if (_hardCap < _debtCeiling) {
            return _hardCap;
        }
        return _debtCeiling;

The problem here is that if creditMinterBuffer < _debtCeiling, then it is not checked whether _hardCap is smaller than creditMinterBuffer. So it could be, although very unlikely, that the wrong debtCeiling will be returned. This would result in the wrong debtCeiling being used in the _decrementGaugeWeight function in GuildToken.sol, making it possible to decrement more tokens than you should be able to:

if (issuance != 0) {
            uint256 debtCeilingAfterDecrement = LendingTerm(gauge).debtCeiling(-int256(weight));
            require(
                issuance <= debtCeilingAfterDecrement,
                "GuildToken: debt ceiling used"
            );
        }

Recommendation

It should also be checked whether _hardCap is smaller than creditMinterBuffer. The minimum should then be determined in this way:

// return min(creditMinterBuffer, hardCap, debtCeiling)
if (creditMinterBuffer < _debtCeiling) {
+		if(_hardCap < creditMinterBuffer) return _hardCap;
		return creditMinterBuffer;
}
if (_hardCap < _debtCeiling) {
    return _hardCap;
}
return _debtCeiling;

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/LendingTerm.sol#L277-L281 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/tokens/ERC20Gauges.sol#L230-L247 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/tokens/GuildToken.sol#L226

Vulnerability details

Description

The function debtCeiling is there to calculate the maximum amount of CREDIT that can be borrowed. You can also set the gaugeWeightDelta parameter so that you can see how the debt ceiling changes when you change the weight of the term. To do this, gaugeWeightDelta is simply subtracted from/added to the current weight of the term:

//File: src/loan/LendingTerm.sol
273:        address _guildToken = refs.guildToken; // cached SLOAD
274:        uint256 gaugeWeight = GuildToken(_guildToken).getGaugeWeight(
275:            address(this)
276:        );
277:        gaugeWeight = uint256(int256(gaugeWeight) + gaugeWeightDelta); //@audit gaugeWeight is changed
278:        uint256 gaugeType = GuildToken(_guildToken).gaugeType(address(this));
279:        uint256 totalWeight = GuildToken(_guildToken).totalTypeWeight( //@audit but totalWeight is incorrectly not changed
280:            gaugeType
281:        );

The problem is that if you change the weight on the term, the totalTypeWeight also changes, which is not taken into account in the debtCeiling function. Here you can see using the example of the incrementGauge function that totalTypeWeight is also changed when you change the weight on a term:

//File: src/tokens/ERC20Gauges.sol
230: function _incrementGaugeWeight(
231:        address user,
232:        address gauge,
233:        uint256 weight
234:    ) internal virtual {
235:        bool added = _userGauges[user].add(gauge); // idempotent add
236:        if (added && _userGauges[user].length() > maxGauges) {
237:            require(canExceedMaxGauges[user], "ERC20Gauges: exceed max gauges");
238:        }
239: 
240:        getUserGaugeWeight[user][gauge] += weight;
241: 
242:        getGaugeWeight[gauge] += weight; //@audit When getGaugeWeight is increased
243: 
244:        totalTypeWeight[gaugeType[gauge]] += weight; //@audit then totalTypeWeight will also increase
245: 
246:        emit IncrementGaugeWeight(user, gauge, weight);
247:    }

So you can see debtCeiling calculated incorrectly. Since this function in _decrementGaugeWeight in GuildToken.sol is used to check whether after the decrement of the gauge weight there is still enough to cover the issuance, this error leads to you being able to decrement less weight than you should be able to.

Tools Used

VSCode

Recommended Mitigation Steps

In the function debtCeiling, totalWeight should also be changed by gaugeWeightDelta:

//File: src/loan/LendingTerm.sol
        gaugeWeight = uint256(int256(gaugeWeight) + gaugeWeightDelta);
        uint256 gaugeType = GuildToken(_guildToken).gaugeType(address(this));
        uint256 totalWeight = GuildToken(_guildToken).totalTypeWeight(
            gaugeType
        );
+       totalWeight = uint256(int256(totalWeight) + gaugeWeightDelta);

Assessed type

Other