code-423n4 / 2023-12-particle-findings

2 stars 1 forks source link

collectLiquidity() Lack of can specify recipient leads to inability to retrieve token1 after entering the blacklist of token0 #36

Open c4-bot-8 opened 10 months ago

c4-bot-8 commented 10 months ago

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

c4-judge commented 10 months ago

0xleastwood marked the issue as primary issue

wukong-particle commented 10 months ago

Good suggestion, receipiant should be added here too: https://github.com/code-423n4/2023-12-particle/blob/main/contracts/libraries/LiquidityPosition.sol#L329

0xleastwood commented 10 months ago

I would argue this is good design and should not be changed to allow for arbitrary recipients. If a token is blacklisted, and a protocol allows the user to circumvent this blacklist, then they may potentially be liable for the behaviour of this individual. Better to take an agnostic approach and leave it as is unless liquidations are ultimately being limited because of this.

c4-judge commented 10 months ago

0xleastwood marked the issue as selected for report

c4-sponsor commented 10 months ago

wukong-particle (sponsor) acknowledged

wukong-particle commented 10 months ago

I agree with the judge. We shouldn't facilitate to temper the blacklist. So only acknowledging the issue.