Ethereum Credit Guild - Sathish9098's results

QA Report

[L-1] Hardcoded POLL_DURATION_BLOCKS cause problems in other chains

As per protocol docs its also possible to deploy in Arbitrum

Deployment: we anticipate to launch on Ethereum mainnet & L2s like Arbitrum.

Ethereum Mainnet: The average block time on Ethereum mainnet has historically been around 13-15 seconds. Using 13 seconds as an average is a common approximation for Ethereum mainnet.

Arbitrum (Layer 2): Layer 2 solutions, like Arbitrum, often have different block times compared to Ethereum mainnet. Arbitrum, for instance, doesn't have a fixed block time and can be significantly faster due to its off-chain processing before batches are submitted to Ethereum.

Given these differences, the hardcoded value of POLL_DURATION_BLOCKS based on a 13-second block time may not be appropriate for all networks where the contract will be deployed:

On Ethereum mainnet, the current implementation is accurate. On Arbitrum or similar Layer 2 networks, the block time can vary and may not align with the 13-second average. Therefore, the duration of polls in terms of real-time could be significantly different from the intended 7 days.

FILE: 2023-12-ethereumcreditguild/src/governance/LendingTermOffboarding.sol

36: uint256 public constant POLL_DURATION_BLOCKS = 46523; // ~7 days @ 13s/block

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

Recommended Mitigation

Consider implementing a mechanism to dynamically adjust POLL_DURATION_BLOCKS based on the actual block time of the network where the contract is deployed. This could be done via an oracle or other means to fetch the average block time for the network.

[L-2] Insufficient Access Control in 'forgive()' Function of AuctionHouse Contract

The function forgive() lacks sufficient access control measures, making it potentially exploitable. Any external caller can trigger the forgive() function without adequate validation of their authority or role. This oversight could lead to unauthorized closure of auctions and forgiveness of loans, impacting the protocol's financial integrity.

FILE: 2023-12-ethereumcreditguild/src/loan/AuctionHouse.sol

/// @notice forgive a loan, by marking the debt as a total loss
    /// @dev this is meant to be used when an auction concludes without anyone bidding,
    /// even if 0 CREDIT is asked in return. This situation could arise
    /// if collateral assets are frozen within the lending term contract.
    function forgive(bytes32 loanId) external {
        // this view function will revert if the auction is not started,
        // or if the auction is already ended.
        (, uint256 creditAsked) = getBidDetail(loanId);
        require(creditAsked == 0, "AuctionHouse: ongoing auction");

        // close the auction in state
        auctions[loanId].endTime = block.timestamp;
        nAuctionsInProgress--;

        // notify LendingTerm of auction result
        address _lendingTerm = auctions[loanId].lendingTerm;
        LendingTerm(_lendingTerm).onBid(
            loanId,
            msg.sender,
            0, // collateralToBorrower
            0, // collateralToBidder
            0 // creditFromBidder
        );

        // emit event
        emit AuctionEnd(
            block.timestamp,
            loanId,
            LendingTerm(_lendingTerm).collateralToken(),
            0, // collateralSold
            0 // debtRecovered
        );
    }

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/AuctionHouse.sol#L198-L230

Recommended Mitigation

Add access control like CoreRoles.GOVERNOR

[L-3] Evaluating the restrictiveness of same-block loan call and auction start checks

The check loan.callTime == block.timestamp assumes that the loan call and the auction start transaction occur in the same block. In reality, due to network congestion or transaction ordering, these two events could be processed in different blocks, even if the transactions are broadcast back-to-back by the user.

If the auction start transaction does not get included in the same block as the loan call (for reasons like gas price competition, block fullness, etc.), it will fail. This could lead to increased transaction costs and operational delays.

FILE: 2023-12-ethereumcreditguild/src/loan/AuctionHouse.sol

require(
            loan.callTime == block.timestamp,
            "AuctionHouse: loan previously called"
        );

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/AuctionHouse.sol#L84-L87

Recommended Mitigation

Instead of requiring the auction to start in the same block as the loan call, allow a reasonable time buffer (e.g., a few blocks or a fixed time period like 15 minutes).

Where BUFFER_PERIOD is the permissible time delay.

+ require(
+            loan.callTime + BUFFER_PERIOD >= block.timestamp,
+            "AuctionHouse: loan previously called"
 +       ); 

[L-4] CoreRoles.GAUGE_PNL_NOTIFIER can initiate the auction even particular loanId does not go underwater

The primary risk here is that any entity with the GAUGE_PNL_NOTIFIER role can initiate an auction for any loan, regardless of its association with that specific loan.

If the GAUGE_PNL_NOTIFIER role is not tightly controlled or if it's assigned to entities that manage multiple loans or different aspects of the system, there's a risk of auctions being initiated inappropriately, either accidentally or maliciously.

FILE: 2023-12-ethereumcreditguild/src/loan/AuctionHouse.sol

function startAuction(bytes32 loanId, uint256 callDebt) external {
        // check that caller is a lending term that still has PnL reporting role
        require(
            core().hasRole(CoreRoles.GAUGE_PNL_NOTIFIER, msg.sender),
            "AuctionHouse: invalid caller"
        );

        // check the loan exists in calling lending term and has been called in the current block
        LendingTerm.Loan memory loan = LendingTerm(msg.sender).getLoan(loanId);
        require(
            loan.callTime == block.timestamp,
            "AuctionHouse: loan previously called"
        );

        // check auction for this loan has not already been created
        require(
            auctions[loanId].startTime == 0,
            "AuctionHouse: auction exists"
        );

        // save auction in state
        auctions[loanId] = Auction({
            startTime: block.timestamp,
            endTime: 0,
            lendingTerm: msg.sender,
            collateralAmount: loan.collateralAmount,
            callDebt: callDebt
        });
        nAuctionsInProgress++;

        // emit event
        emit AuctionStart(
            block.timestamp,
            loanId,
            LendingTerm(msg.sender).collateralToken(),
            loan.collateralAmount,
            callDebt
        );
    }

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/AuctionHouse.sol#L75-L113

[L-5] Potential address collision risk with the loan ID generation in the _borrow function

The function could reject the creation of a new loan if it detects that the generated loan ID already exists, even though it's a new loan request.

If the same borrower requests multiple loans within the same block, there is a theoretical risk of generating the same loan ID. This risk arises because block.timestamp remains constant for all transactions within a single block.

In blockchain transactions, the block timestamp doesn't change from transaction to transaction within the same block. Hence, if a borrower initiates multiple loan requests in transactions that are included in the same block, the only differing factor in the hash function would be the borrower address, which remains the same.

Even a small chance of ID collision could pose a significant risk in financial contexts, as it affects the integrity and reliability of the loan management system.

FILE: 2023-12-ethereumcreditguild/src/loan
/LendingTerm.sol

 loanId = keccak256(
            abi.encode(borrower, address(this), block.timestamp)
        );

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

[L-6] All pending transactions become DOS if borrower borrows all issuance from particular LendingTerm

A potential Denial-of-Service (DoS) attack on the lending platform, specifically targeting the borrow function in conjunction with the hardcap limit.

The borrow function allows a borrower to take out a loan, provided the total amount borrowed (_postBorrowIssuance) does not exceed the platform's hardcap (params.hardCap). There is no maximum limit for

An attacker (or a legitimate borrower with enough resources) could potentially borrow an amount that brings the total issuance right up to, or very close to, the hardcap. This would be done by submitting a borrow transaction with a high borrowAmount that is just below the remaining space under the hardcap.

Once the hardcap is nearly reached or reached, any pending transactions in the mempool that also attempt to borrow funds would fail. These transactions would be denied service because they would cause the total borrowed amount to exceed the hardcap, thus failing the check _postBorrowIssuance <= params.hardCap.

Borrowers with pending transactions (who may have submitted their requests before the attacker but with lower gas fees) find their transactions failing. The platform effectively experiences a DoS situation for new borrow requests until the total borrowed amount falls below the hardcap again.

FILE: 2023-12-ethereumcreditguild/src/loan/LendingTerm.sol

/// @notice initiate a new loan
    function _borrow(
        address borrower,
        uint256 borrowAmount,
        uint256 collateralAmount
    ) internal returns (bytes32 loanId) {
        require(borrowAmount != 0, "LendingTerm: cannot borrow 0");
        require(collateralAmount != 0, "LendingTerm: cannot stake 0");

        loanId = keccak256(
            abi.encode(borrower, address(this), block.timestamp)
        );

        // check that the loan doesn't already exist
        require(loans[loanId].borrowTime == 0, "LendingTerm: loan exists");

        // check that enough collateral is provided
        uint256 creditMultiplier = ProfitManager(refs.profitManager)
            .creditMultiplier();
        uint256 maxBorrow = (collateralAmount *
            params.maxDebtPerCollateralToken) / creditMultiplier;
        require(
            borrowAmount <= maxBorrow,
            "LendingTerm: not enough collateral"
        );

        // check that enough CREDIT is borrowed
        require(
            borrowAmount >= ProfitManager(refs.profitManager).minBorrow(),
            "LendingTerm: borrow amount too low"
        );

        // check the hardcap
        uint256 _issuance = issuance;
        uint256 _postBorrowIssuance = _issuance + borrowAmount;
        require(
            _postBorrowIssuance <= params.hardCap,
            "LendingTerm: hardcap reached"
        );

        // check the debt ceiling
        uint256 totalBorrowedCredit = ProfitManager(refs.profitManager)
            .totalBorrowedCredit();
        uint256 gaugeWeightTolerance = ProfitManager(refs.profitManager)
            .gaugeWeightTolerance();
        uint256 _debtCeiling = (GuildToken(refs.guildToken)
            .calculateGaugeAllocation(
                address(this),
                totalBorrowedCredit + borrowAmount
            ) * gaugeWeightTolerance) / 1e18;
        if (totalBorrowedCredit == 0) {
            // if the lending term is deprecated, `calculateGaugeAllocation` will return 0, and the borrow
            // should revert because the debt ceiling is reached (no borrows should be allowed anymore).
            // first borrow in the system does not check proportions of issuance, just that the term is not deprecated.
            require(_debtCeiling != 0, "LendingTerm: debt ceiling reached");
        } else {
            require(
                _postBorrowIssuance <= _debtCeiling,
                "LendingTerm: debt ceiling reached"
            );
        }

        // save loan in state
        loans[loanId] = Loan({
            borrower: borrower,
            borrowTime: block.timestamp,
            borrowAmount: borrowAmount,
            borrowCreditMultiplier: creditMultiplier,
            collateralAmount: collateralAmount,
            caller: address(0),
            callTime: 0,
            callDebt: 0,
            closeTime: 0
        });
        issuance = _postBorrowIssuance;
        if (params.maxDelayBetweenPartialRepay != 0) {
            lastPartialRepay[loanId] = block.timestamp;
        }

        // mint debt to the borrower
        RateLimitedMinter(refs.creditMinter).mint(borrower, borrowAmount);

        // pull the collateral from the borrower
        IERC20(params.collateralToken).safeTransferFrom(
            borrower,
            address(this),
            collateralAmount
        );

        // emit event
        emit LoanOpen(
            block.timestamp,
            loanId,
            borrower,
            collateralAmount,
            borrowAmount
        );
    }

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

Recommended Mitigation

Add Maximum allowed borrow limits

[L-7] Borrower Debt Increase Due to Reduced creditMultiplier

Impact

When the creditMultiplier is decreased, the effective value of each CREDIT token is reduced. This means that borrowers will owe more CREDIT tokens than they initially borrowed to account for the same value.

• If the value of the collateral held against the loan does not increase in line with the increase in debt due to the lowering of the creditMultiplier, the loan could become undercollateralized.

• This situation poses a risk of liquidation if the collateral value falls below certain thresholds, adding further financial pressure on the borrower.

POC

Initial Scenario:

A borrower takes a loan of 1,000 CREDIT. The creditMultiplier is 1.0 (or in contract terms, 1e18). System Faces Losses:

To address these losses, the creditMultiplier is reduced to 0.8 (0.8e18 in contract terms). Effect on the Loan:

Originally, 1 CREDIT = 1 unit of value (since creditMultiplier was 1.0). After the decrease, 1 CREDIT = 0.8 units of value (since creditMultiplier is now 0.8). This means each CREDIT token now represents less value. Recalculating Debt:

The borrower's debt needs to be recalculated to reflect the true value they borrowed. Initially, 1,000 CREDIT represented 1,000 units of value. Now, to represent the same 1,000 units of value, more CREDIT tokens are needed. The new debt becomes 1,250 CREDIT. This is because each CREDIT token is now worth only 0.8 units of value, so more tokens are required to sum up to the original 1,000 units of value. Impact on Borrower:

The borrower now owes more CREDIT tokens than they initially borrowed. They need to repay 1,250 CREDIT instead of the original 1,000 CREDIT to account for the decrease in value per CREDIT token.

FILE: 2023-12-ethereumcreditguild/src/loan/LendingTerm.sol

 uint256 creditMultiplier = ProfitManager(refs.profitManager)
            .creditMultiplier();
        uint256 principal = (borrowAmount * loan.borrowCreditMultiplier) /
            creditMultiplier;

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

[L-8] Inconsistancy between call and callMany functions visibility

Both functions are working same way. But there is the Inconsistancy between call and callMany.

call Function: This function is marked as external, meaning it can only be called from outside the contract. This is typical for functions that are meant to be part of the contract's interface with the outside world, like user interactions. The external visibility is more gas-efficient for functions that will only be called externally.

callMany Function: On the other hand, the callMany function is marked as public, indicating that it can be called both internally (from within the contract itself) or externally (like from another contract or an EOA - Externally Owned Account). Making a function public is useful when the functionality needs to be accessible from both within and outside the contract.

Security Considerations: Having public functions can potentially introduce security risks if not handled correctly, especially if they can change important state variables or trigger critical functionalities. It’s crucial to ensure proper access control and checks are in place.

FILE: 2023-12-ethereumcreditguild/src/loan/LendingTerm.sol

   function call(bytes32 loanId) external {
   
   function callMany(bytes32[] memory loanIds) public {

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

Recommended Mitigation

Consider the same visibility for both functions

[L-9] Arbitrage Risks in the AuctionHouse Contract's End-of-Auction Phase

Impact

An opportunistic bidder observes the auction and waits until it reaches this final phase. Once the auction is in this phase, they can bid and claim the entire collateral without paying any CREDIT. This results in the protocol losing valuable collateral without recovering any portion of the defaulted loan, while the bidder gains the collateral for free.

This situation creates an unfair advantage for late bidders and a significant risk for the protocol, as it could lose valuable collateral without adequate compensation.

FILE: 2023-12-ethereumcreditguild/src/loan/AuctionHouse.sol

 else {
            // receive the full collateral
            collateralReceived = auctions[loanId].collateralAmount;
            //creditAsked = 0; // implicit
        }

Loan Default and Auction Initiation:

A borrower defaults on their loan, or specific conditions are met (e.g., missed partial repayments), leading to the loan being called. The startAuction function is invoked for this loan, setting up an auction with a startTime.

Auction Phases:

The auction goes through two main phases: First Half (Increasing Collateral Offering): Initially, an increasing portion of the collateral is offered for the full debt amount. This continues until the midPoint of the auction duration. Second Half (Decreasing Debt Asking): After the midPoint, the full collateral is offered, but the amount of CREDIT asked for decreases over time.

Auction Conclusion Phase:

If the auction reaches the end of its duration (block.timestamp >= _startTime + auctionDuration) without any successful bids, the final phase is triggered. In this phase, the getBidDetail function specifies that the full collateral can be claimed for 0 CREDIT. This is a critical point where the arbitrage opportunity arises.

POC

• Consider a loan with 100 ETH collateral and a 200 CREDIT debt.
• The auction starts, but no one bids during the first and second phases.
• As the auction reaches the final phase, where the full 100 ETH is offered for 0 CREDIT, the bidder steps in.
• They execute the bid function, claiming the 100 ETH without paying any CREDIT. The protocol incurs a loss equivalent to the market value of the 100 ETH, while the bidder gains a significant profit.

[L-10] quorum variable not capped with reasonable values

The quorum is not capped, which could lead to governance issues. Extremely high or low values can affect the offboarding process's integrity

FILE : 2023-12-ethereumcreditguild/src/governance/LendingTermOffboarding.sol

/// @notice set the quorum for offboard votes
    function setQuorum(
        uint256 _quorum
    ) external onlyCoreRole(CoreRoles.GOVERNOR) {
        emit QuorumUpdated(quorum, _quorum);
        quorum = _quorum;
    }

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

Recommended Mitigation

Implement min and max value checks

[L-11] No same value input control

setMinBorrow() and setGaugeWeightTolerance() does not check if input parameter is the same as the current state of the variable which is being updated. If function is being called with the same value as the current state - even though there's no change - function will success and emit an event that the change has been made.

FILE: 2023-12-ethereumcreditguild/src/governance/ProfitManager.sol

 /// @notice set the minimum borrow amount
    function setMinBorrow(
        uint256 newValue
    ) external onlyCoreRole(CoreRoles.GOVERNOR) {
        _minBorrow = newValue;
        emit MinBorrowUpdate(block.timestamp, newValue);
    }

/// @notice set the gauge weight tolerance
    function setGaugeWeightTolerance(
        uint256 newValue
    ) external onlyCoreRole(CoreRoles.GOVERNOR) {
        gaugeWeightTolerance = newValue;
        emit GaugeWeightToleranceUpdate(block.timestamp, newValue);
    }

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

Recommended Mitigation

Add same value checks

require(newValue!=_minBorrow );
require(newValue!=gaugeWeightTolerance );

[L-12] Loan Collateralization Without Caps: Implications for Lending Pool Risk Metrics

Excessive collateral for a single loan can skew the overall risk profile of the lending pool. In typical lending scenarios, loans are diversified to spread risk. However, if one or more loans are over-collateralized, it can lead to a concentration of risk, making the pool more vulnerable to market shifts affecting those specific collaterals.

In a scenario where the collateral type is a less liquid or easily manipulated asset, borrowers could artificially inflate the value of their collateral, borrow heavily against it, and subsequently reduce the collateral value, affecting the platform's financial stability.

FILE: 2023-12-ethereumcreditguild/src/loan/LendingTerm.sol

 /// @notice add collateral on an open loan.
    /// a borrower might want to add collateral so that his position does not go underwater due to
    /// interests growing up over time.
    function _addCollateral(
        address borrower,
        bytes32 loanId,
        uint256 collateralToAdd
    ) internal {
        require(collateralToAdd != 0, "LendingTerm: cannot add 0");

        Loan storage loan = loans[loanId];

        // check the loan is open
        require(loan.borrowTime != 0, "LendingTerm: loan not found");
        require(loan.closeTime == 0, "LendingTerm: loan closed");
        require(loan.callTime == 0, "LendingTerm: loan called");

        // update loan in state
        loans[loanId].collateralAmount += collateralToAdd;

        // pull the collateral from the borrower
        IERC20(params.collateralToken).safeTransferFrom(
            borrower,
            address(this),
            collateralToAdd
        );

        // emit event
        emit LoanAddCollateral(
            block.timestamp,
            loanId,
            borrower,
            collateralToAdd
        );
    }

[NC-1] Avoid Commented Code

FILE: 2023-12-ethereumcreditguild/src/loan/AuctionHouse.sol

159:  //creditAsked = 0; // implicit

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

Ethereum Credit Guild Analysis

Overview

The Ethereum Credit Guild is a DeFi protocol featuring a dual-token system: GUILD for governance and gUSDC as a debt token.

Smart Contracts: Utilizes contracts like LendingTerm for loan conditions, and AuctionHouse for asset liquidation.

Gauge Voting System: Allows GUILD holders to vote on credit limits and loan terms, aligning governance with financial outcomes.

Token Mechanics: GUILD tokens are minted via staking gUSDC, with functionalities centered around protocol governance.

Risk and Debt Management: Implements strategies for managing bad debt and liquidation, adjustable through governance votes.

Decentralized Governance: Employs a governance model allowing token holders to make key protocol decisions, ensuring community-driven management.

Project’s Risk Model

Risks of using forgive() function

 function forgive(bytes32 loanId) external {
        // this view function will revert if the auction is not started,
        // or if the auction is already ended.
        (, uint256 creditAsked) = getBidDetail(loanId);
        require(creditAsked == 0, "AuctionHouse: ongoing auction");

        // close the auction in state
        auctions[loanId].endTime = block.timestamp;
        nAuctionsInProgress--;

        // notify LendingTerm of auction result
        address _lendingTerm = auctions[loanId].lendingTerm;
        LendingTerm(_lendingTerm).onBid(
            loanId,
            msg.sender,
            0, // collateralToBorrower
            0, // collateralToBidder
            0 // creditFromBidder
        );

        // emit event
        emit AuctionEnd(
            block.timestamp,
            loanId,
            LendingTerm(_lendingTerm).collateralToken(),
            0, // collateralSold
            0 // debtRecovered
        );
    }

Technical Implications

Debt Obligation Erasure: The borrower’s debt is erased without any repayment or collateral recovery, leading to a direct financial loss for the protocol.

CreditMultiplier Impact: In protocols where a creditMultiplier affects loan calculations, such losses might necessitate adjustments to this multiplier, impacting all borrowers and the tokenomics.

Protocol's Risk Exposure

Financial Health: Accumulation of such losses can strain the protocol's financial reserves.

Risk Management: It highlights potential gaps in the risk assessment framework, particularly in collateral valuation and liquidity.

Tokenomics and Inflationary Pressure:

• If the protocol compensates for losses by minting new tokens (e.g., CREDIT), it could lead to inflationary pressure.
• Impacts the trust and stability of the native token.

Example Scenario

Suppose a borrower has a loan of 5,000 CREDIT against a certain crypto-asset as collateral. A significant market event renders the asset worthless. The auction fails to attract bidders, leading to the invocation of forgive.

Systemic risks

gUSDC Value Fluctuations Risks

• gUSDC, being pegged to USDC, should ideally maintain a stable value. However, if its value deviates significantly from USDC (due to market dynamics, liquidity issues, or rebasing mechanics), this could impact the borrowing and lending operations.

Impact on Lending and Borrowing Operations

Loan-to-Value (LTV) Ratio Volatility:

• gUSDC fluctuations directly impact LTV ratios in lending protocols. A drop in gUSDC value could suddenly increase LTVs, signaling higher risk for lenders.
• Conversely, an increase might lower LTVs, potentially underutilizing collateral.

Under-collateralization and Liquidations:

• A significant decrease in gUSDC value may lead to loans becoming under-collateralized.
• Protocols typically monitor LTV ratios to manage risk; breached thresholds trigger liquidations.
• Forced liquidations can create additional market pressure, exacerbating price instability.

Impact on Borrower's Position:

• Borrowers might be forced to provide additional collateral or repay part of the loan to avoid liquidation.
• Unanticipated market movements can result in financial losses or margin calls for borrowers.

Market Sentiment:

• Persistent instability in gUSDC could erode user confidence in the peg mechanism.
• Potential contagion effects in interconnected DeFi ecosystems where gUSDC is widely used.

GUILD Tokens and Gauge Voting Risk

• GUILD tokens are used for governance decisions, including voting on LendingTerms. Significant changes in the GUILD value could influence voter behavior, potentially skewing decision-making towards short-term gains rather than long-term protocol health.
• If GUILD's value increases significantly, it might attract speculative voting behavior, while a decrease in value could lead to apathy or reduced participation among stakeholders.

Rebasing and gUSDC Stability Risk

• The ERC20RebaseDistributor mechanism might be involved in maintaining gUSDC’s peg to USDC. If rebasing is not accurately aligned with market conditions, it could lead to either inflationary or deflationary pressures on gUSDC.
• Inaccurate rebasing can affect users' trust in gUSDC's stability, which is crucial for its function as a credit token.

Impacts of Inaccurate Rebasing

Inflationary/Deflationary Pressures:

• Inflationary rebasing (increasing supply) can dilute token value, potentially leading to a loss in purchasing power.
• Deflationary rebasing (decreasing supply) can create scarcity, potentially leading to an unsustainable increase in token value.

Collateral Auction Dynamics in AuctionHouse

• The AuctionHouse contract presumably handles liquidations of collateral. Fluctuations in gUSDC or the collateral’s value could impact the efficacy of these auctions.
• In a scenario where gUSDC’s value drops, the proceeds from liquidation auctions might not cover the outstanding loans, leading to bad debt.

Risks and Impact

Bid Dynamics:

• The attractiveness of the auction to bidders depends on the perceived value of the collateral and the stability of gUSDC.
• In a volatile market, bidder hesitation or low turnout can impact the auction’s success.

Impact on Credit Ceiling and Lending Terms

• Changes in gUSDC value could necessitate adjustments in global debt ceilings and other parameters within LendingTerms to manage the protocol's overall risk exposure.
• If gUSDC devalues, the protocol might need to reduce its debt ceiling to maintain a healthy collateralization ratio across its loan portfolio.

Minting and Staking Dynamics

The mechanism for staking gUSDC to mint GUILD, and the reverse process, might be influenced by the changing values of these tokens. A decline in GUILD's value, for instance, could reduce the incentive for users to stake gUSDC.

Technical risks

ERC20RebaseDistributor

Precision and Rounding Errors: Complex rebasing calculations could lead to precision loss or rounding errors, affecting token balances. Rebasing Side Effects: Unintended side effects of rebasing on user balances, particularly if user interactions occur during the rebasing process.

AuctionHouse

Liquidation Logic Flaws: Errors in implementing the liquidation logic could lead to inefficient or incorrect asset liquidation. Smart Contract Interactions: Vulnerabilities in how AuctionHouse interacts with other contracts, like token contracts or external DeFi platforms.

LendingTerm

Loan Parameters Handling: Inaccuracies in setting or modifying loan parameters (interest rates, collateral ratios) could affect lending operations. Loan Lifecycle Management: Bugs in the management of loan states (active, defaulted, liquidated) could result in incorrect system behavior.

General Technical Risks

Parameter Adjustment Vulnerabilities: Mismanagement or errors in adjusting critical protocol parameters could destabilize the system.

Reentrancy Attacks: Functions that transfer funds or interact with external contracts could be vulnerable to reentrancy attacks.

Gas Limitations and Inefficiencies: Functions, particularly those with complex logic, might consume excessive gas, leading to high transaction costs or failures.

Inter-Contract Dependency Risks: Given the interconnected nature of the contracts, a bug or failure in one contract (e.g., ERC20RebaseDistributor) could adversely affect others (like LendingTerm).

Front-Running in AuctionHouse: Potential for front-running in auction bids, where miners or other users could exploit the transaction ordering for their benefit.

Integration risks

Inter-Contract Financial Calculations: Any financial calculations in other contracts (like yield farming rewards, fee distributions, etc.) relying on token balances might yield incorrect results post-rebase.

Consistency in Asset Valuation : The value of assets transferred during the auction needs to be consistent with their valuation in other parts of the system. Discrepancies here can affect the protocol's financial accuracy.

Collateralization and Loan Agreements: In lending scenarios, where the rebased token might be used as collateral, changes in token quantity could affect collateralization ratios. This might inadvertently trigger liquidations or alter loan terms.

Liquidity Pools and DeFi Protocols: If the rebased token is used in liquidity pools or other DeFi protocols, a rebase event can change the value or quantity of tokens in these pools. This could lead to issues like imbalanced pools, affecting staking, lending, or swapping operations.

Rebase Impact on Other Contracts The rebasing mechanism could have unintended impacts on contracts that interact with the rebased token. For example, if other contracts are not designed to handle rebasing tokens correctly, users’ balances or contract states might become inconsistent after a rebase event.

Governance Decisions Propagation: Decisions made through the governance process, particularly by the GOVERNOR, might involve changes in multiple contracts. Ensuring that these changes are synchronized and do not create conflicts or inconsistencies is a significant integration concern.

Role Changes and Access Control: Adjustments in roles or permissions must be propagated correctly across all affected contracts to avoid access control breaches or functional discrepancies.

Data Synchronization: Ensuring that all relevant contracts have up-to-date and synchronized information regarding loan terms, active loans, and borrower details is crucial.

Admin abuse risks

Roles

        // Initial roles setup: direct hierarchy, everything under governor
        _setRoleAdmin(CoreRoles.GOVERNOR, CoreRoles.GOVERNOR);
        _setRoleAdmin(CoreRoles.GUARDIAN, CoreRoles.GOVERNOR);
        _setRoleAdmin(CoreRoles.CREDIT_MINTER, CoreRoles.GOVERNOR);
        _setRoleAdmin(CoreRoles.RATE_LIMITED_CREDIT_MINTER, CoreRoles.GOVERNOR);
        _setRoleAdmin(CoreRoles.GUILD_MINTER, CoreRoles.GOVERNOR);
        _setRoleAdmin(CoreRoles.RATE_LIMITED_GUILD_MINTER, CoreRoles.GOVERNOR);
        _setRoleAdmin(CoreRoles.GAUGE_ADD, CoreRoles.GOVERNOR);
        _setRoleAdmin(CoreRoles.GAUGE_REMOVE, CoreRoles.GOVERNOR);
        _setRoleAdmin(CoreRoles.GAUGE_PARAMETERS, CoreRoles.GOVERNOR);
        _setRoleAdmin(CoreRoles.GAUGE_PNL_NOTIFIER, CoreRoles.GOVERNOR);
        _setRoleAdmin(
            CoreRoles.GUILD_GOVERNANCE_PARAMETERS,
            CoreRoles.GOVERNOR
        );
        _setRoleAdmin(
            CoreRoles.GUILD_SURPLUS_BUFFER_WITHDRAW,
            CoreRoles.GOVERNOR
        );
        _setRoleAdmin(
            CoreRoles.CREDIT_GOVERNANCE_PARAMETERS,
            CoreRoles.GOVERNOR
        );
        _setRoleAdmin(CoreRoles.CREDIT_REBASE_PARAMETERS, CoreRoles.GOVERNOR);
        _setRoleAdmin(CoreRoles.TIMELOCK_PROPOSER, CoreRoles.GOVERNOR);
        _setRoleAdmin(CoreRoles.TIMELOCK_EXECUTOR, CoreRoles.GOVERNOR);
        _setRoleAdmin(CoreRoles.TIMELOCK_CANCELLER, CoreRoles.GOVERNOR);

Sets up a role-based access control system in a Ethereum Credit Guild, with a strong centralization of power under the GOVERNOR role

Risk of Admin Abuse

• The GOVERNOR has administrative power over all roles, potentially leading to a single point of control and decision-making. This centralization can be efficient for management but poses risks of abuse or mismanagement.

• Centralization of control makes the system vulnerable to the decisions or actions of the GOVERNOR. If the GOVERNOR acts maliciously or is compromised, the entire protocol could be at risk.

• With the GOVERNOR controlling all key roles, including those related to minting tokens (CREDIT_MINTER, GUILD_MINTER), managing gauges (GAUGE_ADD, GAUGE_REMOVE, GAUGE_PARAMETERS), and executing time-locked transactions (TIMELOCK_PROPOSER, TIMELOCK_EXECUTOR, TIMELOCK_CANCELLER), there's a risk that this power could be used in ways that are not in the best interest of the protocol or its users.

Future Scalability and Adaptability

Challenges in Protocol Evolution: As the protocol grows and evolves, the centralized GOVERNOR structure might become a bottleneck, hindering the adoption of changes and updates that require a broader consensus.

Potential for Governance Gridlock : Over time, the concentration of power might lead to governance gridlock if the GOVERNOR becomes resistant to changes or if there’s a conflict of interest within the community.

Architecture assessment of business logic

I analyzed the architecture and business logic of abstract contract ERC20RebaseDistributor and two important contracts: AuctionHouse and LendingTerm. This analysis focuses on their structures and operational mechanics.

ERC20RebaseDistributor Contract

Architecture

The ERC20RebaseDistributor contract, designed for a rebase mechanism in a DeFi protocol, exhibits several key implementations,

Rebase Functionality: It allows token supply adjustments (rebasing), proportionally affecting the balance of all token holders who opt into rebasing.

Opt-in Mechanism: Users can choose to participate in rebasing through enterRebase() and opt out using exitRebase(), providing flexibility in how they wish to interact with the token's economics.

Token Burn for Distribution: The contract enables users to burn their tokens to distribute value to all rebasing token holders, enhancing the token's utility as a distribution mechanism.

Share-Based Accounting: Internally, the contract converts user balances to shares when they opt into rebasing. This approach simplifies the calculation of individual holdings post-rebase.

Dynamic Share Price Calculation: The share price is recalculated upon each distribution, ensuring that the value of rebasing shares accurately reflects the new total supply.

Interpolation for Distribution: The contract uses a linear interpolation over a set period (30 days) to gradually distribute the value of burned tokens, preventing sudden supply shocks.

Safeguards Against Manipulation: The contract highlights the need for a meaningful initial balance in rebasing to prevent share price manipulation, echoing concerns from historical DeFi exploits.

AuctionHouse Contract

Architecture

AuctionHouse Intraction Flow

The AuctionHouse contract in a DeFi protocol typically manages the auctioning of assets, often used for liquidating collateral from defaulted loans or for other decentralized governance purposes. It allows users to place bids on assets, handles the logic for determining winning bids, and ensures the transfer of both assets and payment according to the auction rules. The contract is designed to be transparent, fair, and efficient, providing a decentralized mechanism for asset liquidation or sales within the ecosystem

LendingTerm Contract

Architecture

LendingTerm Intraction Flow

The LendingTerm contract in a DeFi protocol is designed to define and manage the terms of lending operations.

Setting Loan Parameters: It specifies the conditions for loans, such as interest rates, collateral requirements, maximum debt per collateral, and repayment schedules.

Loan Origination: Facilitates the creation of new loans under the specified terms, handling collateralization and issuance of debt.

Repayment and Liquidation: Manages loan repayments and enforces liquidation in case of defaults, ensuring compliance with the set terms.

Governance Integration: Often integrated with governance mechanisms for modifying terms or adding new ones, reflecting the decentralized decision-making process.

Testing Analysis

100% Coverage:

Files like Core.sol, CoreRef.sol, LendingTermOnboarding.sol, and several others show 100% coverage across lines, statements, branches, and functions. This indicates that every line of code, every conditional branch, and every function in these files have been executed during testing, which is ideal.

Less Than 100% Coverage:

Files such as GuildGovernor.sol, GuildVetoGovernor.sol, and LendingTerm.sol have less than 100% coverage in some areas. This could be a cause for concern, especially in a DeFi protocol, where untested code might lead to vulnerabilities or unexpected behaviors. The test/forge-std/... files have significantly lower coverage. These might be standard test utilities or mock contracts used for testing other components. While lower coverage here might be less critical, it's still worth investigating if it impacts the reliability of the tests.

Impact of Non-100% Coverage:

Security Risks: In DeFi, untested code could lead to security vulnerabilities, potentially leading to loss of funds or other exploits. Functional Bugs: Lack of coverage might mean that certain edge cases or error conditions are not tested, which could result in bugs or unexpected behavior in production. Integration Issues: In a protocol with multiple interacting contracts, untested code in one contract might cause issues in another, especially in complex transactions or state changes.

Software engineering considerations

Purpose-Specific Modules

Design contracts like ERC20RebaseDistributor, AuctionHouse, and LendingTerm as modular components. This allows for easier upgrades and maintenance.

Separation of Concerns

Ensure each contract focuses on a specific functionality, reducing complexity and potential for bugs.

Unit Testing

Develop comprehensive unit tests for each function in the contracts, covering all possible edge cases.

Stress Testing and Simulation

Perform stress tests simulating extreme market conditions to evaluate the contracts' resilience.

Mathematical Proofs

Use formal verification tools to prove the correctness of critical functions, especially those in ERC20RebaseDistributor involving complex mathematical calculations.

State Invariants

Establish and verify state invariants for contracts to ensure they maintain consistent states throughout transactions.

Reentrancy Guards

Implement checks against reentrancy attacks, particularly in functions that interact externally in AuctionHouse.

Optimize for Gas

Refactor contract code to minimize gas consumption, crucial for functions likely to be called frequently.

Proxy Patterns

If upgradeability is a requirement, consider using proxy patterns, ensuring a clear separation between data and logic.

Version Control

Maintain strict version control and changelogs for all contracts to track changes and upgrades over time.

Transparent Governance Processes

Develop and maintain clear, transparent processes for governance actions, documented in code comments and developer documentation.

Smart Contract Events

Emit detailed events in contracts for better off-chain tracking and to facilitate user interface updates.

Comprehensive Documentation

Maintain detailed documentation for each contract, describing its purpose, functions, and interaction mechanisms.

Code Weakspots

Complex Rebasing Logic in _balance2shares and _shares2balance Functions

function _balance2shares(uint256 balance, uint256 sharePrice) internal pure returns (uint256) {
    return (balance * START_REBASING_SHARE_PRICE) / sharePrice;
}

function _shares2balance(uint256 shares, uint256 sharePrice, uint256 deltaBalance, uint256 minBalance) internal pure returns (uint256) {
    uint256 rebasedBalance = (shares * sharePrice) / START_REBASING_SHARE_PRICE + deltaBalance;
    if (rebasedBalance < minBalance) {
        rebasedBalance = minBalance;
    }
    return rebasedBalance;
}

Why It's Weak:

These functions handle the conversion between balances and shares, which is a core part of the rebasing logic. The risk here is primarily due to potential rounding errors and integer division limitations in Solidity. Incorrect conversions could lead to balance inaccuracies, especially when dealing with large numbers or very small fractional amounts.

Complexity in getBidDetail() Function

function getBidDetail(bytes32 loanId) public view returns (uint256 collateralReceived, uint256 creditAsked) {
    // ... logic for calculating bid details ...
}

Why It's Weak:

The getBidDetail function calculates how much collateral is offered and how much debt is asked for, based on the time elapsed in the auction. The logic is complex and time-dependent, making it prone to miscalculations or manipulation, especially if not properly validated against edge cases or unexpected scenarios (like extreme market conditions).

Handling of Negative Values in notifyPnL

if (amount < 0) {

Why It's Weak:

The handling of losses (negative values) is complex and involves multiple state changes (e.g., adjusting surplus buffers, updating credit multipliers). If not managed accurately, it could lead to financial discrepancies in the system, such as incorrect loss accounting or multiplier adjustments.

creditMultiplier Risk

// update the CREDIT multiplier
                uint256 creditTotalSupply = CreditToken(_credit).totalSupply();
                uint256 newCreditMultiplier = (creditMultiplier *
                    (creditTotalSupply - loss)) / creditTotalSupply;
                creditMultiplier = newCreditMultiplier;
                emit CreditMultiplierUpdate(
                    block.timestamp,
                    newCreditMultiplier
                );

Calculating the New Multiplier

• creditTotalSupply is the total supply of CREDIT tokens in the system.
• loss is the amount of bad debt that needs to be accounted for, beyond the surplus buffer.
• The new creditMultiplier is calculated by reducing the current multiplier proportionally to the ratio of the loss to the total CREDIT supply.

Effect of the Calculation:

• The formula (creditMultiplier * (creditTotalSupply - loss)) / creditTotalSupply effectively reduces the creditMultiplier.
• If loss is significant compared to creditTotalSupply, this reduction can be substantial.
• For example, if the current multiplier is 1.0 (or 1e18 in Solidity's integer math), and the loss is 10% of the total CREDIT supply, the new multiplier would be 0.9 (or 0.9e18), representing a 10% decrease.

Implications:

Decrease in CREDIT Value: The decrease in the creditMultiplier effectively lowers the value of CREDIT tokens throughout the system. This means that every CREDIT token now represents a smaller share of the system's value.

Increased Debt for Borrowers: This decrease means borrowers now owe more CREDIT tokens to cover the same nominal value of debt.

Time spent:

29 hours