Platform: Code4rena
Start Date: 11/12/2023
Pot Size: $24,150 USDC
Total HM: 19
Participants: 5
Period: 10 days
Judge: 0xLeastwood
Total Solo HM: 6
Id: 312
League: ETH
Rank: 1/5
Findings: 5
Award: $0.00
🌟 Selected for report: 2
🚀 Solo Findings: 2
Data not available
If a trader is blacklisted from a blacklistable ERC20 token while has an open position, it may not be possible to liquidate the position.
When liquidate position, it will eventually calculate the amount of token that need to be send to borrower, the amount will be non-zero if the position is profitable or the borrower has excess premium.
function _closePosition( DataStruct.ClosePositionParams calldata params, DataCache.ClosePositionCache memory cache, Lien.Info memory lien, address borrower ) internal { ... // calculate the the amounts owed to LP up to the premium in the lien // must ensure enough amount is left to pay for interest first, then send gains and fund left to borrower ///@dev refundWithCheck ensures actual cannot be more than expected, since amount owed to LP is in actual, /// it ensures (1) on the collateralFrom part of refund, tokenOwed is covered, and (2) on the amountReceived /// part, received is no less than liquidity addback + token owed. 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 ); } // pay for interest lps.addTokensOwed(lien.tokenId, cache.token0Owed, cache.token1Owed); }
The issue with this design is that if a borrower is blacklisted from a blacklistable token, such as USDC, it will not be possible to send profits or excess premium back to the borrower.
This will not only impact the borrower but also the liquidity provider, as the token cannot be added back to the Uniswap V3 LP position.
Manual review
use the "Pull over Push" design for sending profit/excess premium to the borrower.
Token-Transfer
#0 - c4-judge
2023-12-21T22:14:18Z
0xleastwood marked the issue as duplicate of #31
#1 - c4-judge
2023-12-24T12:32:51Z
0xleastwood marked the issue as satisfactory
#2 - c4-judge
2023-12-26T11:33:25Z
0xleastwood changed the severity to 3 (High Risk)
🌟 Selected for report: said
Data not available
https://github.com/code-423n4/2023-12-particle/blob/main/contracts/libraries/Base.sol#L318-L342
When operations need to calculate Uniswap V3 position's fee growth, it used similar function implemented by uniswap v3. However, according to this known issue : https://github.com/Uniswap/v3-core/issues/573. The contract is implicitly relies on underflow/overflow when calculating the fee growth, if underflow is prevented, some operations that rely on fee growth will revert.
It can be observed that current implementation of getFeeGrowthInside
not allow underflow/overflow to happen when calculating feeGrowthInside0X128
and feeGrowthInside1X128
, because the contract used solidity 0.8.23.
https://github.com/code-423n4/2023-12-particle/blob/main/contracts/libraries/Base.sol#L318-L342
function getFeeGrowthInside( address token0, address token1, uint24 fee, int24 tickLower, int24 tickUpper ) internal view returns (uint256 feeGrowthInside0X128, uint256 feeGrowthInside1X128) { IUniswapV3Pool pool = IUniswapV3Pool(UNI_FACTORY.getPool(token0, token1, fee)); (, int24 tickCurrent, , , , , ) = pool.slot0(); (, , uint256 lowerFeeGrowthOutside0X128, uint256 lowerFeeGrowthOutside1X128, , , , ) = pool.ticks(tickLower); (, , uint256 upperFeeGrowthOutside0X128, uint256 upperFeeGrowthOutside1X128, , , , ) = pool.ticks(tickUpper); if (tickCurrent < tickLower) { feeGrowthInside0X128 = lowerFeeGrowthOutside0X128 - upperFeeGrowthOutside0X128; feeGrowthInside1X128 = lowerFeeGrowthOutside1X128 - upperFeeGrowthOutside1X128; } else if (tickCurrent < tickUpper) { uint256 feeGrowthGlobal0X128 = pool.feeGrowthGlobal0X128(); uint256 feeGrowthGlobal1X128 = pool.feeGrowthGlobal1X128(); feeGrowthInside0X128 = feeGrowthGlobal0X128 - lowerFeeGrowthOutside0X128 - upperFeeGrowthOutside0X128; feeGrowthInside1X128 = feeGrowthGlobal1X128 - lowerFeeGrowthOutside1X128 - upperFeeGrowthOutside1X128; } else { feeGrowthInside0X128 = upperFeeGrowthOutside0X128 - lowerFeeGrowthOutside0X128; feeGrowthInside1X128 = upperFeeGrowthOutside1X128 - lowerFeeGrowthOutside1X128; } }
This could impact crucial operation that rely on this call, such as liquidation, could revert unexpectedly. This behavior is quite often especially for pools that use lower fee.
Coded PoC :
Add the following test to /test/OpenPosition.t.sol
:
function testLiquidationRevert() public { address LIQUIDATOR = payable(address(0x7777)); uint128 REPAY_LIQUIDITY_PORTION = 1000; _setupLowerOutOfRange(); testBaseOpenLongPosition(); // get lien info (, uint128 liquidityInside, , , , , , ) = particlePositionManager.liens( keccak256(abi.encodePacked(SWAPPER, uint96(0))) ); // start reclaim vm.startPrank(LP); vm.warp(block.timestamp + 1); particlePositionManager.reclaimLiquidity(_tokenId); vm.stopPrank(); // add back liquidity requirement vm.warp(block.timestamp + 7 days); IUniswapV3Pool _pool = IUniswapV3Pool(uniswapV3Factory.getPool(address(USDC), address(WETH), FEE)); (uint160 currSqrtRatioX96, , , , , , ) = _pool.slot0(); (uint256 amount0ToReturn, uint256 amount1ToReturn) = LiquidityAmounts.getAmountsForLiquidity( currSqrtRatioX96, _sqrtRatioAX96, _sqrtRatioBX96, liquidityInside ); (uint256 usdCollateral, uint256 ethCollateral) = particleInfoReader.getRequiredCollateral(liquidityInside, _tickLower, _tickUpper); // get swap data uint160 currentPrice = particleInfoReader.getCurrentPrice(address(USDC), address(WETH), FEE); uint256 amountSwap = ethCollateral - amount1ToReturn; ISwapRouter.ExactInputSingleParams memory params = ISwapRouter.ExactInputSingleParams({ tokenIn: address(WETH), tokenOut: address(USDC), fee: FEE, recipient: address(particlePositionManager), deadline: block.timestamp, amountIn: amountSwap, amountOutMinimum: 0, sqrtPriceLimitX96: currentPrice + currentPrice / SLIPPAGE_FACTOR }); bytes memory data = abi.encodeWithSelector(ISwapRouter.exactInputSingle.selector, params); // liquidate position vm.startPrank(LIQUIDATOR); vm.expectRevert(abi.encodeWithSelector(Errors.InsufficientRepay.selector)); particlePositionManager.liquidatePosition( DataStruct.ClosePositionParams({lienId: uint96(0), amountSwap: amountSwap, data: data}), SWAPPER ); vm.stopPrank(); }
Also modify FEE
inside /test/Base.t.sol
to 500
:
contract ParticlePositionManagerTestBase is Test { using Lien for mapping(bytes32 => Lien.Info); IERC20 public constant WETH = IERC20(0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2); IERC20 public constant USDC = IERC20(0xA0b86991c6218b36c1d19D4a2e9Eb0cE3606eB48); IERC20 public constant DAI = IERC20(0x6B175474E89094C44Da98b954EedeAC495271d0F); uint256 public constant USDC_AMOUNT = 50000000 * 1e6; uint256 public constant DAI_AMOUNT = 50000000 * 1e18; uint256 public constant WETH_AMOUNT = 50000 * 1e18; address payable public constant ADMIN = payable(address(0x4269)); address payable public constant LP = payable(address(0x1001)); address payable public constant SWAPPER = payable(address(0x1002)); address payable public constant WHALE = payable(address(0x6666)); IQuoter public constant QUOTER = IQuoter(0xb27308f9F90D607463bb33eA1BeBb41C27CE5AB6); int24 public constant TICK_SPACING = 60; uint256 public constant BASIS_POINT = 1_000_000; - uint24 public constant FEE = 3000; // uniswap swap fee + uint24 public constant FEE = 500; // uniswap swap fee .. }
Run the test :
forge test --fork-url $MAINNET_RPC_URL --fork-block-number 18750931 --match-contract OpenPositionTest --match-test testRevertUnderflow -vvvv
Log output :
It can be observed that the liquidation revert due to the underflow.
Manual review.
Use unchecked when calculating feeGrowthInside0X128
and feeGrowthInside1X128
.
Error
#0 - c4-judge
2023-12-21T21:45:35Z
0xleastwood marked the issue as primary issue
#1 - romeroadrian
2023-12-22T16:26:58Z
Really interesting side effect of upgrading code from solidity < 0.8 👍
#2 - wukong-particle
2023-12-23T02:49:44Z
Oh this one is great. Will update the code to uncheck feeGrowthInside0X128
and feeGrowthInside1X128
calculations.
#3 - c4-judge
2023-12-23T19:41:37Z
0xleastwood marked the issue as selected for report
#4 - c4-sponsor
2023-12-24T08:58:55Z
wukong-particle (sponsor) confirmed
Data not available
The liquidation eligibility check is performed after the premiums provided by the trader are decreased by liquidation rewards. This poses a risk for traders and could lead to a condition where a trader's position unexpectedly becomes eligible for liquidation.
It can be observed that currently, the liquidation eligibility check is performed after decreasing closeCache.tokenFromPremium
and closeCache.tokenToPremium
by liquidateCache.liquidationRewardFrom
and liquidateCache.liquidationRewardTo
.
/// @inheritdoc IParticlePositionManager function liquidatePosition( DataStruct.ClosePositionParams calldata params, address borrower ) external override nonReentrant { bytes32 lienKey = keccak256(abi.encodePacked(borrower, params.lienId)); Lien.Info memory lien = liens.getInfo(lienKey); // check lien is valid if (lien.liquidity == 0) revert Errors.RecordEmpty(); // local cache to avoid stack too deep DataCache.ClosePositionCache memory closeCache; DataCache.LiquidatePositionCache memory liquidateCache; // get liquidation parameters ///@dev calculate premium outside of _closePosition to allow liquidatePosition to take reward from premium ( closeCache.tokenFrom, closeCache.tokenTo, liquidateCache.tokenFromOwed, liquidateCache.tokenToOwed, closeCache.tokenFromPremium, closeCache.tokenToPremium, closeCache.collateralFrom, ) = Base.getOwedInfoConverted( DataStruct.OwedInfoParams({ tokenId: lien.tokenId, liquidity: lien.liquidity, feeGrowthInside0LastX128: lien.feeGrowthInside0LastX128, feeGrowthInside1LastX128: lien.feeGrowthInside1LastX128, token0PremiumPortion: lien.token0PremiumPortion, token1PremiumPortion: lien.token1PremiumPortion }), lien.zeroForOne ); // 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(); } ...
With this design, when the protocol decides to increase the LIQUIDATION_REWARD_FACTOR
, it could directly impact traders' positions and potentially cause them to become liquidatable.
function updateLiquidationRewardFactor(uint128 liquidationRewardFactor) external override onlyOwner { if (liquidationRewardFactor > _LIQUIDATION_REWARD_FACTOR_MAX) revert Errors.InvalidValue(); LIQUIDATION_REWARD_FACTOR = liquidationRewardFactor; emit UpdateLiquidationRewardFactor(liquidationRewardFactor); }
Manual review
Perform the check before decreasing closeCache.tokenFromPremium
and closeCache.tokenToPremium
so that traders' position condition is easier to predict and maintain.
Context
#0 - c4-judge
2023-12-21T22:25:30Z
0xleastwood marked the issue as primary issue
#1 - romeroadrian
2023-12-22T16:22:30Z
Duplicate #51
#2 - c4-judge
2023-12-22T19:22:09Z
0xleastwood marked the issue as duplicate of #51
#3 - c4-judge
2023-12-24T12:33:16Z
0xleastwood marked the issue as satisfactory
🌟 Selected for report: said
Data not available
Excess tokens from margins provided by traders when opening position could become stuck inside the ParticlePositionManager
due to precision loss when calculating the token premium portion.
When traders call openPosition
, eventually it will calculate cache.token0PremiumPortion
and cache.token1PremiumPortion
before put it into lien data.
/// @inheritdoc IParticlePositionManager function openPosition( DataStruct.OpenPositionParams calldata params ) public override nonReentrant returns (uint96 lienId, uint256 collateralTo) { ... if (params.zeroForOne) { >>> cache.token0PremiumPortion = Base.uint256ToUint24( ((params.marginFrom + cache.amountFromBorrowed - cache.feeAmount - cache.amountSpent) * Base.BASIS_POINT) / cache.collateralFrom ); >>> cache.token1PremiumPortion = Base.uint256ToUint24( ((cache.amountReceived + cache.amountToBorrowed + params.marginTo - collateralTo) * Base.BASIS_POINT) / collateralTo ); if ( cache.token0PremiumPortion < params.tokenFromPremiumPortionMin || cache.token1PremiumPortion < params.tokenToPremiumPortionMin ) revert Errors.InsufficientPremium(); } else { >>> cache.token1PremiumPortion = Base.uint256ToUint24( ((params.marginFrom + cache.amountFromBorrowed - cache.feeAmount - cache.amountSpent) * Base.BASIS_POINT) / cache.collateralFrom ); >>> cache.token0PremiumPortion = Base.uint256ToUint24( ((cache.amountReceived + cache.amountToBorrowed + params.marginTo - collateralTo) * Base.BASIS_POINT) / collateralTo ); if ( cache.token0PremiumPortion < params.tokenToPremiumPortionMin || cache.token1PremiumPortion < params.tokenFromPremiumPortionMin ) revert Errors.InsufficientPremium(); } // create a new lien 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 }); emit OpenPosition(msg.sender, lienId, collateralTo); }
It can be observed that when calculating cache.token0PremiumPortion
and cache.token1PremiumPortion
, precision loss could occur, especially when the token used has a high decimals (e.g. 18) while BASIS_POINT
used only 1_000_000
. When this happens, the token could become stuck inside the ParticlePositionManager
.
Coded PoC :
The PoC goal is to check the amount of tokens inside the ParticlePositionManager
after the position is opened, premium is added, liquidated, the LP withdraws the tokens and collects fees, and the admin collects the treasury.
Add the following test inside test/OpenPosition.t.sol
and also add import "forge-std/console.sol";
to the test contract.
function testLiquidateAndClearLiquidity() public { address LIQUIDATOR = payable(address(0x7777)); uint128 REPAY_LIQUIDITY_PORTION = 1000; testBaseOpenLongPosition(); // add enough premium uint128 premium0 = 500 * 1e6; uint128 premium1 = 0.5 ether; vm.startPrank(WHALE); USDC.transfer(SWAPPER, premium0); WETH.transfer(SWAPPER, premium1); vm.stopPrank(); vm.startPrank(SWAPPER); TransferHelper.safeApprove(address(USDC), address(particlePositionManager), premium0); TransferHelper.safeApprove(address(WETH), address(particlePositionManager), premium1); particlePositionManager.addPremium(0, premium0, premium1); vm.stopPrank(); // get lien info (, uint128 liquidityInside, , , , , , ) = particlePositionManager.liens( keccak256(abi.encodePacked(SWAPPER, uint96(0))) ); // start reclaim vm.startPrank(LP); vm.warp(block.timestamp + 1); particlePositionManager.reclaimLiquidity(_tokenId); vm.stopPrank(); // add back liquidity requirement vm.warp(block.timestamp + 7 days); IUniswapV3Pool _pool = IUniswapV3Pool(uniswapV3Factory.getPool(address(USDC), address(WETH), FEE)); (uint160 currSqrtRatioX96, , , , , , ) = _pool.slot0(); (uint256 amount0ToReturn, uint256 amount1ToReturn) = LiquidityAmounts.getAmountsForLiquidity( currSqrtRatioX96, _sqrtRatioAX96, _sqrtRatioBX96, liquidityInside + liquidityInside / REPAY_LIQUIDITY_PORTION ); (, uint256 ethCollateral) = particleInfoReader.getRequiredCollateral(liquidityInside, _tickLower, _tickUpper); // get swap data uint160 currentPrice = particleInfoReader.getCurrentPrice(address(USDC), address(WETH), FEE); uint256 amountSwap = ethCollateral - amount1ToReturn; ISwapRouter.ExactInputSingleParams memory params = ISwapRouter.ExactInputSingleParams({ tokenIn: address(WETH), tokenOut: address(USDC), fee: FEE, recipient: address(particlePositionManager), deadline: block.timestamp, amountIn: amountSwap, amountOutMinimum: 0, sqrtPriceLimitX96: currentPrice + currentPrice / SLIPPAGE_FACTOR }); bytes memory data = abi.encodeWithSelector(ISwapRouter.exactInputSingle.selector, params); // liquidate position vm.startPrank(LIQUIDATOR); particlePositionManager.liquidatePosition( DataStruct.ClosePositionParams({lienId: uint96(0), amountSwap: amountSwap, data: data}), SWAPPER ); vm.stopPrank(); vm.startPrank(LP); // console.log("liquidity before : "); // console.log(_liquidity); (, , , , , , , _liquidity, , , , ) = nonfungiblePositionManager.positions(_tokenId); (uint256 amount0Decreased, uint256 amount1Decreased) = particlePositionManager.decreaseLiquidity( _tokenId, _liquidity ); (uint256 amount0Returned, uint256 amount1Returned) = particlePositionManager.collectLiquidity(_tokenId); vm.stopPrank(); vm.startPrank(ADMIN); particlePositionManager.withdrawTreasury(address(USDC), ADMIN); particlePositionManager.withdrawTreasury(address(WETH), ADMIN); vm.stopPrank(); uint256 usdcBalanceAfter = USDC.balanceOf(LP); uint256 wethBalanceAfter = WETH.balanceOf(LP); console.log("balance LP after - USDC:"); console.log(usdcBalanceAfter); console.log("balance LP after - WETH:"); console.log(wethBalanceAfter); console.log("balance inside manager - USDC:"); console.log(USDC.balanceOf(address(particlePositionManager))); console.log("balance inside manager - WETH:"); console.log(WETH.balanceOf(address(particlePositionManager))); }
Run the test :
forge test -vv --fork-url $MAINNET_RPC_URL --fork-block-number 18750931 --match-contract OpenPositionTest --match-test testLiquidateAndClearLiquidity -vvv
Output log :
balance LP after - USDC: 15000228083 balance LP after - WETH: 9999997057795940602 balance inside manager - USDC: 1123 balance inside manager - WETH: 516472404479
It can be observed that some tokens stuck inside the ParticlePositionManager
contract.
Manual review + foundry
Use a higher scaling for BASIS_POINT
, considering the fact that tokens commonly have a high number of decimals
Math
#0 - c4-judge
2023-12-21T22:15:30Z
0xleastwood marked the issue as primary issue
#1 - wukong-particle
2023-12-23T00:12:48Z
Good point. Appreciated the detailed PoC. On our end, I think we should have documented the use of tokenPremiumPortion
more clearly. It's a compromise we took to ensure the lien
structure stored on chain is less than 3 uint256. We compressed a lot and were only left with uint24
.
We think a dust of ~1000 USDC when a user is spending 15B USDC is acceptable. We could add a function to collect dust if needed (might be complicated though).
If there can be serious attack around this dust, please do raise it. Thanks!
#2 - c4-judge
2023-12-23T23:05:08Z
0xleastwood marked the issue as selected for report
#3 - c4-sponsor
2023-12-24T08:58:37Z
wukong-particle (sponsor) acknowledged
#4 - romeroadrian
2023-12-30T14:34:18Z
These are leftover amounts that are caused by the rounding of the premium as a portion (premium -> portion -> premium). It looks more on the QA side to me.
#5 - 0xleastwood
2023-12-31T13:21:50Z
These are leftover amounts that are caused by the rounding of the premium as a portion (premium -> portion -> premium). It looks more on the QA side to me.
This seems considerable and will accrue significantly over the lifetime of the protocol. I think the severity is justified as per the judging criteria.
2 — Med: Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.
Data not available
https://github.com/code-423n4/2023-12-particle/blob/main/contracts/libraries/LiquidityPosition.sol#L253-L263 https://github.com/code-423n4/2023-12-particle/blob/main/contracts/libraries/LiquidityPosition.sol#L178-L204
According to uniswap V3 docs for increase and decrease liquidity:
In production, amount0Min and amount1Min should be adjusted to create slippage protections.
However, current implementation inside Particle doesn't allow users to provide the amount minimum.
This are the current implementation for increasing and decreasing liquidity, it can be observed that amount0Min
and amount1Min
provided are 0 :
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 }) ); // reset approval TransferHelper.safeApprove(token0, Base.UNI_POSITION_MANAGER_ADDR, 0); TransferHelper.safeApprove(token1, Base.UNI_POSITION_MANAGER_ADDR, 0); }
function decreaseLiquidity(uint256 tokenId, uint128 liquidity) internal returns (uint256 amount0, uint256 amount1) { (amount0, amount1) = Base.UNI_POSITION_MANAGER.decreaseLiquidity( INonfungiblePositionManager.DecreaseLiquidityParams({ tokenId: tokenId, liquidity: liquidity, amount0Min: 0, amount1Min: 0, deadline: block.timestamp }) ); }
There are several instances where increasing and decreasing liquidity are called :
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 decreaseLiquidity( uint256 tokenId, uint128 liquidity ) external override nonReentrant returns (uint256 amount0Decreased, uint256 amount1Decreased) { (amount0Decreased, amount1Decreased) = lps.decreaseLiquidity(tokenId, liquidity); }
function _closePosition( DataStruct.ClosePositionParams calldata params, DataCache.ClosePositionCache memory cache, Lien.Info memory lien, address borrower ) internal { ... // 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 ); } ... }
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 ); ... }
This causes the operations to have minimal slippage protection when involving decreasing or increasing liquidity
Manual review
When it is possible, allow user to provide amount0Min
and amount1Min
Uniswap
#0 - c4-judge
2023-12-21T21:38:58Z
0xleastwood marked the issue as duplicate of #2
#1 - c4-judge
2023-12-23T23:01:21Z
0xleastwood marked the issue as satisfactory
Data not available
https://github.com/code-423n4/2023-12-particle/blob/main/contracts/libraries/Base.sol#L183 https://github.com/code-423n4/2023-12-particle/blob/main/contracts/libraries/Base.sol#L326
slot0 is the most recent data point that can be manipulated trough swap and flash loan. Operations that rely in this data are susceptible to price manipulation attack.
It can be observed that slot0 price is used when calculating getRequiredRepay
, this will calculate amount need to be repaid when closing or liquidating traders position.
https://github.com/code-423n4/2023-12-particle/blob/main/contracts/libraries/Base.sol#L163-L192
function getRequiredRepay( uint128 liquidity, uint256 tokenId ) internal view returns (uint256 amount0, uint256 amount1) { DataCache.RepayCache memory repayCache; ( , , repayCache.token0, repayCache.token1, repayCache.fee, repayCache.tickLower, repayCache.tickUpper, , , , , ) = UNI_POSITION_MANAGER.positions(tokenId); IUniswapV3Pool pool = IUniswapV3Pool(UNI_FACTORY.getPool(repayCache.token0, repayCache.token1, repayCache.fee)); >>> (repayCache.sqrtRatioX96, , , , , , ) = pool.slot0(); repayCache.sqrtRatioAX96 = TickMath.getSqrtRatioAtTick(repayCache.tickLower); repayCache.sqrtRatioBX96 = TickMath.getSqrtRatioAtTick(repayCache.tickUpper); (amount0, amount1) = LiquidityAmounts.getAmountsForLiquidity( repayCache.sqrtRatioX96, repayCache.sqrtRatioAX96, repayCache.sqrtRatioBX96, liquidity ); }
Traders can leverage flash loan to sandwich the operation so it become profitable for them when calculating the amount that need to be provided back to the LPs liquidity position.
function _closePosition( DataStruct.ClosePositionParams calldata params, DataCache.ClosePositionCache memory cache, Lien.Info memory lien, address borrower ) internal { // optimistically use the input numbers to swap for repay /// @dev amountSwap overspend will be caught by refundWithCheck step in below (cache.amountSpent, cache.amountReceived) = Base.swap( cache.tokenFrom, cache.tokenTo, params.amountSwap, 0, /// @dev we check cache.amountReceived is sufficient to repay LP in below DEX_AGGREGATOR, params.data ); // based on borrowed liquidity, compute the required return amount /// @dev the from-to swapping direction is reverted compared to openPosition >>> (cache.amountToAdd, cache.amountFromAdd) = Base.getRequiredRepay(lien.liquidity, lien.tokenId); if (!lien.zeroForOne) (cache.amountToAdd, cache.amountFromAdd) = (cache.amountFromAdd, cache.amountToAdd); // the liquidity to add must be no less than the available amount /// @dev the max available amount contains the tokensOwed, will have another check in below at refundWithCheck if ( cache.amountFromAdd > cache.collateralFrom + cache.tokenFromPremium - cache.amountSpent || cache.amountToAdd > cache.amountReceived + cache.tokenToPremium ) { revert Errors.InsufficientRepay(); } // 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 ); } ... }
Manual review.
Use TWAP for the price inside the operation.
Uniswap
#0 - c4-judge
2023-12-21T22:22:11Z
0xleastwood marked the issue as duplicate of #23
#1 - romeroadrian
2023-12-22T17:21:01Z
The LP owner is interesting in getting the liquidity back, the underlying amounts of each token are meaningless to this operation. The attacker only looses flash loan and trading fees.
#2 - c4-judge
2023-12-23T19:34:58Z
0xleastwood changed the severity to QA (Quality Assurance)
#3 - c4-judge
2023-12-26T11:31:37Z
0xleastwood marked the issue as grade-a