Ethereum Credit Guild - CaeraDenoir's results

Lines of code

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

Vulnerability details

Description

The issue comes from the notifyPnL function, more specifically when it handles loss:

function notifyPnL(
        address gauge,
        int256 amount
    ) external onlyCoreRole(CoreRoles.GAUGE_PNL_NOTIFIER) {
        uint256 _surplusBuffer = surplusBuffer;
        uint256 _termSurplusBuffer = termSurplusBuffer[gauge];
        address _credit = credit;

        // handling loss
        if (amount < 0) {
            uint256 loss = uint256(-amount);

            // save gauge loss
            GuildToken(guild).notifyGaugeLoss(gauge);

            // deplete the term surplus buffer, if any, and
            // donate its content to the general surplus buffer
            if (_termSurplusBuffer != 0) {
                termSurplusBuffer[gauge] = 0;
                emit TermSurplusBufferUpdate(block.timestamp, gauge, 0);
                _surplusBuffer += _termSurplusBuffer;
            }

            if (loss < _surplusBuffer) {
                // deplete the surplus buffer
                surplusBuffer = _surplusBuffer - loss;
                emit SurplusBufferUpdate(
                    block.timestamp,
                    _surplusBuffer - loss
                );
                CreditToken(_credit).burn(loss);
            } else {
                // empty the surplus buffer
                loss -= _surplusBuffer;
                surplusBuffer = 0;
                CreditToken(_credit).burn(_surplusBuffer);
                emit SurplusBufferUpdate(block.timestamp, 0);
                // update the CREDIT multiplier
                uint256 creditTotalSupply = CreditToken(_credit).totalSupply();
                uint256 newCreditMultiplier = (creditMultiplier *
                    (creditTotalSupply - loss)) / creditTotalSupply;
                creditMultiplier = newCreditMultiplier;
                emit CreditMultiplierUpdate(
                    block.timestamp,
                    newCreditMultiplier
                );
            }
        }

The function notifyPnL calculates the new creditMultiplier with this code:

uint256 newCreditMultiplier = (creditMultiplier *(creditTotalSupply - loss)) / creditTotalSupply;

creditMultiplier => The current creditMultiplier of the credit.

creditTotalSupply => The totalSupply of the credit token.

loss => The amount value turned positive minus the _surplusBuffer

Since the notifyPnL function is called when a loan is forgiven, the amount parameter would be equal to the total credit borrowed scaled by the current creditMultiplier and then inverted:

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

This principal value can be higher than the totalSupply, which would trigger the underflow and subsequent revert on notifyPnL if there is not enough _surplusBuffer.

Impact

The issue is severe since this would not enable the protocol to update the creditMultiplier in some edge cases, but it is unlikely to happen.

Proof of Concept

A lending term is created with a collateral named X.

Some investors mint 1e10 credit tokens. (low amount to see the creditMultiplier change visibly, higher amounts would not affect the logic of this bug, it would just make it harder to depict the value change of creditMultiplier)

Two borrowers create a loan:

borrower A takes a loan equal to the surplus + 1 gwei and redeems it. borrower B takes a loan equal to totalSupply of credits at the moment and redeems it.

Debts:
 borrower A: surplus amount + 1 gwei

 borrower B: totalSupply of CREDIT token

creditMultiplier: 1e18

A problem occurs with the collateral, and its transfers are paused indefinitely. This causes panic on the protocol , and the offboard of the term is called.

Since the collateral cannot be transfered, any bid would revert, leading to the governor having to forgive the loans to update the credit score and decrease the issuance.

The forgive function is called to forgive the loan A, draining the entire surplus funds, and updating the creditMultiplier with a loss of 1 gwei.

Debts:
 borrower A: 0

 borrower B: totalSupply of CREDIT token

creditMultiplier: 9e17

The forgive function is called to forgive the loan B, which has a debt of the entire supply:

Expected result:

Debts:
 borrower A: 0

 borrower B: 0

creditMultiplier: 0

Actual result: revert due to an underflow when calculating creditTotalSupply - loss. Loss ends up being higher than creditTotalSupply because the total debt B, which is the totalSupply itself, is amplified by the change on the creditMultiplier.

Debts:
 borrower A: 0

 borrower B: totalSupply of CREDIT token

creditMultiplier: 9e17

Tools Used

VSCode

Recommended Mitigation Steps

If the loss is higher than the creditTotalSupply, the creditMultiplier should go to zero since it would mean that all the money was lost.

Assessed type

DoS

Lines of code

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

Vulnerability details

Explanation

When the borrower calls the partialRepay function, it triggers a call to the ProfitManager contract, specifically to the notifyPnL function. This function manages the profits from the repayment, and part of the profits are added to the surplus.

As there is no restrictions to staking and unstaking, a bad actor can mint a lot of credit tokens and stake them into the surplus contract right before the partialRepay is called by the borrower, and unstake and burn the tokens right after, which makes it possible to extract value without really investing on the protocol, stealing from the lenders.

This also makes it possible for the borrower to end up paying a little less in fees, as he can do the same with a flash loan, minting credit tokens, staking them, calling the partialRepay function, unstaking and then burning the tokens, and repaying the flash loan if it seems profitable.

Impact

As there is a potential for a sandwich attack that can drain the fees from the surplus, it can lead to no one being incentivized to provide liquidity for the surplus.

Proof of Concept

Borrower tries to repay a part of the loan

Borrower sends the partialRepay function call to a public mempool

Order:
1- borrower repayment

Bad actor snipes it, sets the sandwich attack

Bad actor mints CREDIT tokens in exchange of his own liquidity and stakes them before the borrower  makes the call

Order: 
1- bad actor mint + stake
2- borrower repayment

Bad actor makes a second transaction go right after the borrower repayment

Bad actor unstakes the CREDIT tokens then burns them for his own liquidity + rewards

Order: 
1- bad actor mint + stake
2- borrower repayment
3- bad actor unstake + burn

This is due to the partialRepay function calling the notifyPnL function from the ProfitManager contract, which handles the profit made in the same tx

Tools Used

VSCode

Recommended Mitigation Steps

The rewards could be queued to be distributed and need to be executed on a future block to prevent this kind of attack.

Assessed type

Context