Ethereum Credit Guild - Aymen0909's results

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/loan/LendingTerm.sol#L634-L675 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/loan/LendingTerm.sol#L241

Vulnerability details

Impact

When a lending term doesn't utilize partial repayment, indicated by maxDelayBetweenPartialRepay == 0, borrowers lack the incentive to repay the accrued interest on their loan. This is because they cannot be liquidated (called) unless the entire gauge is deprecated. Such a situation may lead to malicious users borrowing from an honest gauge and refusing to repay the loan. Consequently, the term must be forced to get offboarded to liquidate the unsolvent loan. However, this action puts all other loans (that are solvent) at risk of liquidation, resulting in the loss of collateral for honest borrowers.

Proof of Concept

The vulnerability emerges in lending terms without a partial repayment delay for loans, represented by a term having maxDelayBetweenPartialRepay == 0. In such terms, a borrower's loan can only be called (liquidated in auction) if the entire term is offboarded (deprecated). This limitation arises due to a check within the _call function, allowing a call for a loan only if it missed partial repayment or if the term is deprecated:

function _call(
    address caller,
    bytes32 loanId,
    address _auctionHouse
) internal {
    // ...

    // check that the loan can be called
    //@audit will not allow a call if an active term has no partial repayments
    require(
        GuildToken(refs.guildToken).isDeprecatedGauge(address(this)) ||
            partialRepayDelayPassed(loanId),
        "LendingTerm: cannot call"
    );

    ...
}

It is evident from the code that partialRepayDelayPassed(loanId) always returns false for terms without partial repayments enabled (maxDelayBetweenPartialRepay == 0). Thus, the only way to call a loan in this term is by deprecating the entire gauge/term.

In normal conditions, honest borrowers prevent the lending term they borrowed from getting deprecated to retain their collateral funds, ensuring they repay their loans. However, a malicious user can exploit this by borrowing from a term without partial repayments and never repaying the loan. This allows bad debt to accumulate, and no one can call/liquidate the loan until a vote is organized to offboard (deprecate) the term, leading to the liquidation of collateral funds for honest borrowers.

The result of this issue is that honest borrowers in terms without partial repayment delay are always at risk of seeing the term they borrow from get deprecated because of such malicious action, and this may make such term not usable until the protocol introduces a solution to this scenario.

This scenario is possible because, unlike other DEFI lending/borrowing protocols, this protocol lacks a health factor that can be assigned to each loan to determine if a loan is liquidatable or not, especially when the accrued debt surpasses the collateral deposited.

Tools Used

Manual review

Recommended Mitigation Steps

One solution to address this issue is to associate each loan opened in a term with a health factor (ratio between accrued debt and collateral deposited). This way, any loan falling below the health threshold (set by the protocol) can be called (liquidated), thereby avoiding the aforementioned scenario.

Assessed type

Context

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/loan/LendingTerm.sol#L323-L330 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/tokens/GuildToken.sol#L226-L230

Vulnerability details

Impact

The debtCeiling function contains a flaw in the return statement logic, specifically, if _hardCap is less than creditMinterBuffer and both are less than _debtCeiling, the function may not return the expected minimum value, affecting the behavior of debt ceiling calculations. As a result, the debt ceiling may not be accurately calculated, potentially allowing the system to exceed intended limits and will lead to incorrect behavior when the debtCeiling function is called by GuildToken._decrementGaugeWeight, impacting the overall stability and security of the protocol.

Proof of Concept

The vulnerability is demonstrated by the inadequacy of the return statement when comparing _hardCap, creditMinterBuffer, and _debtCeiling. The code snippet below highlights the potential issue:

function debtCeiling(
    int256 gaugeWeightDelta
) public view returns (uint256) {
    // ...

    uint256 _debtCeiling = _issuance + maxBorrow;
    // return min(creditMinterBuffer, hardCap, debtCeiling)
    //@audit will not return the min value as expected if _hardCap < creditMinterBuffer < _debtCeiling
    if (creditMinterBuffer < _debtCeiling) {
        return creditMinterBuffer;
    }
    if (_hardCap < _debtCeiling) {
        return _hardCap;
    }
    return _debtCeiling;
}

The issue lies in the fact that the function does not return the minimum value when _hardCap is less than creditMinterBuffer and both are less than _debtCeiling, in that case instead of returning _hardCap as the maximum debt value the function returns creditMinterBuffer which allow higher debt. This unexpected result may lead to the wrong behaviour when the function is called inside GuildToken._decrementGaugeWeight, potentially causing the expected debt ceiling to be surpassed.

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

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

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

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

Tools Used

Manual review

Recommended Mitigation Steps

To address this issue, ensure that the debtCeiling function returns the minimum value correctly when comparing _hardCap, creditMinterBuffer, and _debtCeiling. Modify the return statements as follows:

// ...

//@audit return correct debt ceiling value
uint256 minDebtCeiling = creditMinterBuffer < _hardCap ? creditMinterBuffer : _hardCap;
return minDebtCeiling < _debtCeiling ? minDebtCeiling : _debtCeiling;

Assessed type

Context

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/tokens/ERC20Gauges.sol#L515-L536

Vulnerability details

Impact

The _decrementWeightUntilFree function contains a logic flaw that may result in an incorrect calculation of the totalWeight and totalTypeWeight values. This issue can lead to inaccurate representations of the user's free weight and the overall system's total weight, potentially affecting the proper functioning of the gauge system.

Proof of Concept

The vulnerability arises from the placement of the unchecked { ++i; } statement within the if statement block inside the for loop, causing the loop to potentially skip certain iterations and leading to an incomplete update of the totalWeight variable. This can result in an inconsistent state where the total weight is not accurately reflecting the actual weights of the gauges, impacting the reliability of the gauge system.

function _decrementWeightUntilFree(address user, uint256 weight) internal {
    // ...
    for (
        uint256 i = 0;
        i < size && (userFreeWeight + userFreed) < weight;

    ) {
        address gauge = gaugeList[i];
        uint256 userGaugeWeight = getUserGaugeWeight[user][gauge];
        if (userGaugeWeight != 0) {
            userFreed += userGaugeWeight;
            _decrementGaugeWeight(user, gauge, userGaugeWeight);

            // If the gauge is live (not deprecated), include its weight in the total to remove
            if (!_deprecatedGauges.contains(gauge)) {
                totalTypeWeight[gaugeType[gauge]] -= userGaugeWeight;
                totalFreed += userGaugeWeight;
            }

            //@audit should be placed outside of the if statement
            unchecked {
                ++i;
            }
        }
    }

    totalWeight -= totalFreed;
}

By moving the unchecked { ++i; } statement outside the if statement, the loop will increment properly, ensuring that all user gauges are properly processed and the totalWeight is updated accurately.

Tools Used

Manual review

Recommended Mitigation Steps

To address this issue, relocate the unchecked { ++i; } statement outside the if statement to ensure that the for loop increments correctly, allowing all relevant user gauges to be processed.

Assessed type

Loop