Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

increaseLiquidity/decreaseLiquidity Lack of slippage protection #29

Closed
c4-bot-1 opened this issue Dec 21, 2023 · 2 comments
Closed

increaseLiquidity/decreaseLiquidity Lack of slippage protection #29

c4-bot-1 opened this issue Dec 21, 2023 · 2 comments
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working duplicate-2 satisfactory satisfies C4 submission criteria; eligible for awards

Comments

@c4-bot-1
Copy link
Contributor

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

@c4-bot-1 c4-bot-1 added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working labels Dec 21, 2023
c4-bot-1 added a commit that referenced this issue Dec 21, 2023
@c4-judge
Copy link
Contributor

0xleastwood marked the issue as duplicate of #2

@c4-judge c4-judge added duplicate-2 satisfactory satisfies C4 submission criteria; eligible for awards labels Dec 21, 2023
@c4-judge
Copy link
Contributor

0xleastwood marked the issue as satisfactory

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working duplicate-2 satisfactory satisfies C4 submission criteria; eligible for awards
Projects
None yet
Development

No branches or pull requests

2 participants