c4-bot-5 commented 8 months ago

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/test/proposals/gips/GIP_0.sol#L175-L179 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/AuctionHouse.sol#L118-L196

Vulnerability details

HIGH: Auction manipulation by block stuffing and reverting on ERC-777 hooks

GitHub Links

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/test/proposals/gips/GIP_0.sol#L175-L179

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/AuctionHouse.sol#L118-L196

Summary

The protocol stated out in the C4 description that the deployment script of the protocol, located in test/proposals/gips/GIP_0.sol is also in scope, as protocol deployment/configuration mistakes could be made. A low immutable auction duration set in this deployment script can lead to profitable block stuffing attacks on the desired L2 chains. This attack vector can be further improved under the condition that the collateral token is ERC-777 compatible.

Vulnerability Details

The auction house contract is deployed with the following parameters (auctionDuration and midPoint are immutables):

AuctionHouse auctionHouse = new AuctionHouse(
    AddressLib.get("CORE"),
    650, // midPoint = 10m50s
    1800 // auctionDuration = 30m
);

During the first half of the auction (before midPoint), an increasing amount of the collateral is offered, for the full CREDIT amount.

During the second half of the action (after midPoint), all collateral is offered, for a decreasing CREDIT amount.

The calculation can be seen in the getBidDetail function:

uint256 public immutable midPoint;
uint256 public immutable auctionDuration;

function getBidDetail(bytes32 loanId) public view returns (uint256 collateralReceived, uint256 creditAsked) {
  // check the auction for this loan exists
  uint256 _startTime = auctions[loanId].startTime;
  require(_startTime != 0, 'AuctionHouse: invalid auction');

  // check the auction for this loan isn't ended
  require(auctions[loanId].endTime == 0, 'AuctionHouse: auction ended');

  // assertion should never fail because when an auction is created,
  // block.timestamp is recorded as the auction start time, and we check in previous
  // lines that start time != 0, so the auction has started.
  assert(block.timestamp >= _startTime);

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

    // 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
    creditAsked = _callDebt - (_callDebt * elapsed) / PHASE_2_DURATION;
  }
  // second phase fully elapsed, anyone can receive the full collateral and give 0 CREDIT
  // in practice, somebody should have taken the arb before we reach this condition.
  else {
    // receive the full collateral
    collateralReceived = auctions[loanId].collateralAmount;
    //creditAsked = 0; // implicit
  }
}

This means that as longer the auction goes till a bid is made (which instantly buys the auction), the more profit can be made by executing the auction.

The following conditions allow an attacker to manipulate auctions by stuffing blocks to increase profits:

auctionDuration is set to 1800 seconds (30min) in the deployment script
auction midPoint is set to 650 seconds (10m50s) in the deployment script
the protocol will be deployed on L2s let's take optimism for example (as this chain was mentioned by the devs in discord):
- blocks in optimism are minted every 2s
- the block gas limit of optimism is 30M as in ethereum mainnet
- at the time of writting this the cost of stuffing a full block on optimism is around $6.39 (https://www.cryptoneur.xyz/en/gas-fees-calculator?gas-input=15000000&gas-price-option=on)
- therefore prevent a bid for one minute costs $6.39 * 30 blocks = $191.7
- therefore prevent a bid for 30 minutes costs $191.7 * 30 minutes = $5751
- but the attacker will almost never need to stuff blocks for around 30 minutes as the system of the auction is not profitable in the beginning and it starts to be really profitable after the midpoint, and it also starts to be much more damaging to the system at this point. Which is after 10m50s and the costs to stuff blocks for 10m50s is $191.7 * 10.5 minutes = $2012.85 and as mentioned before the auction is not profitable at the beginning, therefore no one will bid on second one and the attacker does not need to stuff blocks instantly. This means the attack will most likely cost less than $2000.
therefore if at any given timestamp the profit of the auction outweighs the cost of stuffing blocks for the time till the deal becomes profitable for the attacker a system damaging incentive appears and manipulating auctions becomes a profitable attack vector.

But the impact increases further in terms of griefing as loss for terms can occur after the midPoint which will instantly lead to slashing and therefore all stakers of the given term will lose all their credit tokens weighted on this term.

The following code snippets showcase the slashing mechanism that lead to a total loss for the stakers if the term receives any loss during these block stuffing attack:


function bid(bytes32 loanId) external {
    ...

    LendingTerm(_lendingTerm).onBid(
        loanId,
        msg.sender,
        auctions[loanId].collateralAmount - collateralReceived, // collateralToBorrower
        collateralReceived, // collateralToBidder
        creditAsked // creditFromBidder
    );

    ...
}

function onBid(
    bytes32 loanId,
    address bidder,
    uint256 collateralToBorrower,
    uint256 collateralToBidder,
    uint256 creditFromBidder
) external {
    ...

    int256 pnl;
    uint256 interest;
    if (creditFromBidder >= principal) {
        interest = creditFromBidder - principal;
        pnl = int256(interest);
    } else {
        pnl = int256(creditFromBidder) - int256(principal);
        principal = creditFromBidder;
        require(
            collateralToBorrower == 0,
            "LendingTerm: invalid collateral movement"
        );
    }

    ...

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

    ...
}

function notifyPnL(
    address gauge,
    int256 amount
) external onlyCoreRole(CoreRoles.GAUGE_PNL_NOTIFIER) {
    ...

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

function notifyGaugeLoss(address gauge) external {
    require(msg.sender == profitManager, "UNAUTHORIZED");

    // save gauge loss
    lastGaugeLoss[gauge] = block.timestamp;
    emit GaugeLoss(gauge, block.timestamp);
}

/// @notice apply a loss that occurred in a given gauge
/// anyone can apply the loss on behalf of anyone else
function applyGaugeLoss(address gauge, address who) external {
    // check preconditions
    uint256 _lastGaugeLoss = lastGaugeLoss[gauge];
    uint256 _lastGaugeLossApplied = lastGaugeLossApplied[gauge][who];
    require(
        _lastGaugeLoss != 0 && _lastGaugeLossApplied < _lastGaugeLoss,
        "GuildToken: no loss to apply"
    );

    // read user weight allocated to the lossy gauge
    uint256 _userGaugeWeight = getUserGaugeWeight[who][gauge];

    // remove gauge weight allocation
    lastGaugeLossApplied[gauge][who] = block.timestamp;
    _decrementGaugeWeight(who, gauge, _userGaugeWeight);
    if (!_deprecatedGauges.contains(gauge)) {
        totalTypeWeight[gaugeType[gauge]] -= _userGaugeWeight;
        totalWeight -= _userGaugeWeight;
    }

    // apply loss
    _burn(who, uint256(_userGaugeWeight));
    emit GaugeLossApply(
        gauge,
        who,
        uint256(_userGaugeWeight),
        block.timestamp
    );
}

This attack vector can be further improved under the condition that the collateral token is ERC-777 compatible. It is advised to first read the report called Bad debt can occur if the collateral token blacklists a borrower leading to total loss of stake for all lenders on that term which showcases how the auction time is increased till the midPoint of the auction if transferring the collateral tokens to the borrower reverts.

The attack path would be as follows:

attacker borrows a loan (with a ERC-777 compatible collateral token) using a contract that revert on receiving the collateral back if the tx.origin != address(attacker)
the attacker receives the credit token from the loan and deposits collateral into the protocol
the attacker does not repay the loan after the first partial duration and call it for auction
as showcased in the mentioned report every call till the midPoint will revert as trying to transfer the collateral back to the attacker will revert (this will drastically decrease the costs for the attacker as he does not need to stuff blocks till the midPoint is reached)
after the midPoint is reached the attacker can start stuffing blocks till bad debt occurs, at this time the attacker can bid on the auction and will therefore buy the full collateral back for less credit tokens than the attacker received for the loan
if it is possible for the attacker to stuff blocks and wait till the asked credit amount goes to 0 the attacker was able to steal almost the full loan amount (which can potentially be way bigger than the gas amount for stuffing the blocks as the calculation in the beginning showed)

Impact

The attacker can prevent other users from bidding on the auction and therefore manipulate the auction to a point where the attacker would be able to buy the full collateral for almost zero credit tokens. As loss for the term occurs in such an event, all stakers of the given term will lose all their credit tokens weighted on this term. If the given collateral token is ERC-777 compatible, the costs of such an attack can be drastically reduced. And the attack can potentially become a self liquidation attack.

Recommendations

Increase the auction duration, as the longer the auction goes the less profitable such an attack would be and implement the mentioned fix in the Bad debt can occur if the collateral token blacklists a borrower leading to total loss of stake for all lenders on that term report.

Assessed type

MEV

c4-pre-sort commented 8 months ago

0xSorryNotSorry marked the issue as sufficient quality report

c4-pre-sort commented 8 months ago

0xSorryNotSorry marked the issue as primary issue

eswak commented 8 months ago

I'm skeptical of this being a valid finding (despite fomo3d), at least it's not a high. It is true that there is more spam/censorship risk on Optimism & Arbitrum. On mainnet liquidators can use flashbots etc, while on Arbitrum the sequencer is first come first served. If the liquidator raises the gas price of one tx, the spammer has to raise the gas price of all tx that fill the block. Costs can get out of hand pretty quickly. Take-away I think is that on L2s, the auctions should be long enough to account for this. We've also been discussing very long auctions internally to minimize liquidity risk (and allow people to have time to bridge assets in order to participate in auctions), which also mitigates this vector.

c4-sponsor commented 8 months ago

eswak (sponsor) acknowledged

c4-sponsor commented 8 months ago

eswak marked the issue as disagree with severity