Ethereum Credit Guild - niroh's results

Lines of code

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

Vulnerability details

Description

The debtCeiling function uses the minimum of hardCap and credit minter remaining buffer (creditMinterBuffer) as a hard limit on the term's debtCeiling in two places:
line 298

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;
        }

Line 324

 if (creditMinterBuffer < _debtCeiling) {
            return creditMinterBuffer;
        }

However, unlike hardCap, the creditMinterBuffer-induced limit should be issuance + creditMinterBuffer instead. This is because the buffer is a limit on additional borrows, not on the total of current issuance and additional borrows. This error triggers a revert in GuildToken::_decrementGaugeWeight whenever a gauge's current issuance exceeds the remaining buffer. This occurs even if the post-decrement true debtCeiling is above the issuance. Consequently, guild voters and surplusGuildMinder stakers are unjustifiably prevented from removing their votes/stakes from a gauge. This situation likely to occur naturally when borrowing demand is high, or as a DOS attack where a malicious actor borrows the sum needed to keep a term's issuance above the remaining buffer, blocking voters and surplus stakers from exising their positions.

Impact

Preventing guild voters/surplus stakers from removing votes/stakes from gauges they see as unsafe breaks the main system mechanism through which term quality is maintained and risk is reduced. In addition, voters/surplus stakers who are unable to exit their positions from term they consider too risky may later have those votes slashed when the term inccurs a loss, causing them considerable financial damage.

Proof of Concept

This POC shows how a single term with a single voter can not remove 2% of the votes because the term's issuance is above the current buffer. Note: there is another issue with debtCeiling() (see my other submission on the matter) that would prevent removing more than 16.666% of the votes of a single-term market, however removing 2% should have succedded inspite of the other issue.

To run: Add the code below to the test file integrationTestBadDebtFlows.sol. forge test --match-test 'testRemoveVotesSingleTerm' --fork-url $ETH_RPC_URL -vvv

function testDebtCeilingBufferError() public {
    //causes this contract to vote on term
    testAllocateGaugeToSDAI();

    //borrow 51% of the credit buffer to simulate issuance being above
    //remaining buffer
    uint256 borrowAmount = rateLimitedCreditMinter.buffer() * 51 / 100;
    uint128 supplyAmount = uint128(borrowAmount);
    bytes32 loanId = _supplyCollateralUserOne(borrowAmount, supplyAmount);

    //try to remove 2%  of the vote
    uint256 decrementAmount = guild.balanceOf(address(this)) * 2 / 100;
    vm.expectRevert("GuildToken: debt ceiling used");
    guild.decrementGauge(address(term), decrementAmount);

    //Reverts due to finding error. Decrementing 2% should succedd in the case
    //of a single term but fails because current issuance is above the remaining buffer.
}

Tools Used

Foundry

Recommended Mitigation Steps

In debtCeiling() use issuance + creditMinterBuffer instead of creditMinterBuffer.

Assessed type

Other

Lines of code

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

Vulnerability details

Description

The debtCeiling() function in LendingTerm aims to calculate a terms debt ceiling given gauge allocations after gaugeWeightDelta is applied to the term. However, the function only apllies the delta to gaugeWeight and fails to do so to totalWeight:

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
);

The resulting errouneous debtCeiling mainly affects GuildToken::_decrementGaugeWeight where the ceiling is used to determine weather or not decrementing weight will result in the term issuance being above its post-decrement ceiling, and reverts if it does. The POC shows how this reverts a weight decrement that should have succeeded in the case where a single term exists in the system (though the problem occurs also with multiple terms).

Impact

Wrongfully preventing guild voters/surplus stakers from removing votes/stakes from gauges they see as unsafe breaks the main system mechanism through which term quality is maintained and risk is reduced. In addition, voters/surplus stakers who are unable to remove their votes/stakes from a term they deem too risky may later have those votes slashed when the term inccurs a loss, causing them considerable financial damage.

Proof of Concept

This POC shows how with one term in the system and 100e18 issuance, removing 20% of the votes fails (even though for a single term with all the votes this should have succeeded as seen here). Note: in the POC example, removing 16.666% of the votes or less would succeed because of the code in line 313 where the gauge tollerance (set to 20%) would 'compensate' for the gaugeWeightDelta that wasn't removed from totalWeight, as long as the delta is up to 16.666%.

To run: Add the code below to the test file integrationTestBadDebtFlows.sol. forge test --match-test 'testRemoveVotesSingleTerm' --fork-url $ETH_RPC_URL -vvv

function testRemoveVotesSingleTerm() public {
        //causes this contract to vote all its guild balance on term
        testAllocateGaugeToSDAI();

        uint256 borrowAmount = 100e18;
        uint128 supplyAmount = 100e18;
        /// supply collateral and borrow
        bytes32 loanId = _supplyCollateralUserOne(borrowAmount, supplyAmount);

        //try to remove 20%  of the vote
         uint256 decrementAmount = guild.balanceOf(address(this)) * 20 / 100;
        vm.expectRevert("GuildToken: debt ceiling used");
        guild.decrementGauge(address(term), decrementAmount);
    }

Tools Used

Foundry

Recommended Mitigation Steps

Adjust totalWeight with the delta by adding the following line: totalWeight = uint256(int256(totalWeight) + gaugeWeightDelta);

Assessed type

DoS