Nested Finance contest - Chom's results

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.

Caching the length in for loops

This reduce gas cost as show here https://forum.openzeppelin.com/t/a-collection-of-gas-optimisation-tricks/19966/5

1. if it is a storage array, this is an extra sload operation (100 additional extra gas (EIP-2929 2) for each iteration except for the first),
2. if it is a memory array, this is an extra mload operation (3 additional gas for each iteration except for the first),
3. if it is a calldata array, this is an extra calldataload operation (3 additional gas for each iteration except for the first)

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/NestedFactory.sol#L121-L130

    function addOperator(bytes32 operator) external override onlyOwner {
        require(operator != bytes32(""), "NF: INVALID_OPERATOR_NAME");
        bytes32[] memory operatorsCache = operators;
        for (uint256 i = 0; i < operatorsCache.length; i++) {
            require(operatorsCache[i] != operator, "NF: EXISTENT_OPERATOR");
        }
        operators.push(operator);
        rebuildCache();
        emit OperatorAdded(operator);
    }

Can be optimized to

    function addOperator(bytes32 operator) external override onlyOwner {
        require(operator != bytes32(""), "NF: INVALID_OPERATOR_NAME");
        bytes32[] memory operatorsCache = operators;
        uint256 operatorsCacheLength = operatorsCache.length;
        for (uint256 i = 0; i < operatorsCacheLength; i++) {
            require(operatorsCache[i] != operator, "NF: EXISTENT_OPERATOR");
        }
        operators.push(operator);
        rebuildCache();
        emit OperatorAdded(operator);
    }

The increment in for loop postcondition can be made unchecked

This reduce gas cost as show here https://forum.openzeppelin.com/t/a-collection-of-gas-optimisation-tricks/19966/5

Gas savings: roughly speaking this can save 30-40 gas per loop iteration. For lengthy loops, this can be significant!

Apply this to all part in your code with for loops. For example

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/NestedFactory.sol#L196-L199

        for (uint256 i = 0; i < batchedOrdersLength; i++) {
            (uint256 fees, IERC20 tokenSold) = _submitInOrders(nftId, _batchedOrders[i], false);
            _transferFeeWithRoyalty(fees, tokenSold, nftId);
        }

Can be optimized to

        for (uint256 i = 0; i < batchedOrdersLength; ) {
            (uint256 fees, IERC20 tokenSold) = _submitInOrders(nftId, _batchedOrders[i], false);
            _transferFeeWithRoyalty(fees, tokenSold, nftId);

            unchecked { i++; }
        }

Consider using custom errors instead of revert strings

This reduce gas cost as show here https://forum.openzeppelin.com/t/a-collection-of-gas-optimisation-tricks/19966/5

Solidity 0.8.4 introduced custom errors. They are more gas efficient than revert strings, when it comes to deployment cost as well as runtime cost when the revert condition is met. Use custom errors instead of revert strings for gas savings.

Any require statement in your code can be replaced with custom error for example:

require(batchedOrdersLength != 0, "NF: INVALID_MULTI_ORDERS");

Can be replaced with

// declare error before contract declaration
error InvalidMultiOrders();

if (batchedOrdersLength == 0) revert InvalidMultiOrders();