branch_indigo - Lenders lose interests and pay deposit fees due to no slippage control

[sherlock-admin]

branch_indigo

medium

Lenders lose interests and pay deposit fees due to no slippage control

Summary

When a lender deposits quote tokens below the minimum of LUP(Lowest Utilization Price) and HTP(Highest Threshold Price), the deposits will not earn interest and will also be charged deposit fees, according to docs. When a lender deposits to a bucket, they are vulnerable to pool LUP slippage which might cause them to lose funds due to fee charges against their will.

Vulnerability Detail

A lender would call addQuoteToken() to deposit. This function only allows entering expiration time for transaction settlement, but there is no slippage protection.

//Pool.sol
    function addQuoteToken(
        uint256 amount_,
        uint256 index_,
        uint256 expiry_
    ) external override nonReentrant returns (uint256 bucketLP_) {
        _revertAfterExpiry(expiry_);
        PoolState memory poolState = _accruePoolInterest();
        // round to token precision
        amount_ = _roundToScale(amount_, poolState.quoteTokenScale);
        uint256 newLup;
        (bucketLP_, newLup) = LenderActions.addQuoteToken(
            buckets,
            deposits,
            poolState,
            AddQuoteParams({
                amount: amount_,
                index:  index_
            })
        );
       ...

In LenderActions.sol, addQuoteToken() takes current DepositsState in storage and current poolState_.debt in storage to calculate spot LUP prior to deposit. And this LUP is compared with user input bucket index_ to determine if the lender will be punished with deposit fees. The deposit amount is then written to storage.

//LenderActions.sol
    function addQuoteToken(
        mapping(uint256 => Bucket) storage buckets_,
        DepositsState storage deposits_,
        PoolState calldata poolState_,
        AddQuoteParams calldata params_
    ) external returns (uint256 bucketLP_, uint256 lup_) {
  ...
          // charge unutilized deposit fee where appropriate
 |>       uint256 lupIndex = Deposits.findIndexOfSum(deposits_, poolState_.debt);
        bool depositBelowLup = lupIndex != 0 && params_.index > lupIndex;
        if (depositBelowLup) {
            addedAmount = Maths.wmul(addedAmount, Maths.WAD - _depositFeeRate(poolState_.rate));
        }
...
   Deposits.unscaledAdd(deposits_, params_.index, unscaledAmount);
...

It should be noted that current deposits_ and poolState_.debt can be different from when the user invoked the transaction, which will result in a different LUP spot price unforeseen by the lender to determine deposit fees. Even though lenders can input a reasonable expiration time expirty_, this will only prevent stale transactions to be executed and not offer any slippage control.

When there are many lenders depositing around the same time, LUP spot price can be increased and if the user transaction settles after a whale lender which moves the LUP spot price up significantly, the user might get accidentally punished for depositing below LUP. Or there could also be malicious lenders trying to ensure their transactions settle at a favorable LUP/HTP and front-run the user transaction, in which case the user transaction might still settle after the malicious lender and potentially get charged for fees.

Impact

Lenders might get charged deposit fees due to slippage against their will with or without MEV attacks, lenders might also lose on interest by depositing below HTP.

Code Snippet

https://github.com/ajna-finance/ajna-core/blob/e3632f6d0b196fb1bf1e59c05fb85daf357f2386/src/base/Pool.sol#L146-L150

https://github.com/ajna-finance/ajna-core/blob/e3632f6d0b196fb1bf1e59c05fb85daf357f2386/src/libraries/external/LenderActions.sol

https://github.com/ajna-finance/ajna-core/blob/e3632f6d0b196fb1bf1e59c05fb85daf357f2386/src/libraries/external/LenderActions.sol#L195

Tool used

Manual Review

Recommendation

Add slippage protection in Pool.sol addQuoteToken(). A lender can enable slippage protection, which will enable comparing deposit index_ with lupIndex in LenderActions.sol.

[ith-harvey]

We think this should be a low. Although not explicitly stated that this can happen in docs it is assumed based off of implementation. We were aware, not concerned.

[grandizzy]

https://github.com/ajna-finance/contracts/pull/915

[0xffff11]

I can still see the issue as a medium. Sponsor agreed to the issue and it has some impact on the fees that lender might have to pay

[dmitriia]

ajna-finance/contracts#915

Looks ok, but shouldn't the same flag be introduced to moveQuoteToken(), e.g.:

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

    if (vars.fromBucketPrice >= lup_ && vars.toBucketPrice < lup_) {
+       if (params_.revertIfBelowLup) revert PriceBelowLUP();
        movedAmount_ = Maths.wmul(movedAmount_, Maths.WAD - _depositFeeRate(poolState_.rate));
    }

[grandizzy]

ajna-finance/contracts#915

Looks ok, but shouldn't the same flag be introduced to moveQuoteToken(), e.g.:

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

    if (vars.fromBucketPrice >= lup_ && vars.toBucketPrice < lup_) {
+       if (params_.revertIfBelowLup) revert PriceBelowLUP();
        movedAmount_ = Maths.wmul(movedAmount_, Maths.WAD - _depositFeeRate(poolState_.rate));
    }

implemented with https://github.com/ajna-finance/contracts/pull/918 Beside suggested change there are 2 additional updates

• MoveQuoteParams struct moved from inline in order to avoid stack too deep error
• shrink Pool contract size by reading structs in view functions only once

[dmitriia]

implemented with ajna-finance/contracts#918

Looks ok, the above logic now added.

[ctf-sec]

Escalate for 10 USDC.

As the sponsor said

We think this should be a low. Although not explicitly stated that this can happen in docs it is assumed based off of implementation. We were aware, not concerned.

this is more like a feature request, not bug

[sherlock-admin]

Escalate for 10 USDC.

As the sponsor said

We think this should be a low. Although not explicitly stated that this can happen in docs it is assumed based off of implementation. We were aware, not concerned.

this is more like a feature request, not bug

You've created a valid escalation for 10 USDC!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[bzpassersby]

I think this issue is a valid medium. It points to a possible scenario where a lender has to pay fees and lose interest against their will due to no slippage protection, which can be exploited through MEV attack. Because of the scenario of lenders losing funds against their intention, it should be medium.

Due to the fact that slippage can be exploited to cause users to lose funds, this should be considered a vulnerability or bug.

[0xffff11]

I disagree with the escalation in this case. I think it should be a medium. Despite being a design decision, allows users to be exposed to high slippage on behalf of their decision.

[dmitriia]

Looks like valid medium to me, the probability of the material impact can be said to be medium, so is the impact itself.

[MLON33]

implemented with ajna-finance/contracts#918

Looks ok, the above logic now added.

Moving sign-off by @dmitriia here for clarity.

[hrishibhat]

Result: Medium Has duplicates Considering a valid medium based on the above comments

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• ctf-sec: rejected