QA Report

[code423n4]

Unlimited allowance is very dangerous

Nested finance use unlimited allowance in all contract that sent some token

contracts/libraries/ExchangeHelpers.sol

        address _swapTarget,
        bytes memory _swapCallData
    ) internal returns (bool) {
        setMaxAllowance(_sellToken, _swapTarget);
    /// @param _token The token to use for the allowance setting
    /// @param _spender Spender to allow
    function setMaxAllowance(IERC20 _token, address _spender) internal {

contracts/mocks/DummyRouter.sol

        NestedFactory(factory).destroy(nftId, IERC20(address(weth)), attackOrders);
    }

    function setMaxAllowance(IERC20 _token, address _spender) external {
        ExchangeHelpers.setMaxAllowance(_token, _spender);
    }

    function setAllowance(

contracts/libraries/StakingLPVaultHelpers.sol

        uint256 lpTokenToDeposit = lpToken.balanceOf(address(this)) - lpTokenBalanceBefore;
        ExchangeHelpers.setMaxAllowance(lpToken, vault);
        uint256 lpTokenBalanceBefore = lpToken.balanceOf(address(this));
        ExchangeHelpers.setMaxAllowance(IERC20(token), address(pool));

        if (poolCoinAmount == 2) {

contracts/operators/Beefy/lp/BeefyZapBiswapLPVaultOperator.sol

            tokenAmountIn = amount1;
        }

        ExchangeHelpers.setMaxAllowance(IERC20(swapToken), router);

        address[] memory path = new address[](2);
        require(pair.factory() == biswapRouter.factory(), "BLVO: INVALID_VAULT");

        ExchangeHelpers.setMaxAllowance(IERC20(address(pair)), address(vault));

contracts/operators/Beefy/lp/BeefyZapUniswapLPVaultOperator.sol

            swapToken = token1;
            tokenAmountIn = amount1;
        }

        ExchangeHelpers.setMaxAllowance(IERC20(swapToken), router);
        require(pair.factory() == uniswapRouter.factory(), "BLVO: INVALID_VAULT");

        ExchangeHelpers.setMaxAllowance(IERC20(address(pair)), address(vault));

        address cachedToken0 = pair.token0();

contracts/operators/Paraswap/ParaswapOperator.sol

        ExchangeHelpers.setMaxAllowance(sellToken, tokenTransferProxy);
        (bool success, ) = augustusSwapper.call(swapCallData);

contracts/operators/Beefy/BeefyVaultOperator.sol

        uint256 tokenBalanceBefore = token.balanceOf(address(this));

        ExchangeHelpers.setMaxAllowance(token, vault);

contracts/operators/Yearn/YearnCurveVaultOperator.sol

        uint256 vaultBalanceBefore = IERC20(vault).balanceOf(address(this));
        uint256 ethBalanceBefore = weth.balanceOf(address(this));

        ExchangeHelpers.setMaxAllowance(IERC20(address(weth)), address(withdrawer));

contracts/NestedFactory.sol

    ) private {
        address originalOwner = nestedAsset.originalOwner(_nftId);
        ExchangeHelpers.setMaxAllowance(_token, address(feeSplitter));
            ExchangeHelpers.setMaxAllowance(IERC20(address(weth)), address(withdrawer));
            withdrawer.withdraw(_amount);

If a contract that has max allowance is malicious, it may steal all tokens in the allowing contract. For example, if feeSplitter is malicious, it may steal all tokens in NestedFactory

poolCoinAmount validation

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/libraries/StakingLPVaultHelpers.sol

poolCoinAmount must be 2, 3, 4
so, if it not fall in this range it should be reverted but now it doesn't

On every functions in this file add

if (poolCoinAmount < 2 || poolCoinAmount > 4) revert

Change code to

// SPDX-License-Identifier: GPL-3.0-or-later
pragma solidity 0.8.14;

import "./../Withdrawer.sol";
import "./../libraries/ExchangeHelpers.sol";
import "./../libraries/CurveHelpers/CurveHelpers.sol";
import "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import "./../interfaces/external/ICurvePool/ICurvePool.sol";
import "./../interfaces/external/ICurvePool/ICurvePoolETH.sol";
import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
import "./../interfaces/external/IStakingVault/IStakingVault.sol";
import "./../interfaces/external/ICurvePool/ICurvePoolNonETH.sol";

error InvalidPoolCoinAmount(uint256 poolCoinAmount);

/// @notice Library for LP Staking Vaults deposit/withdraw
library StakingLPVaultHelpers {
    using SafeERC20 for IERC20;

    /// @dev  Add liquidity in a Curve pool with ETH and deposit
    ///       the LP token in a staking vault
    /// @param vault The staking vault address to deposit into
    /// @param pool The Curve pool to add liquitiy in
    /// @param lpToken The Curve pool LP token
    /// @param poolCoinAmount The number of token in the Curve pool
    /// @param eth ETH address
    /// @param amount ETH amount to add in the Curve pool
    function _addLiquidityAndDepositETH(
        address vault,
        ICurvePoolETH pool,
        IERC20 lpToken,
        uint256 poolCoinAmount,
        address eth,
        uint256 amount
    ) internal {
        if (poolCoinAmount < 2 || poolCoinAmount > 4) revert InvalidPoolCoinAmount(poolCoinAmount);

        uint256 lpTokenBalanceBefore = lpToken.balanceOf(address(this));

        if (poolCoinAmount == 2) {
            pool.add_liquidity{ value: amount }(CurveHelpers.getAmounts2Coins(pool, eth, amount), 0);
        } else if (poolCoinAmount == 3) {
            pool.add_liquidity{ value: amount }(CurveHelpers.getAmounts3Coins(pool, eth, amount), 0);
        } else {
            pool.add_liquidity{ value: amount }(CurveHelpers.getAmounts4Coins(pool, eth, amount), 0);
        }

        uint256 lpTokenToDeposit = lpToken.balanceOf(address(this)) - lpTokenBalanceBefore;
        ExchangeHelpers.setMaxAllowance(lpToken, vault);
        IStakingVault(vault).deposit(lpTokenToDeposit);
    }

    /// @dev  Add liquidity in a Curve pool and deposit
    ///       the LP token in a staking vault
    /// @param vault The staking vault address to deposit into
    /// @param pool The Curve pool to add liquitiy in
    /// @param lpToken The Curve pool lpToken
    /// @param poolCoinAmount The number of token in the Curve pool
    /// @param token Token to add in the Curve pool liquidity
    /// @param amount Token amount to add in the Curve pool
    function _addLiquidityAndDeposit(
        address vault,
        ICurvePoolNonETH pool,
        IERC20 lpToken,
        uint256 poolCoinAmount,
        address token,
        uint256 amount
    ) internal {
        if (poolCoinAmount < 2 || poolCoinAmount > 4) revert InvalidPoolCoinAmount(poolCoinAmount);

        uint256 lpTokenBalanceBefore = lpToken.balanceOf(address(this));
        ExchangeHelpers.setMaxAllowance(IERC20(token), address(pool));

        if (poolCoinAmount == 2) {
            pool.add_liquidity(CurveHelpers.getAmounts2Coins(pool, token, amount), 0);
        } else if (poolCoinAmount == 3) {
            pool.add_liquidity(CurveHelpers.getAmounts3Coins(pool, token, amount), 0);
        } else {
            pool.add_liquidity(CurveHelpers.getAmounts4Coins(pool, token, amount), 0);
        }

        uint256 lpTokenToDeposit = lpToken.balanceOf(address(this)) - lpTokenBalanceBefore;
        ExchangeHelpers.setMaxAllowance(lpToken, vault);
        IStakingVault(vault).deposit(lpTokenToDeposit);
    }

    /// @dev Withdraw the LP token from the staking vault and
    ///      remove the liquidity from the Curve pool
    /// @param vault The staking vault address to withdraw from
    /// @param amount The amount to withdraw
    /// @param pool The Curve pool to remove liquitiy from
    /// @param lpToken The Curve pool LP token
    /// @param poolCoinAmount The number of token in the Curve pool
    /// @param outputToken Output token to receive
    function _withdrawAndRemoveLiquidity128(
        address vault,
        uint256 amount,
        ICurvePool pool,
        IERC20 lpToken,
        uint256 poolCoinAmount,
        address outputToken
    ) internal {
        if (poolCoinAmount < 2 || poolCoinAmount > 4) revert InvalidPoolCoinAmount(poolCoinAmount);

        uint256 lpTokenBalanceBefore = lpToken.balanceOf(address(this));
        IStakingVault(vault).withdraw(amount);

        bool success = CurveHelpers.removeLiquidityOneCoin(
            pool,
            lpToken.balanceOf(address(this)) - lpTokenBalanceBefore,
            outputToken,
            poolCoinAmount,
            bytes4(keccak256(bytes("remove_liquidity_one_coin(uint256,int128,uint256)")))
        );

        require(success, "SDCSO: CURVE_RM_LIQUIDITY_FAILED");
    }

    /// @dev Withdraw the LP token from the staking vault and
    ///      remove the liquidity from the Curve pool
    /// @param vault The staking vault address to withdraw from
    /// @param amount The amount to withdraw
    /// @param pool The Curve pool to remove liquitiy from
    /// @param lpToken The Curve pool LP token
    /// @param poolCoinAmount The number of token in the Curve pool
    /// @param outputToken Output token to receive
    function _withdrawAndRemoveLiquidity256(
        address vault,
        uint256 amount,
        ICurvePool pool,
        IERC20 lpToken,
        uint256 poolCoinAmount,
        address outputToken
    ) internal {
        if (poolCoinAmount < 2 || poolCoinAmount > 4) revert InvalidPoolCoinAmount(poolCoinAmount);

        uint256 lpTokenBalanceBefore = lpToken.balanceOf(address(this));
        IStakingVault(vault).withdraw(amount);

        bool success = CurveHelpers.removeLiquidityOneCoin(
            pool,
            lpToken.balanceOf(address(this)) - lpTokenBalanceBefore,
            outputToken,
            poolCoinAmount,
            bytes4(keccak256(bytes("remove_liquidity_one_coin(uint256,uint256,uint256)")))
        );

        require(success, "SDCSO: CURVE_RM_LIQUIDITY_FAILED");
    }
}

@openzeppelin/contracts should be updated to ^4.4.2 as ^4.3.2 has many vulnerables

https://github.com/code-423n4/2022-06-nested/blob/main/package.json is using

"@openzeppelin/contracts": "^4.3.2",

@openzeppelin/contracts 4.3.2 has these vulnerabilities from https://snyk.io/vuln/npm:%40openzeppelin%2Fcontracts

• Function Call With Incorrect Argument
• Deserialization of Untrusted Data
• Numeric Errors
• Improper Initialization
• Improper Input Validation

You should update @openzeppelin/contracts to ^4.4.2 to avoid these vulnerabilities.

[Yashiru]

poolCoinAmount validation (Confirmed)

Context

• StakingLPVaultHelpers.sol should be used only with vault that use Curve LP token (so we could rename it)
• Each StakingLPVaultHelpers.sol PoolCoinAmount comes from the storage of the calling operator, so only a Nested administrator can provide a poolCoinAmount
• Even if the administrator writes a value greater than 4 in the operator storage, the helpers will react the same way for all poolCoinAmounts that are greater than or equal to 4 => the process will fall in else and will not cause any problems.

Way to make the process fail

• The administrator writes poolCoinAmount = 1 in the operator storage
• The msg.sender try to withdraw with a token that is not in the targeted Curve pool

Conclusion

Quality assurance is confirmed for the verification that poolCoinAmount is greater than 1

[obatirou]

Unlimited allowance is very dangerous (disputed)

Nested Factory does not hold funds

[obatirou]

@openzeppelin/contracts should be updated to ^4.4.2 as ^4.3.2 has many vulnerables (confirmed)