search

sherlock-audit / 2023-04-ajna-judging

4 stars 3 forks source link

hyh - Due to excessive HTP check moveQuoteToken can be unavailable for big deposits #84

Open sherlock-admin opened 1 year ago

sherlock-admin commented 1 year ago

hyh

medium

Due to excessive HTP check moveQuoteToken can be unavailable for big deposits

Summary

moveQuoteToken() will revert if deposit removal causes LUP to be less than HTP, while the whole operation being the removal and addition to another index, so the check structured this way is excessive and prohibit a substantial share of active debt management operations. I.e. it has to be HTP <= LUP before and after the move, not in-between.

Vulnerability Detail

The one of the main purposes of moveQuoteToken() is to allow for dynamic management of deposit placement within the pool. This is crucial for controlling the associated risks: quote funds within the buckets can be traded with collateral at bucket's price, high price buckets can be frozen for liquidation debt buffer, then they can take part in debt write off. On the other hand low price buckets will not receive any yield while their price is below HTP (Ajna white paper 4.1 Deposit and others). This way for any depositor the ability to move the quote funds between buckets is crucial.

The unavailability of moveQuoteToken() due to excess HTP > LUP check can directly lead to losses for the corresponding depositor. I.e. they can place funds to the higher price bucket, expecting to manage them closely to mitigate risks, so they can monitor the situation, being ready to move the funds out immediately when situation worsens (collateral market price moves down, liquidation volume spikes, and so on), but find themselves unable to do so.

Impact

moveQuoteToken() can be frequently unavailable (especially in big deposits case), which can directly lead to the depositor's losses.

Probability of unavailability looks to be high (removal of big enough deposit can frequently cause HTP > LUP state), while the probability of the following loss is medium, so placing the severity to medium as well.

Code Snippet

moveQuoteToken() is for moving deposit from fromIndex_ to toIndex_:

https://github.com/sherlock-audit/2023-04-ajna/blob/main/ajna-core/src/base/Pool.sol#L176-L207

    function moveQuoteToken(
        uint256 maxAmount_,
        uint256 fromIndex_,
        uint256 toIndex_,
        uint256 expiry_
    ) external override nonReentrant returns (uint256 fromBucketLP_, uint256 toBucketLP_, uint256 movedAmount_) {
        ...

        (
            fromBucketLP_,
            toBucketLP_,
            movedAmount_,
            newLup
>>      ) = LenderActions.moveQuoteToken(
            buckets,
            deposits,
            poolState,
            MoveQuoteParams({
                maxAmountToMove: maxAmount_,
                fromIndex:       fromIndex_,
                toIndex:         toIndex_,
                thresholdPrice:  Loans.getMax(loans).thresholdPrice
            })
        );
        ...
    }

moveQuoteToken() controls for vars.htp > lup_ with LUP being calculated after deposit removal, but before adding it back to the destination bucket:

https://github.com/sherlock-audit/2023-04-ajna/blob/main/ajna-core/src/libraries/external/LenderActions.sol#L237-L312

    function moveQuoteToken(
        mapping(uint256 => Bucket) storage buckets_,
        DepositsState storage deposits_,
        PoolState calldata poolState_,
        MoveQuoteParams calldata params_
    ) external returns (uint256 fromBucketRedeemedLP_, uint256 toBucketLP_, uint256 movedAmount_, uint256 lup_) {
        ...

>>      (movedAmount_, fromBucketRedeemedLP_, vars.fromBucketRemainingDeposit) = _removeMaxDeposit(
            deposits_,
            RemoveDepositParams({
                depositConstraint: params_.maxAmountToMove,
                lpConstraint:      vars.fromBucketLenderLP,
                bucketLP:          vars.fromBucketLP,
                bucketCollateral:  vars.fromBucketCollateral,
                price:             vars.fromBucketPrice,
                index:             params_.fromIndex,
                dustLimit:         poolState_.quoteTokenScale
            })
        );

>>      lup_ = Deposits.getLup(deposits_, poolState_.debt);
        // apply unutilized deposit fee if quote token is moved from above the LUP to below the LUP
        if (vars.fromBucketPrice >= lup_ && vars.toBucketPrice < lup_) {
            movedAmount_ = Maths.wmul(movedAmount_, Maths.WAD - _depositFeeRate(poolState_.rate));
        }

        vars.toBucketUnscaledDeposit = Deposits.unscaledValueAt(deposits_, params_.toIndex);
        vars.toBucketScale           = Deposits.scale(deposits_, params_.toIndex);
        vars.toBucketDeposit         = Maths.wmul(vars.toBucketUnscaledDeposit, vars.toBucketScale);

        toBucketLP_ = Buckets.quoteTokensToLP(
            toBucket.collateral,
            toBucket.lps,
            vars.toBucketDeposit,
            movedAmount_,
            vars.toBucketPrice,
            Math.Rounding.Down
        );

        // revert if (due to rounding) the awarded LP in to bucket is 0
        if (toBucketLP_ == 0) revert InsufficientLP();

>>      Deposits.unscaledAdd(deposits_, params_.toIndex, Maths.wdiv(movedAmount_, vars.toBucketScale));

        vars.htp = Maths.wmul(params_.thresholdPrice, poolState_.inflator);

        // check loan book's htp against new lup, revert if move drives LUP below HTP
>>      if (params_.fromIndex < params_.toIndex && vars.htp > lup_) revert LUPBelowHTP();

Tool used

Manual Review

Recommendation

Consider controlling params_.fromIndex < params_.toIndex && vars.htp > lup_ with lup_ being the final LUP, calculated after Deposits.unscaledAdd(deposits_, params_.toIndex, Maths.wdiv(movedAmount_, vars.toBucketScale)).

Deposit fee can be calculated from initial LUP only: it looks that if deposit fee condition is true for initial LUP then LUP will not change, if it is true for final LUP then it wasn't changed.

grandizzy commented 1 year ago

fix in https://github.com/ajna-finance/contracts/pull/891

dmitriia commented 1 year ago

fix in ajna-finance/contracts#891

Looks ok, LUP is being recalculated:

https://github.com/ajna-finance/contracts/blob/0332f341856e1efe4da8bb675886c8cfbee57b71/src/libraries/external/LenderActions.sol#L310-L313

        Deposits.unscaledAdd(deposits_, params_.toIndex, Maths.wdiv(movedAmount_, vars.toBucketScale));

        // recalculate LUP after adding amount in to bucket only if to bucket price is greater than LUP
        if (vars.toBucketPrice > lup_) lup_ = Deposits.getLup(deposits_, poolState_.debt);