Ethereum Credit Guild - nonseodion's results

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/LendingTermOffboarding.sol#L153-L199

Vulnerability details

Description

Lending terms can be deprecated and added back to the ECG system. This can be done through a voting process on the LendingTermOffboarding contract or by the GAUGE_REMOVE and GAUGE_ADD core roles on the Guild token contract.

LendingTermOffboarding contract also comes with a function called cleanup that is used to revoke the lending term's roles after all its loans have been repaid.

function cleanup(address term) external whenNotPaused {
    require(canOffboard[term], "LendingTermOffboarding: quorum not met");
    require(
        LendingTerm(term).issuance() == 0,
        "LendingTermOffboarding: not all loans closed"
    );
    require(
        GuildToken(guildToken).isDeprecatedGauge(term),
        "LendingTermOffboarding: re-onboarded"
    );

    // update protocol config
    core().revokeRole(CoreRoles.RATE_LIMITED_CREDIT_MINTER, term);
    core().revokeRole(CoreRoles.GAUGE_PNL_NOTIFIER, term);

    // unpause psm redemptions
    if (
        --nOffboardingsInProgress == 0 && SimplePSM(psm).redemptionsPaused()
    ) {
        SimplePSM(psm).setRedemptionsPaused(false);
    }

    canOffboard[term] = false;
    emit Cleanup(block.timestamp, term);
}

The cleanup function will revert if a term already offboarded is onboarded back before it is called. This is enforced in the third require statement of the function. It also requires canOffboard[term] to be true in the first require statement. The issue is that the offboard function also relies on canOffboard[term] to be true to offboard a loan.

function offboard(address term) external whenNotPaused {
    require(canOffboard[term], "LendingTermOffboarding: quorum not met");

    // update protocol config
    // this will revert if the term has already been offboarded
    // through another mean.
    GuildToken(guildToken).removeGauge(term);

    // pause psm redemptions
    if (
        nOffboardingsInProgress++ == 0 &&
        !SimplePSM(psm).redemptionsPaused()
    ) {
        SimplePSM(psm).setRedemptionsPaused(true);
    }

    emit Offboard(block.timestamp, term);
}

If a lending term is re-onboarded. before cleanup is called, cleanup can no longer be called to reset canOffboard[term], and anyone can call offboard to deprecate the loan since canOffboard[term] will be true. This will allow the malicious user to call on all the loans in the term and put them up for auction.

It is important to note that onboarding an offboarded term is an expected function of the protocol regardless of whether it is cleaned up or not.

Impact

All the loans in a lending term can be forcefully put up for auction.

Proof of Concept

The POC shows Bob deprecating a gauge offboarded by LendingTermOffboarding but re-onboarded before cleanup could be called. The test can be run in LendingTermOffboarding.t.sol.

The code can be run in LendingTermOffboarding.t.sol.

function testAnyoneCanOffboardLoan() public {
    // prepare
    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();

    // gauge is added back by CoreRoles.GAUGE_ADD
    guild.addGauge(1, address(term));

    assertFalse(guild.isDeprecatedGauge(address(term)));
    // Bad boy Bob deprecates gauge
    vm.startPrank(bob);
    offboarder.offboard(address(term));
    assertTrue(guild.isDeprecatedGauge(address(term)));
    // Bad boy Bob closes Alice's loans (the loans were added in setup)
    term.call(aliceLoanId);
    assertEq(auctionHouse.getAuction(aliceLoanId).startTime, block.timestamp);
    vm.stopPrank();
}

Tools Used

Manual Analysis

Recommended Mitigation Steps

The offboard function should update canOffboard[term] to false immediately after offboarding the term. It should also be updated to check if the gauge has already been deprecated and update canOffboard[term] to false before returning.

The cleanup function should be updated to rely on checking if the term to be cleaned up is deprecated and has zero issuance before cleaning it up. This also comes with the added advantage of allowing a loan to be cleaned up even if it wasn't offboarded using the LendingTermOffboarding contract.

Assessed type

Access Control

Lines of code

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

Vulnerability details

Description

The debtCeiling function in LendingTerm contract should never return a value higher than hardCap. The hardcap is the maximum amount of debt the lending term can issue at any given time. It may be lesser than the current issuance of CREDIT by the lending term.

    /// @notice the absolute maximum amount of debt this lending term can issue
    /// at any given time, regardless of the gauge allocations.
    uint256 hardCap;

The last section of the debt ceiling function is meant to return a minimum between creditMinterBuffer, hardCap and debtCeiling as stated by the comment. But the first if statement for the implementation will return creditMinterBuffer if it is less than debtCeiling without considering the value of hardCap. This allows creditMinterBuffer to be returned even if it is greater than hardCap.

function debtCeiling(
        int256 gaugeWeightDelta
    ) public view returns (uint256) {
        ...
        // return min(creditMinterBuffer, hardCap, debtCeiling)
        // @audit-issue does not check the correct minimum between creditMinterBuffer, hardCap, debtCeiling
        if (creditMinterBuffer < _debtCeiling) {
            return creditMinterBuffer;
        }
        if (_hardCap < _debtCeiling) {
            return _hardCap;
        }
        return _debtCeiling;
    }

Impact

This will allow Guild voters to decrease their stake in a gauge that has its hardCap less than its issuance and the creditMinterBuffer greater than its issuance ( hardCap < issuance < creditMinterBuffer) when the call to debtCeiling returns creditMinterBuffer instead of hardcap. Check the require statement from _decrementGaugeWeight function below.

function _decrementGaugeWeight(
        address user,
        address gauge,
        uint256 weight
    ) internal override {
        ...
            // @audit what's stops users from borrowing to lock up a position (frontrun) anytime a GUILD holder wants to reduce weight
            require(
                issuance <= debtCeilingAfterDecrement,
                "GuildToken: debt ceiling used"
            );
        }

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

Proof of Concept

In this POC we make debtCeiling return a debtCeiling greater than the hardCap.

function testDebtCeiling() public {
    vm.prank(governor);
    rlcm.setBufferCap(uint128(_HARDCAP * 2));
    guild.mint(address(this), _HARDCAP * 3);

    // create a new term and borrow with it to increase totalBorrowCredit
    LendingTerm term2 = LendingTerm(Clones.clone(address(new LendingTerm())));
    guild.addGauge(1, address(term2));
    guild.incrementGauge(address(term2), _HARDCAP);
    term2.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
        })
    );
    vm.startPrank(governor);
    core.grantRole(CoreRoles.RATE_LIMITED_CREDIT_MINTER, address(term2));
    core.grantRole(CoreRoles.GAUGE_PNL_NOTIFIER, address(term2));
    vm.stopPrank();

    uint256 borrowAmount = profitManager.minBorrow();
    uint256 collateralAmount = 12e18;

    // borrow with the new term
    collateral.mint(address(this), collateralAmount * 2);
    collateral.approve(address(term2), collateralAmount * 2);
    term2.borrow(borrowAmount, collateralAmount);

    uint debtCeiling = term.debtCeiling();
    uint128 bufferCap = uint128(debtCeiling - 99);
    uint256 hardCap = debtCeiling - 999;

    // set hardcap to be less than buffer cap
    vm.startPrank(governor);        
    term.setHardCap(hardCap);
    rlcm.setBufferCap(bufferCap);
    vm.stopPrank();
    
    // debt ceiling returned is greater than hardcap
    assertGt(rlcm.bufferCap(), term.getParameters().hardCap);
    assertEq(term.debtCeiling(), rlcm.bufferCap());
}

Tools Used

Manual Analysis

Recommended Mitigation Steps

Compare _debtCeiling with _hardCap and return _hardCap if it is less before comparing with creditMinterBuffer.

Assessed type

Other