Ethereum Credit Guild - Soltho's results

Lines of code

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

Vulnerability details

Impact

If the creditMultiplier reaches 0 it will be DoS-ed since it won't never increase again. This would happen if a loan that has all the credit of a loan token is forgiven(e.g. auction could not be settled) and therefore a 'loss' equal to totalCreditSupply is notified to the ProfitManager, resulting in a creditMultiplier equal to 0 if there is no surpluss buffer, that will DoS all the lending terms for that token, and brick them permanently since the CreditMultiplier is used as a divisor, causing a division or modulo by 0 error for any borrow. This scenario is very unlikely but still possible, and malicious upgradeable tokens could contribute to this happening.

Proof of Concept

Note: For this PoC to work remove the "renounceRole" line in the 'setUp' function so we can grant more roles in the test

function testCreditMultiplierDoS() public {
        // create a second term for this same loan token
        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(0x1),// another collataral token
                maxDebtPerCollateralToken: 2000e18, // 2000, same decimals
                interestRate: 0.10e18, // 10% APR
                maxDelayBetweenPartialRepay: 0,
                minPartialRepayPercent: 0,
                openingFee: 0,
                hardCap: 20_000_000e18 // 20M CREDIT
            })
        );
        core.grantRole(CoreRoles.GAUGE_PNL_NOTIFIER, address(term2));
        core.grantRole(CoreRoles.RATE_LIMITED_CREDIT_MINTER, address(term2));

        // add gauge and vote for it
        guild.addGauge(1, address(term2));
        guild.mint(address(this), 1e18);
        guild.incrementGauge(address(term2), 1e18);


        // prepare
        uint256 borrowAmount = 500e18;
        uint256 collateralAmount = 15e18;
        collateral.mint(address(borrower), collateralAmount * 2);
    
        // borrow
        vm.startPrank(borrower);
        collateral.approve(address(term), collateralAmount);
        bytes32 loanId = term.borrow(borrowAmount, collateralAmount);
        vm.stopPrank();

        // 1 year later, interest accrued
        // assume it was not fully repaid because the collateral is frozen
        vm.warp(block.timestamp + term.YEAR());
        vm.roll(block.number + 1);

        // the debt is forgiven
        vm.prank(governor);
        term.forgive(loanId);
        // the credit multiplier is 0
        assertEq(profitManager.creditMultiplier(), 0);
        
        // try borrowing from another term
        collateral.approve(address(term2), collateralAmount);
        // DoS
        vm.expectRevert(); // panic: division or modulo by zero
        term2.borrow(borrowAmount, collateralAmount);
        vm.stopPrank();
    }

Tools Used

Manual Review

Recommended Mitigation Steps

Use a "virtual shares" calculation to make sure the credit multiplier never reaches 0 in notifyPnl:

     // update the CREDIT multiplier
                uint256 creditTotalSupply = CreditToken(_credit).totalSupply();
                uint256 newCreditMultiplier = ((creditMultiplier *
                   (creditTotalSupply - loss)) +1) / (creditTotalSupply +1);

By incorporating this modification, the credit multiplier is calculated using a formula that prevents it from reaching 0, thereby addressing the DoS vulnerability and ensuring the continued functionality of the lending terms.

Assessed type

DoS