Particle Leverage AMM Protocol Invitational - adriro's results

Lines of code

https://github.com/code-423n4/2023-12-particle/blob/a3af40839b24aa13f5764d4f84933dbfa8bc8134/contracts/protocol/ParticlePositionManager.sol#L163-L164

Vulnerability details

Summary

Fees are incorrectly snapshotted when a new lien is created, potentially leading to a fee overpay.

Impact

The Particle LAMM protocol tracks fees using the same internal tracking built in Uniswap V3. Positions in Uniswap V3 contain a couple of variables feeGrowthInside0LastX128 and feeGrowthInside1LastX128 that track the last state in which fees were accrued to the position.

These values are flushed every time the NonfungiblePositionManager updates the position, for example in increaseLiquidity(), decreaseLiquidity() or collect(). The Uniswap implementation calculates the current values for feeGrowthInside0LastX128 and feeGrowthInside1LastX128, applies the difference using the last stored values to accrue the pending fees, and updates the latest values in position.feeGrowthInside0LastX128 and position.feeGrowthInside1LastX128. Using increaseLiquidity() as an example:

https://github.com/Uniswap/v3-periphery/blob/697c2474757ea89fec12a4e6db16a574fe259610/contracts/NonfungiblePositionManager.sol#L232-L250

232:         (, uin256 feeGrowthInside0LastX128, uint256 feeGrowthInside1LastX128, , ) = pool.positions(positionKey);
233: 
234:         position.tokensOwed0 += uint128(
235:             FullMath.mulDiv(
236:                 feeGrowthInside0LastX128 - position.feeGrowthInside0LastX128,
237:                 position.liquidity,
238:                 FixedPoint128.Q128
239:             )
240:         );
241:         position.tokensOwed1 += uint128(
242:             FullMath.mulDiv(
243:                 feeGrowthInside1LastX128 - position.feeGrowthInside1LastX128,
244:                 position.liquidity,
245:                 FixedPoint128.Q128
246:             )
247:         );
248: 
249:         position.feeGrowthInside0LastX128 = feeGrowthInside0LastX128;
250:         position.feeGrowthInside1LastX128 = feeGrowthInside1LastX128;

This means that NonfungiblePositionManager doesn't always hold the up-to-date values for these variables: if the position isn't updated, these variables indicate the values at the time of the last action performed over the position.

When a user opens a new position in Particle, the implementation grabs these two variables and stores them in the Lien structure, so that when the position is closed it can calculate the owed fees by taking the difference between the actual state of feeGrowthInside0LastX128 and feeGrowthInside1LastX128.

https://github.com/code-423n4/2023-12-particle/blob/a3af40839b24aa13f5764d4f84933dbfa8bc8134/contracts/protocol/ParticlePositionManager.sol#L151-L179

151:     function openPosition(
152:         DataStruct.OpenPositionParams calldata params
153:     ) public override nonReentrant returns (uint96 lienId, uint256 collateralTo) {
154:         if (params.liquidity == 0) revert Errors.InsufficientBorrow();
155: 
156:         // local cache to avoid stack too deep
157:         DataCache.OpenPositionCache memory cache;
158: 
159:         // prepare data for swap
160:         (
161:             cache.tokenFrom,
162:             cache.tokenTo,
163:             cache.feeGrowthInside0LastX128,
164:             cache.feeGrowthInside1LastX128,
165:             cache.collateralFrom,
166:             collateralTo
167:         ) = Base.prepareLeverage(params.tokenId, params.liquidity, params.zeroForOne);
168: 
169:         // decrease liquidity from LP position, pull the amount to this contract
170:         (cache.amountFromBorrowed, cache.amountToBorrowed) = LiquidityPosition.decreaseLiquidity(
171:             params.tokenId,
172:             params.liquidity
173:         );
174:         LiquidityPosition.collectLiquidity(
175:             params.tokenId,
176:             uint128(cache.amountFromBorrowed),
177:             uint128(cache.amountToBorrowed),
178:             address(this)
179:         );
...
247:         liens[keccak256(abi.encodePacked(msg.sender, lienId = _nextRecordId++))] = Lien.Info({
248:             tokenId: uint40(params.tokenId),
249:             liquidity: params.liquidity,
250:             token0PremiumPortion: cache.token0PremiumPortion,
251:             token1PremiumPortion: cache.token1PremiumPortion,
252:             startTime: uint32(block.timestamp),
253:             feeGrowthInside0LastX128: cache.feeGrowthInside0LastX128,
254:             feeGrowthInside1LastX128: cache.feeGrowthInside1LastX128,
255:             zeroForOne: params.zeroForOne
256:         });

As we can see in the previous snippet of code, the implementation fetches the values using Base.prepareLeverage(), which simply retrieves them from the positions() function of Uniswap NonfungiblePositionManager.

    function prepareLeverage(
        uint256 tokenId,
        uint128 liquidity,
        bool zeroForOne
    )
        internal
        view
        returns (
            address tokenFrom,
            address tokenTo,
            uint256 feeGrowthInside0LastX128,
            uint256 feeGrowthInside1LastX128,
            uint256 collateralFrom,
            uint256 collateralTo
        )
    {
        int24 tickLower;
        int24 tickUpper;
        (
            ,
            ,
            tokenFrom,
            tokenTo,
            ,
            tickLower,
            tickUpper,
            ,
            feeGrowthInside0LastX128,
            feeGrowthInside1LastX128,
            ,

        ) = UNI_POSITION_MANAGER.positions(tokenId);

However, these values are fetched before doing any modification to the position. The Uniswap position is modified after fetching the values for feeGrowthInside0LastX128 and feeGrowthInside1LastX128 by the calls to decreaseLiquidity() and collectLiquidity(). The values here belong to the current values at the time of the last action over the position, prior to opening the position in Particle.

This means that the borrower will overpay for any fee pending to be accrued in the LP prior to opening the position. When the borrower closes the position (or is liquidated) the protocol will calculate the fees by taking the difference between the current values and the snapshotted values in the Lien, which also include any pending fees present before opening the position.

1. LP is updated at t1, by any call to increaseLiquidity(), decreaseLiquidity(), etc.
2. Swaps are executed in the pool, earning fees for the LP.
3. A user opens a new position in Particle at t2.
4. The user closes the position at t3.

In this scenario, the user will not only pay for any accrued fees between t2 and t3, but also from t1 and t2.

Proof of Concept

The following test reproduces the issue. Here, we simulate several large swaps in the pool to earn fees for the LP. Then, right after opening a new position, we can see there are already owed fees in the newly created position.

Note: the snippet shows only the relevant code for the test. Full test file can be found here.

function test_ParticlePositionMananger_FeeOverpay() public {
    // artificially generate fees in the pool so the minted position gets fees
    address feeGenerator = makeAddr("feeGenerator");

    // mint 1M USDC to feeGenerator
    deal(address(USDC), feeGenerator, 1_000_000 * 1e6);

    for (uint256 i = 0; i < 10; i++) {
        uint256 usdcAmount = USDC.balanceOf(feeGenerator);
        _swap(feeGenerator, address(USDC), address(WETH), FEE, usdcAmount);

        uint256 wethAmount = WETH.balanceOf(feeGenerator);
        _swap(feeGenerator, address(WETH), address(USDC), FEE, wethAmount);
    }

    // update state
    IUniswapV3Pool pool = IUniswapV3Pool(uniswapV3Factory.getPool(address(USDC), address(WETH), FEE));
    (_sqrtRatioX96, _tick, , , , , ) = pool.slot0();

    // open position in particle
    uint128 borrowerLiquidity = _liquidity / _borrowerLiquidityPorition;
    (uint256 amount0ToBorrow, uint256 amount1ToBorrow) = LiquidityAmounts.getAmountsForLiquidity(
        _sqrtRatioX96,
        _sqrtRatioAX96,
        _sqrtRatioBX96,
        borrowerLiquidity
    );
    (, uint256 requiredEth) = particleInfoReader.getRequiredCollateral(borrowerLiquidity, _tickLower, _tickUpper);
    uint256 amountNeeded = QUOTER.quoteExactOutputSingle(
        address(USDC),
        address(WETH),
        FEE,
        requiredEth - amount1ToBorrow,
        0
    );
    uint256 amountIn = amountNeeded + amountNeeded / 1e6 - amount0ToBorrow; // 1e-6 tolerance

    _borrowToLong(SWAPPER, address(USDC), _tokenId, amountIn, amount0ToBorrow, borrowerLiquidity);

    // Get state of owed fees
    (uint256 token0Owed, uint256 token1Owed, , , , ) = particleInfoReader.getOwedInfo(SWAPPER, 0);

    // Fees are non-zero right after opening the position!
    assertGt(token0Owed, 0);
    assertGt(token1Owed, 0);

    console.log("Owed token0:", token0Owed);
    console.log("Owed token1:", token1Owed);
}

Recommendation

Fetch both feeGrowthInside0LastX128 and feeGrowthInside1LastX128 after the calls to withdraw the liquidity from the Uniswap position. This will ensure the values are correctly up to date, as the calls to decreaseLiquidity() or collectLiquidity() will update the Uniswap position.

    // prepare data for swap
    (
        cache.tokenFrom,
        cache.tokenTo,
-       cache.feeGrowthInside0LastX128,
-       cache.feeGrowthInside1LastX128,
+       ,
+       ,
        cache.collateralFrom,
        collateralTo
    ) = Base.prepareLeverage(params.tokenId, params.liquidity, params.zeroForOne);
    
    // decrease liquidity from LP position, pull the amount to this contract
    (cache.amountFromBorrowed, cache.amountToBorrowed) = LiquidityPosition.decreaseLiquidity(
        params.tokenId,
        params.liquidity
    );
    LiquidityPosition.collectLiquidity(
        params.tokenId,
        uint128(cache.amountFromBorrowed),
        uint128(cache.amountToBorrowed),
        address(this)
    );
    if (!params.zeroForOne)
        (cache.amountFromBorrowed, cache.amountToBorrowed) = (cache.amountToBorrowed, cache.amountFromBorrowed);
        
+   (, , , , , , , , cache.feeGrowthInside0LastX128, cache.feeGrowthInside1LastX128, , ) = Base
+       .UNI_POSITION_MANAGER
+       .positions(params.tokenId);

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-12-particle/blob/a3af40839b24aa13f5764d4f84933dbfa8bc8134/contracts/protocol/ParticlePositionManager.sol#L399

Vulnerability details

Summary

Swaps involved in liquidations may negatively impact the owner of the lien, since there is no incentive to execute a favorable trade as long as the received amount is enough to recover the liquidity.

Impact

When an existing position is closed, the protocol executes a swap to exchange back the "from" token (the long side) for the "to" token in order to recover the required tokens needed to fill back the borrowed liquidity.

https://github.com/code-423n4/2023-12-particle/blob/a3af40839b24aa13f5764d4f84933dbfa8bc8134/contracts/protocol/ParticlePositionManager.sol#L391-L406

391:     function _closePosition(
392:         DataStruct.ClosePositionParams calldata params,
393:         DataCache.ClosePositionCache memory cache,
394:         Lien.Info memory lien,
395:         address borrower
396:     ) internal {
397:         // optimistically use the input numbers to swap for repay
398:         /// @dev amountSwap overspend will be caught by refundWithCheck step in below
399:         (cache.amountSpent, cache.amountReceived) = Base.swap(
400:             cache.tokenFrom,
401:             cache.tokenTo,
402:             params.amountSwap,
403:             0, /// @dev we check cache.amountReceived is sufficient to repay LP in below
404:             DEX_AGGREGATOR,
405:             params.data
406:         );

As we can see, the swap is executed using a dex aggregator, but the operation is mostly opaque to the Particle protocol. The implementation only needs the swapped amounts, amountSpent and amountReceived, to determine and verify the rest of the close position logic.

When this action is executed by the borrower, i.e. the owner of the lien, the caller is incentivized to perform a favorable trade (that is, a good trade route that maximizes the outcome with proper slippage control). Since amountReceived determines the outcome of the tokens refunded to the borrower, the borrower is naturally interested in performing a beneficial trade (and failing to do so will only cause damage to themselves).

However, the same logic doesn't apply during liquidations. Liquidators earn a fee that is a percentage of the lien premiums. There is no incentive to execute a favorable trade for the borrower. The liquidator can even sandwich the transaction themselves, also profiting from the swap.

Recommendation

It is difficult to provide a recommendation that doesn't introduce friction to the protocol. A potential solution could be to introduce liquidation rewards that are also based on the borrower's outcome: part of the refunded tokens to the borrower go to the liquidator, this way the liquidator is incentivized to also maximize the borrower's outcome.

Another solution, though more complex and riskier, could be to introduce an oracle that controls the output amount from the trade. As tokens are pulled from Uniswap pools, this could be implemented directly by using the oracles provided by Uniswap.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-12-particle/blob/a3af40839b24aa13f5764d4f84933dbfa8bc8134/contracts/protocol/ParticlePositionManager.sol#L377-L378

Vulnerability details

Summary

Some ERC20 implementations revert on zero value transfers. Since liquidation rewards are based on a fraction of the available position's premiums, this may cause an accidental denial of service that prevents the successful execution of liquidations.

Impact

Liquidations in the LAMM protocol are incentivized by a reward that is calculated as a fraction of the premiums available in the position.

https://github.com/code-423n4/2023-12-particle/blob/a3af40839b24aa13f5764d4f84933dbfa8bc8134/contracts/protocol/ParticlePositionManager.sol#L348-L354

348:         // calculate liquidation reward
349:         liquidateCache.liquidationRewardFrom =
350:             ((closeCache.tokenFromPremium) * LIQUIDATION_REWARD_FACTOR) /
351:             uint128(Base.BASIS_POINT);
352:         liquidateCache.liquidationRewardTo =
353:             ((closeCache.tokenToPremium) * LIQUIDATION_REWARD_FACTOR) /
354:             uint128(Base.BASIS_POINT);

These amounts are later transferred to the caller, the liquidator, at the end of the liquidatePosition() function.

https://github.com/code-423n4/2023-12-particle/blob/a3af40839b24aa13f5764d4f84933dbfa8bc8134/contracts/protocol/ParticlePositionManager.sol#L376-L378

376:         // reward liquidator
377:         TransferHelper.safeTransfer(closeCache.tokenFrom, msg.sender, liquidateCache.liquidationRewardFrom);
378:         TransferHelper.safeTransfer(closeCache.tokenTo, msg.sender, liquidateCache.liquidationRewardTo);

Reward amounts, liquidationRewardFrom and liquidationRewardTo, can be calculated as zero if tokenFromPremium or tokenToPremium are zero, if the liquidation ratio gets rounded down to zero, or if LIQUIDATION_REWARD_FACTOR is zero.

Coupled with that fact that some ERC20 implementations revert on zero value transfers, this can cause an accidental denial of service in the implementation of liquidatePosition(), blocking certain positions from being liquidated.

Recommendation

Check that the amounts are greater than zero before executing the transfer.

        // reward liquidator
+       if (liquidateCache.liquidationRewardFrom > 0) {
          TransferHelper.safeTransfer(closeCache.tokenFrom, msg.sender, liquidateCache.liquidationRewardFrom);
+       }
+       if (liquidateCache.liquidationRewardTo > 0) {
          TransferHelper.safeTransfer(closeCache.tokenTo, msg.sender, liquidateCache.liquidationRewardTo);
+       }

Assessed type

ERC20

Lines of code

https://github.com/code-423n4/2023-12-particle/blob/a3af40839b24aa13f5764d4f84933dbfa8bc8134/contracts/libraries/LiquidityPosition.sol#L144 https://github.com/code-423n4/2023-12-particle/blob/a3af40839b24aa13f5764d4f84933dbfa8bc8134/contracts/libraries/LiquidityPosition.sol#L197 https://github.com/code-423n4/2023-12-particle/blob/a3af40839b24aa13f5764d4f84933dbfa8bc8134/contracts/libraries/LiquidityPosition.sol#L260

Vulnerability details

Summary

The protocol is using block.timestamp as the deadline argument while interacting with the Uniswap NFT Position Manager, which completely defeats the purpose of using a deadline.

Impact

Actions in the Uniswap NonfungiblePositionManager contract are protected by a deadline parameter to limit the execution of pending transactions. Functions that modify the liquidity of the pool check this parameter against the current block timestamp in order to discard expired actions.

These interactions with the Uniswap position are present in the LiquidityPosition library. The functions mint(), increaseLiquidity() and decreaseLiquidity() call their corresponding functions in the Uniswap Position Manager, providing block.timestamp as the argument for the deadline parameter:

https://github.com/code-423n4/2023-12-particle/blob/a3af40839b24aa13f5764d4f84933dbfa8bc8134/contracts/libraries/LiquidityPosition.sol#L131-L146

131:         // mint the position
132:         (tokenId, liquidity, amount0Minted, amount1Minted) = Base.UNI_POSITION_MANAGER.mint(
133:             INonfungiblePositionManager.MintParams({
134:                 token0: params.token0,
135:                 token1: params.token1,
136:                 fee: params.fee,
137:                 tickLower: params.tickLower,
138:                 tickUpper: params.tickUpper,
139:                 amount0Desired: params.amount0ToMint,
140:                 amount1Desired: params.amount1ToMint,
141:                 amount0Min: params.amount0Min,
142:                 amount1Min: params.amount1Min,
143:                 recipient: address(this),
144:                 deadline: block.timestamp
145:             })
146:         );

https://github.com/code-423n4/2023-12-particle/blob/a3af40839b24aa13f5764d4f84933dbfa8bc8134/contracts/libraries/LiquidityPosition.sol#L189-L199

189:         // increase liquidity via position manager
190:         (liquidity, amount0Added, amount1Added) = Base.UNI_POSITION_MANAGER.increaseLiquidity(
191:             INonfungiblePositionManager.IncreaseLiquidityParams({
192:                 tokenId: tokenId,
193:                 amount0Desired: amount0,
194:                 amount1Desired: amount1,
195:                 amount0Min: 0,
196:                 amount1Min: 0,
197:                 deadline: block.timestamp
198:             })
199:         );

https://github.com/code-423n4/2023-12-particle/blob/a3af40839b24aa13f5764d4f84933dbfa8bc8134/contracts/libraries/LiquidityPosition.sol#L254-L262

254:         (amount0, amount1) = Base.UNI_POSITION_MANAGER.decreaseLiquidity(
255:             INonfungiblePositionManager.DecreaseLiquidityParams({
256:                 tokenId: tokenId,
257:                 liquidity: liquidity,
258:                 amount0Min: 0,
259:                 amount1Min: 0,
260:                 deadline: block.timestamp
261:             })
262:         );

Using block.timestamp as the deadline is effectively a no-operation that has no effect nor protection. Since block.timestamp will take the timestamp value when the transaction gets mined, the check will end up comparing block.timestamp against the same value, i.e. block.timestamp <= block.timestamp (see https://github.com/Uniswap/v3-periphery/blob/697c2474757ea89fec12a4e6db16a574fe259610/contracts/base/PeripheryValidation.sol#L7).

Failure to provide a proper deadline value enables pending transactions to be maliciously executed at a later point. Transactions that provide an insufficient amount of gas such that they are not mined within a reasonable amount of time, can be picked by malicious actors or MEV bots and executed later in detriment of the submitter.

See this issue for an excellent reference on the topic (the author runs a MEV bot).

Recommendation

Add a deadline parameter to each of the functions that are used to manage the liquidity position, ParticlePositionManager.mint(), ParticlePositionManager.increaseLiquidity() and ParticlePositionManager.decreaseLiquidity(). Forward this parameter to the corresponding underlying calls to the Uniswap NonfungiblePositionManager contract.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-12-particle/blob/a3af40839b24aa13f5764d4f84933dbfa8bc8134/contracts/protocol/ParticlePositionManager.sol#L424 https://github.com/code-423n4/2023-12-particle/blob/a3af40839b24aa13f5764d4f84933dbfa8bc8134/contracts/protocol/ParticlePositionManager.sol#L432

Vulnerability details

Summary

When a position is closed, there is no check to ensure that the effective added liquidity covers the original borrowed liquidity from the LP.

Impact

Closing a position in the Particle LAMM protocol must ensure that the borrowed liquidity gets fully added back to the LP. Independently of the outcome of the trade, the LP should get its liquidity back. The implementation of _closePosition() calculates the required amounts and executes a call to LiquidityPosition.increaseLiquidity(), which ends up calling increaseLiquidity() in the Uniswap Position Manager.

https://github.com/code-423n4/2023-12-particle/blob/a3af40839b24aa13f5764d4f84933dbfa8bc8134/contracts/protocol/ParticlePositionManager.sol#L422-L439

422:         // add liquidity back to borrower
423:         if (lien.zeroForOne) {
424:             (cache.liquidityAdded, cache.amountToAdd, cache.amountFromAdd) = LiquidityPosition.increaseLiquidity(
425:                 cache.tokenTo,
426:                 cache.tokenFrom,
427:                 lien.tokenId,
428:                 cache.amountToAdd,
429:                 cache.amountFromAdd
430:             );
431:         } else {
432:             (cache.liquidityAdded, cache.amountFromAdd, cache.amountToAdd) = LiquidityPosition.increaseLiquidity(
433:                 cache.tokenFrom,
434:                 cache.tokenTo,
435:                 lien.tokenId,
436:                 cache.amountFromAdd,
437:                 cache.amountToAdd
438:             );
439:         }

As we can see in lines 424 and 432, the effective results of the liquidity increment are returned from the call to Uniswap NPM. Both amountToAdd and amountFromAdd are correctly overwritten with the actual amounts used by the addLiquidity() action, but liquidityAdded is simply assigned to the cache and never checked.

The effective added liquidity may fall short to cover the original borrowed liquidity, and if so the lien will be closed without returning the full amount to the LP.

Recommendation

Ensure the effective added liquidity covers the original borrowed amount.

        // add liquidity back to borrower
        if (lien.zeroForOne) {
            (cache.liquidityAdded, cache.amountToAdd, cache.amountFromAdd) = LiquidityPosition.increaseLiquidity(
                cache.tokenTo,
                cache.tokenFrom,
                lien.tokenId,
                cache.amountToAdd,
                cache.amountFromAdd
            );
        } else {
            (cache.liquidityAdded, cache.amountFromAdd, cache.amountToAdd) = LiquidityPosition.increaseLiquidity(
                cache.tokenFrom,
                cache.tokenTo,
                lien.tokenId,
                cache.amountFromAdd,
                cache.amountToAdd
            );
        }
        
+       require(cache.liquidityAdded >= lien.liquidity, "Failed to cover borrowed liquidity");

Assessed type

Other

require(cache.liquidityAdded + lien.liquidity / 1_000_000 >= lien.liquidity, "Failed to cover borrowed liquidity");

Lines of code

https://github.com/code-423n4/2023-12-particle/blob/a3af40839b24aa13f5764d4f84933dbfa8bc8134/contracts/protocol/ParticlePositionManager.sol#L501

Vulnerability details

Summary

Fees are applied to premiums when a new position is opened, but the same mechanism is not enforced when margin is added to an existing position.

Impact

When a new position is created in the LAMM protocol, fees are collected in favor of the LP owner that provides the liquidity for the position. The fee is calculated by applying a configurable rate over the tokens "from" amounts:

https://github.com/code-423n4/2023-12-particle/blob/a3af40839b24aa13f5764d4f84933dbfa8bc8134/contracts/protocol/ParticlePositionManager.sol#L191-L201

191:         // pay for fee
192:         if (FEE_FACTOR > 0) {
193:             cache.feeAmount = ((params.marginFrom + cache.amountFromBorrowed) * FEE_FACTOR) / Base.BASIS_POINT;
194:             cache.treasuryAmount = (cache.feeAmount * _treasuryRate) / Base.BASIS_POINT;
195:             _treasury[cache.tokenFrom] += cache.treasuryAmount;
196:             if (params.zeroForOne) {
197:                 lps.addTokensOwed(params.tokenId, uint128(cache.feeAmount - cache.treasuryAmount), 0);
198:             } else {
199:                 lps.addTokensOwed(params.tokenId, 0, uint128(cache.feeAmount - cache.treasuryAmount));
200:             }
201:         }

The fee is applied to amountFromBorrowed, the "from" amount from the LP liquidity, and marginFrom, the user provided margin to cover the required amount for the swap and provide a premium in the position.

However, the same logic is not followed when the borrower adds more margin using addPremium(). In this case, the amounts are added in full to the position's premiums without incurring any fees.

This not only represents an inconsistent behavior (which may be a design decision), but allows the borrower to avoid paying part of the fees. The borrower can provide enough marginTo so that the swap can reach the required collateralTo amount, leaving the token "from" premium at zero. Then, in a second call, provide the needed premium to cover for eventual LP fees using addPremium(). This can be done with null liquidation risk and is greatly simplified by leveraging the multicall functionality. The borrower can bundle both operations in a single transaction, without even deploying a contract.

Proof of Concept

A user can skip paying part of the fees by bundling two operations in a single transaction:

1. A call to openPosition() with the minimal required amount of marginFrom so that amountReceived + amountToBorrowed + marginTo >= collateralTo.
2. A call to addPremium() to provide the required premiums to cover for eventual fees.

Recommendation

Apply the same fee schedule to addPremium(). Depending on the value of zeroForOne, determine the "from" token, apply the FEE_FACTOR to it, and accrue the fee to the corresponding liquidity position.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-12-particle/blob/a3af40839b24aa13f5764d4f84933dbfa8bc8134/contracts/protocol/ParticlePositionManager.sol#L365

Vulnerability details

Summary

Protocol admins can modify the loan term settings. This action can inadvertently default existing loans created under different terms.

Impact

Positions in the Particle LAMM protocol are created for a configurable period of time, defined by the LOAN_TERM variable. If the loan exceeds this duration, and the LP owner stops renewals that affect their position, the lien can be liquidated.

https://github.com/code-423n4/2023-12-particle/blob/a3af40839b24aa13f5764d4f84933dbfa8bc8134/contracts/protocol/ParticlePositionManager.sol#L358-L368

358:         // check for liquidation condition
359:         ///@dev the liquidation condition is that
360:         ///     (EITHER premium is not enough) OR (cutOffTime > startTime AND currentTime > startTime + LOAN_TERM)
361:         if (
362:             !((closeCache.tokenFromPremium < liquidateCache.tokenFromOwed ||
363:                 closeCache.tokenToPremium < liquidateCache.tokenToOwed) ||
364:                 (lien.startTime < lps.getRenewalCutoffTime(lien.tokenId) &&
365:                     lien.startTime + LOAN_TERM < block.timestamp))
366:         ) {
367:             revert Errors.LiquidationNotMet();
368:         }

The liquidation condition in line 365 does the check using the current value of LOAN_TERM. As the loan term can be updated using updateLoanTerm(), this means that reducing this value may inadvertently cause the liquidation of existing positions that were originally intended for a longer period of time.

Proof of concept

Let's say the current configured loan term in ParticlePositionManager is 2 weeks.

1. A user creates a new position, expecting it to last at least 2 weeks.
2. The owner of the LP calls reclaimLiquidity() to stop it from being renewed.
3. The protocol changes the loan term setting to 1 week.
4. The user is liquidated after 1 week.

Recommendation

Store the loan term value at the time the position was created in the Lien structure, e.g. in lien.loanTerm. When checking the liquidation condition, calculate the end time using this value to honor the original loan term.

    if (
        !((closeCache.tokenFromPremium < liquidateCache.tokenFromOwed ||
            closeCache.tokenToPremium < liquidateCache.tokenToOwed) ||
            (lien.startTime < lps.getRenewalCutoffTime(lien.tokenId) &&
-               lien.startTime + LOAN_TERM < block.timestamp))
+               lien.startTime + lien.loanTerm < block.timestamp))
    ) {
        revert Errors.LiquidationNotMet();
    }

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-12-particle/blob/a3af40839b24aa13f5764d4f84933dbfa8bc8134/contracts/protocol/ParticlePositionManager.sol#L355-L356

Vulnerability details

Summary

The premiums used to determine the liquidation condition have the liquidation reward already discounted, potentially causing a lien to be considered underwater while technically it is not.

Impact

Positions in Particle LAMM can be liquidated if the owed tokens exceed the available premiums in the lien. Liquidators are incentivized with a reward to keep the protocol sane. This can be found in the implementation of liquidatePosition():

https://github.com/code-423n4/2023-12-particle/blob/a3af40839b24aa13f5764d4f84933dbfa8bc8134/contracts/protocol/ParticlePositionManager.sol#L348-L368

348:         // calculate liquidation reward
349:         liquidateCache.liquidationRewardFrom =
350:             ((closeCache.tokenFromPremium) * LIQUIDATION_REWARD_FACTOR) /
351:             uint128(Base.BASIS_POINT);
352:         liquidateCache.liquidationRewardTo =
353:             ((closeCache.tokenToPremium) * LIQUIDATION_REWARD_FACTOR) /
354:             uint128(Base.BASIS_POINT);
355:         closeCache.tokenFromPremium -= liquidateCache.liquidationRewardFrom;
356:         closeCache.tokenToPremium -= liquidateCache.liquidationRewardTo;
357: 
358:         // check for liquidation condition
359:         ///@dev the liquidation condition is that
360:         ///     (EITHER premium is not enough) OR (cutOffTime > startTime AND currentTime > startTime + LOAN_TERM)
361:         if (
362:             !((closeCache.tokenFromPremium < liquidateCache.tokenFromOwed ||
363:                 closeCache.tokenToPremium < liquidateCache.tokenToOwed) ||
364:                 (lien.startTime < lps.getRenewalCutoffTime(lien.tokenId) &&
365:                     lien.startTime + LOAN_TERM < block.timestamp))
366:         ) {
367:             revert Errors.LiquidationNotMet();
368:         }

The liquidation rewards are calculated as a portion of the premiums, and discounted from these to keep them reserved from any further calculation that may involve tokenFromPremium or tokenToPremium. However, these are discounted before checking the liquidation condition.

The values used to check the conditions at lines 362 and 363 already incorporate the discounted rewards. This implies that the borrower might still maintain a healthy position if it weren't for the subtracted amounts. The lien could be liquidated even if it's not technically underwater.

Recommendation

Subtract the liquidation rewards from premiums after checking the liquidation condition.

-       // calculate liquidation reward
-       liquidateCache.liquidationRewardFrom =
-           ((closeCache.tokenFromPremium) * LIQUIDATION_REWARD_FACTOR) /
-           uint128(Base.BASIS_POINT);
-       liquidateCache.liquidationRewardTo =
-           ((closeCache.tokenToPremium) * LIQUIDATION_REWARD_FACTOR) /
-           uint128(Base.BASIS_POINT);
-       closeCache.tokenFromPremium -= liquidateCache.liquidationRewardFrom;
-       closeCache.tokenToPremium -= liquidateCache.liquidationRewardTo;

        // check for liquidation condition
        ///@dev the liquidation condition is that
        ///     (EITHER premium is not enough) OR (cutOffTime > startTime AND currentTime > startTime + LOAN_TERM)
        if (
            !((closeCache.tokenFromPremium < liquidateCache.tokenFromOwed ||
                closeCache.tokenToPremium < liquidateCache.tokenToOwed) ||
                (lien.startTime < lps.getRenewalCutoffTime(lien.tokenId) &&
                    lien.startTime + LOAN_TERM < block.timestamp))
        ) {
            revert Errors.LiquidationNotMet();
        }
        
+       // calculate liquidation reward
+       liquidateCache.liquidationRewardFrom =
+           ((closeCache.tokenFromPremium) * LIQUIDATION_REWARD_FACTOR) /
+           uint128(Base.BASIS_POINT);
+       liquidateCache.liquidationRewardTo =
+           ((closeCache.tokenToPremium) * LIQUIDATION_REWARD_FACTOR) /
+           uint128(Base.BASIS_POINT);
+       closeCache.tokenFromPremium -= liquidateCache.liquidationRewardFrom;
+       closeCache.tokenToPremium -= liquidateCache.liquidationRewardTo;

Assessed type

Other

closeCache.tokenFromPremium = liquidateCache.tokenFromOwed || 
closeCache.tokenToPremium = liquidateCache.tokenToOwed

Lines of code

https://github.com/code-423n4/2023-12-particle/blob/a3af40839b24aa13f5764d4f84933dbfa8bc8134/contracts/protocol/ParticlePositionManager.sol#L142

Vulnerability details

Summary

LP owners can reclaim liquidity to stop it from being extended for current liens but this doesn't stop from being used in new positions.

Impact

LP owners can signal their intention to pull liquidity by calling reclaimLiquidity(). This function updates the value of renewalCutoffTime in the liquidity position, which works as a barrier to stop current borrowers from extending the use of liquidity. Affected positions may be liquidated after the elapsed loan term:

https://github.com/code-423n4/2023-12-particle/blob/a3af40839b24aa13f5764d4f84933dbfa8bc8134/contracts/protocol/ParticlePositionManager.sol#L358-L368

        // check for liquidation condition
        ///@dev the liquidation condition is that
        ///     (EITHER premium is not enough) OR (cutOffTime > startTime AND currentTime > startTime + LOAN_TERM)
        if (
            !((closeCache.tokenFromPremium < liquidateCache.tokenFromOwed ||
                closeCache.tokenToPremium < liquidateCache.tokenToOwed) ||
                (lien.startTime < lps.getRenewalCutoffTime(lien.tokenId) &&
                    lien.startTime + LOAN_TERM < block.timestamp))
        ) {
            revert Errors.LiquidationNotMet();
        }

However, this only impacts existing positions. Borrowers may be forced to return liquidity to avoid liquidations, but this doesn't stop it from being reclaimed for a new position before the LP owner can decrease it from the position.

Note that this scenario can happen accidentally or intentionally. An attacker can purposely open positions for a specific tokenId in order to prevent liquidity from being pulled: a simple multicall transaction can close the position only to open it again and renew the loan period, forcing the LP owner to call reclaimLiquidity() again, at which point the attacker can also repeat the process near the end of the loan term. Also, market conditions can lead to repeatedly opening positions due to favorable conditions. An existing borrower may return liquidity, but other players in the market can immediately open new positions from the returned liquidity before the LP owners gets a chance to decrease the liquidity from their position.

Recommendation

Add a new setting to let the owner of the LP configure to stop accepting new positions from their tokenId. This setting can be stored in the LiquidityPosition.Info struct. Prevent new positions from being created if this flag is enabled, so that the owner of LP can eventually collect all the liquidity after existing positions expire.

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2023-12-particle/blob/a3af40839b24aa13f5764d4f84933dbfa8bc8134/contracts/protocol/ParticlePositionManager.sol#L118 https://github.com/code-423n4/2023-12-particle/blob/a3af40839b24aa13f5764d4f84933dbfa8bc8134/contracts/protocol/ParticlePositionManager.sol#L127

Vulnerability details

Summary

The owner of the LP cannot specify the slippage parameters while interacting with this position in increaseLiquidity() and decreaseLiquidity().

Impact

Liquidity interaction with Uniswap is protected by minimum amount parameters that control the slippage of these actions. Minting a new position, increasing liquidity or decreasing liquidity in the Uniswap NonfungiblePositionManager contract allows defining minimum output parameters that control the outcome of the actions (see parameters structs in INonfungiblePositionManager).

On the Particle side, liquidity providers can only control slippage in the mint action. The implementation of mint() includes slippage parameters and correctly forwards those to the NPM contract:

https://github.com/code-423n4/2023-12-particle/blob/a3af40839b24aa13f5764d4f84933dbfa8bc8134/contracts/libraries/LiquidityPosition.sol#L131-L146

131:         // mint the position
132:         (tokenId, liquidity, amount0Minted, amount1Minted) = Base.UNI_POSITION_MANAGER.mint(
133:             INonfungiblePositionManager.MintParams({
134:                 token0: params.token0,
135:                 token1: params.token1,
136:                 fee: params.fee,
137:                 tickLower: params.tickLower,
138:                 tickUpper: params.tickUpper,
139:                 amount0Desired: params.amount0ToMint,
140:                 amount1Desired: params.amount1ToMint,
141:                 amount0Min: params.amount0Min,
142:                 amount1Min: params.amount1Min,
143:                 recipient: address(this),
144:                 deadline: block.timestamp
145:             })
146:         );

However, in both increaseLiquidity() and decreaseLiquidity(), these parameters are hardcoded to zero:

https://github.com/code-423n4/2023-12-particle/blob/a3af40839b24aa13f5764d4f84933dbfa8bc8134/contracts/libraries/LiquidityPosition.sol#L189-L199

189:         // increase liquidity via position manager
190:         (liquidity, amount0Added, amount1Added) = Base.UNI_POSITION_MANAGER.increaseLiquidity(
191:             INonfungiblePositionManager.IncreaseLiquidityParams({
192:                 tokenId: tokenId,
193:                 amount0Desired: amount0,
194:                 amount1Desired: amount1,
195:                 amount0Min: 0,
196:                 amount1Min: 0,
197:                 deadline: block.timestamp
198:             })
199:         );

https://github.com/code-423n4/2023-12-particle/blob/a3af40839b24aa13f5764d4f84933dbfa8bc8134/contracts/libraries/LiquidityPosition.sol#L254-L262

254:         (amount0, amount1) = Base.UNI_POSITION_MANAGER.decreaseLiquidity(
255:             INonfungiblePositionManager.DecreaseLiquidityParams({
256:                 tokenId: tokenId,
257:                 liquidity: liquidity,
258:                 amount0Min: 0,
259:                 amount1Min: 0,
260:                 deadline: block.timestamp
261:             })
262:         );

Since these parameters are always hardcoded to zero, the LP owner cannot possibly protect against slippage while managing their positions

Recommendation

Similar to the implementation of mint(), add the corresponding slippage parameters to ParticlePositionManager.increaseLiquidity() and ParticlePositionManager.decreaseLiquidity() and forward these to the underlying calls to the Uniswap Position Manager contract.

Assessed type

Other

Report

Summary

Low Issues

Total of 7 issues:

Non Critical Issues

Total of 7 issues:

Low Issues

<a name="L-1"></a>[L-1] LP tokens cannot be removed

Besides minting positions in Uniswap, the protocol accepts existing positions by transferring the UniV3 NFT to the Particle contract. The onERC721Received() callback is triggered when executing a safeTransferFrom() and will correctly create the lp position in ParticlePositionManager.

However, while the position can be managed through the Particle contract, there's no existing functionality to recover the NFT back.

<a name="L-2"></a>[L-2] LP owners cannot selectively choose which loans not to renew

LP owners can reclaim liquidity from their loans, but calling reclaimLiquidity() will update the renewalCutoffTime variable, affecting all existing positions.

Owners of liquidity positions cannot selectively decide which loans not to renew, needing to reclaim all active positions even if just a single one needs to be pulled off.

<a name="L-3"></a>[L-3] ParticlePositionManager should not accept ERC721 transfer from NFT collections other than UniswapV3

The implementation of ParticlePositionManager accepts NFT transfers via the onERC721Received() that is triggered

https://github.com/code-423n4/2023-12-particle/blob/a3af40839b24aa13f5764d4f84933dbfa8bc8134/contracts/protocol/ParticlePositionManager.sol#L98-L111

098:     function onERC721Received(
099:         address,
100:         address from,
101:         uint256 tokenId,
102:         bytes calldata
103:     ) external override returns (bytes4) {
104:         if (msg.sender == Base.UNI_POSITION_MANAGER_ADDR) {
105:             // matched with Uniswap v3 position NFTs
106:             lps[tokenId] = LiquidityPosition.Info({owner: from, renewalCutoffTime: 0, token0Owed: 0, token1Owed: 0});
107:             (, , , , , , , uint128 liquidity, , , , ) = Base.UNI_POSITION_MANAGER.positions(tokenId);
108:             emit LiquidityPosition.SupplyLiquidity(tokenId, from, liquidity);
109:         }
110:         return this.onERC721Received.selector;
111:     }

As we can see, the function registers the liquidity position if the caller is the Uniswap V3 NFT collection, but also accepts other NFT collections as the correct selector value is returned for all cases.

<a name="L-4"></a>[L-4] Incompatibility with fee-on-transfer tokens

The protocol is mostly incompatible with ERC20 implementations that take a fee or tax on transfer.

There are different occurrences across the codebase in which ERC20 transfers are expected to receive the same amount as tokens transferred, an invariant that doesn't hold in fee-on-transfer tokens.

Examples of this are mint() and increaseLiquidity() in the LiquidityPosition library, the swap() function of the SwapPosition library, or the margin transfers in openPosition() and addPremium() of the ParticlePositionManager contract.

<a name="L-5"></a>[L-5] Missing validations in ParticleInfoReader initializer

The initializer of the ParticleInfoReader contract doesn't validate that the given address is not the empty address, a check that is present in the associated setter updateParticleAddress().

https://github.com/code-423n4/2023-12-particle/blob/a3af40839b24aa13f5764d4f84933dbfa8bc8134/contracts/protocol/ParticleInfoReader.sol#L37-L42

37:     function initialize(address particleAddr) external initializer {
38:         __UUPSUpgradeable_init();
39:         __Ownable_init();
40:         PARTICLE_POSITION_MANAGER_ADDR = particleAddr;
41:         _particlePositionManager = ParticlePositionManager(particleAddr);
42:     }

Consider adding a check to ensure that particleAddr != address(0).

<a name="L-6"></a>[L-6] ParticleInfoReader.getOwedInfo() misses to return information about the trade side

https://github.com/code-423n4/2023-12-particle/blob/a3af40839b24aa13f5764d4f84933dbfa8bc8134/contracts/protocol/ParticleInfoReader.sol#L353

The implementation of getOwedInfo() returns information using the token0/token1 which doesn't indicate which is the "long" side of the position.

Consider using the tokenFrom/tokenTo naming scheme (and internally flipping these depending on the value of zeroForOne), or returning the lien's zeroForOne value so that the caller can interpret the returned values.

<a name="L-7"></a>[L-7] Missing validations in ParticlePositionManager initializer

None of the arguments in the ParticlePositionManager initializer are checked to be valid, though all of these are validated in their respective setters.

https://github.com/code-423n4/2023-12-particle/blob/a3af40839b24aa13f5764d4f84933dbfa8bc8134/contracts/protocol/ParticlePositionManager.sol#L60-L74

60:     function initialize(
61:         address dexAggregator,
62:         uint256 feeFactor,
63:         uint128 liquidationRewardFactor,
64:         uint256 loanTerm,
65:         uint256 treasuryRate
66:     ) external initializer {
67:         __UUPSUpgradeable_init();
68:         __Ownable_init();
69:         DEX_AGGREGATOR = dexAggregator;
70:         FEE_FACTOR = feeFactor;
71:         LIQUIDATION_REWARD_FACTOR = liquidationRewardFactor;
72:         LOAN_TERM = loanTerm;
73:         _treasuryRate = treasuryRate;
74:     }

These missing validation could lead to having an initial set of invalid values after the contract is initialized.

Consider adding the following checks:

• dexAggregator != address(0)
• feeFactor <= _FEE_FACTOR_MAX
• liquidationRewardFactor <= _LIQUIDATION_REWARD_FACTOR_MAX
• LOAN_TERM <= _LOAN_TERM_MAX
• treasuryRate <= _TREASURY_RATE_MAX

Non Critical Issues

<a name="NC-1"></a>[NC-1] Unused parameter in OpenPositionParams struct

The OpenPositionParams struct contains a field called marginPremiumRatio that is not used in the implementation.

06:     struct OpenPositionParams {
07:         uint256 tokenId;
08:         uint256 marginFrom;
09:         uint256 marginTo;
10:         uint256 amountSwap;
11:         uint128 liquidity;
12:         uint24 tokenFromPremiumPortionMin;
13:         uint24 tokenToPremiumPortionMin;
14:         uint8 marginPremiumRatio;
15:         bool zeroForOne;
16:         bytes data;
17:     }

Consider removing this field from the struct to avoid any confusion.

<a name="NC-2"></a>[NC-2] Unnecessary multicall feature in ParticleInfoReader

https://github.com/code-423n4/2023-12-particle/blob/a3af40839b24aa13f5764d4f84933dbfa8bc8134/contracts/protocol/ParticleInfoReader.sol#L21

The ParticleInfoReader contract inherits the Multicall mixin, but since this contract is mostly composed of read only functions to query the state of the Position Manager, it is not entirely clear the purpose of having a multicall feature in this type of contract.

Consider removing the Multicall feature in ParticleInfoReader.

<a name="NC-3"></a>[NC-3] Uniswap fee tiers can be dynamic

The implementation of getDeepPool() loops through the default fee tiers in order to fetch the pool with the deepest liquidity.

https://github.com/code-423n4/2023-12-particle/blob/a3af40839b24aa13f5764d4f84933dbfa8bc8134/contracts/protocol/ParticleInfoReader.sol#L102-L117

102:     function getDeepPool(address token0, address token1) external view returns (address deepPool) {
103:         uint24[3] memory feeTiers = [uint24(500), uint24(3000), uint24(10000)];
104:         uint128 maxLiquidity = 0;
105: 
106:         for (uint256 i = 0; i < feeTiers.length; i++) {
107:             address poolAddress = Base.UNI_FACTORY.getPool(token0, token1, feeTiers[i]);
108:             if (poolAddress != address(0)) {
109:                 IUniswapV3Pool pool = IUniswapV3Pool(poolAddress);
110:                 uint128 liquidity = pool.liquidity();
111:                 if (liquidity > maxLiquidity) {
112:                     maxLiquidity = liquidity;
113:                     deepPool = poolAddress;
114:                 }
115:             }
116:         }
117:     }

Even though the implementation is using the default fee tiers (500, 3000 and 10000), it is important to note that fee tiers are dynamic and can be configured in Uniswap. See the implementation of enableFeeAmount().

<a name="NC-4"></a>[NC-4] owner variable is shadowed in getLiquidityPosition()

The implementation of getLiquidityPosition() uses the owner variable name to refer to the owner of the liquidity position.

This naming is clashing with the owner variable coming from the Ownable2StepUpgradeable base contract.

<a name="NC-5"></a>[NC-5] Unused GetLienCache struct

https://github.com/code-423n4/2023-12-particle/blob/a3af40839b24aa13f5764d4f84933dbfa8bc8134/contracts/protocol/ParticleInfoReader.sol#L254

The GetLienCache struct defined as part of the ParticleInfoReader contract is not used in any part of the codebase and can be safely removed to avoid any potential confusion.

<a name="NC-6"></a>[NC-6] Unused SwapPosition library in ParticlePositionManager

The ParticlePositionManager contract is importing the SwapPosition library with a using construct.

https://github.com/code-423n4/2023-12-particle/blob/a3af40839b24aa13f5764d4f84933dbfa8bc8134/contracts/protocol/ParticlePositionManager.sol#L29

29:     using SwapPosition for mapping(bytes32 => SwapPosition.Info);

This import is not used in the implementation of ParticlePositionManager and can be safely removed.

<a name="NC-7"></a>[NC-7] Incorrect call to base contract initializer

The initializer of the ParticlePositionManager contract is calling the __Ownable_init() corresponding to the Ownable contract, even though the contract is actually inheriting from Ownable2StepUpgradeable.

https://github.com/code-423n4/2023-12-particle/blob/a3af40839b24aa13f5764d4f84933dbfa8bc8134/contracts/protocol/ParticlePositionManager.sol#L60-L68

60:     function initialize(
61:         address dexAggregator,
62:         uint256 feeFactor,
63:         uint128 liquidationRewardFactor,
64:         uint256 loanTerm,
65:         uint256 treasuryRate
66:     ) external initializer {
67:         __UUPSUpgradeable_init();
68:         __Ownable_init();

Consider using the proper call to the __Ownable2Step_init() base initializer.