Bucket index from (which liquidity is moved from) shouldn't be removed if the returned redeemed LP is lower than fromPosition.lps

[code423n4]

Lines of code

https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-core/src/PositionManager.sol#L300

Vulnerability details

Impact

A lender can move liquidity from a bucket to another via moveLiquidity method. This method calls IPool(relevant pool).moveQuoteToken to move the quote tokens in the pool. On moveQuoteToken's success, it returns two values lpbAmountFrom and lpbAmountTo. PositionManager then update position LP state for each as follows:

    fromPosition.lps -= vars.lpbAmountFrom;
    toPosition.lps   += vars.lpbAmountTo;

https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-core/src/PositionManager.sol#L320-L321

The issue is that, the "bucket index from" is removed from positionIndexes as follows:

    // remove bucket index from which liquidity is moved from tracked positions
    if (!positionIndex.remove(params_.fromIndex)) revert RemovePositionFailed();

https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-core/src/PositionManager.sol#L300

If fromPosition.lps is still greater than zero after deducting the moved LP, then the lender will lose those LPs since the "bucket index from" is no longer tracked. This results in a loss of LP for the lender.

Proof of Concept

PositionManager.moveLiquidity calculates the max amount of quote tokens that can be moved, given the tracked LP

    vars.maxQuote = _lpToQuoteToken(
        vars.bucketLP,
        vars.bucketCollateral,
        vars.bucketDeposit,
        fromPosition.lps,
        vars.bucketDeposit,
        _priceAt(params_.fromIndex)
    );

https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-core/src/PositionManager.sol#L287-L295

Then it moves the maximum quote tokens in the pool

    // move quote tokens in pool
    (
        vars.lpbAmountFrom,
        vars.lpbAmountTo,
    ) = IPool(params_.pool).moveQuoteToken(
        vars.maxQuote,
        params_.fromIndex,
        params_.toIndex,
        params_.expiry
    );

https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-core/src/PositionManager.sol#L306-L315

However, the returned lpbAmountFrom is not always guaranteed to be equal to fromPosition.lps. If we check Pool.moveQuoteToken, it calls LenderActions.moveQuoteToken method. In this method we have the following code block that determines what to return:

    uint256 scaledLpConstraint = Maths.wmul(params_.lpConstraint, exchangeRate);
    if (
        params_.depositConstraint < scaledDepositAvailable &&
        params_.depositConstraint < scaledLpConstraint
    ) {
        // depositConstraint is binding constraint
        removedAmount_ = params_.depositConstraint;
        redeemedLP_    = Maths.wdiv(removedAmount_, exchangeRate);
    } else if (scaledDepositAvailable < scaledLpConstraint) {
        // scaledDeposit is binding constraint
        removedAmount_ = scaledDepositAvailable;
        redeemedLP_    = Maths.wdiv(removedAmount_, exchangeRate);
    } else {
        // redeeming all LP
        redeemedLP_    = params_.lpConstraint;
        removedAmount_ = Maths.wmul(redeemedLP_, exchangeRate);
    }

https://github.com/code-423n4/2023-05-ajna/blob/main/ajna-core/src/libraries/external/LenderActions.sol#L711-L727

There are 2 conditions checked before redeeming all LP. If one of these conditions is true, then we know that the returned value won't be all LP. In other words, In PositionManager, lpbAmountFrom will not be equal to fromPosition.lps.

Tools Used

Manual analysis

Recommended Mitigation Steps

Only remove the fromPosition from positionIndexs set if fromPosition.lps is zero.

Assessed type

Other

[c4-judge]

Picodes marked the issue as duplicate of #503

[c4-judge]

Picodes changed the severity to 3 (High Risk)

[c4-judge]

Picodes marked the issue as satisfactory