code-423n4 / 2023-12-ethereumcreditguild-findings

9 stars 5 forks source link

Upgraded Q -> 2 from #556 [1706627592320] #1275

Closed c4-judge closed 5 months ago

c4-judge commented 5 months ago

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

[L-04] debtCeiling() does not properly calculate the debt ceiling of gauges experiencing an adjustment up

Bug Description

The debtCeiling function will return a gauge's debt allocation, with respect to an applied adjustment (gauge is being staked into or unstaked from):

LendingTerm::debtCeiling#L270-L281

270:    function debtCeiling(
271:        int256 gaugeWeightDelta
272:    ) public view returns (uint256) {
273:        address _guildToken = refs.guildToken; // cached SLOAD
274:        uint256 gaugeWeight = GuildToken(_guildToken).getGaugeWeight(
275:            address(this)
276:        );
277:        gaugeWeight = uint256(int256(gaugeWeight) + gaugeWeightDelta);
278:        uint256 gaugeType = GuildToken(_guildToken).gaugeType(address(this));
279:        uint256 totalWeight = GuildToken(_guildToken).totalTypeWeight(
280:            gaugeType
281:        );

As seen above, the gaugeWeightDelta is the adjustment being applied to the gaugeWeight of the gauge. If the adjusted gaugeWeight is 0, then the debt ceiling is considered 0. If the gaugeWeight is equal to the totalWeight for all the gauge's of this type, then the gauge is considered to have 100% debt allocation:

LendingTerm::debtCeiling#L285-L292

285:        if (gaugeWeight == 0) {
286:            return 0; // no gauge vote, 0 debt ceiling
287:        } else if (gaugeWeight == totalWeight) {
288:            // one gauge, unlimited debt ceiling
289:            // returns min(hardCap, creditMinterBuffer)
290:            return
291:                _hardCap < creditMinterBuffer ? _hardCap : creditMinterBuffer;
292:        }

However, as we can see in the above code snippets, the adjusted totalWeight for the gauge type is not considered. This can result in a gauge being adjusted up by a gaugeWeightDelta that results in the adjusted gaugeWeight being equal to the non-adjusted totalWeight. If this gauge is in a multi-gauge system then it's calculated debt allocation will be considered 100%, despite the weight of other gauges in the system. Lets consider an example:

- totalWeight = 2000e18
- Gauge A weight =  1000e18 => 50% debt allocation
- Gauge B weight = 1000e18 => 50% debt allocation

- gaugeA.debtCeiling(1000e18):
- gaugeWeightDelta = 1000e18
- gaugeWeight = gaugeAWeight + gaugeWeightDelta = 2000e18
- gaugeWeight == totalWeight => 100% debt allocation

Actual debt allocations after adjustment:
- totalWeight (post adjustment) = 3000e18
- Gauge A weight (post adjustment) = 2000e18
- Gauge B weight = 1000e18

- Gauge A debt allocation = 2000e18 / 3000e18 => ~ 66%
- Gauge B debt allocation = 1000e18 / 3000e18 => ~ 33%

As we can see above, the debtCeiling function will not return the proper debt ceiling of a gauge when the gauge is being adjusted up with an amount that results in the gaugeWeight being equal to the totalWeight.

Impact

The debtCeiling() function is currently only used for calculating an adjustment down (unstaking) in gauges. However, seeing as this function is meant to take in any adjustment (up or down), 3rd party integrators or future ECG contracts that use this function can potentially receive incorrect values.

Proof of Concept

Place the following test inside of /test/unit/loan/LendingTerm.t.sol:

    function testWrongDebtCeilingForAdjustmentUp() public {
        // add additional term
        LendingTerm term2 = LendingTerm(Clones.clone(address(new LendingTerm())));
        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
            })
        );
        guild.addGauge(1, address(term2));

        // set weights for both terms
        guild.decrementGauge(address(term), _HARDCAP);
        guild.incrementGauge(address(term), 1000e18);
        guild.incrementGauge(address(term2), 1000e18);

        assertEq(guild.getGaugeWeight(address(term)), 1000e18);
        assertEq(guild.getGaugeWeight(address(term2)), 1000e18);
        assertEq(guild.totalTypeWeight(1), 2000e18);

        // simulate a borrow so `totalBorrowedCredit` > 0
        uint256 borrowAmount = 20_000e18;
        uint256 collateralAmount = 12e18;
        collateral.mint(address(this), collateralAmount);
        collateral.approve(address(term), collateralAmount);

        term.borrow(borrowAmount, collateralAmount);

        // confirm debtCeilings via `calculateGaugeAllocation` (no tolerance)
        assertEq(guild.calculateGaugeAllocation(address(term), _HARDCAP), _HARDCAP / 2); // 50% debt allocation
        assertEq(guild.calculateGaugeAllocation(address(term2), _HARDCAP), _HARDCAP / 2); // 50% debt allocation

        // confirm debtCeilings via `debtCeiling()` (with tolerance)
        // debt ceiling is 50% of totalSupply (20_000e18)
        // + 20% of tolerance (10_000e18 + 2_000e18)
        assertEq(term.debtCeiling(), 12_000e18);
        // 20_000e18 + 10_000e18 since issuance == 0
        assertEq(term2.debtCeiling(), 30_000e18);

        // simulate adjustment up via `debtCeiling()`
        assertEq(term.debtCeiling(int256(1000e18)), _HARDCAP); // gauge viewed as having 100% debt allocation after increasing by 1000e18

        // actually increase the weight of the term
        guild.incrementGauge(address(term), 1000e18);
        assertEq(guild.getGaugeWeight(address(term)), 2000e18);
        assertEq(guild.getGaugeWeight(address(term2)), 1000e18);
        assertEq(guild.totalTypeWeight(1), 3000e18);

        // confirm debtCeilings via `calculateGaugeAllocation` (no tolerance)
        assertEq(guild.calculateGaugeAllocation(address(term), _HARDCAP), 13333333333333333333333333); // ~66% debt allocation
        assertEq(guild.calculateGaugeAllocation(address(term2), _HARDCAP), 6666666666666666666666666); // ~33% debt allocation

        // confirm debtCeilings via `debtCeiling()` (with tolerance)
        // totalSupply == 20_000e18
        // ~ 66% of total supply + 20% tolerance == ~ 13_333e18 + 2_666e18
        assertEq(term.debtCeiling(), 16_000e18);
        // maxBorrow == ~ 13_333e18, since issuance == 0
        assertEq(term2.debtCeiling(), 13333333333333333333333);
    }

Recommendation

If a gauge is experiencing an adjustment up, i.e. gaugeWeightDelta > 0, then an updated totalWeight should be considered (totalWeight = totalWeight + gaugeWeightDelta).

c4-judge commented 5 months ago

Trumpero marked the issue as satisfactory

c4-judge commented 5 months ago

Trumpero marked the issue as duplicate of #586