Using memory instead of storage in 'redeemPositions' will result in incorrect LP Balance

[code423n4]

Lines of code

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

Vulnerability details

Impact

This bug could lead to a situation where a user can 'redeem' their positions without the associated liquidity positions (LPs) being properly reset. This could result in the user being able to artificially inflate their LP balance, which could lead to financial loss for other system users.

Proof of Concept

The 'redeemPositions' function in the 'PositionManager' contract allows users to remove their liquidity positions (LPs) from the contract and return them to the NFT owner.

Here is the relevant section of the code in question:

https://github.com/code-423n4/2023-05-ajna/blob/fc70fb9d05b13aee2b44be2cb652478535a90edd/ajna-core/src/PositionManager.sol#L352-L393

 function reedemPositions(
        RedeemPositionsParams calldata params_
    ) external override mayInteract(params_.pool, params_.tokenId) {
        EnumerableSet.UintSet storage positionIndex = positionIndexes[params_.tokenId];

        IPool pool = IPool(params_.pool);

        uint256 indexesLength = params_.indexes.length;
        uint256[] memory lpAmounts = new uint256[](indexesLength);

        uint256 index;

        for (uint256 i = 0; i < indexesLength; ) {
            index = params_.indexes[i];

            Position memory position = positions[params_.tokenId][index];

            if (position.depositTime == 0 || position.lps == 0) revert RemovePositionFailed();

            // check that bucket didn't go bankrupt after memorialization
            if (_bucketBankruptAfterDeposit(pool, index, position.depositTime)) revert BucketBankrupt();

            // remove bucket index at which a position has added liquidity
            if (!positionIndex.remove(index)) revert RemovePositionFailed();

            lpAmounts[i] = position.lps;

            // remove LP tracked by position manager at bucket index
            delete positions[params_.tokenId][index];

            console.log(position.lps);
            console.log(position.depositTime);

            unchecked { ++i; }
        }

        address owner = ownerOf(params_.tokenId);

        // approve owner to take over the LP ownership (required for transferLP pool call)
        pool.increaseLPAllowance(owner, params_.indexes, lpAmounts);
        // update pool lps accounting and transfer ownership of lps from PositionManager contract
        pool.transferLP(address(this), owner, params_.indexes);

        emit RedeemPosition(owner, params_.tokenId, params_.indexes);
    }

Let's have a breakdown what's happening on each line.

The function first retrieves the pool and creates a memory array lpAmounts to hold the liquidity token amounts of the positions that are being redeemed.

Next, the function enters a for loop that iterates over the index values provided in params_.indexes.

In each iteration, it does the following:

1. Retrieves the Position structure related to the provided token ID and the current index. The Position structure contains details about the liquidity position, including the deposit time and the number of liquidity tokens (lps) held.
2. Checks if the position is valid by ensuring that the deposit time and the number of liquidity tokens are non-zero. If either of these values is zero, the function reverts with the message "RemovePositionFailed".
3. Checks if the bucket associated with the position has gone bankrupt after the position was deposited. If it has, the function reverts with the message "BucketBankrupt".
4. Attempts to remove the current index from positionIndex, which is a set of position indexes associated with the provided token ID. If this removal fails, the function reverts with the message "RemovePositionFailed".
5. Stores the number of liquidity tokens of the current position in the lpAmounts array.
6. Deletes the Position structure associated with the current index and the provided token ID from the positions mapping.

The problem here is in step 1 and 6. It retrieves the position in memory

   Position memory position = positions[params_.tokenId][index];

and then attempts to delete it

delete positions[params_.tokenId][index];

If we add some logs

            delete positions[params_.tokenId][index];

            console.log(position.lps);
            console.log(position.depositTime);

After adding some console.logs,

https://github.com/code-423n4/2023-05-ajna/blob/fc70fb9d05b13aee2b44be2cb652478535a90edd/ajna-core/src/PositionManager.sol#L387-L391

            // remove LP tracked by position manager at bucket index
            delete positions[params_.tokenId][index];
            console.log(position.lps);
            console.log(position.depositTime);

Running the testNFTTransferByApprove we see that the console log output shows 'position.lps' and 'position.depositTime' values remain unchanged even after the 'delete' operation. This means that the LP balance associated with the position is not being properly reset upon redemption.

However, when the position variable is changed to memory,

   Position storage position = positions[params_.tokenId][index];

            if (position.depositTime == 0 || position.lps == 0) revert RemovePositionFailed();

            // check that bucket didn't go bankrupt after memorialization
            if (_bucketBankruptAfterDeposit(pool, index, position.depositTime)) revert BucketBankrupt();

            // remove bucket index at which a position has added liquidity
            if (!positionIndex.remove(index)) revert RemovePositionFailed();

            lpAmounts[i] = position.lps;

            // remove LP tracked by position manager at bucket index
            delete positions[params_.tokenId][index];
            console.log(position.lps);
            console.log(position.depositTime);

Conversely, when the 'position' variable is declared as a storage variable, the deletion operation correctly resets the 'position.lps' and 'position.depositTime' values.

This issue could be exploited by a malicious user by redeeming their positions. Still, since the LPs for that position are not reset, the user could potentially call memorializePosition again with the same tokenId and index, effectively getting free LPs.

https://github.com/code-423n4/2023-05-ajna/blob/fc70fb9d05b13aee2b44be2cb652478535a90edd/ajna-core/src/PositionManager.sol#L210-L212

            position.lps += lpBalance;
            // set token's position deposit time to the original lender's deposit time
            position.depositTime = depositTime;

Side note: It could also be exploitable when moving liquidity.

Consider the following scenario:

1. User redeems positions for tokenId: 5 and positionIndex: 1
2. Due to the bug, the position.lps and position.depositTime do not get reset.
3. When calling memorializePositions, the old lps is incremented with the current lp balance
4. $

Tools Used

Manual Review

https://docs.soliditylang.org/en/latest/types.html#delete

Recommended Mitigation Steps

To fix this issue, change the declaration of the 'position' variable from memory to storage. This will ensure that the deletion operation correctly resets the LP balance associated with the position:

+            Position storage position = positions[params_.tokenId][index];

            if (position.depositTime == 0 || position.lps == 0) revert RemovePositionFailed();

            // check that bucket didn't go bankrupt after memorialization
            if (_bucketBankruptAfterDeposit(pool, index, position.depositTime)) revert BucketBankrupt();

            // remove bucket index at which a position has added liquidity
            if (!positionIndex.remove(index)) revert RemovePositionFailed();

            lpAmounts[i] = position.lps;

            // remove LP tracked by position manager at bucket index
            delete positions[params_.tokenId][index];

Assessed type

Other

[Picodes]

Indeed when logging console.log(position.lps); console.log(position.depositTime); values aren't 0 but this is the expected behavior as values have been copied in memory before the deletion of the storage slots.

The memory variables are scoped and will be cleared anyway, and the storage variables are properly deleted here.

[c4-judge]

Picodes marked the issue as unsatisfactory: Invalid