Ethereum Credit Guild - 0xbepresent's results

Lines of code

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

Vulnerability details

Impact

GUILD holders can cause the removal of an active lending term by initially proposing its removal with LendingTermOffboarding::proposeOffboard() and subsequently voting in favor of it with LendingTermOffboarding::supportOffboard(). Once a quorum has been reached the term can be removed (canOffboard[term] = true) with the help of the function LendingTermOffboarding::offboard(). Moreover, a previously removed term can be reinstated, as indicated in the following comment::

// note that terms that have been offboarded in the past can be re-onboarded
// and won't fail this check. This is intentional, because some terms might be offboarded
// due to specific market conditions, and it might be desirable to re-onboard them
// at a later date.

The problem emerges when a term is added again after being previously removed, as the canOffboard[term] variable remains true, enabling subsequent calls of the LendingTermOffboarding::offboard() function without a proposal and voting process.

File: LendingTermOffboarding.sol
153:     function offboard(address term) external whenNotPaused {
154:         require(canOffboard[term], "LendingTermOffboarding: quorum not met");
155: 
156:         // update protocol config
157:         // this will revert if the term has already been offboarded
158:         // through another mean.
159:         GuildToken(guildToken).removeGauge(term);
160: 
161:         // pause psm redemptions
162:         if (
163:             nOffboardingsInProgress++ == 0 &&
164:             !SimplePSM(psm).redemptionsPaused()
165:         ) {
166:             SimplePSM(psm).setRedemptionsPaused(true);
167:         }
168: 
169:         emit Offboard(block.timestamp, term);
170:     }

In the event that a previously removed term is added again, there may be market variables that require a proposal and voting process to be restarted so that the lending term can be removed again. Currently, anyone can remove a term that has been added again, thus affecting the loans corresponding to that term.

Proof of Concept

The next test shows how a term is initially removed and then reinstated, but a malicious actor (address(1337)) is able to remove again the term even without having a vote.

1. Propose, vote and offBoard a term.
2. Time goes by 1000 blocks.
3. For some reason, the term need to be available again. Re-onboard the same term.
4. Attacker in the same block/time can remove the term again offBoard without a propose/vote process, affecting the lending term.

// File: test/unit/governance/LendingTermOffboarding.t.sol:LendingTermOffboardingUnitTest
// $ forge test --match-test "testOffboardAfterReonboard" -vvv
//
    function testOffboardAfterReonboard() public {
        // If a term has been removed and then added again (`onBoard` -> `offBoard` -> `onBoard`),
        // attacker can call `offBoard` again without needing to propose/vote again.
        // Mitigation: Once an term is removed `offBoard`, if the same term is added again `onBoarding`,
        // it is necessary to create again a `offBoarding` proposing/voting process if needed.
        //
        // 1. Propose, vote and offBoard a term.
        guild.mint(bob, _QUORUM);
        vm.startPrank(bob);
        guild.delegate(bob);
        uint256 snapshotBlock = block.number;
        offboarder.proposeOffboard(address(term));
        vm.roll(block.number + 1);
        vm.warp(block.timestamp + 13);
        offboarder.supportOffboard(snapshotBlock, address(term));
        offboarder.offboard(address(term));
        vm.stopPrank();
        //
        // 2. Time goes by 1000 blocks.
        vm.roll(block.number + 1 * 1000);
        vm.warp(block.timestamp + 13 * 1000);
        //
        // 3. For some reason, the term need to be available again. Re-onboard the same term
        guild.addGauge(1, address(term));
        //
        // 4. Attacker in the same block/time can remove the term again `offBoard` without a propose/vote process, affecting the lending term.
        vm.prank(address(1337));
        offboarder.offboard(address(term));
    }

Tools used

Manual review

Recommended Mitigation Steps

Upon a lending term removal and reinstatement, a restart of the proposal and voting process is necessary for its subsequent removal.

Assessed type

Governance

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/tokens/ERC20Gauges.sol#L219 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/ProfitManager.sol#L292

Vulnerability details

Impact

When a user votes for a gauge using the function GuildToken::incrementGauge(), the user votes are counted to calculate the rewards assigned to the voters for that gauge. The profit can be when there are loan repayments with interest. On the other hand, a loss notification will result in gauge slashing at GuildToken::applyGaugeLoss(), causing the voter to lose the guild tokens.

A problem arises when a malicious user can increase their votes for a gauge in the same block that a profit is reported for that gauge, thereby obtaining rewards without being exposed to the slashing due to losses. The malicious user can monitor the transactions and increase their votes to a gauge only when there are profits, then they could exit and prevent it from being slashed in the future if that were the case.

Proof of Concept

The following test, the malicious user increases the votes to a gauge just before the profits are reported for that gauge, then obtains the rewards and then decreases his votes, steps of the test:

1. Alice increments gauge1 for 50e18.
2. Simulate 20 profit on gauge1.
3. Alice claim rewards.
4. Alice decrements gague1 in order to avoid to be slashed in the future.

// File: ProfitManager.t.sol
// $ forge test --match-test "testCanIncrementAndClaimPendingRewardsInSameBlock" -vvv
//
    function testCanIncrementAndClaimPendingRewardsInSameBlock() public {
        // Alice can increment gauge votes in the same block as the profit is notified, 
        // causing Alice to be able to obtain rewards without having been exposed to slashs
        //
        // grant roles to test contract
        vm.startPrank(governor);
        core.grantRole(CoreRoles.GOVERNOR, address(this));
        core.grantRole(CoreRoles.CREDIT_MINTER, address(this));
        core.grantRole(CoreRoles.GUILD_MINTER, address(this));
        core.grantRole(CoreRoles.GAUGE_ADD, address(this));
        core.grantRole(CoreRoles.GAUGE_PARAMETERS, address(this));
        core.grantRole(CoreRoles.GAUGE_PNL_NOTIFIER, address(this));
        vm.stopPrank();
        // setup
        // 50-50 profit split between GUILD & CREDIT
        uint256 initialAliceCreditAmount = 50e18;
        credit.mint(alice, initialAliceCreditAmount);
        vm.prank(governor);
        profitManager.setProfitSharingConfig(
            0, // surplusBufferSplit
            0.5e18, // creditSplit
            0.5e18, // guildSplit
            0, // otherSplit
            address(0) // otherRecipient
        );
        guild.setMaxGauges(1);
        guild.addGauge(1, gauge1);
        guild.mint(alice, 150e18);
        //
        // 1. Alice increments gauge1 50e18
        vm.prank(alice);
        guild.incrementGauge(gauge1, 50e18);
        assertEq(guild.numUserGauges(alice), 1);
        assertEq(guild.getUserWeight(alice), 50e18);
        //
        // 2. Simulate 20 profit on gauge1
        //    10 goes to alice (guild voting)
        //    10 goes to test (rebasing credit)
        credit.mint(address(profitManager), 20e18);
        profitManager.notifyPnL(gauge1, 20e18);
        //
        // 3. Alice claim rewards
        (address[] memory aliceGauges, uint256[] memory aliceGaugeRewards, uint256 aliceTotalRewards) = profitManager.getPendingRewards(alice);
        assertEq(aliceGauges.length, 1);
        assertEq(aliceGauges[0], gauge1);
        assertEq(aliceGaugeRewards.length, 1);
        uint256 rewardsCredit = 10e18;
        assertEq(aliceGaugeRewards[0], rewardsCredit);
        assertEq(aliceTotalRewards, rewardsCredit);
        assertEq(profitManager.claimRewards(alice), rewardsCredit);
        assertEq(credit.balanceOf(alice), initialAliceCreditAmount + rewardsCredit);
        //
        // 4. Alice decrements gague1 in order to avoid gague to be slashed
        vm.warp(block.timestamp + 13 seconds);
        vm.roll(block.number + 1);
        vm.prank(alice);
        guild.decrementGauge(gauge1, 50e18);
        assertEq(guild.numUserGauges(alice), 0);
        assertEq(guild.getUserWeight(alice), 0);
        assertEq(guild.balanceOf(alice), 150e18);
    }

Tools used

Manual review

Recommended Mitigation Steps

The malicious user can perform Staking in SurplusGuildMinter, thereby acquiring gauge votes and subsequently rewards. Therefore the rewards should be based on the user's staking duration. On the other hand, if guild tokens transfers are activated, the rewards should be calculated based on the moment in which the user increases his votes for a gauge.

These measures are necessary so that the user only obtains rewards for the time in which he has voted for a gauge.

Assessed type

MEV

Summary

L-01 - In LendingTerm::_partialRepay() it uses the > sign to validate the minBorrow amount, however in LendingTerm::_borrow() it uses >=

In the LendingTerm::_borrow() function the following validation is used to check that the borrower is getting a minimum loan amount, this validation uses the >= sign:

File: LendingTerm.sol
365:         require(
366:             borrowAmount >= ProfitManager(refs.profitManager).minBorrow(),
367:             "LendingTerm: borrow amount too low"
368:         );

On the other hand, the LendingTerm::_partialRepay() function uses the same validation but uses the > sign:

File: LendingTerm.sol
527:         require(
528:             borrowAmount - issuanceDecrease >
529:                 ProfitManager(refs.profitManager).minBorrow(),
530:             "LendingTerm: below min borrow"
531:         );

This means that in LendingTerm::_partialRepay() the borrower needs to maintain more debt than specified when requesting the loan in LendingTerm::_borrow().

Recommended Mitigation Steps

In LendingTerm::_partialRepay() maintain the same logic as in LendingTerm::_borrow():

File: LendingTerm.sol
    function _partialRepay(
        address repayer,
        bytes32 loanId,
        uint256 debtToRepay
    ) internal {
...
...
        require(
--          borrowAmount - issuanceDecrease >
++          borrowAmount - issuanceDecrease >=
                ProfitManager(refs.profitManager).minBorrow(),
            "LendingTerm: below min borrow"
        );

L-02 - The LendingTerm::forgive() function does not validate that the loan is on call

The LendingTerm::forgive() function helps the protocol accept loan losses and update the system accordingly. On the other hand, the LendingTerm::_call() function helps to put the loan up for auction.

The problem is that the LendingTerm::forgive() function does not validate that the loan is on call or in auction, causing the system to update incorrectly.

Consider the following scenario:

1. For some reason, there is a proposal to call LendingTerm::forgive() on loanId 10.
2. The loanId 10 enters the auction and the variable loans[loanId].callTime is set to block.timestamp.
3. The proposal from step 1 is executed before the auction ends, setting loans[loanId].closeTime to the value of block.timestamp.
4. The auction continues and is reversed by the following validation, making the auction unable to be completed since the code will be reversed:

   File: LendingTerm.sol
   738: require(loans[loanId].closeTime == 0, "LendingTerm: loan closed");

5. Since the auction will always be reversed in LendingTerm::onBid() the nAuctionsInProgress variable will not be updated, not allowing the variable to be subtracted and an auction house cannot be updated using the LendingTerm::setAuctionHouse() function.

Recommended Mitigation Steps

The LendingTerm::forgive() function must validate that the loan is not up for auction:

    function forgive(bytes32 loanId) external onlyCoreRole(CoreRoles.GOVERNOR) {
        Loan storage loan = loans[loanId];

        // check that the loan exists
        require(loan.borrowTime != 0, "LendingTerm: loan not found");

        // check that the loan is not already closed
        require(loan.closeTime == 0, "LendingTerm: loan closed");

++      // Check the loan is not in auction
++      require(loan.callTime == 0, "LendingTerm: loan on cal");

        // close the loan
        loans[loanId].closeTime = block.timestamp;
        issuance -= loan.borrowAmount;

        // mark loan as a total loss
        uint256 creditMultiplier = ProfitManager(refs.profitManager)
            .creditMultiplier();
        uint256 borrowAmount = loans[loanId].borrowAmount;
        uint256 principal = (borrowAmount *
            loans[loanId].borrowCreditMultiplier) / creditMultiplier;
        int256 pnl = -int256(principal);
        ProfitManager(refs.profitManager).notifyPnL(address(this), pnl);

        // set hardcap to 0 to prevent new borrows
        params.hardCap = 0;

        // emit event
        emit LoanClose(block.timestamp, loanId, LoanCloseType.Forgive, 0);
    }

L-03 - The RateLimitedMinter().replenishBuffer() is not called when ProfitManager.surplusBuffer or ProfitManager.termSurplusBuffer paid for the bad debt

When there is bad debt in the lending term, the profit manager is called via the ProfitManager::notifyPnL() function, then if the surplusBuffer paid for the bad debt. The problem is that once the surplusBuffer paid for the bad debt, the RateLimitedMinter::replenishBuffer() should be called.

Recommended Mitigation Steps

Call the RateLimitedMinter::replenishBuffer() function if the ProfitManager.suplusBuffer paid for the bad debt.

File: ProfitManager.sol
292:     function notifyPnL(
293:         address gauge,
294:         int256 amount
295:     ) external onlyCoreRole(CoreRoles.GAUGE_PNL_NOTIFIER) {
...
...
314: 
315:             if (loss < _surplusBuffer) {
316:                 // deplete the surplus buffer
317:                 surplusBuffer = _surplusBuffer - loss;
318:                 emit SurplusBufferUpdate(
319:                     block.timestamp,
320:                     _surplusBuffer - loss
321:                 );
322:                 CreditToken(_credit).burn(loss);
++                   RateLimitedMinter(creditMinter).replenishBuffer(loss);
323:             } else {
324:                 // empty the surplus buffer
325:                 loss -= _surplusBuffer;
326:                 surplusBuffer = 0;
327:                 CreditToken(_credit).burn(_surplusBuffer);
++                   RateLimitedMinter(creditMinter).replenishBuffer(_surplusBuffer);
328:                 emit SurplusBufferUpdate(block.timestamp, 0);
329: 
330:                 // update the CREDIT multiplier
331:                 uint256 creditTotalSupply = CreditToken(_credit).totalSupply();
332:                 uint256 newCreditMultiplier = (creditMultiplier *
333:                     (creditTotalSupply - loss)) / creditTotalSupply;
334:                 creditMultiplier = newCreditMultiplier;
335:                 emit CreditMultiplierUpdate(
336:                     block.timestamp,
337:                     newCreditMultiplier
338:                 );
339:             }
340:         }
...
...

N-01 - Unused RateLimitedMinter contract import

The RateLimitedMinter contract import is not used:

File: SimplePSM.sol
12: import {RateLimitedMinter} from "@src/rate-limits/RateLimitedMinter.sol";

N-02 - Return custom error when user does not have gauge weight in the applyGaugeLoss() function

The applyGaugeLoss() function can be called in order to apply loss to an specific term and user. The problem is that the funcion will revert without any custom error when getUserGaugeWeight returns zero. The next shows how the function is reverted without any custom error:

// File: test/unit/loan/SurplusGuildMinter.t.sol:SurplusGuildMinterUnitTest
// $ forge test --match-test "testUnstakeWithLossIsFrontRan" -vvv
//
    function testCustomErrorWhenThereIsNotUserWeight() public {
        vm.prank(governor);
        profitManager.setProfitSharingConfig(
            0.5e18, // surplusBufferSplit
            0, // creditSplit
            0.5e18, // guildSplit
            0, // otherSplit
            address(0) // otherRecipient
        );
        credit.mint(address(profitManager), 35e18);
        profitManager.notifyPnL(term, -27.5e18);
        guild.applyGaugeLoss(term, address(sgm));
    }