Ethereum Credit Guild - beber89's results

A trust minimized pooled lending protocol.

General Information

Platform: Code4rena

Start Date: 11/12/2023

Pot Size: $90,500 USDC

Total HM: 29

Participants: 127

Period: 17 days

Judge: TrungOre

Total Solo HM: 4

Id: 310

League: ETH

Ethereum Credit Guild

Findings Distribution

Researcher Performance

Rank: 42/127

Findings: 3

Award: $329.16

QA:
grade-a
Analysis:
grade-b

🌟 Selected for report: 0

🚀 Solo Findings: 0

Awards

3.0466 USDC - $3.05

Labels

bug
3 (High Risk)
satisfactory
sufficient quality report
upgraded by judge
duplicate-473

External Links

Lines of code

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

Vulnerability details

Impact

Contract Name: SurplusGuildMinter

The vulnerability lies in the getRewards function, where an invalid validation process incorrectly marks users for slashing (slashed = true). As a result of this erroneous validation, users experience a severe impact, including the loss of their staked amount and the forfeiture of rewards that they were entitled to receive as shown in the following:

if (slashed) {
    emit Unstake(block.timestamp, term, uint256(userStake.credit));
    userStake = UserStake({
        stakeTime: uint48(0),
        lastGaugeLoss: uint48(0),
        profitIndex: uint160(0),
        credit: uint128(0),
        guild: uint128(0)
    });
    updateState = true;
}

// store the updated stake, if needed
if (updateState) {
    _stakes[user][term] = userStake;
}

Therefore, Users' stakes are mistakenly slashed due to the invalid validation in line 229, resulting in the loss of the staked amount they contributed to the contract.

Proof of Concept

https://gist.github.com/beber89/88795f5eeb273b5edf3828fe20df8a91

Tools Used

Foundry

The validation in line 229 can be fixed just if line 234 in the function: userStake = _stakes[user][term]; is moved ahead of the if condition in line 229.

Assessed type

Invalid Validation

#0 - c4-pre-sort

2023-12-29T18:05:43Z

0xSorryNotSorry marked the issue as sufficient quality report

#1 - c4-pre-sort

2023-12-29T18:06:09Z

0xSorryNotSorry marked the issue as duplicate of #1164

#2 - c4-judge

2024-01-28T20:17:41Z

Trumpero marked the issue as satisfactory

#3 - c4-judge

2024-01-31T13:46:47Z

Trumpero changed the severity to 3 (High Risk)

Awards

211.2258 USDC - $211.23

Labels

bug
grade-a
QA (Quality Assurance)
sufficient quality report
edited-by-warden
Q-04

External Links

Potential Loss of Assets in emergencyAction Function due to Unchecked msg.value

Contract Name: CoreRef

Scope: https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/core/CoreRef.sol#L87-L107

Impact

During the security audit of the smart contract, a critical issue has been identified in the emergencyAction function, particularly in its handling of native coin transfers to external contracts (i.e. targets). The function allows the caller to send msg.value to multiple targets without ensuring that the total value sent does not exceed the targets' contributions. This oversight may lead to the loss of excess assets that remain inaccessible within the contract.

In other words, The emergencyAction function allows the caller to send native coins (msg.value) to multiple external contracts without verifying that the total value sent does not exceed the summation of the values sent to the targets. The excess native coins sent beyond the summation of values to the targets remain locked in the contract resulting in potential asset loss.

Proof of Concept

https://gist.github.com/beber89/cc6129be4d29a12c9e53d5ce0844f4cf

To mitigate the risks associated with unchecked msg.value in the emergencyAction function, it is recommended to:

Implement Validation Checks: Introduce a validation checks within the emergencyAction function to ensure that the total msg.value sent does not exceed the intended amount for external calls. No need for another for-loop to carry out that check, leverage the existing for loop and carry out the check after the loop is executed.

Refund Mechanism: Consider implementing a refund mechanism to return excess msg.value to the caller if it exceeds the total required for external calls. This enhances transparency and prevents asset loss.

Unspent Native Coin gets rejected on calling emergencyAction function

Contract Name: CoreRef

Scope: https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/core/CoreRef.sol#L87-L107

Impact

During the security audit of the smart contract, a potential vulnerability has been identified in the emergencyAction function. This function performs low-level calls to external contracts, including the transfer of native coins (msg.value). The audit reveals that the implementation does not account for the possibility that external contracts might return unspent native coins, leading to potential loss of assets. It is a common pattern in which contracts accept native coin payments and return the unspent coin back to the caller.

Proof of Concept

https://gist.github.com/beber89/cc6129be4d29a12c9e53d5ce0844f4cf

Implement Asset Recovery Mechanism: Incorporate a mechanism within the emergencyAction function to handle and recover native coins that are rejected or returned by external contracts to then be returned to msg.sender. This also requires to add receive() method to the contract implementation.

Use of assert Instead of require in Smart Contract

Contract Name: LendingTerm

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

Impact

The developers have utilized assert statement in initialize function in line 167 of the code where require may be more suitable. Unlike require, which is used to validate user inputs and conditions, assert is typically employed to check for internal errors and invariant conditions. Inappropriate use of assert can lead to unnecessary loss of funds.

    function initialize(
        address _core,
        LendingTermReferences calldata _refs,
        LendingTermParams calldata _params
    ) external {
        // can initialize only once
        assert(address(core()) == address(0));
        assert(_core != address(0));
        ...
    }

Another negative impact is Inadequate Error Messaging: Unlike require, which provides a clear error message indicating the cause of the failure, assert simply terminates the contract without detailed information. This lack of informative error messaging hinders developers and users in understanding the root cause of contract termination.

Replace assert with require: Utilize require for user input validation and condition checking where failure should be gracefully handled with informative error messages.

Implement Custom Error Messaging: Enhance error messaging in require statements to provide clear information about the conditions that need to be met and the consequences of failure.

CEI Should Be Followed

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/governance/LendingTermOnboarding.sol#L114

In the above instance , an external call is performed before updating the state at L154 and L165. It is crucial to follow CEI throughout the codebase to prevent from attacks such as reentrancy (even if not exploitable).

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/governance/ProfitManager.sol#L260

transfer done before state updates.

Add A Min Check On setGaugeWeightTolerance

The gaugeWeightTolerance is used to facilitate growth of the protocol and is expressed as a percentage with 18 decimals , i.e. 1e18 means 0% deviation and this might lead to a deadlock situation where no new borrows are allowed. There should be a minimum check here

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/governance/ProfitManager.sol#L190

i.e. require(gaugeWeightTolerance > 1e18) to ensure a healthy weight tolerance.

surplusBufferSplit/guildSplit/othersSplit Might Round Down To 0

The governor can set the profit sharing config here

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/governance/ProfitManager.sol#L195

If the current system requirements were to keep otherSplit to be as low as possible (for example) and maximise the guild and supplyBuffer split , say otherSplit is 1e8 , then otherSplit would round down to 0 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/governance/ProfitManager.sol#L215 Because of this the split that should have been assigned to otherAddress would be lost forever.

A possible remediation for this can be to ensure otherSplit/1e9 > 0 or have a minimum split requirement.

A Market Can Not Be Deployed For A Token > 18 Decimals

Each market has its own SimplePSM (peg stability module) , and inside the constructor of the SimplePSM the decimalCorrection is calculated as follows ->

decimalCorrection = 10 ** (18 - decimals);

In case of USDC as the peg token it would be 10 ** 12

It is evident that it would be impossible to deploy a SimplePSM for a token with decimals > 12 since the constructor would keep reverting due to the underflow in 10 ** (18 - decimals);

To remediate , make sure if the decimal of the peg token is more than 18 then it is normalised properly.

Have A Minimum Value For maxDelayBetweenPartialRepay

A loan can be called if a partial repayment is missed , the check to see if a partial repayment is missed is done here ->

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

The code to check if a partial repayment is missed ->


lastPartialRepay[loanId] <
            block.timestamp - params.maxDelayBetweenPartialRepay;
            

If the maxDelayBetweenPartialRepay was set maliciously to a very low value then an attacker can deliberately make the victim miss a partial repayment so that his loan goes off to auction , to make the victim miss a partial repayment the attacker can perform block stuffing just before the victim's call to partial repay.

To remediate make sure there is a minimum value for the maxDelayBetweenPartialRepay of a loan.

Missing 0 Address/Value Checks (Missed By The Bots)

There are instances where 0-address checks are missing for variables set in the constructor that are missed by the automation . These are ->

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/governance/LendingTermOnboarding.sol#L83-L88 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/governance/ProfitManager.sol#L182 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/governance/GuildVetoGovernor.sol#L73 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/tokens/ERC20MultiVotes.sol#L149

A zero address in a crucial role can cause serious issues, as it cannot perform actions like a normal address, and any funds sent to it will be irretrievable.

Incorrect Comment

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/governance/ProfitManager.sol#L58

The comment above says if the sum of guildSplit + otherSplit is < 1e9 then remaining is sent to the lenders of the system. Rather it should be if the sum of guildSplit + otherSplit + surplusBufferSplit is < 1e9 , since surplusBuffer is also a part of the split.

Handle The 0 surplusBuffer Case In The If Clause

Inside the function notifyPnL() , there can arise a condition where loss is equal to the surplus buffer (in this case the surplus buffer would be 0) , currently this case is handled in the else clause here https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/governance/ProfitManager.sol#L323

but it would be more effecient to handle this case in the if block. This would be done by changing the if condition to if (loss <= _surplusBuffer)

This would be more streamlined plus less gas as else has more code to run.

Sanity Checks

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/tokens/ERC20MultiVotes.sol#L349

A sanity check can be added before this , require(amount >= _delegatesVotesCount[delegator][delegatee])

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/tokens/ERC20MultiVotes.sol#L451

This is wrapped inside a require, but this can never fail due to the code at L437 (delegateList only contains the delegatees inside the _delegates[user])

Make Sure CREDIT Can Not Be Redeemed To The SimplePSM Itself

CREDIT can be redeemed for underlying token in the SimplePSM.sol , https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/loan/SimplePSM.sol#L135 But if the to address is provied as address(this) , then the redeemed CREDIT token would go back to the SimplePSM contract and the pegTokenBalance would be reduced at L141 whereas it remained the same. Make sure to is not the address(this).

#0 - 0xSorryNotSorry

2024-01-05T18:10:45Z

A Market Can Not Be Deployed For A Token > 18 Decimals -> dup of #957

#1 - c4-pre-sort

2024-01-05T18:10:51Z

0xSorryNotSorry marked the issue as sufficient quality report

#2 - Trumpero

2024-01-31T07:49:55Z

Potential Loss of Assets in emergencyAction Function due to Unchecked msg.value -> L

Unspent Native Coin gets rejected on calling emergencyAction function -> L

Use of assert Instead of require in Smart Contract -> bot finding (N-12)

CEI Should Be Followed -> bot finding (L-19)

Add A Min Check On setGaugeWeightTolerance -> L

surplusBufferSplit/guildSplit/othersSplit Might Round Down To 0 -> L

A Market Can Not Be Deployed For A Token > 18 Decimals -> info (dup of #957)

Have A Minimum Value For maxDelayBetweenPartialRepay -> info

Missing 0 Address/Value Checks (Missed By The Bots) -> L

Incorrect Comment -> NC

Handle The 0 surplusBuffer Case In The If Clause -> info

Sanity Checks -> NC

Make Sure CREDIT Can Not Be Redeemed To The SimplePSM Itself -> info

#4 - c4-judge

2024-01-31T12:22:56Z

Trumpero marked the issue as grade-a

Findings Information

Labels

analysis-advanced
grade-b
insufficient quality report
A-01

Awards

114.8801 USDC - $114.88

External Links

Ethereum Credit Guild Analysis

Description:

The Ethereum Credit Guild aims to find a balance between systems where most people are honest and direct person-to-person lending. It uses rewards and safeguards to enable savings and loans without needing to trust a central authority. The system can adapt to market changes through an open process. It assumes "the minority" is honest, and if you have enough tokens to be part of the honest minority group then you don't need a centralised party. If something goes wrong, the system will stop lending via the lending terms (a loan) and compensate borrowers. ECG brings a unique liquidation mechanism for unhealthy borrow positions (a borrow position is essentially a loan) , if the borrower skips a partial repayment then the collateral in the position is auctioned off where bidders can bid on the collateral. This is how the "market price" is decided. The GUILD is the governance token which is used to decide on decisions for the protocol.

1. System Overview

1.1 Scoping Details

  • Core.sol - The core access control contract of the ECG , maintains roles and access control.This contract sets initial roles before going live.

  • CoreRef.sol - This is a reference to the core contract and defines some modifiers and utilities around interacting with Core. This has the emergencyAction() function.

  • CoreRoles.sol - Holds a complete list of all roles which can be held by contracts inside the Ethereum Credit Guild.

  • GuildGovernor.sol - Governor contract based on the OZ governance implementations (inherits and overrides Governor.sol , GovernorSettings.sol GovernorTimelockControl.sol , GovernorVotes.sol , GovernorCountingSimple.sol) and for access control inferits the Core contracts.

  • GuildTimelockController - This contract overrides the OZ Timelock implementation and the access control is handled via the Core contracts.

  • LendingTermOffboarding.sol - Think of a lending term as a term which offers loans to a user(borrowers pay collateral in exchange for CREDIT tokens like gUSDC). Using GUILD the governance token the users can poll to offboard a lending term.No new loans can be issues on the offboarded lending term.

  • LendingTermOnboarding.sol - Similar to what it sounds like , a contract used to onboard a lending term(users can only queue the onboarding proposals). To veto this onboarding proposal CREDIT holders can vote to object this(veto the proposal) . A term that has been offboarded in the past can also be onboarded again.

  • ProfitManager.sol - This contract manages all the profits that are generated in the system , the users voting for a lednding term which generated a profit will be rewarded through this contract . In case of a loss there is a surplus buffer which is a buffer to "soak-up" the losses.

  • GuildVetoGovernor.sol - This is the governor for the veto mechanism in the ECG. "governor will only be able to execute the action of cancelling an action" This means through this contract only votes against a proposal can be casted and the quorum only accounts for the against votes . When enough against votes (above veto quorum) are cast, the veto vote is considered successful

  • ERC20Gauges.sol - This is an ERC20 with a gauge style voting mechanism. Support gauge style votes with weights associated with resource allocation. The word lending term can be interchanged with a gauge , user voting for a lending terms means voting for a gauge.

  • ERC20MultiVotes.sol - an ERC20 extension which allows delegations to multiple delegatees up to a user's balance on a given block.

  • ERC20RebaseDistributor.sol - A user (lenders) can choose to enter rebase (their balance is converted to a number of shares internally, and their balance is sharePrice * nShares. Then as distributions happen, the share price increases, so their balance increase) The contract is an ERC20 with rebase support.

  • CreditToken.sol - This is the CREDIT token contract (ex - gUSDC for USDC underlying) , This is the debt token of the Ethereum Credit Guild. Lenders lend USDC -> they are minted gUSDC(CREDIT).

  • GuildToken.sol - This is the governance token of the ECG . GUILDis minted through the SurplusGuildMinter (staked)

  • AuctionHouse.sol - The auction mechanism of the collateral of a position is handled here(collateral of borrowers is auctioned to cover their CREDIT debt.)

  • LendingTerm.sol - The core lending term mechanism contract , issues CREDIT as debt and takes collateral from the borrower. interest rate is non-compounding and the percentage is expressed per period of YEAR seconds.

  • SimplePSM.sol - Peg stability module contract which maintains the peg of the CREDIT to the underlying (gUSDC and USDC) , allows mint/redeem of CREDIT token. The amount of CREDIT to be minted (or value of CREDIT to be more precise) is controlled via the creditMultiplier.

  • SurplusGuildMinter.sol - This contract mints GUILD for CREDIT tokens , the CREDIT tokens are provided as the first loss capital to the profit manager.

  • RateLimitedMinter - This contract is used to mint tokens with a rate limit. All the minting throughout the system is done through this smart contract.

  • RateLimitV2 - An abstract implementation for the rate limit mechanism.

1.2 Access Control Roles

There are multiple access controls for different types of functionalities , these roles range from governance functionalities to timelock functionalities.

Roles -> GOVERNOR

GUARDIAN

CREDIT_MINTER

RATE_LIMITED_CREDIT_MINTER

GUILD_MINTER

RATE_LIMITED_GUILD_MINTER

GAUGE_ADD

GAUGE_REMOVE

GAUGE_PARAMETERS

GAUGE_PNL_NOTIFIER

TIMELOCK_PROPOSER

GUILD_GOVERNANCE_PARAMETERS

GUILD_SURPLUS_BUFFER_WITHDRAW

CREDIT_REBASE_PARAMETERS

TIMELOCK_EXECUTOR

TIMELOCK_CANCELLER.

Architecture Overview:

Diagram For Governance:

Diagram for Governance

1.) The LendingTerm.sol contains the logic for the borrowing of CREDIT tokens . A borrower would deposit collateral in exchange for the CREDIT tokens and he might/might not be subject to partial repayments , if there were partial repayments enabled on the loan and the borrower misses a partial repayment then the collateral of the borrower is subject to be "auctioned off" where a bidder can bid on the collateral in exchange for debt.

2.) A lender can decide wether to enter rebase or not , if entered rebase the balance of the user is converted to shares and if the share price increases the balance of the user increases.

Since the Discord server was full of questions I decided to put all of the questions and answers in one place (to help the community they were shared via twitter https://twitter.com/SakshamGuruji/status/1737733359567356401), these are as follows ->

1.) Governor should make sure he is sending enough eth in the emergencyAction functionality (to cover all the call operations). This can be only done by the governor , therefore it makes it a good candidate for centralisation risk.

2.) The greedy algorithm for freeing votes ->

If a user has 3000 tokens, 3 delegations of 1000 tokens each, and attempts to transfer/burn 1500 tokens, the loop will free delegations in chunks of 1000 so the user ends up freeing 2000 voting power, transfers out 1500 tokens, and remains with 500 undelegated votes and only one of the 3 delegations of 1000 tokens still active

3.) Why is there is a distribution period of 30 days when distributing rewards via ERC20RebaseDistributor?

Then anyone could atomically : a.) flashloan USDC somewhere b.) mint gUSDC, enter rebase c.) repay a loan (and the profit that goes to lenders makes everyone rebase up instantly) d.) repay flashloan This would allow the loan repayer to pocket most of the interest if they flashloan a large amount & represent a majority of the rebasing supply

If the rewards are distributed over 30 days, every block only "drips" a tiny amount of rewards (by adjusting the share price through an interpolation), so the user who repays cannot "sandwich" their interest payment with a large deposit/withdrawal into gUSDC

4.) ProfitManager works with multipleLendingTerms

termSurplusBuffer is specific to individual lending terms, and surplusBuffer is a global buffer for the entire system. In the case of a loss, termSurplusBuffer is depleted first, then surplusBuffer, then the creditMultiplier is adjusted down (loss to lenders)

5.) Global debt ceiling is the maximum amount that can be borrowed across all lending terms. In the early period it is used to set a maximum borrowed amount during the guarded launch. In the future it can be used to rate limit borrowing to help protect the protocol against malicious borrows

6.) There's one SimplePSM per CreditToken , There can be various lending terms per CreditToken with different collateral tokens and different parameters. But you only need one SimplePSM per CreditToken to mantain peg with it's pegged token (USDC <> gUSDC

7.) Most users will enter rebase. Our UI will call mintAndEnterRebase for first deposit. It's mainly for smart contracts that might not support rebase, or arbitrageurs that want to minimize gas cost of credit token transfers.

8.) gauge == lending term gauge type is to support multiple markets (all lending terms of the gUSDC market will have type 1, all lending terms of the gWETH market will have type 2, etc)

9.) When a user enters rebase, their balance is converted to a number of shares internally, and their balance is sharePrice * nShares. Then as distributions happen, the share price increases, so their balance increase

10.) 'Lenders' is users who provide USDC to the PSM to get gUSDC and enter the savings rate (enterRebase) 'Borrowers' is users who provide collateral (sDAI, wstETH, etc) to borrow gUSDC in LendingTerms and redeem these for USDC in the PSM.

11.) when staking credit tokens, one can access GUILD tokens in 2 ways : some are minted on the SurplusGuildMinter & can only be used to do gauge votes, some are earned as rewards & go directly in the user's wallet when borrowers pay interests, the SurplusGuildMinter, because it is voting in the gauges with its GUILD tokens, earns some part of these credit tokens as rewards. These rewards are forwarded to the users who staked, and they also get on top of that an amount of GUILD tokens (that is the amount of credit rewards * the reward ratio)

12.) 1 market is composed of: a.) 1 creditToken (e.g: gUSDC) b.) 1 PSM: lender can deposit USDC and get gUSDC, or borrower can redeeem gUSDC against USDC c.) N term: borrow gUSDC against a collateral (sDAI, wstETH, etc). 1 collateral = 1 term d.) 1 ProfitManager e.) 1 RateLimitedMinter (gUSDC)

13.) https://docs.google.com/spreadsheets/d/1yTSHd7AlCQjK5KrMIjHQuH2Ppq7uQeG-FH-oU1ou0-M/edit#gid=153429491 (A spreadsheet explaining user actions)

14.) The borrower themselves could bid in the auction before it reaches market price, because they get returned both the collateral share of the bidder & the borrower (so they recover their full collateral), this could be a rational action to avoid "arbitrage fees" (nobody external will bid until enough collateral of the borrower goes to them to cover both the debt + the gas fees / slippage to do the liquidation)

when someone bid() in the AuctionHouse, the AH calls back to the LendingTerm.onBid(), that calls back to ProfitManager.notifyPnL, that calls back to GuildToken.notifyGaugeLoss if there is a loss, and that is where the slashing happens (more precisely, that is when tokens are 'marked for slashing' and cannot be transferred anymore, then the slashing has to be realized through GuildToken.applyGaugeLoss)

15.) Feature of "imposing periodic partial repayments" (maxDelayBetweenPartialRepay + minPartialRepayPercent) is going to be important, because if lending terms are with uncorrelated assets like wstETH no problem, but if it's on correlated assets, we'll need periodic payments. E.g. for a term that allows to borrow 1.0 gUSDC per 1.05 DAI collateral, with 5% interest, after 1 year the loan will become insolvent (debt = 1.05 USDC worth for 1.05 DAI collateral), so we'll require quarterly payments of 1.25% for instance, and either the position remains healthy forever, either the loan missed a partial payment & can be called. This kind of setup is also better for lenders, because interest is realized regularly, and the profit distribution (rebase savings rate) happens regularly too

16.) Q - is credit multiplier intended to be applied to interest and opening fee (not just principal)? A - yes, the intent is to keep a constant pegToken value for open loans when the creditMultiplier is updated. E.g. if you owed X gUSDC worth 2000 USDC before, you must now owe Y gUSDC still worth 2000 USDC

17.) The profit manager has the ability to split the profits it receives between the surplus buffer (market wide insurance fund), the GUILD stakers, the gUSDC savers, and an optional fourth recipient intended to permit upgrades, for example forwarding profits to an upgraded profit manager, or sending to a DAO treasury separate from the surplus buffer.

18.)termSurplusBuffer is the total staked tokens (first loss capital) surplusBuffer can be thought as the gains the market has made when borrowers repays their loans.

19.) gaugeProfitIndex - index of number of CREDIT rewards per weight allocated to the gauge

20.) If userGaugeProfitIndex is lower than gaugeProfitIndex, it means that user has unclaimed rewards

3. Codebase Assessment:

The total number of lines of code in the audit was ~3800 and the overall complexity of the system is pretty high . There are a lot of moving parts in the codebase , uses OZ governance heavily and it needs a structures approach to make sense of the whole system .

  • The codebase makes use pause functionality for system critical functions like stake and mint.(the guardian pause feature is to prevent 'new usage' of the protocol but not prevent user withdrawals)

  • General Readability - Even though the overall system is complex the code has been written in a way that anyone with basic programming knowledge can make sense , the variables and comments are well defined and structured.

  • Code Style - The code adheres to a uniform style and indentation pattern, complemented by in-line comments that provide comprehensive guidance throughout it's mechanisms.

  • Gas - Throughout the codebase it can be seen that the code was written in a way to optimise gas , this shows the professionalism of the devs.

  • Test-Suite - The test suite is complete with more than 97% coverage and well written tests in foundry.

  • Errors and Events - Errors and events have been taken care of for every functinality.(Events/Errors are very well defined)

4. Level Of Documentation:

The documentation in the c4 audit repo was enough to make sense of the basic moving parts of the codebase , but for a deeper understanding of the codebase a more robust documentation is required , this was seen as most of the wardens were confused about someof the moving parts in the system such as auctions and interpolation . More graphs with more explanation for every smallest detail is very much needed.

5. Systemic And Centralisation Risks

5.1 Systemic Risks

The codebase was battlehard , it had undergone formal verification plus a well structures test suite and all of that made it hard to find flaws in the system. Although the code was written with security being the top most priority I was sure that there would be flaws/bugs in the system due to the complexity. There a certain risks which are connected/associated with the systemic design ->

  • GUILD holders can make system critical decisions like off boarding / on boarding a lending term and initially GUILD would be airdropped to the users , meaning a set of malicious users can make decisions such as offboarding a normal lending term etc.

  • Any malicious actor can make malicious loans , for example a loan where the maximum deplay for partial repayments is very low and can instantly auctions off the collateral of the borrower.

5.2 Centralisation Risks

The ECG has tackled the "centralisation" problem beautifully , they did this by having different roles assigned for different functionalities. These roles include ->

GOVERNOR , GUARDIAN , CREDIT_MINTER , RATE_LIMITED_CREDIT_MINTER , GUILD_MINTER , RATE_LIMITED_GUILD_MINTER , GAUGE_ADD , GAUGE_REMOVE , GAUGE_PARAMETERS , GAUGE_PNL_NOTIFIER , TIMELOCK_PROPOSER , GUILD_GOVERNANCE_PARAMETERS , GUILD_SURPLUS_BUFFER_WITHDRAW , CREDIT_REBASE_PARAMETERS, TIMELOCK_EXECUTOR , TIMELOCK_CANCELLER.

Also , a lot of the power resides in the hands of the GUILD holders.

But , though it may seem that a lot of the power has been disbursed or decentralised there are still roles (GOVERNOR for example) with privileges over system critical functions , because of this it should be made sure (for every role) ->

1.) Multisigs are used for privileged roles

2.) Multisigs should come with a timelock

3.) Each key should reside on a different server in case one of the server is compromised.

Time spent:

32 hours

#0 - c4-pre-sort

2024-01-05T17:24:31Z

0xSorryNotSorry marked the issue as insufficient quality report

#1 - c4-judge

2024-01-31T04:54:07Z

Trumpero marked the issue as grade-b

AuditHub

A portfolio for auditors, a security profile for protocols, a hub for web3 security.

Built bymalatrax © 2024

Auditors

Browse

Contests

Browse

Get in touch

ContactTwitter