Ethereum Credit Guild - smiling_heretic's results

Lines of code

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

Vulnerability details

Description

Let’s inspect these lines of code in SurplusGuildMinter.getRewards:

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

The intention behind lines:

        if (lastGaugeLoss > uint256(userStake.lastGaugeLoss)) {
            slashed = true;
        }

is that user will get slashed only for a loss in a gauge, that happened when user was staking for this gauge. It’s also to ensure that the same user won’t get slashed for one loss in gauge many times.

However, in the above condition, userStake has been declared in return values but it hasn’t been read from the storage yet. Therefore, in this condition, userStake.lastGaugeLoss is always zero.

Because of this bug, the condition for slashing becomes:

        if (lastGaugeLoss > 0) {
            slashed = true;
        }

This means that if a loss has happened for a gauge even once, all users staking for this gauge will always get slashed.

In particular, if a term gets offboarded due to losses and temporary market conditions and then onboarded again, all users staking for this term after re-onboarding will still get slashed.

Impact

Users staking for a term in SurplusGuildMinter will get unfairly slashed.

Proof of Concept

Add the following test case to test/unit/loan/SurplusGuildMinter.t.sol

    function testSlashForAncientLoss() public {
        // the guild token earn interests
        vm.prank(governor);
        profitManager.setProfitSharingConfig(
            0.5e18, // surplusBufferSplit
            0, // creditSplit
            0.5e18, // guildSplit
            0, // otherSplit
            address(0) // otherRecipient
        );
        credit.mint(address(profitManager), 35e18);
        profitManager.notifyPnL(term, 35e18);

        // time passes
        vm.warp(block.timestamp + 13);
        vm.roll(block.number + 1);

        // loss in gauge
        profitManager.notifyPnL(term, -27.5e18);

        // times passes again
        vm.warp(block.timestamp + 13);
        vm.roll(block.number + 1);

        // user stakes after loss
        credit.mint(address(this), 150e18);
        credit.approve(address(sgm), 150e18);
        sgm.stake(term, 150e18);

        // trying to get rewards
        (, , bool slashed) = sgm.getRewards(address(this), term);
        // but getting slashed
        assertEq(slashed, true);

        // times passes again
        vm.warp(block.timestamp + 13);
        vm.roll(block.number + 1);

        // user stakes again...
        credit.mint(address(this), 150e18);
        credit.approve(address(sgm), 150e18);
        sgm.stake(term, 150e18);

        // ...and gets slashed for the same loss in gauge 2nd time
        (, , slashed) = sgm.getRewards(address(this), term);
        assertEq(slashed, true);
    }

Tools Used

Manual Analysis, Foundry

Recommended Mitigation Steps

Move read from storage before the slashing check.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/LendingTermOffboarding.sol#L191-L195 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/LendingTermOffboarding.sol#L162-L167 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/LendingTermOffboarding.sol#L183

Vulnerability details

Description

In LendingTermOffboarding, there’s nOffboardingsInProgress storage variable that gets incremented by one on offboard and decremented by one on cleanup.

When nOffboardingsInProgress increases from zero to one on offboard, pausing redemptions in SimplePSM gets triggered. The redemptions get unpaused only when nOffboardingsInProgress reaches zero again.

However, it’s possible that a term gets re-onboarded. In this case, offboard is called without following it up with cleanup. As a results, nOffboardingsInProgress stays equal to 1, even though there are no longer any terms to cleanup and decrement nOffboardingsInProgress back to zero.

Therefore, re-onboarding causes redemptions to stay paused forever.

Even if some other address with Governor role unpauses redemptions after this situation is reached, then we’re in, arguably, even worse situation because we’re likely to get into a situation in which there are offboardings in progress but redemptions work just fine.

Impact

DoS of SimplePSM.redeem which might cause depeg of CREDIT.

Proof of Concept

Add this test case to test/unit/governance/LendingTermOffboarding.t.sol:

    function testPauseForever() public {
        // prepare (1)
        guild.mint(bob, _QUORUM);
        vm.startPrank(bob);
        guild.delegate(bob);
        uint256 snapshotBlock = block.number;
        offboarder.proposeOffboard(address(term));
        vm.roll(block.number + 1);
        vm.warp(block.timestamp + 13);
        offboarder.supportOffboard(snapshotBlock, address(term));
        offboarder.offboard(address(term));

        // get enough CREDIT to pack back interests
        vm.stopPrank();
        vm.roll(block.number + 1);
        vm.warp(block.timestamp + 13);
        uint256 debt = term.getLoanDebt(aliceLoanId);
        credit.mint(alice, debt - aliceLoanSize);

        // close loans
        vm.startPrank(alice);
        credit.approve(address(term), debt);
        term.repay(aliceLoanId);
        vm.stopPrank();

        // re-onboard
        guild.addGauge(1, address(term));

        // re-offboard
        vm.roll(block.number + offboarder.POLL_DURATION_BLOCKS());
        vm.warp(block.timestamp + offboarder.POLL_DURATION_BLOCKS() * 13);
        vm.startPrank(bob);
        snapshotBlock = block.number;
        offboarder.proposeOffboard(address(term));
        vm.roll(block.number + 1);
        vm.warp(block.timestamp + 13);
        offboarder.supportOffboard(snapshotBlock, address(term));
        offboarder.offboard(address(term));

        // cleanup
        offboarder.cleanup(address(term));
        assertEq(offboarder.nOffboardingsInProgress(), 1);
        assertEq(psm.redemptionsPaused(), true);
        assertEq(offboarder.canOffboard(address(term)), false);

        // now there's nothing more to cleanup
        // to set nOffboardingsInProgress back to zero
        // and unpause redemptions
    }

Tools Used

Foundry, Manual Analysis

Recommended Mitigation Steps

Rethink how to track if there are any offboardings in progress.

Assessed type

DoS

Contract: LendingTermOnboarding.sol

199:         bool isGauge = GuildToken(guildToken).isGauge(term);

Lines of code

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

Vulnerability details

Description

Users can use their GUILD tokens to vote for gauges (terms). When profit for a term happens (e.g. interest on a debt gets repaid), a portion of gained CREDIT tokens is distributed to users participating in this gauge voting.

Amount of CREDIT rewards, that a user gets from this distribution, is proportional to gauge weight that this user has allocated to the profiting term at exact time of profit happening.

Debt ceiling for a term increases when users allocate more gauge weight to this term.

The (supposed) intention behind this mechanism is that users are incentivized to allocate gauge weight to terms that are likely to be profitable and very unlikely to to cause any loss (because of applyGaugeLoss slashing mechanism).

If users are behaving as intended, debt ceilings are high for profitable terms and low for “bad“ terms.

However, because it’s enough to have GUILD gauge weight allocated to a term at exact time of profit happening, there exists a better strategy for users that isn’t aligned with the protocol.

Namely, a user can frontrun profit in gauge with incrementGauge and vote their full balance of GUILD for this profiting term. After the profit happens, user calls decrementGauge to free full balance of GUILD to avoid risk of getting slashed, in case of loss happening in the future. It also prevents getting locked in this gauge voting position, in case someone uses up increased debt ceiling by borrowing.

This strategy has several advantages over the intended, “honest“ strategy:

1. higher rewards because users can use their full GUILD balance for each profit in each term
2. users keep their gauge weight free, so they won’t get locked in because of used up debt ceiling
3. no risk of getting slashed in case a loss happens in a gauge

Some potential disadvantages of this strategy are:

1. gas costs of more function calls
2. costs of doing MEV

Nevertheless, it’s likely that this strategy will be less risky and more profitable for many users at least on some chains.

Impact

Many users will play MEV game instead of allocating their GUILD to gauges for terms that are likely to bring profit (and increase debt ceilings of these terms).

In other words, the incentive mechanism of gauge voting with GUILD token doesn’t work as intended.

Because of this, debt ceilings will be low and most of GUILD won’t be allocated to gauges.

Honest users will get less rewards than freeriders playing MEV game.

Proof of Concept

Add this test case to test/unit/governance/ProfitManager.t.sol:

    function testAlternativeStrategy() public {
        address freerider = address(666);
        vm.label(freerider, "freerider");

        // everyone starts with zero CREDIT
        assertEq(credit.balanceOf(alice), 0);
        assertEq(credit.balanceOf(bob), 0);
        assertEq(credit.balanceOf(freerider), 0);

        // grant roles to test contract
        vm.startPrank(governor);
        core.grantRole(CoreRoles.GOVERNOR, 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.GAUGE_PNL_NOTIFIER, address(this));
        // this PoC is about rewards from GUILD gauge voting
        profitManager.setProfitSharingConfig(
            0, // surplusBufferSplit
            0, // creditSplit
            1e18, // guildSplit
            0, // otherSplit
            address(0) // otherRecipient
        );
        vm.stopPrank();

        // add two gauges
        guild.setMaxGauges(100);
        guild.addGauge(1, gauge1);
        guild.addGauge(1, gauge2);

        // alice and bob are honest GUILD holders
        guild.mint(alice, 500e18);
        guild.mint(bob, 500e18);

        // they use their GUILD to increment gauges
        // for terms that are likely to be profitable in their opinion
        // and keep their GUILD voted long-term
        vm.startPrank(alice);
        guild.incrementGauge(gauge1, 400e18);
        guild.incrementGauge(gauge2, 100e18);
        vm.stopPrank();
        vm.startPrank(bob);
        guild.incrementGauge(gauge1, 100e18);
        guild.incrementGauge(gauge2, 400e18);
        vm.stopPrank();

        // freerider is also a GUILD holder
        guild.mint(freerider, 500e18);

        // some time passes
        vm.roll(block.number + 1);
        vm.warp(block.timestamp + 13);

        // freerider frontruns profit in gauge
        // and votes all their GUILD for this gauge
        // to maximize rewards
        vm.startPrank(freerider);
        guild.incrementGauge(gauge1, guild.balanceOf(freerider));
        vm.stopPrank();
        // profit in gauge1 occurs
        credit.mint(address(profitManager), 100e18);
        profitManager.notifyPnL(gauge1, 100e18);
        // freerider decrements gauge weight back to zero
        // to avoid risk of getting slashed with applyGaugeLoss
        // and risk of getting locked in this gauge voting position
        // by not being able to lower debt ceiling
        // if someone borrows in meantime
        vm.startPrank(freerider);
        guild.decrementGauge(gauge1, guild.balanceOf(freerider));
        vm.stopPrank();

        // repeat for gauge2
        vm.roll(block.number + 1);
        vm.warp(block.timestamp + 13);

        vm.startPrank(freerider);
        guild.incrementGauge(gauge2, guild.balanceOf(freerider));
        vm.stopPrank();
        credit.mint(address(profitManager), 100e18);
        profitManager.notifyPnL(gauge2, 100e18);
        vm.startPrank(freerider);
        guild.decrementGauge(gauge2, guild.balanceOf(freerider));
        vm.stopPrank();

        // make sure all rewards are claimed
        profitManager.claimRewards(alice);
        profitManager.claimRewards(bob);
        profitManager.claimRewards(freerider);

        // freerider got 2x more CREDIT rewards than each of honest users
        assertEq(credit.balanceOf(alice), 50e18);
        assertEq(credit.balanceOf(bob), 50e18);
        assertEq(credit.balanceOf(freerider), 100e18);
        // freerider has all their GUILD gauge weight free
        assertEq(guild.userUnusedWeight(freerider), 500e18);
    }

Tools Used

Foundry, Manual Analysis

Recommended Mitigation Steps

A relatively simple fix to this problems would be to add some delay (at least one block) between incrementGauge and it starting accruing rewards for user. That would prevent frontrunning.

However, it’s probably better to rethink how this incentive mechanism should work.

Assessed type

MEV