Block addqt above auction price

[mattcushman]

Description

Block adding/move quote token to buckets with price above any existing auction price.

Purpose

• addQuoteToken() and moveQuoteToken() will now revert if the price of the target bucket exceeds the
  price of the liquidation at the head of the queue. Also, when auctions are added the queue, the
  threshold price is max-ed with the tail (if any), forcing that the threshold prices (and hence auction prices)
  are increasing.
  This prevents users from adding liquidity to a bucket and immediately arbTaking-ing an auction, eliminating
  attack and greifing vectors involving the bond payoffs and take penalties.

• Solves what we internally have called Dmitrii - 9, an issue discovered by @dmitriia (although not eligible for the Sherlock report) ->

**Summary of the Vulnerability:**
The code outlines a scenario where a borrower can exploit the auction mechanism post-default to incur minimal penalties and unfairly benefit at the expense of the lender (kicker). This is achieved by the borrower exiting the auction early at an artificially high price, avoiding significant taker penalties, and causing collateral damage to the lender. The penalty paid by the borrower is negligible compared to the advantage gained, rendering the penalty almost irrelevant.

** Link an example of the issue: **
https://github.com/ajna-finance/contracts/pull/981/files#diff-e59157bfa5bad910c2f5c2a00cfba7d222f0ad6f1969eb5ab4b2fbd22903b951R212

**Sudo Code Representation of the Attack:**

1. Lender adds liquidity to the pool.

2. Borrower draws a large debt from the pool.

3. Time skip of 100 days.

4. Lender initiates a kick due to default.

5. Borrower exploits the situation by depositing into a high price bucket.

6. Borrower takes from the bucket, reducing their penalty significantly.

7. Borrower removes a large portion of the collateral at an inflated price.

8. Post-removal, the lender's position is significantly weakened, with minimal gains.

9. Borrower ends up with a minor loss in USDC and almost no loss in WETH.

10. Lender gains are marginal compared to the potential loss incurred.

**Analysis:**
- The borrower controls the collateral price, impacting the distribution between borrower and taker. However, when the borrower and taker are the same, the price becomes irrelevant.
- The taker penalty, which should be a deterrent, is reduced to an insignificant amount due to the borrower's control over the collateral price.
- The kicker (lender) faces a substantial loss of bond, while the borrower benefits by virtually eliminating the penalty at a negligible cost.
- The potential for this exploit increases if issue #16 mentioned is fixed, allowing borrowers to execute this strategy more easily and atomically.

This vulnerability creates an imbalance in the protocol, favoring borrowers who default and strategically manipulate the auction process, while significantly disadvantaging lenders who face the brunt of the collateral damage. The protocol needs to address this imbalance to ensure fair and secure interactions between all parties involved.

Impact

Tasks

• Changes to protocol contracts are covered by unit tests executed by CI.
• Protocol contract size limits have not been exceeded.
• Gas consumption for impacted transactions have been compared with the target branch, and nontrivial changes cited in the Impact section above.
• Scope labels have been assigned as appropriate.
• Invariant tests have been manually executed as appropriate for the nature of the change.

[ith-harvey]

Why is this commented?

[MikeHathaway]

Would a _pendingInflator internal function usage here be worthwhile?

[EdNoepel]

There is one on line 452. We cannot call it from the same library.

[MikeHathaway]

Noticed a few small things, but otherwise LGTM

[EdNoepel]

nit: missing trailing blankline

[ith-harvey]

resolved -> d5d0ca7

[EdNoepel]

There is one on line 452. We cannot call it from the same library.

[EdNoepel]

Would like the typographic issues resolved before merging.

[ith-harvey]

Would like the typographic issues resolved before merging.

Think I got them all here -> d5d0ca7

[ith-harvey]

LGTM

[dmitriia]

Looks ok. As discussed (per @mattcushman idea), it may be worth to remove some margin from auction price for the _revertIfAuctionPriceBelow() check, e.g. time can be used as a more universal metric (asset volatilities vary, but attacker's risk is basically proportional to the time of exposure):

https://github.com/ajna-finance/contracts/blob/83072b109a45e887f544458df80ef458ea907394/src/libraries/helpers/RevertsHelper.sol#L86-L94

    function _revertIfAuctionPriceBelow(
        ...
    ) view {
        address head = auctions_.head;
        if (head != address(0)) {
-           uint256 auctionPrice = _auctionPrice(auctions_.liquidations[head].referencePrice, auctions_.liquidations[head].kickTime);
+           uint256 auctionPrice = _auctionPrice(auctions_.liquidations[head].referencePrice, auctions_.liquidations[head].kickTime + 10 minutes);
            if (_priceAt(index_) >= auctionPrice) revert AddAboveAuctionPrice();
        }