Ethereum Credit Guild - rbserver's results

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/e3d3e581e0e51a9ecf3a5a0c4e4bd4af32552fc0/src/governance/LendingTermOffboarding.sol#L116-L148 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/e3d3e581e0e51a9ecf3a5a0c4e4bd4af32552fc0/src/governance/LendingTermOffboarding.sol#L153-L170 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/e3d3e581e0e51a9ecf3a5a0c4e4bd4af32552fc0/src/governance/LendingTermOffboarding.sol#L175-L199

Vulnerability details

Impact

After an offboarding proposal gains enough votes through voters' LendingTermOffboarding.supportOffboard function calls, which can be before POLL_DURATION_BLOCKS after snapshotBlock is reached, canOffboard[term] for the corresponding term changes to true.

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/e3d3e581e0e51a9ecf3a5a0c4e4bd4af32552fc0/src/governance/LendingTermOffboarding.sol#L116-L148

    function supportOffboard(
        uint256 snapshotBlock,
        address term
    ) external whenNotPaused {
        require(
            block.number <= snapshotBlock + POLL_DURATION_BLOCKS,
            "LendingTermOffboarding: poll expired"
        );
        uint256 _weight = polls[snapshotBlock][term];
        require(_weight != 0, "LendingTermOffboarding: poll not found");
        uint256 userWeight = GuildToken(guildToken).getPastVotes(
            msg.sender,
            snapshotBlock
        );
        require(userWeight != 0, "LendingTermOffboarding: zero weight");
        require(
            userPollVotes[msg.sender][snapshotBlock][term] == 0,
            "LendingTermOffboarding: already voted"
        );

        userPollVotes[msg.sender][snapshotBlock][term] = userWeight;
        polls[snapshotBlock][term] = _weight + userWeight;
        if (_weight + userWeight >= quorum) {
            canOffboard[term] = true;
        }
        ...
    }

After the LendingTermOffboarding.offboard function is called for such term, the LendingTermOffboarding.cleanup function can be called to change canOffboard[term] for the corresponding term to false.

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

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

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

        ...
    }

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

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

        ...

        canOffboard[term] = false;
        ...
    }

Before POLL_DURATION_BLOCKS after snapshotBlock is reached for such term, if these LendingTermOffboarding.offboard and LendingTermOffboarding.cleanup functions are called, where snapshotBlock is the same snapshotBlock used in the previous LendingTermOffboarding.supportOffboard function calls for the same term, a user, who has at least 1 vote at the end of snapshotBlock and is not one of the voters who made the previous LendingTermOffboarding.supportOffboard function calls for such term, can call the LendingTermOffboarding.supportOffboard function with the same snapshotBlock for the same term to change such term's canOffboard[term] back to true. This can be done because polls[snapshotBlock][term] remains more than quorum after the LendingTermOffboarding.offboard and LendingTermOffboarding.cleanup functions were called for the corresponding term.

Then, if this offboarded term is re-onboarded in the future, the LendingTermOffboarding.offboard and LendingTermOffboarding.cleanup functions can be directly called without any LendingTermOffboarding.supportOffboard function calls. In this case, even if only less than enough voters support offboarding the corresponding term at that time, such term can still be offboarded unexpectedly.

Proof of Concept

The following steps can occur.

1. An offboarding proposal is created for a term at block.number being 12345678.
2. Multiple voters called the LendingTermOffboarding.supportOffboard function with snapshotBlock being 12345678 for the same term, which changes such term's canOffboard[term] to true after the quorum is reached. This is done before POLL_DURATION_BLOCKS after snapshotBlock is reached.
3. Also before POLL_DURATION_BLOCKS after snapshotBlock, which is 12345678, is reached, the LendingTermOffboarding.offboard and LendingTermOffboarding.cleanup functions are called so the corresponding term's canOffboard[term] is changed to false.
4. Again before POLL_DURATION_BLOCKS after snapshotBlock, which is 12345678, is reached, a user, who has just 1 vote at the end of block 12345678 and is not one of the voters who called the LendingTermOffboarding.supportOffboard function in Step 2, can call the LendingTermOffboarding.supportOffboard function with snapshotBlock being 12345678 for the same term. This changes such term's canOffboard[term] back to true.
5. In the future, such term is re-onboarded but the LendingTermOffboarding.offboard and LendingTermOffboarding.cleanup functions can be called directly without any LendingTermOffboarding.supportOffboard function calls. Although only less than enough voters support offboarding the corresponding term at that time, such term can still be offboarded.

Tools Used

Manual Review

Recommended Mitigation Steps

After a term is offboarded through the LendingTermOffboarding.offboard function call, such term can be locked for a reasonable duration, such as a period of time that equals the duration for POLL_DURATION_BLOCKS. During such lock time period, the LendingTermOffboarding.supportOffboard function would not be allowed to be called for such term.

Assessed type

Context

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/e3d3e581e0e51a9ecf3a5a0c4e4bd4af32552fc0/src/governance/ProfitManager.sol#L292-L405 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/e3d3e581e0e51a9ecf3a5a0c4e4bd4af32552fc0/src/tokens/GuildToken.sol#L242-L261 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/e3d3e581e0e51a9ecf3a5a0c4e4bd4af32552fc0/src/governance/ProfitManager.sol#L409-L436 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/e3d3e581e0e51a9ecf3a5a0c4e4bd4af32552fc0/src/tokens/GuildToken.sol#L207-L234

Vulnerability details

Impact

When a profit is reported for a gauge through calling the following ProfitManager.notifyPnL function, gaugeProfitIndex[gauge] can be updated to be greater than 1e18 if both amountForGuild and _gaugeWeight are not 0 for the corresponding gauge.

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/e3d3e581e0e51a9ecf3a5a0c4e4bd4af32552fc0/src/governance/ProfitManager.sol#L292-L405

    function notifyPnL(
        address gauge,
        int256 amount
    ) external onlyCoreRole(CoreRoles.GAUGE_PNL_NOTIFIER) {
        ...
        // handling loss
        if (amount < 0) {
            ...
        }
        // handling profit
        else if (amount > 0) {
            ...
            // distribute to the guild
            if (amountForGuild != 0) {
                ...
                uint256 _gaugeWeight = uint256(
                    GuildToken(guild).getGaugeWeight(gauge)
                );
                if (_gaugeWeight != 0) {
                    uint256 _gaugeProfitIndex = gaugeProfitIndex[gauge];
                    if (_gaugeProfitIndex == 0) {
                        _gaugeProfitIndex = 1e18;
                    }
                    gaugeProfitIndex[gauge] =
                        _gaugeProfitIndex +
                        (amountForGuild * 1e18) /
                        _gaugeWeight;
                }
            }
        }
        ...
    }

After a profit is reported for such gauge and before all of the users, who have voted for such gauge prior to that the profit is reported, claim their rewards, a malicious actor, who has not voted for such gauge before, can vote for it through calling the following GuildToken._incrementGaugeWeight function, which further calls the ProfitManager.claimGaugeRewards function below for the same gauge. At this moment, the malicious actor's weight for such gauge is still 0 so the ProfitManager.claimGaugeRewards function would just return 0 and does not perform any other actions.

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/e3d3e581e0e51a9ecf3a5a0c4e4bd4af32552fc0/src/tokens/GuildToken.sol#L242-L261

    function _incrementGaugeWeight(
        address user,
        address gauge,
        uint256 weight
    ) internal override {
        ...

        ProfitManager(profitManager).claimGaugeRewards(user, gauge);

        ...
    }

Then, the malicious actor can immediately call the ProfitManager.claimGaugeRewards function directly or the GuildToken._decrementGaugeWeight function that also calls the ProfitManager.claimGaugeRewards function. When calling the ProfitManager.claimGaugeRewards function at this moment, the malicious actor's weight for such gauge is no longer 0, userGaugeProfitIndex[user][gauge] is 0 that causes _userGaugeProfitIndex to be set to 1e18, and gaugeProfitIndex[gauge], which is _gaugeProfitIndex, is greater than 1e18 since the profit was reported for such gauge already. Because _gaugeProfitIndex is greater than _userGaugeProfitIndex, deltaIndex and creditEarned would both be positive. Hence, the malicious actor can receive creditEarned credit tokens even though she or he has only voted after the profit is reported for such gauge; moreover, calling the GuildToken._decrementGaugeWeight function immediately after calling the GuildToken._incrementGaugeWeight function can remove all of the malicious actor's votes for such gauge and eliminate the risk of having her or his votes slashed in case a loss occurs for such gauge.

In this situation, since the users, who have voted for such gauge before the profit is reported, have not claimed their rewards, if the malicious actor increments a weight that equals the total weight voted by these users for such gauge, she or he can receive a creditEarned credit token amount that equals the total gauge rewards that these users are entitled to when her or his ProfitManager.claimGaugeRewards function call finishes. Essentially, these users' rewards are stolen by the malicious actor successfully.

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/e3d3e581e0e51a9ecf3a5a0c4e4bd4af32552fc0/src/governance/ProfitManager.sol#L409-L436

    function claimGaugeRewards(
        address user,
        address gauge
    ) public returns (uint256 creditEarned) {
        uint256 _userGaugeWeight = uint256(
            GuildToken(guild).getUserGaugeWeight(user, gauge)
        );
        if (_userGaugeWeight == 0) {
            return 0;
        }
        uint256 _gaugeProfitIndex = gaugeProfitIndex[gauge];
        uint256 _userGaugeProfitIndex = userGaugeProfitIndex[user][gauge];
        if (_gaugeProfitIndex == 0) {
            _gaugeProfitIndex = 1e18;
        }
        if (_userGaugeProfitIndex == 0) {
            _userGaugeProfitIndex = 1e18;
        }
        uint256 deltaIndex = _gaugeProfitIndex - _userGaugeProfitIndex;
        if (deltaIndex != 0) {
            creditEarned = (_userGaugeWeight * deltaIndex) / 1e18;
            userGaugeProfitIndex[user][gauge] = _gaugeProfitIndex;
        }
        if (creditEarned != 0) {
            emit ClaimRewards(block.timestamp, user, gauge, creditEarned);
            CreditToken(credit).transfer(user, creditEarned);
        }
    }

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/e3d3e581e0e51a9ecf3a5a0c4e4bd4af32552fc0/src/tokens/GuildToken.sol#L207-L234

    function _decrementGaugeWeight(
        address user,
        address gauge,
        uint256 weight
    ) internal override {
        ...

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

        ...
    }

Proof of Concept

The following steps can occur.

1. Alice increments Gauge A by a weight of 15e18.
2. Bob increments Gauge A by a weight of 5e18.
3. A profit is reported for Gauge A in which amountForGuild is calculated to be 25e18 in the ProfitManager contract. The corresponding credit token amount is also sent to the ProfitManager contract.
4. Alice and Bob have not yet claimed their gauge rewards.
5. Charlie increments Gauge A by a weight of 20e18, which is the sum of Alice's and Bob's weights for the same gauge.
6. Charlie decrements Gauge A by a weight of 20e18. After calling the GuildToken._decrementGaugeWeight function, which calls the ProfitManager.claimGaugeRewards function, Charlie receives 25e18 credit tokens.
7. As a result, all of Alice's and Bob's gauge rewards are stolen by Charlie.

Tools Used

Manual Review

Recommended Mitigation Steps

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/e3d3e581e0e51a9ecf3a5a0c4e4bd4af32552fc0/src/governance/ProfitManager.sol#L409-L436 can be updated to the following code.

function claimGaugeRewards(
	address user,
	address gauge
) public returns (uint256 creditEarned) {
	uint256 _userGaugeWeight = uint256(
		GuildToken(guild).getUserGaugeWeight(user, gauge)
	);

	uint256 _gaugeProfitIndex = gaugeProfitIndex[gauge];
	if (_gaugeProfitIndex == 0) {
		_gaugeProfitIndex = 1e18;
	}

	if (_userGaugeWeight == 0) {
		userGaugeProfitIndex[user][gauge] = _gaugeProfitIndex;
		return 0;
	}

	uint256 _userGaugeProfitIndex = userGaugeProfitIndex[user][gauge];

	uint256 deltaIndex = _gaugeProfitIndex - _userGaugeProfitIndex;
	if (deltaIndex != 0) {
		creditEarned = (_userGaugeWeight * deltaIndex) / 1e18;
		userGaugeProfitIndex[user][gauge] = _gaugeProfitIndex;
	}
	if (creditEarned != 0) {
		emit ClaimRewards(block.timestamp, user, gauge, creditEarned);
		CreditToken(credit).transfer(user, creditEarned);
	}
}

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/e3d3e581e0e51a9ecf3a5a0c4e4bd4af32552fc0/src/governance/LendingTermOnboarding.sol#L105-L168 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/e3d3e581e0e51a9ecf3a5a0c4e4bd4af32552fc0/src/loan/LendingTerm.sol#L200-L233 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/e3d3e581e0e51a9ecf3a5a0c4e4bd4af32552fc0/src/loan/LendingTerm.sol#L490-L559

Vulnerability details

Impact

In the LendingTermOnboarding.createTerm function, both params.interestRate and params.openingFee can be set to 0 for creating an interest-free loan term, which is a possible real life use case. Such interest-free loan term can require partial repayments since params.maxDelayBetweenPartialRepay and params.minPartialRepayPercent can be set to positive values in the LendingTermOnboarding.createTerm function.

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/e3d3e581e0e51a9ecf3a5a0c4e4bd4af32552fc0/src/governance/LendingTermOnboarding.sol#L105-L168

    function createTerm(
        address implementation,
        LendingTerm.LendingTermParams calldata params
    ) external returns (address) {
        ...

        require(
            params.interestRate < 1e18, // interest rate [0, 100[% APR
            "LendingTermOnboarding: invalid interestRate"
        );


        require(
            // 31557601 comes from the constant LendingTerm.YEAR() + 1
            params.maxDelayBetweenPartialRepay < 31557601, // periodic payment every [0, 1 year]
            "LendingTermOnboarding: invalid maxDelayBetweenPartialRepay"
        );


        require(
            params.minPartialRepayPercent < 1e18, // periodic payment sizes [0, 100[%
            "LendingTermOnboarding: invalid minPartialRepayPercent"
        );


        require(
            params.openingFee <= 0.1e18, // open fee expected [0, 10]%
            "LendingTermOnboarding: invalid openingFee"
        );

        ...
    }

For each loan of such interest-free loan term, the following LendingTerm.getLoanDebt function would return the loan's loanDebt, which is (loanDebt * loan.borrowCreditMultiplier) / creditMultiplier where the loanDebt in the multiplication is just its borrowAmount.

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/e3d3e581e0e51a9ecf3a5a0c4e4bd4af32552fc0/src/loan/LendingTerm.sol#L200-L233

    function getLoanDebt(bytes32 loanId) public view returns (uint256) {
        Loan storage loan = loans[loanId];
        ...


        // compute interest owed
        uint256 borrowAmount = loan.borrowAmount;
        uint256 interest = (borrowAmount *
            params.interestRate *
            (block.timestamp - borrowTime)) /
            YEAR /
            1e18;
        uint256 loanDebt = borrowAmount + interest;
        uint256 _openingFee = params.openingFee;
        if (_openingFee != 0) {
            loanDebt += (borrowAmount * _openingFee) / 1e18;
        }
        uint256 creditMultiplier = ProfitManager(refs.profitManager)
            .creditMultiplier();
        loanDebt = (loanDebt * loan.borrowCreditMultiplier) / creditMultiplier;


        return loanDebt;
    }

Because such interest-free loan term requires partial repayments, its borrowers need to call the LendingTerm._partialRepay function to repay partially. In the LendingTerm._partialRepay function, principal is calculated to be (borrowAmount * loan.borrowCreditMultiplier) / creditMultiplier, which is same as loanDebt returned by the LendingTerm.getLoanDebt function for each loan of such interest-free loan term. This means principalRepaid and debtToRepay would be the same, which causes interestRepaid to be 0. Because the require statement checks interestRepaid != 0, calling the LendingTerm._partialRepay function for each loan of such interest-free loan term would revert.

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/e3d3e581e0e51a9ecf3a5a0c4e4bd4af32552fc0/src/loan/LendingTerm.sol#L490-L559

    function _partialRepay(
        address repayer,
        bytes32 loanId,
        uint256 debtToRepay
    ) internal {
        Loan storage loan = loans[loanId];


        // check the loan is open
        uint256 borrowTime = loan.borrowTime;
        require(borrowTime != 0, "LendingTerm: loan not found");
        require(
            borrowTime < block.timestamp,
            "LendingTerm: loan opened in same block"
        );
        require(loan.closeTime == 0, "LendingTerm: loan closed");
        require(loan.callTime == 0, "LendingTerm: loan called");


        // compute partial repayment
        uint256 loanDebt = getLoanDebt(loanId);
        require(debtToRepay < loanDebt, "LendingTerm: full repayment");
        uint256 percentRepaid = (debtToRepay * 1e18) / loanDebt; // [0, 1e18[
        uint256 borrowAmount = loan.borrowAmount;
        uint256 creditMultiplier = ProfitManager(refs.profitManager)
            .creditMultiplier();
        uint256 principal = (borrowAmount * loan.borrowCreditMultiplier) /
            creditMultiplier;
        uint256 principalRepaid = (principal * percentRepaid) / 1e18;
        uint256 interestRepaid = debtToRepay - principalRepaid;
        uint256 issuanceDecrease = (borrowAmount * percentRepaid) / 1e18;
        require(
            principalRepaid != 0 && interestRepaid != 0,
            "LendingTerm: repay too small"
        );
        ...
    }

As a result, borrowers of such interest-free loan term, which requires partial repayments, who can only afford to repay partially, cannot make partial repayments in time. After the maximum delay between partial repayments is reached, such term's loans can be called, and these borrowers' collaterals would be auctioned, which cause them to lose their collaterals.

Proof of Concept

The following steps can occur.

1. Term A requires no interest or opening fee but does require partial repayments.
2. Alice borrows from Term A by transferring some collateral tokens to Term A.
3. After some time, because Alice can only afford to repay her loan of Term A partially, she calls the LendingTerm._partialRepay function to make a partial repayment for her loan of Term A.
4. Calling the LendingTerm._partialRepay function reverts for Alice's loan of Term A since debtToRepay would not contain any interest, which causes Alice to not be able to make a partial repayment in time.
5. After the maximum delay between partial repayments is reached for Alice's loan of Term A, Bob can call such loan and auction Alice's collateral.
6. Although Alice has enough credit tokens to make a partial repayment for her loan of Term A, she still loses her collateral.

Tools Used

Manual Review

Recommended Mitigation Steps

The LendingTerm._partialRepay function can be updated to not revert when interestRepaid is 0 if the corresponding loan term does not require any interest or opening fee.

Assessed type

Context

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/e3d3e581e0e51a9ecf3a5a0c4e4bd4af32552fc0/src/loan/LendingTerm.sol#L270-L331 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/e3d3e581e0e51a9ecf3a5a0c4e4bd4af32552fc0/src/tokens/GuildToken.sol#L207-L234

Vulnerability details

Impact

After _debtCeiling is calculated, the LendingTerm.debtCeiling function compares it to creditMinterBuffer first and then to _hardCap if creditMinterBuffer < _debtCeiling is false. However, it does not further compare creditMinterBuffer and _hardCap. As its comment, // return min(creditMinterBuffer, hardCap, debtCeiling), mentions, the LendingTerm.debtCeiling function should return the minimum of creditMinterBuffer, _hardCap, and debtCeiling. This is not the case if creditMinterBuffer is greater than _hardCap and creditMinterBuffer is less than _debtCeiling, which would incorrectly return creditMinterBuffer, when it should return _hardCap instead.

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/e3d3e581e0e51a9ecf3a5a0c4e4bd4af32552fc0/src/loan/LendingTerm.sol#L270-L331

    function debtCeiling(
        int256 gaugeWeightDelta
    ) public view returns (uint256) {
        ...
        uint256 _debtCeiling = _issuance + maxBorrow;
        // return min(creditMinterBuffer, hardCap, debtCeiling)
        if (creditMinterBuffer < _debtCeiling) {
            return creditMinterBuffer;
        }
        if (_hardCap < _debtCeiling) {
            return _hardCap;
        }
        return _debtCeiling;
    }

In this case, in the GuildToken._decrementGaugeWeight function, debtCeilingAfterDecrement is set to the incorrectly returned creditMinterBuffer instead of _hardCap. If the corresponding gauge's issuance is more than _hardCap but less than creditMinterBuffer, calling the GuildToken._decrementGaugeWeight function would not revert when calling it should revert. As a result, users can decrement their weights for such gauge when they should not be allowed to.

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/e3d3e581e0e51a9ecf3a5a0c4e4bd4af32552fc0/src/tokens/GuildToken.sol#L207-L234

    function _decrementGaugeWeight(
        address user,
        address gauge,
        uint256 weight
    ) internal override {
        ...

        // 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) {
            uint256 debtCeilingAfterDecrement = LendingTerm(gauge).debtCeiling(-int256(weight));
            require(
                issuance <= debtCeilingAfterDecrement,
                "GuildToken: debt ceiling used"
            );
        }


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

Proof of Concept

The following steps can occur.

1. Alice calls the GuildToken._decrementGaugeWeight function to decrement her weight for Gauge A.
2. In the LendingTerm.debtCeiling function, creditMinterBuffer is 1e21, _hardCap is 0.998e21, and _debtCeiling is calculated to be 1.001e21 for Gauge A. min(creditMinterBuffer, hardCap, debtCeiling) should be _hardCap that is 0.998e21.
3. In the GuildToken._decrementGaugeWeight function, issuance is 0.999e21, which is less than creditMinterBuffer, which is 1e21, that is incorrectly returned by the LendingTerm.debtCeiling function. As a result, Alice's GuildToken._decrementGaugeWeight function call does not revert, and she is able to decrement her weight for Gauge A.
4. If the LendingTerm.debtCeiling function can correctly return _hardCap, which is 0.998e21, that is min(creditMinterBuffer, hardCap, debtCeiling), such issuance is greater than _hardCap so Alice's GuildToken._decrementGaugeWeight function call should revert, and she actually should not be allowed to decrement her weight for Gauge A.

Tools Used

Manual Review

Recommended Mitigation Steps

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/e3d3e581e0e51a9ecf3a5a0c4e4bd4af32552fc0/src/loan/LendingTerm.sol#L323-L330 can be updated to the following code.

        // return min(creditMinterBuffer, hardCap, debtCeiling)
        uint256 _min = _hardCap < creditMinterBuffer ? _hardCap : creditMinterBuffer;
        if (_min < _debtCeiling) {
            return _min;
        }
        return _debtCeiling;

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/e3d3e581e0e51a9ecf3a5a0c4e4bd4af32552fc0/src/tokens/GuildToken.sol#L207-L234 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/e3d3e581e0e51a9ecf3a5a0c4e4bd4af32552fc0/src/loan/LendingTerm.sol#L270-L331

Vulnerability details

Impact

The following GuildToken._decrementGaugeWeight function calls the LendingTerm.debtCeiling function to set debtCeilingAfterDecrement and reverts if the corresponding gauge's issuance does not exceed debtCeilingAfterDecrement.

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/e3d3e581e0e51a9ecf3a5a0c4e4bd4af32552fc0/src/tokens/GuildToken.sol#L207-L234

    function _decrementGaugeWeight(
        address user,
        address gauge,
        uint256 weight
    ) internal override {
        ...

        // 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) {
            uint256 debtCeilingAfterDecrement = LendingTerm(gauge).debtCeiling(-int256(weight));
            require(
                issuance <= debtCeilingAfterDecrement,
                "GuildToken: debt ceiling used"
            );
        }


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

The following LendingTerm.debtCeiling function has this code comment that is // one gauge, unlimited debt ceiling // returns min(hardCap, creditMinterBuffer) and does return _hardCap < creditMinterBuffer ? _hardCap : creditMinterBuffer if gaugeWeight == totalWeight is true. Here, gaugeWeight is uint256(int256(gaugeWeight) + gaugeWeightDelta), where the gaugeWeight that is a part of the summation is GuildToken(_guildToken).getGaugeWeight(address(this)); because of the addition of gaugeWeightDelta, the resulting gaugeWeight can be less than the corresponding gauge's weight when gaugeWeightDelta is negative. Yet, since totalWeight is GuildToken(_guildToken).totalTypeWeight(gaugeType) that is not adjusted by gaugeWeightDelta, totalWeight is just the total weight of the corresponding gauge's type. Therefore, when the corresponding gauge is the only gauge for its type, gaugeWeight == totalWeight would be false. In this case, if debtCeilingBefore based on the negative gaugeWeightDelta is calculated to be less than issuance, the LendingTerm.debtCeiling function would return debtCeilingBefore, which would cause issuance <= debtCeilingAfterDecrement to be false in the GuildToken._decrementGaugeWeight function. In this case, calling the GuildToken._decrementGaugeWeight function reverts.

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/e3d3e581e0e51a9ecf3a5a0c4e4bd4af32552fc0/src/loan/LendingTerm.sol#L270-L331

    function debtCeiling(
        int256 gaugeWeightDelta
    ) public view returns (uint256) {
        address _guildToken = refs.guildToken; // cached SLOAD
        uint256 gaugeWeight = GuildToken(_guildToken).getGaugeWeight(
            address(this)
        );
        gaugeWeight = uint256(int256(gaugeWeight) + gaugeWeightDelta);
        uint256 gaugeType = GuildToken(_guildToken).gaugeType(address(this));
        uint256 totalWeight = GuildToken(_guildToken).totalTypeWeight(
            gaugeType
        );
        uint256 creditMinterBuffer = RateLimitedMinter(refs.creditMinter)
            .buffer();
        uint256 _hardCap = params.hardCap; // cached SLOAD
        if (gaugeWeight == 0) {
            return 0; // no gauge vote, 0 debt ceiling
        } else if (gaugeWeight == totalWeight) {
            // one gauge, unlimited debt ceiling
            // returns min(hardCap, creditMinterBuffer)
            return
                _hardCap < creditMinterBuffer ? _hardCap : creditMinterBuffer;
        }
        uint256 _issuance = issuance; // cached SLOAD
        uint256 totalBorrowedCredit = ProfitManager(refs.profitManager)
            .totalBorrowedCredit();
        uint256 gaugeWeightTolerance = ProfitManager(refs.profitManager)
            .gaugeWeightTolerance();
        if (totalBorrowedCredit == 0 && gaugeWeight != 0) {
            // first-ever CREDIT mint on a non-zero gauge weight term
            // does not check the relative debt ceilings
            // returns min(hardCap, creditMinterBuffer)
            return
                _hardCap < creditMinterBuffer ? _hardCap : creditMinterBuffer;
        }
        uint256 toleratedGaugeWeight = (gaugeWeight * gaugeWeightTolerance) /
            1e18;
        uint256 debtCeilingBefore = (totalBorrowedCredit *
            toleratedGaugeWeight) / totalWeight;
        if (_issuance >= debtCeilingBefore) {
            return debtCeilingBefore; // no more borrows allowed
        }
        ...
    }

If the LendingTerm.debtCeiling function could correctly return min(hardCap, creditMinterBuffer) since the corresponding gauge is the only gauge of its type, then calling the GuildToken._decrementGaugeWeight function would not revert given that min(hardCap, creditMinterBuffer) could be greater than such gauge's issuance. Because totalWeight is not adjusted by gaugeWeightDelta when comparing gaugeWeight to it, the LendingTerm.debtCeiling function can incorrectly return a debt ceiling that is less than min(hardCap, creditMinterBuffer) when the corresponding gauge is the only gauge of its type, which can cause users to be unable to decrement their weights for such gauge when they should be allowed to.

Proof of Concept

The following steps can occur.

1. Gauge A is the only gauge of its type at this moment.
2. Alice calls GuildToken._decrementGaugeWeight function to decrement her weight, which is 10e18, for Gauge A.
3. When the LendingTerm.debtCeiling function is called with gaugeWeightDelta being -10e18, Gauge A's weight is reduced by 10e18 to calculate gaugeWeight but totalWeight is not reduced by 10e18. Because gaugeWeight == totalWeight is not true, the LendingTerm.debtCeiling function would not return _hardCap < creditMinterBuffer ? _hardCap : creditMinterBuffer.
4. In the LendingTerm.debtCeiling function, debtCeilingBefore can be calculated to be less than this gauge's issuance since gaugeWeightDelta is negative.
5. After the LendingTerm.debtCeiling function returns such debtCeilingBefore, issuance <= debtCeilingAfterDecrement would be false in the GuildToken._decrementGaugeWeight function, which causes Alice's GuildToken._decrementGaugeWeight function call to revert.
6. As a result, Alice is unable to decrement her weight for Gauge A when she should be allowed to since min(hardCap, creditMinterBuffer) is actually greater than Gauge A's issuance at this moment.

Tools Used

Manual Review

Recommended Mitigation Steps

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/e3d3e581e0e51a9ecf3a5a0c4e4bd4af32552fc0/src/loan/LendingTerm.sol#L287-L292 can be updated to the following code.

        } else if (gaugeWeight == uint256(int256(totalWeight) + gaugeWeightDelta)) {
            // one gauge, unlimited debt ceiling
            // returns min(hardCap, creditMinterBuffer)
            return
                _hardCap < creditMinterBuffer ? _hardCap : creditMinterBuffer;
        }

Assessed type

Other