Ethereum Credit Guild - ether_sky's results

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/SurplusGuildMinter.sol#L136 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/tokens/GuildToken.sol#L147 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/ProfitManager.sol#L396-L399 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/SurplusGuildMinter.sol#L239-L242 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/ProfitManager.sol#L428-L431

Vulnerability details

Impact

Multiple users can stake in the same term through SurplusGuildMinter, causing the gauge weight for that term to increase. When rewards are assigned to the guild holders, these rewards will be distributed to the actual users in SurplusGuildMinter in proportion to their stakes. Once any loss is applied to that gauge and SurplusGuildMinter incorporates that loss, the gauge weight of SurplusGuildMinter will be reduced to 0. Consequently, any rewards from this point onward won't be assigned to SurplusGuildMinter. Some users may stake new tokens in this term, causing the gauge weight to increase again, and new rewards will be assigned to SurplusGuildMinter. But previous users who staked before the loss can also claim these rewards. This results in new users being unable to receive their rewards, or the protocol may be halted due to insufficient funds to send these misallocated rewards. I marked this as high because users can lose their rewards inadvertently, potentially leading to a protocol halt.

Proof of Concept

When a user stakes in a term through SurplusGuildMinter, it will increase the gauge weight of SurplusGuildMinter. https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/SurplusGuildMinter.sol#L136

function stake(address term, uint256 amount) external whenNotPaused {
    GuildToken(guild).incrementGauge(term, guildAmount);
}

If that term experiences any loss, and the SurplusGuildMinter applies that loss, the gauge weight of SurplusGuildMinter is reduced to 0. https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/tokens/GuildToken.sol#L147

function applyGaugeLoss(address gauge, address who) external {
    _decrementGaugeWeight(who, gauge, _userGaugeWeight);
}

This means that any upcoming rewards won't be assigned to SurplusGuildMinter; in other words, users who staked before this loss won't receive upcoming rewards. Other users can still stake in this term, resulting in an increase in the gauge weight of SurplusGuildMinter.

When rewards are assigned to the guild holders, the gaugeProfitIndex of that term is increased. https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/ProfitManager.sol#L396-L399

function notifyPnL(address gauge, int256 amount) {
    uint256 _gaugeProfitIndex = gaugeProfitIndex[gauge];
    gaugeProfitIndex[gauge] = _gaugeProfitIndex +
        (amountForGuild * 1e18) / _gaugeWeight;
}

Let's consider the case where the user claims his rewards through the getRewards function in SurplusGuildMinter. https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/SurplusGuildMinter.sol#L239-L242

function getRewards(address user, address term) {
    ProfitManager(profitManager).claimRewards(address(this)); 
    uint256 _profitIndex = ProfitManager(profitManager)
        .userGaugeProfitIndex(address(this), term);
    uint256 _userProfitIndex = uint256(userStake.profitIndex);
}

In the claimRewards function in ProfitManager, the userGaugeProfitIndex is updated to the latest gaugeProfitIndex. https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/ProfitManager.sol#L428-L431

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

However, it's important to note that this gaugeProfitIndex is intended for new users. Previous users who staked before the loss should not be able to receive these rewards. But they can receive, and new users may not receive their deserved rewards, or the protocol could be halted due to ProfitManager lacking sufficient funds to send these duplicated rewards.

The PoC for this is as below:

function testWrongRewardCalculation() public {
        ProfitManager newProfitManager;
        MockERC20 newCollateral;
        CreditToken newCredit;
        GuildToken newGuild;
        RateLimitedMinter newRlcm;
        AuctionHouse newAuctionHouse;
        LendingTerm newTerm;
        SimplePSM newPsm;
        RateLimitedMinter newRlgm;
        SurplusGuildMinter newSgm;

        newProfitManager = new ProfitManager(address(core));
        newCollateral = new MockERC20();
        newCredit = new CreditToken(address(core), "name", "symbol");
        newGuild = new GuildToken(
            address(core),
            address(newProfitManager)
        );
        newRlcm = new RateLimitedMinter(
            address(core) /*_core*/,
            address(newCredit) /*_token*/,
            CoreRoles.RATE_LIMITED_CREDIT_MINTER /*_role*/,
            type(uint256).max /*_maxRateLimitPerSecond*/,
            type(uint128).max /*_rateLimitPerSecond*/,
            type(uint128).max /*_bufferCap*/
        );
        newAuctionHouse = new AuctionHouse(address(core), 650, 1800);
        newTerm = LendingTerm(Clones.clone(address(new LendingTerm())));
        newTerm.initialize(
            address(core),
            LendingTerm.LendingTermReferences({
                profitManager: address(newProfitManager),
                guildToken: address(newGuild),
                auctionHouse: address(newAuctionHouse),
                creditMinter: address(newRlcm),
                creditToken: address(newCredit)
            }),
            LendingTerm.LendingTermParams({
                collateralToken: address(newCollateral),
                maxDebtPerCollateralToken: 2000e18,
                interestRate: 0.10e18,
                maxDelayBetweenPartialRepay: 63115200, // 2 years
                minPartialRepayPercent: 0.2e18,
                openingFee: 0,
                hardCap: 20_000_000e18
            })
        );
        newPsm = new SimplePSM(
            address(core),
            address(newProfitManager),
            address(newCredit),
            address(newCollateral)
        );
        newRlgm = new RateLimitedMinter(
            address(core), /*_core*/
            address(newGuild), /*_token*/
            CoreRoles.RATE_LIMITED_GUILD_MINTER, /*_role*/
            type(uint256).max, /*_maxRateLimitPerSecond*/
            type(uint128).max, /*_rateLimitPerSecond*/
            type(uint128).max /*_bufferCap*/
        );
        newSgm = new SurplusGuildMinter(
            address(core),
            address(newProfitManager),
            address(newCredit),
            address(newGuild),
            address(newRlgm),
            MINT_RATIO,
            REWARD_RATIO
        );

        vm.startPrank(governor);
        core.grantRole(CoreRoles.CREDIT_MINTER, address(newPsm));
        core.grantRole(CoreRoles.CREDIT_MINTER, address(newRlcm));
        core.grantRole(CoreRoles.RATE_LIMITED_CREDIT_MINTER, address(newTerm));
        core.grantRole(CoreRoles.GAUGE_PNL_NOTIFIER, address(newTerm));
        core.grantRole(CoreRoles.GUILD_MINTER, address(newRlgm));
        core.grantRole(CoreRoles.RATE_LIMITED_GUILD_MINTER, address(newSgm));
        core.grantRole(CoreRoles.GUILD_SURPLUS_BUFFER_WITHDRAW, address(newSgm));
        newProfitManager.initializeReferences(address(newCredit), address(newGuild), address(newPsm));
        vm.stopPrank();

        // add gauge and vote for it
        newGuild.setMaxGauges(10);
        newGuild.addGauge(1, address(newTerm));
        newGuild.mint(address(this), 2000e18);
        newGuild.incrementGauge(address(newTerm), uint112(2000e18));

        uint128 X = 100e18;

        // stake
        newCredit.mint(address(this), X);
        newCredit.approve(address(newSgm), X);
        newSgm.stake(address(newTerm), X);
    
        newCollateral.mint(address(this), X);
        newCollateral.approve(address(newPsm), X);
        newPsm.mint(address(this), X);

        // borrow from this term
        newCollateral.mint(address(this), X);
        newCollateral.approve(address(newTerm), X);
        bytes32 loanId = newTerm.borrow(X, X);

        vm.warp(block.timestamp + 3 days);
        vm.startPrank(address(newProfitManager));
        // apply loss
        newGuild.notifyGaugeLoss(address(newTerm));
        vm.stopPrank();

        // Since the gauge weight becomes 0, no rewards should be assigned.
        newGuild.applyGaugeLoss(address(newTerm), address(newSgm));

        vm.warp(block.timestamp + 3 days);
        address user = address(3);
        newCredit.mint(user, X);
        vm.startPrank(user);
        newCredit.approve(address(newSgm), X);
        /*
        If another user stakes in that term again, and the gauge weight becomes larger than 0, rewards will be gathered. 
        However, it's crucial to ensure that these rewards are attributed to this user.
        */
        newSgm.stake(address(newTerm), X);
        vm.stopPrank();


        vm.startPrank(governor);
        newProfitManager.setProfitSharingConfig(
            0, // surplusBufferSplit
            0.5e18, // creditSplit
            0.5e18, // guildSplit
            0, // otherSplit
            address(0)
        );
        vm.stopPrank();
        newCredit.mint(address(this), 3 * X);
        newCredit.approve(address(newTerm), 3 * X);
        vm.warp(block.timestamp + 365 days);
        // Interest will be assigned to SurplusGuildMinter.
        newTerm.repay(loanId);

        uint256 balanceBefore = newCredit.balanceOf(address(this));
        newSgm.getRewards(address(this), address(newTerm));
        uint256 balanceAfter = newCredit.balanceOf(address(this));
        // address(this) should not receive any rewards, but...
        assertGt(balanceAfter, balanceBefore); // balanceBefore = 389842573579739904176, balanceAfter = 390304274780660817576

        vm.startPrank(user);
        uint256 balanceBeforeOfUser = newCredit.balanceOf(user);
        // LendingTerm does not have sufficient funds to send rewards to the SurplusGuildMinter due to a misduplication of rewards.
        vm.expectRevert("ERC20: transfer amount exceeds balance");
        newSgm.getRewards(user, address(newTerm));
        uint256 balanceAfterOfUser = newCredit.balanceOf(user);
        // As a result, this user unfortunately cannot receive any rewards.
        assertEq(balanceAfterOfUser, balanceBeforeOfUser);
        vm.stopPrank();
}

Tools Used

Recommended Mitigation Steps

It is necessary to store the last updated gaugeProfitIndex for the term when the SurplusGuildMinter applies the loss.

Assessed type

Error

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/ProfitManager.sol#L396-L399 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/tokens/GuildToken.sol#L258-L260 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/ProfitManager.sol#L413-L418

Vulnerability details

Impact

The rewards originating from the loans are meant to be distributed to guild holders based on the proportion of their weight. However, a logic error exists that enables any user who adds weight after rewards have been distributed to claim the previous rewards. Consequently, early claimants might receive rewards intended for other users, while late claimants may not receive their rightful rewards. This issue is marked as high because users stand to lose their rewards inadvertently, and late claim requests may be reverted due to insufficient funds.

Proof of Concept

When a certain amount of rewards is distributed, the gaugeProfitIndex will be incremented accordingly. https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/ProfitManager.sol#L396-L399

function notifyPnL(address gauge, int256 amount) {
    uint256 _gaugeProfitIndex = gaugeProfitIndex[gauge];
    gaugeProfitIndex[gauge] = _gaugeProfitIndex + (amountForGuild * 1e18) / _gaugeWeight;
}

Subsequently, when any user attempts to add gauge weight, he will claim rewards accumulated so far and increase his weight. https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/tokens/GuildToken.sol#L258-L260

function _incrementGaugeWeight(address user, address gauge, uint256 weight) {
    ProfitManager(profitManager).claimGaugeRewards(user, gauge);

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

However, if the user's previous weight was 0, the userGaugeProfitIndex won't be updated to reflect the latest gaugeProfitIndex. https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/ProfitManager.sol#L413-L418

function claimGaugeRewards(address user, address gauge) {
    uint256 _userGaugeWeight = uint256(
        GuildToken(guild).getUserGaugeWeight(user, gauge)
    );
    if (_userGaugeWeight == 0) {
        return 0;
    }
}

This implies that the user can claim rewards based on the current gaugeProfitIndex and the new weight. In reality, this gaugeProfitIndex pertains to other users, and the new weight of this user should not be able to receive rewards.

The PoC of this is as below:

function testGetProfitOfOthers() public {
        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));
        vm.stopPrank();

        profitManager.setProfitSharingConfig(
            0, // surplusBufferSplit
            0.5e18, // creditSplit
            0.5e18, // guildSplit
            0, // otherSplit
            address(0) // otherRecipient
        );
        
        guild.setMaxGauges(3);
        guild.addGauge(1, gauge1);
    
        guild.mint(alice, 100e18);
        vm.startPrank(alice);
        // Alice increases the gauge weight.
        guild.incrementGauge(gauge1, 100e18);
        vm.stopPrank();

        credit.mint(address(profitManager), 20e18);
        // These rewards should be assigned to the Alice
        profitManager.notifyPnL(gauge1, 20e18);

        guild.mint(bob, 100e18);
        vm.startPrank(bob);
        // Bob increases the gauge weight after the rewards, so he shouldn't be able to receive previous rewards.
        guild.incrementGauge(gauge1, 100e18);
        vm.stopPrank();

        uint256 beforeBalanceOfBob = credit.balanceOf(bob);
        profitManager.claimGaugeRewards(bob, gauge1);
        uint256 afterBalanceOfBob = credit.balanceOf(bob);
        // Oh no, Bob receives Alice's rewards.
        assertGt(afterBalanceOfBob, beforeBalanceOfBob); // afterBalanceOfBob = 10000000000000000000, beforeBalanceOfBob = 0

        uint256 beforeBalanceOfAlice = credit.balanceOf(alice);
        vm.expectRevert("ERC20: transfer amount exceeds balance");
        // Alice's claim request is reversed because the ProfitManager has already transferred Alice's rewards to Bob.
        profitManager.claimGaugeRewards(alice, gauge1);
        uint256 afterBalanceOfAlice = credit.balanceOf(alice);
        // As a result, Alice can't receive any rewards.
        assertEq(afterBalanceOfAlice, beforeBalanceOfAlice);
}

Tools Used

Recommended Mitigation Steps

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

Assessed type

Error

Lines of code

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

Vulnerability details

Impact

Any user can stake in any term through the SurplusGuildMinter. Once that term incurs a loss, all users' funds will be slashed. However, users who stake after that loss should not be affected by it. Nevertheless, all users interacting with this term will consistently lose their funds. I marked this as high because this vulnerability can inadvertently deplete user funds.

Proof of Concept

When a user attempts to stake in a term, the lastGaugeLoss of this stake will have the same value as the current lastGaugeLoss value of that term.

function stake(address term, uint256 amount) external whenNotPaused {
    userStake = UserStake({
        lastGaugeLoss: SafeCastLib.safeCastTo48(lastGaugeLoss),
    });
}

In the getRewards function, we verify whether the user staked before the gauge loss or not. However, there is an error in this code that consistently flags users as staking before the gauge loss, even when some users stake after the gauge loss has occurred. https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/SurplusGuildMinter.sol#L229-L231

function getRewards(address user, address term) {
    if (lastGaugeLoss > uint256(userStake.lastGaugeLoss)) {
        slashed = true;
    }

    userStake = _stakes[user][term];
}

In the above code, we utilize the userStake.lastGaugeLoss value before it is declared.

The PoC of this is as below:

function testWrongSlashedCheck() public {
        ProfitManager newProfitManager;
        MockERC20 newCollateral;
        CreditToken newCredit;
        GuildToken newGuild;
        RateLimitedMinter newRlcm;
        AuctionHouse newAuctionHouse;
        LendingTerm newTerm;
        SimplePSM newPsm;
        RateLimitedMinter newRlgm;
        SurplusGuildMinter newSgm;

        newProfitManager = new ProfitManager(address(core));
        newCollateral = new MockERC20();
        newCredit = new CreditToken(address(core), "name", "symbol");
        newGuild = new GuildToken(
            address(core),
            address(newProfitManager)
        );
        newRlcm = new RateLimitedMinter(
            address(core) /*_core*/,
            address(newCredit) /*_token*/,
            CoreRoles.RATE_LIMITED_CREDIT_MINTER /*_role*/,
            type(uint256).max /*_maxRateLimitPerSecond*/,
            type(uint128).max /*_rateLimitPerSecond*/,
            type(uint128).max /*_bufferCap*/
        );
        newAuctionHouse = new AuctionHouse(address(core), 650, 1800);
        newTerm = LendingTerm(Clones.clone(address(new LendingTerm())));
        newTerm.initialize(
            address(core),
            LendingTerm.LendingTermReferences({
                profitManager: address(newProfitManager),
                guildToken: address(newGuild),
                auctionHouse: address(newAuctionHouse),
                creditMinter: address(newRlcm),
                creditToken: address(newCredit)
            }),
            LendingTerm.LendingTermParams({
                collateralToken: address(newCollateral),
                maxDebtPerCollateralToken: 2000e18,
                interestRate: 0.10e18,
                maxDelayBetweenPartialRepay: 63115200, // 2 years
                minPartialRepayPercent: 0.2e18,
                openingFee: 0,
                hardCap: 20_000_000e18
            })
        );
        newPsm = new SimplePSM(
            address(core),
            address(newProfitManager),
            address(newCredit),
            address(newCollateral)
        );
        newRlgm = new RateLimitedMinter(
            address(core), /*_core*/
            address(newGuild), /*_token*/
            CoreRoles.RATE_LIMITED_GUILD_MINTER, /*_role*/
            type(uint256).max, /*_maxRateLimitPerSecond*/
            type(uint128).max, /*_rateLimitPerSecond*/
            type(uint128).max /*_bufferCap*/
        );
        newSgm = new SurplusGuildMinter(
            address(core),
            address(newProfitManager),
            address(newCredit),
            address(newGuild),
            address(newRlgm),
            MINT_RATIO,
            REWARD_RATIO
        );

        vm.startPrank(governor);
        core.grantRole(CoreRoles.CREDIT_MINTER, address(newPsm));
        core.grantRole(CoreRoles.CREDIT_MINTER, address(newRlcm));
        core.grantRole(CoreRoles.RATE_LIMITED_CREDIT_MINTER, address(newTerm));
        core.grantRole(CoreRoles.GAUGE_PNL_NOTIFIER, address(newTerm));
        core.grantRole(CoreRoles.GUILD_MINTER, address(newRlgm));
        core.grantRole(CoreRoles.RATE_LIMITED_GUILD_MINTER, address(newSgm));
        core.grantRole(CoreRoles.GUILD_SURPLUS_BUFFER_WITHDRAW, address(newSgm));
        newProfitManager.initializeReferences(address(newCredit), address(newGuild), address(newPsm));
        vm.stopPrank();

        // add gauge and vote for it
        newGuild.setMaxGauges(10);
        newGuild.addGauge(1, address(newTerm));
        newGuild.mint(address(this), 50e18);
        newGuild.incrementGauge(address(newTerm), uint112(50e18));

        uint128 X = 100e18;
        newCollateral.mint(address(this), X);
        newCollateral.approve(address(newPsm), X);
        newPsm.mint(address(this), X);

        newCollateral.mint(address(this), X);
        newCollateral.approve(address(newTerm), X);
        bytes32 loanId = newTerm.borrow(X, X);

        vm.startPrank(governor);
        newTerm.forgive(loanId);
        vm.stopPrank();

        vm.warp(block.timestamp + 3 days);
        newCredit.mint(address(this), X);
        newCredit.approve(address(newSgm), X);
        newSgm.stake(address(newTerm), X);

        (, , bool slashed) = newSgm.getRewards(address(this), address(newTerm));
        // The user should not be slashed, but ...
        assertEq(slashed, true);
}

Tools Used

Recommended Mitigation Steps

function getRewards(address user, address term) {
    + userStake = _stakes[user][term];
    if (lastGaugeLoss > uint256(userStake.lastGaugeLoss)) {
        slashed = true;
    }

    - userStake = _stakes[user][term];
}

Assessed type

Error

Lines of code

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

Vulnerability details

Impact

We determine the borrowed credit by subtracting the pegged amount linked to underlying tokens from the total supply of credit tokens. While the logic is correct, there's a potential for underflow in the totalBorrowedCredit function. If such an underflow occurs, any borrowing attempt across all terms in the market will be reverted, as this function is invoked whenever someone attempts to borrow from a term. I marked this as high because if such a case occurs, this market would become unusable.

Proof of Concept

The function responsible for calculating the total borrowed credit is as below: https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/ProfitManager.sol#L172-L176

function totalBorrowedCredit() external view returns (uint256) {
    return
        CreditToken(credit).targetTotalSupply() -
        SimplePSM(psm).redeemableCredit();
}

When borrowers repay their debts or their debts are called by others, the total borrowed credit is reduced to 0. However, there is no guarantee that the following statement is always true:

CreditToken(credit).targetTotalSupply() >= SimplePSM(psm).redeemableCredit()

I will illustrate one example to demonstrate such a case. While this scenario is both simple and uncommon, it serves as an illustration of potential underflow, and there may be other cases leading to similar issues.

A lender initially lends 10^20 credit tokens. Borrower A borrows 10^20 from a term, followed by borrower B also borrowing 10^20 from the same term. At this point, the total supply is 3 * 10^20, and the creditMultiplier is 10^18. Subsequently, the debt for borrower A is either called or forgiven, leading to a new creditMultiplier of 666,666,666,666,666,666. Afterward, borrower B's debt is similarly called or forgiven, resulting in a new creditMultiplier of 333,333,333,333,333,332. The target total supply remains at 3 * 10^20, with a redeemableCredit of 300,000,000,000,000,001,200.

X = 10^20, p1 = 10^18;
// first forgive
p1 * (3 * X - X * 10^18 / p1) = p2 * 3 * X; // p2 = 666,666,666,666,666,666
// second forgive
p2 * (3 * X - X * 10^18 / p2) = p3 * 3 * X; // p3 = 333,333,333,333,333,332
SimplePSM(psm).redeemableCredit() = 300,000,000,000,000,001,200;
targetTotalSupply = 300,000,000,000,000,000,000;

The result of the PoC test is as follows:

|   [11310] 0x535B3D7A252fa034Ed71F0C53ec0C6F784cB64E1::borrow(500000000000000000000 [5e20], 100000000000000000000 [1e20]) 
    │   ├─ [11130] LendingTerm::borrow(500000000000000000000 [5e20], 100000000000000000000 [1e20]) [delegatecall]
    │   │   ├─ [394] profitManager::creditMultiplier() [staticcall]
    │   │   │   └─ ← 333333333333333332 [3.333e17]
    │   │   ├─ [608] profitManager::minBorrow() [staticcall]
    │   │   │   └─ ← 300000000000000001200 [3e20]
    │   │   ├─ [3188] profitManager::totalBorrowedCredit() [staticcall]
    │   │   │   ├─ [1361] SimplePSM::redeemableCredit() [staticcall]
    │   │   │   │   ├─ [394] profitManager::creditMultiplier() [staticcall]
    │   │   │   │   │   └─ ← 333333333333333332 [3.333e17]
    │   │   │   │   └─ ← 300000000000000001200 [3e20]
    │   │   │   ├─ [576] credit::targetTotalSupply() [staticcall]
    │   │   │   │   └─ ← 300000000000000000000 [3e20]
    │   │   │   └─ ← "Arithmetic over/underflow"
    │   │   └─ ← "Arithmetic over/underflow"
    │   └─ ← "Arithmetic over/underflow"
    └─ ← "Arithmetic over/underflow"

And the PoC for this is as below:

function testTotalBorrowedCredit() public {
        // create a new lending term
        vm.startPrank(governor);
        LendingTerm newTerm = LendingTerm(Clones.clone(address(new LendingTerm())));
        newTerm.initialize(
            address(core),
            LendingTerm.LendingTermReferences({
                profitManager: address(profitManager),
                guildToken: address(guild),
                auctionHouse: address(auctionHouse),
                creditMinter: address(rlcm),
                creditToken: address(credit)
            }),
            LendingTerm.LendingTermParams({
                collateralToken: address(collateral),
                maxDebtPerCollateralToken: _CREDIT_PER_COLLATERAL_TOKEN,
                interestRate: _INTEREST_RATE,
                maxDelayBetweenPartialRepay: _MAX_DELAY_BETWEEN_PARTIAL_REPAY,
                minPartialRepayPercent: _MIN_PARTIAL_REPAY_PERCENT,
                openingFee: 0,
                hardCap: _HARDCAP
            })
        );
        core.grantRole(CoreRoles.RATE_LIMITED_CREDIT_MINTER, address(newTerm));
        core.grantRole(CoreRoles.GAUGE_PNL_NOTIFIER, address(newTerm));
        vm.stopPrank();

        guild.addGauge(1, address(newTerm));
        guild.incrementGauge(address(newTerm), _HARDCAP / 100);

        uint128 X = 100e18;
        vm.startPrank(governor);
        core.grantRole(CoreRoles.CREDIT_MINTER, address(psm));
        vm.stopPrank();

        collateral.mint(address(this), X);
        collateral.approve(address(psm), X);
        psm.mint(address(this), X);

        collateral.mint(address(this), X);
        collateral.approve(address(term), X);
        bytes32 loanID_1 = term.borrow(X, X);

        vm.warp(block.timestamp + 3 days);
        collateral.mint(address(this), X);
        collateral.approve(address(term), X);
        bytes32 loanID_2 = term.borrow(X, X);

        vm.startPrank(governor);
        term.forgive(loanID_1);
        term.forgive(loanID_2);
        vm.stopPrank();

        // all borrowing requests will be reverted due to arithmetic underflow in totalBorrowedCredit() function.
        newTerm.borrow(5 * X, X);
}

Tools Used

https://www.calculator.net/big-number-calculator.html

Recommended Mitigation Steps

In the totalBorrowedCredit function, please incorporate a check for underflow. In such a case, return 0. This adjustment won't impact the protocol, as the value is negligible and can be safely ignored.

function totalBorrowedCredit() external view returns (uint256) {
        + if (SimplePSM(psm).redeemableCredit() > CreditToken(credit).targetTotalSupply()) return 0;
        return
            CreditToken(credit).targetTotalSupply() -
            SimplePSM(psm).redeemableCredit();
}

Assessed type

Under/Overflow

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/LendingTermOffboarding.sol#L138-L140 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/LendingTermOffboarding.sol#L159-L167 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/LendingTermOffboarding.sol#L177-L180

Vulnerability details

Impact

We can propose to offboard the term, and if it reaches a sufficient quorum, anyone can offboard it. However, we cannot clean that term immediately if there are open loans in that term. At this point, we can figure out that offboarding that term is not desirable, and can propose to reopen the term. If the proposal passes, we can reopen the term again. This scenario has the potential to cause a permanent DoS. Anyone can offboard this term again without voting. And the SimplePSM will be permanently paused. While it is a rare case and may not occur in real situations, there is no certainty. Therefore, I marked this as a medium because if it were to happen, it could break the protocol entirely.

Proof of Concept

If the proposal to offboard the term reaches the quorum, canOffboard will be true. https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/LendingTermOffboarding.sol#L138-L140

function supportOffboard() {
    if (_weight + userWeight >= quorum) {
        canOffboard[term] = true;
    }
}

Anyone can offboard this term. This increases nOffboardingsInProgress and pauses the SimplePSM. https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/LendingTermOffboarding.sol#L159-L167

function offboard(address term) external whenNotPaused {
    GuildToken(guildToken).removeGauge(term);
    if (nOffboardingsInProgress++ == 0 && !SimplePSM(psm).redemptionsPaused()) {
        SimplePSM(psm).setRedemptionsPaused(true);
    }
}

If the term has open loans, cleaning is not possible at this stage. https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/LendingTermOffboarding.sol#L177-L180

function cleanup(address term) external whenNotPaused {
    require(
        LendingTerm(term).issuance() == 0,
        "LendingTermOffboarding: not all loans closed"
    );
}

Subsequently, we can propose to reopen that term, and it can be reopened without any error. However, we still can't clean the active term.

function cleanup(address term) external whenNotPaused {
    require(
        GuildToken(guildToken).isDeprecatedGauge(term),
        "LendingTermOffboarding: re-onboarded"
    );
}

Afterward, canOffboard remains true for this term, enabling anyone to offboard it without voting. nOffboardingsInProgress increases once again. Even if we can clean this term after some time, nOffboardingsInProgress will never reach 0. Consequently, the SimplePSM will be paused permanently.

The PoC for this is as below:

function testOnboardBeforeCleanup() public {
        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));
        vm.stopPrank();

        assertEq(guild.isGauge(address(term)), false);

        // cannot cleanup because loans are active
        vm.expectRevert("LendingTermOffboarding: not all loans closed");
        offboarder.cleanup(address(term));

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

        // Anybody can offboard without voting.
        offboarder.offboard(address(term));

        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();
        assertEq(offboarder.nOffboardingsInProgress(), 2);
        offboarder.cleanup(address(term));
        // still pause
        assertEq(psm.redemptionsPaused(), true);
        assertEq(offboarder.nOffboardingsInProgress(), 1);
}

Tools Used

Recommended Mitigation Steps

Avoid reopening the term before completing the cleanup process.

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/SurplusGuildMinter.sol#L136 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/ProfitManager.sol#L259-L264

Vulnerability details

Impact

There are multiple markets, and users try to augment the weight of a specific term through the SurplusGuildMinter. To achieve this, they utilize the credit token associated with that term. This action enables them to boost the potential debt ceiling for the specific term and also qualify for rewards.

However, an issue arises when users stake in the ETH term using the gUSDC credit token. While this increases the weight of the term in an entirely different market, it prevents the user from receiving any rewards generated in that term. Consequently, some rewards become trapped in that market, inaccessible for any claim.

In the end, the user can reclaim their gUSDC tokens if the ETH term does not incur any losses. I've marked this as a medium because it violates the protocol, hindering the complete distribution of rewards to guild holders, credit holders, and the surplus buffer.

Proof of Concept

There is no check in place when a user attempts to stake in a term through the SurplusGuildMinter to determine whether that term belongs to this market. https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/SurplusGuildMinter.sol#L136

function stake(address term, uint256 amount) external whenNotPaused {
    ProfitManager(profitManager).donateToTermSurplusBuffer(term, amount);
    GuildToken(guild).incrementGauge(term, guildAmount);
}

For instance, a user can stake in an ETH term using a gUSDC credit token, even if that term is not part of the USDC market. The staked gUSDC credits are then directly transferred to the ProfitManager of the USDC market. https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/ProfitManager.sol#L259-L264

function donateToTermSurplusBuffer(address term, uint256 amount) external {
    CreditToken(credit).transferFrom(msg.sender, address(this), amount);
    uint256 newSurplusBuffer = termSurplusBuffer[term] + amount;     
    termSurplusBuffer[term] = newSurplusBuffer;
}

Clearly, since the ETH term is outside the USDC market, there is no opportunity to utilize these credit tokens in the surplus buffer for this term. This is because the ProfitManager for the USDC market never calls the notifyPnL function with this term. Instead, the ETH term will trigger the notifyPnL function in the ProfitManager for the ETH market.

The gauge weight of this ETH term includes the staked weight using gUSDC credit tokens. When rewards are generated in this term, they should be distributed to all guild holders who contributed to the gauge weight. However, the user who staked using gUSDC cannot receive his rewards. This is due to the fact that the rewards are recorded in the ProfitManager for the ETH market, not the USDC market. Consequently, the rewards for this user remain in the SurplusGuildMinter for the ETH market, and no user can claim these rewards.

In the event of a loss in the ETH term, the user is unable to withdraw gUSDC from the Surplus Buffer for that term in the ProfitManager for the USDC Market because his stake information will be reset. Consequently, the gUSDC credit tokens will be stuck in the ProfitManager for the USDC Market.

The PoC for this is as below:

function testStakeToOtherMarket() public {
        ProfitManager profitManager_1;
        MockERC20 collateral_1;
        CreditToken credit_1;
        GuildToken guild_1;
        RateLimitedMinter rlcm_1;
        AuctionHouse auctionHouse_1;
        LendingTerm term_1;
        SimplePSM psm_1;
        RateLimitedMinter rlgm_1;
        SurplusGuildMinter sgm_1;

        profitManager_1 = new ProfitManager(address(core));
        collateral_1 = new MockERC20();
        credit_1 = new CreditToken(address(core), "name_1", "symbol_1");
        guild_1 = new GuildToken(
            address(core),
            address(profitManager_1)
        );
        rlcm_1 = new RateLimitedMinter(
            address(core) /*_core*/,
            address(credit_1) /*_token*/,
            CoreRoles.RATE_LIMITED_CREDIT_MINTER /*_role*/,
            type(uint256).max /*_maxRateLimitPerSecond*/,
            type(uint128).max /*_rateLimitPerSecond*/,
            type(uint128).max /*_bufferCap*/
        );
        auctionHouse_1 = new AuctionHouse(address(core), 650, 1800);
        term_1 = LendingTerm(Clones.clone(address(new LendingTerm())));
        term_1.initialize(
            address(core),
            LendingTerm.LendingTermReferences({
                profitManager: address(profitManager_1),
                guildToken: address(guild_1),
                auctionHouse: address(auctionHouse_1),
                creditMinter: address(rlcm_1),
                creditToken: address(credit_1)
            }),
            LendingTerm.LendingTermParams({
                collateralToken: address(collateral_1),
                maxDebtPerCollateralToken: 2000e18,
                interestRate: 0.10e18,
                maxDelayBetweenPartialRepay: 63115200, // 2 years
                minPartialRepayPercent: 0.2e18,
                openingFee: 0,
                hardCap: 20_000_000e18
            })
        );
        psm_1 = new SimplePSM(
            address(core),
            address(profitManager_1),
            address(credit_1),
            address(collateral_1)
        );
        rlgm_1 = new RateLimitedMinter(
            address(core), /*_core*/
            address(guild_1), /*_token*/
            CoreRoles.RATE_LIMITED_GUILD_MINTER, /*_role*/
            type(uint256).max, /*_maxRateLimitPerSecond*/
            type(uint128).max, /*_rateLimitPerSecond*/
            type(uint128).max /*_bufferCap*/
        );
        sgm_1 = new SurplusGuildMinter(
            address(core),
            address(profitManager_1),
            address(credit_1),
            address(guild_1),
            address(rlgm_1),
            MINT_RATIO,
            REWARD_RATIO
        );
        vm.startPrank(governor);
        core.grantRole(CoreRoles.CREDIT_MINTER, address(psm_1));
        core.grantRole(CoreRoles.CREDIT_MINTER, address(rlcm_1));
        core.grantRole(CoreRoles.RATE_LIMITED_CREDIT_MINTER, address(term_1));
        core.grantRole(CoreRoles.GAUGE_PNL_NOTIFIER, address(term_1));
        core.grantRole(CoreRoles.GUILD_MINTER, address(rlgm_1));
        core.grantRole(CoreRoles.RATE_LIMITED_GUILD_MINTER, address(sgm_1));
        core.grantRole(CoreRoles.GUILD_SURPLUS_BUFFER_WITHDRAW, address(sgm_1));
        profitManager_1.initializeReferences(address(credit_1), address(guild_1), address(psm_1));
        vm.stopPrank();

        address term_2 = address(2);

        guild_1.setMaxGauges(10);
        // term for USDC market
        guild_1.addGauge(1, address(term_1));
        // term for ETH market
        guild_1.addGauge(2, term_2);

        uint128 X = 100e18;

        // stake
        credit_1.mint(address(this), X);
        credit_1.approve(address(sgm_1), X);
        // We stake in the ETH term using gUSDC.
        sgm_1.stake(term_2, X);
        // Successfully added weight to the ETH term.
        assertGt(guild_1.getUserGaugeWeight(address(sgm_1), term_2), 0); // 200000000000000000000

        uint256 beforeBalance = credit_1.balanceOf(address(this));
        sgm_1.unstake(term_2, X);
        uint256 afterBalance = credit_1.balanceOf(address(this));
        // Successfully unstaked.
        assertEq(afterBalance, beforeBalance + X);
}

Tools Used

Recommended Mitigation Steps

function stake(address term, uint256 amount) external whenNotPaused {
+    require(LendingTerm(term).getReferences().creditToken == credit, "Different Market");
}

Assessed type

Error

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/tokens/GuildToken.sol#L41-L52 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/tokens/GuildToken.sol#L123-L124 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/tokens/GuildToken.sol#L220 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/tokens/GuildToken.sol#L258

Vulnerability details

Impact

The Guild token serves as the governance token for ECG, and there is only one Guild token within the entire protocol. While there may be multiple markets based on underlying tokens, each market has its unique credit token and ProfitManager. However, it's important to note that there is a single ProfitManager associated with the Guild token. In the case of multiple markets, all markets, except one, do not have their specific ProfitManager. This poses a significant risk.

Proof of Concept

In the Guild token, there is a single ProfitManager, and we set it in the constructor. https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/tokens/GuildToken.sol#L41-L52

    address public profitManager;
    constructor(address _core, address _profitManager) {
        profitManager = _profitManager;
    }

Let's consider two markets: one for USDC and another for ETH. The first market has type 1 and 2 terms (term_U1, term_U2). The second market has type 2 and 2 terms (term_E1, term_E2). Certainly, there are distinct ProfitManagers for both markets. Therefore, the ProfitManager in the Guild token is one of these two ProfitManagers. Let's assume it is the ProfitManager for the USDC market.

The term_E1 experiences a loss, prompting the ProfitManager for the ETH market to notify this loss to the Guild token. However, since the ProfitManager in the Guild token is specifically for the USDC market, the transaction will revert. https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/tokens/GuildToken.sol#L123-L124

function notifyGaugeLoss(address gauge) external {
   require(msg.sender == profitManager, "UNAUTHORIZED");
}

Users attempt to adjust gauge weights for ETH terms. Users claim rewards first before making any changes to the weight. This step is essential to ensure that the updated weight does not affect previous rewards. However, since the ProfitManager in the Guild token is designed for the USDC market, it does not update the states for the ETH market. As a result, gauge weights are modified without claiming the previous rewards in ETH terms. This situation implies that users in ETH terms may be able to claim significant funds or potentially incur losses. https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/tokens/GuildToken.sol#L220

function _decrementGaugeWeight() {
    ProfitManager(profitManager).claimGaugeRewards(user, gauge);
}

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

function _incrementGaugeWeight() {
    ProfitManager(profitManager).claimGaugeRewards(user, gauge);
}

Tools Used

Recommended Mitigation Steps

Introduce new variables in the Guild token to store the ProfitManagers, as illustrated below:

mapping(uint256 => address) profitManagers;
mapping(address => bool) isProfitManager;

function notifyGaugeLoss(address gauge) external {
+     require(isProfitManager[msg.sender], "UNAUTHORIZED");
-     require(msg.sender == profitManager, "UNAUTHORIZED");
}

Assessed type

Error

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

[L-9] Any borrower can receive rewards by adding weight to the term before repayment.

When interests accrue from borrowers, these interests are immediately distributed to token holders based on their respective weights. https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/ProfitManager.sol#L396-L399

function notifyPnL() {
    gaugeProfitIndex[gauge] = _gaugeProfitIndex + (amountForGuild * 1e18) / _gaugeWeight;
}

Hence, any borrower can add weight before repayment, receive rewards, and subsequently withdraw their added weight. Need a logic similar to rebasing tokens.

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/tokens/ERC20RebaseDistributor.sol#L338-L339 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/tokens/ERC20RebaseDistributor.sol#L364-L368

Vulnerability details

Impact

Rewards are typically distributed over a 30-day period. If new rewards are introduced during this timeframe, the distribution speed is updated to accommodate both the remaining and new rewards within the next 30 days. However, an issue arises due to the lack of access restrictions on the distribute function. While this is designed to permit ProfitManager to call the function, the absence of access limits allows any user, including potential malicious actors, to manipulate the distribution speed by calling the function with just 1 wei. I marked this as medium due to its potential impact on user expectations, particularly for those who seek timely reward distribution.

Proof of Concept

When new rewards are introduced, the distribute function is invoked, subsequently adjusting the distribution speed to allocate both the existing and newly added rewards over the next 30 days. https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/tokens/ERC20RebaseDistributor.sol#L338-L339

function distribute(uint256 amount) external {
    require(amount != 0, "ERC20RebaseDistributor: cannot distribute zero");

    uint256 endTimestamp = block.timestamp + DISTRIBUTION_PERIOD;
    uint256 newTargetSharePrice = (amount * START_REBASING_SHARE_PRICE +
       __rebasingSharePrice.targetValue * _totalRebasingShares) / _totalRebasingShares;
}

While this function's legitimate use involves calls by ProfitManager in accordance with the protocol's design, a vulnerability arises from its unrestricted access. Any user, including malicious actors, can call this function with a minimal amount (1 wei). Although this action doesn't directly augment a user's rewards, it has the adverse effect of slowing down the rate at which users receive their rewards. The malicious user can exploit this by repeatedly calling the function, causing the originally planned rewards for the initial 30 days to be delayed by several months.

The PoC for this is as below:

function testDistributeAnyAmount() public {
        uint256 distributeAmount = 100e18;

        token.mint(alice, distributeAmount);
        token.mint(address(this), distributeAmount);
        
        vm.prank(alice);
        token.enterRebase();

        token.distribute(distributeAmount);
        vm.warp(block.timestamp + token.DISTRIBUTION_PERIOD() / 2);
        // Receives 50% of the total rewards within half of the period.
        assertEq(token.balanceOf(alice), distributeAmount * 3 / 2);

        token.mint(address(this), 1);
        // Introduce a delay to the distribution speed by utilizing only 1 wei.
        token.distribute(1);

        vm.warp(block.timestamp + token.DISTRIBUTION_PERIOD() / 2);
        // Receive only 75% of the expected value.
        assertEq(token.balanceOf(alice), distributeAmount * 7 / 4);
}

Tools Used

Recommended Mitigation Steps

Restrict access to the distribute function so that only ProfitManager has the capability to invoke this function.

Assessed type

Access Control

Lines of code

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

Vulnerability details

Impact

When a user attempts to reduce gauge weight, the system calculates the potential debt ceiling after removing that weight. If the calculated value is smaller than the current issuance, the user should not be able to reduce the weight. This check is crucial to maintain a balance in available borrow amounts based on the weight proportion. However, in some cases, the debtCeiling function may return the wrong value, allowing the reduction of undesired weight.

Proof of Concept

In the debtCeiling function, we need to return the minimum value among the following three values:

creditMinterBuffer, _debtCeiling, _hardCap

But we can return an intermediate value due to an error. https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/LendingTerm.sol#L324-L330

function debtCeiling(int256 gaugeWeightDelta) { 
    if (creditMinterBuffer < _debtCeiling) {
        return creditMinterBuffer;
    }
    if (_hardCap < _debtCeiling) {
        return _hardCap;
    }
    return _debtCeiling;
}

For example, the hardCap of that term is 2000e18, and the current issuance is 1500e18. The Governor updated the hardCap of that term to 1000e18.

function setHardCap(uint256 _newValue) external onlyCoreRole(CoreRoles.GOVERNOR) {
    params.hardCap = _newValue;
}

One user attempts to reduce gauge weight. Suppose the _debtCeiling is 17e18, and creditMinterBuffer is 16e18. In this case, the debtCeiling function will return 16e18 because creditMinterBuffer is less than _debtCeiling. So the _hardCap is missing in the check. This means that the user can reduce their weight. However, in reality, this term needs to reduce its debt first.

Tools Used

Recommended Mitigation Steps

function debtCeiling(int256 gaugeWeightDelta) { 
    + if (_debtCeiling <= creditMinterBuffer && _debtCeiling <= _hardCap) {
    +     return _debtCeiling;
    + }
    + return _hardCap < creditMinterBuffer ? _hardCap : creditMinterBuffer;

    - if (creditMinterBuffer < _debtCeiling) {
    -     return creditMinterBuffer;
    - }
    - if (_hardCap < _debtCeiling) {
    -     return _hardCap;
    - }
    - return _debtCeiling;
}

Assessed type

Error

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/LendingTerm.sol#L371-L387 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/GuildToken.sol#L224-L231

Vulnerability details

Impact

The lending term includes a debtCeiling function. To reduce weight from the gauge, it is essential to verify that the potential debt ceiling after the reduction is greater than the current issuance. This is crucial for maintaining balance in possible issuances between terms with the same type. However, incorrect calculation of the debt ceiling can impede the possibility of a decrease. I marked this as a medium because it has the potential to be a cause of a DoS issue in some cases.

Proof of Concept

Consider a scenario with two terms in one market, where the first term has a weight of W, and the second term has a weight of 2 * W. The first user is able to borrow up to the minimum of the hard cap of the term and the current available mint amount. For instance, let's say the first user borrowed an amount X from the first term. Now, a second user attempts to borrow the same amount X from the second term. I will now proceed to calculate some values in the _borrow function to illustrate this scenario. https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/LendingTerm.sol#L371-L387

function _borrow() {
    uint256 _issuance = issuance;
    uint256 _postBorrowIssuance = _issuance + borrowAmount;
    uint256 totalBorrowedCredit = ProfitManager(refs.profitManager)
        .totalBorrowedCredit();
    uint256 gaugeWeightTolerance = ProfitManager(refs.profitManager)
         .gaugeWeightTolerance();
    uint256 _debtCeiling = (GuildToken(refs.guildToken)
        .calculateGaugeAllocation(
            address(this),
            totalBorrowedCredit + borrowAmount
        ) * gaugeWeightTolerance) / 1e18;
}

_issuance = 0;
_postBorrowIssuance = X; // borrowAmount = X
totalBorrowedCredit = X; // borrowed amount in the first term
gaugeWeightTolerance = 1.2e18;
_debtCeiling = 1.6X; // (X + X) * (2 * W / (3 * W)) * 1.2

This implies that the second user has the ability to borrow X from the second term.

Now, a user attempts to decrease the gauge weight (W) from the second term. As mentioned earlier, it is crucial to ensure that the potential debt ceiling of the second term, after the decrease, is greater than the current issuance. https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/tokens/GuildToken.sol#L224-L231

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

The debtCeiling function will be called with the parameter -W. https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/LendingTerm.sol#L277-L281

function debtCeiling(int256 gaugeWeightDelta) {
    gaugeWeight = uint256(int256(gaugeWeight) + gaugeWeightDelta);
    uint256 gaugeType = GuildToken(_guildToken).gaugeType(address(this));
    uint256 totalWeight = GuildToken(_guildToken).totalTypeWeight(gaugeType);
    uint256 totalBorrowedCredit = ProfitManager(refs.profitManager).totalBorrowedCredit();
    uint256 gaugeWeightTolerance = ProfitManager(refs.profitManager).gaugeWeightTolerance();
    uint256 toleratedGaugeWeight = (gaugeWeight * gaugeWeightTolerance) / 1e18;
    uint256 debtCeilingBefore = (totalBorrowedCredit * toleratedGaugeWeight) / totalWeight;
    if (_issuance >= debtCeilingBefore) {
        return debtCeilingBefore; // no more borrows allowed
    }
}

I will calculate some values in the process.

gaugeWeight = W; // 2 * W + (-W)
totalWeight = 3 * W; 
totalBorrowedCredit = 2 * X; // from 2 terms
gaugeWeightTolerance = 1.2e18;
toleratedGaugeWeight = 1.2W;
debtCeilingBefore = 0.8X; // 2 * X * 1.2 * W / (3 * W)

The debtCeilingBefore is lower than the current issuance, so the attempt to decrease the gauge weight will revert. However, the total weight after the decrease should be 2 * W, not 3 * W. This is because we only deduct the parameter from the gauge weight of that term. It's evident that the total weight is also reduced when the weight of one term decreases.

If we use the exact total weight (2 * W), debtCeilingBefore will be 1.2X. Consequently, the user's attempt to decrease the gauge weight will succeed.

The PoC for this is as below:

function testDecreaseGaugeWeight() public {
        // create a new lending term
        vm.startPrank(governor);
        LendingTerm newTerm = LendingTerm(Clones.clone(address(new LendingTerm())));
        newTerm.initialize(
            address(core),
            LendingTerm.LendingTermReferences({
                profitManager: address(profitManager),
                guildToken: address(guild),
                auctionHouse: address(auctionHouse),
                creditMinter: address(rlcm),
                creditToken: address(credit)
            }),
            LendingTerm.LendingTermParams({
                collateralToken: address(collateral),
                maxDebtPerCollateralToken: _CREDIT_PER_COLLATERAL_TOKEN,
                interestRate: _INTEREST_RATE,
                maxDelayBetweenPartialRepay: _MAX_DELAY_BETWEEN_PARTIAL_REPAY,
                minPartialRepayPercent: _MIN_PARTIAL_REPAY_PERCENT,
                openingFee: 0,
                hardCap: _HARDCAP
            })
        );
        core.grantRole(CoreRoles.RATE_LIMITED_CREDIT_MINTER, address(newTerm));
        core.grantRole(CoreRoles.GAUGE_PNL_NOTIFIER, address(newTerm));
        vm.stopPrank();

        // There are two terms in this market.
        guild.addGauge(1, address(newTerm));

        uint256 W = _HARDCAP;

        guild.mint(address(this), 2 * W);
        guild.incrementGauge(address(newTerm), 2 * W);

        // The weight of this term is 2 * W.
        assertEq(guild.getGaugeWeight(address(newTerm)), 2 * W);
        // Total weight is 3 * W = W(term) + 2 * W(newTerm)
        assertEq(guild.totalTypeWeight(1), 3 * W);

        uint128 X = 100e18;
        collateral.mint(address(this), X);
        collateral.approve(address(term), X);
        term.borrow(X, X);

        collateral.mint(address(this), X);
        collateral.approve(address(newTerm), X);
        newTerm.borrow(X, X);

        // It is not possible to decrease the weight.
        vm.expectRevert("GuildToken: debt ceiling used");
        guild.decrementGauge(address(newTerm), W);
}

Tools Used

Recommended Mitigation Steps

function debtCeiling() {
-     uint256 totalWeight = GuildToken(_guildToken).totalTypeWeight(
-        gaugeType
-     );

+     uint256 totalWeight = uint256(int256(GuildToken(_guildToken).totalTypeWeight(
+         gaugeType
+     )) + gaugeWeightDelta);
}

Assessed type

Error

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

[L-10] There is a potential for underflow in the decreaseUnmintedRebaseRewards function within the ERC20RebaseDistributor token.

When updating shares, we adjust the share price accordingly. https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/tokens/ERC20RebaseDistributor.sol#L213-L217

function updateTotalRebasingShares() {
    uint256 delta = uint256(val.targetValue) - currentRebasingSharePrice;
    if (delta != 0) {
        uint256 percentChange = (sharesAfter * START_REBASING_SHARE_PRICE) / sharesBefore;
        uint256 targetNewSharePrice = currentRebasingSharePrice + (delta * START_REBASING_SHARE_PRICE) / percentChange;
}

No need to calculate percentChange; the updated price will be as follows:

targetNewSharePrice = currentRebasingSharePrice + delta * sharesBefore / sharesAfter```

Due to rounding, the calculated price may be slightly larger. Consequently, when attempting to decrease unminted rewards, there is a risk of underflow.

function decreaseUnmintedRebaseRewards(uint256 amount) internal {
    lastValue: SafeCastLib.safeCastTo224(
        _unmintedRebaseRewards - amount
    ), // adjusted current
}

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

[L-8] Please use targetTotalSupply instead of totalSupply.

When applying loss, we calculate the newCreditMultiplier based on the loss and total supply. https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/ProfitManager.sol#L331-L333

function notifyPnL() {
    uint256 creditTotalSupply = CreditToken(_credit).totalSupply();
    uint256 newCreditMultiplier = (creditMultiplier *
        (creditTotalSupply - loss)) / creditTotalSupply;
}

If there are unminted tokens, the correct total supply should be targetTotalSupply.

[L-1] Loss-experienced gauge can prevent the transfer of tokens.

When a user attempts to transfer tokens and there are insufficient free tokens, the system gradually decreases the weight from the gauges one by one until a sufficient number of free tokens becomes available. https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/tokens/ERC20Gauges.sol#L524

function _decrementWeightUntilFree(address user, uint256 weight) internal {
    for () {
        _decrementGaugeWeight(user, gauge, userGaugeWeight);
    }
}

If the gauge experiences any loss, and the user has assigned weight to this gauge, it indicates that the user has not applied loss yet. Therefore, the below function will always revert in such cases. https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/tokens/GuildToken.sol#L214-L217

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

}

Actually, the guild tokens that were added to the loss-experienced gauges are not considered free tokens. When a user applies loss, these tokens are removed from the user, and they do not contribute to increasing the free tokens required for a transfer. Therefore, in the _decrementWeightUntilFree function, it is necessary to skip the loss-experience gauges. This action helps prevent undesired DoS scenarios in token transfers.

I am uncertain whether this falls into the category of medium risk.

[L-2] We are using tokens in the Surplus buffer of a term only for one loss that originates from that specific term.

Users have the option to select a healthy term and stake in that term using SurplusGuildMinter. Subsequently, the credit tokens of these users are contributed to the Surplus Buffer for that specific term. https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/SurplusGuildMinter.sol#L130

function stake(address term, uint256 amount) external whenNotPaused {
    ProfitManager(profitManager).donateToTermSurplusBuffer(term, amount);
}

The accumulated amounts can be substantial, given that many users stake in the same term. However, even a minor loss in that term can lead to these amounts being moved from the Surplus Buffer for that term and made available for use across all terms. https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/ProfitManager.sol#L309-L313

function notifyPnL() {
    if (_termSurplusBuffer != 0) {
        termSurplusBuffer[gauge] = 0;
        _surplusBuffer += _termSurplusBuffer;
    }
}

Additionally, users are unable to withdraw their credit tokens once staked. I guess the idea behind this solution is likely to simplify the process of redistributing staked credit tokens in SurplusGuildMinter when users decide to unstake. However, it may be feasible to explore redistribution based on the proportion of their stake amounts.

In the event of a loss occurring in that term, if the surplus buffer contains enough tokens, we can straightforwardly deduct the loss from it. This proposed modification could enhance the protocol in alignment with user preferences.

[L-3] The weight of a deprecated gauge is excluded from the debt ceiling calculation, but the issuance of a deprecated gauge is still taken into account in the debt ceiling calculation.

When a gauge is removed, its weight is subtracted from the total weight. As a result, it does not impact the debt ceiling calculation. https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/tokens/ERC20Gauges.sol#L434-L437

function _removeGauge(address gauge) internal {
    uint256 weight = getGaugeWeight[gauge];  
    if (weight != 0) {
        totalTypeWeight[gaugeType[gauge]] -= weight;
        totalWeight -= weight;
    }
}

However, the amount already issued from the deprecated gauge continues to influence the debt ceiling calculation. https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/LendingTerm.sol#L294-L295

function debtCeiling() {
    uint256 totalBorrowedCredit = ProfitManager(refs.profitManager).totalBorrowedCredit();
}

The same applies to the _borrow function. https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/LendingTerm.sol#L379-L380

function _borrow() {
    uint256 totalBorrowedCredit = ProfitManager(refs.profitManager).totalBorrowedCredit();
}

It has the potential to disrupt the balance in the possible issuance amount across terms.

[L-4] It is possible to delegate 0 tokens or add 0 gauge weight

Any user can delegate 0 tokens to others. There is no check to verify whether the delegated amount is 0 or not. https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/tokens/ERC20MultiVotes.sol#L264-L269

function incrementDelegation(address delegatee, uint256 amount) public virtual {
    _incrementDelegation(msg.sender, delegatee, amount);
}

There is a capped limit on the amount a user can delegate. When a user delegates 0 tokens, it signifies that they are not delegating, but it is still recorded as the user utilizing one possible delegation https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/tokens/ERC20MultiVotes.sol#L329

function _incrementDelegation() {
    bool newDelegate = _delegates[delegator].add(delegatee); 
}

The same logic applies to the addition of gauge weight. We should include a check for a 0 amount.

[L-5] Enhance the _decrementWeightUntilFree function.

To ensure a sufficient number of free tokens for a transfer, we reduce the weight from gauges one by one. https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/tokens/ERC20Gauges.sol#L521-L535

function _decrementWeightUntilFree(address user, uint256 weight) internal {
    for (uint256 i = 0; i < size && (userFreeWeight + userFreed) < weight;) {
        uint256 userGaugeWeight = getUserGaugeWeight[user][gauge];
        _decrementGaugeWeight(user, gauge, userGaugeWeight);
    }
}

However, in this process, we have the opportunity to skip certain gauges. In some gauges, attempting to reduce the weight may result in a revert due to the issuance check. https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/tokens/GuildToken.sol#L225-L230

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

We can obviously skip these gauges. Moreover, it is preferable to remove the weight from the deprecated gauges.

[L-6] SimplePSM is not compatible with tokens whose decimals exceed 18.

If the decimals of _pegToken exceed 18, deploying SimplePSM for that token is not feasible. https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/SimplePSM.sol#L75-L76

constructor() {
    uint256 decimals = uint256(ERC20(_pegToken).decimals());
    decimalCorrection = 10 ** (18 - decimals);
}

[L-7] When the shares become 0, any unminted rewards will be lost.

In ERC20RebaseDistributor, when the shares become 0, all values associated with the unminted rewards and distribution speed are reset. https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/tokens/ERC20RebaseDistributor.sol#L190-L201

function updateTotalRebasingShares() {
    if (sharesAfter == 0) {
        __rebasingSharePrice = InterpolatedValue({}); // reset
        __unmintedRebaseRewards = InterpolatedValue({}); // reset       
    }
}

[L-8] Please use targetTotalSupply instead of totalSupply.

When applying loss, we calculate the newCreditMultiplier based on the loss and total supply. https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/ProfitManager.sol#L331-L333

function notifyPnL() {
    uint256 creditTotalSupply = CreditToken(_credit).totalSupply();
    uint256 newCreditMultiplier = (creditMultiplier *
        (creditTotalSupply - loss)) / creditTotalSupply;
}

If there are unminted tokens, the correct total supply should be targetTotalSupply.

[L-9] Any borrower can receive rewards by adding weight to the term before repayment.

When interests accrue from borrowers, these interests are immediately distributed to token holders based on their respective weights. https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/ProfitManager.sol#L396-L399

function notifyPnL() {
    gaugeProfitIndex[gauge] = _gaugeProfitIndex + (amountForGuild * 1e18) / _gaugeWeight;
}

Hence, any borrower can add weight before repayment, receive rewards, and subsequently withdraw their added weight. Need a logic similar to rebasing tokens.

[L-10] There is a potential for underflow in the decreaseUnmintedRebaseRewards function within the ERC20RebaseDistributor token.

When updating shares, we adjust the share price accordingly. https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/tokens/ERC20RebaseDistributor.sol#L213-L217

function updateTotalRebasingShares() {
    uint256 delta = uint256(val.targetValue) - currentRebasingSharePrice;
    if (delta != 0) {
        uint256 percentChange = (sharesAfter * START_REBASING_SHARE_PRICE) / sharesBefore;
        uint256 targetNewSharePrice = currentRebasingSharePrice + (delta * START_REBASING_SHARE_PRICE) / percentChange;
}

No need to calculate percentChange; the updated price will be as follows:

targetNewSharePrice = currentRebasingSharePrice + delta * sharesBefore / sharesAfter```

Due to rounding, the calculated price may be slightly larger. Consequently, when attempting to decrease unminted rewards, there is a risk of underflow.

function decreaseUnmintedRebaseRewards(uint256 amount) internal {
    lastValue: SafeCastLib.safeCastTo224(
        _unmintedRebaseRewards - amount
    ), // adjusted current
}