When Vault Executors set one _init to 0, deposit() will be broken

Summary

When a vault executor calls ArrakisStandardManager::setModule() to set a new module and also initialize a new module, the passed payloads_ can contain 1 _init that is 0 which is intended to be a valid setup, however this breaks integration with SovereignPools. This can cause a Vault Executor to set a new module that can be initialised incorrectly, leading to broken deposit functionality.

Vulnerability Detail

ArrakisStandardManager::setModule()

    function setModule(
        address vault_,
        address module_,
        bytes[] calldata payloads_
    ) external whenNotPaused onlyWhitelistedVault(vault_) {
        if (vaultInfo[vault_].executor != msg.sender) {
            revert NotExecutor();
        }

        IArrakisMetaVault(vault_).setModule(module_, payloads_);

        emit LogSetModule(vault_, module_, payloads_);
    }

Vault Executor passes payloads_ to be utilised when Initializing a new module. ArrakisMetaVault::setModule()

    function setModule(
        address module_,
        bytes[] calldata payloads_
    ) external onlyManager nonReentrant {
        // store in memory to save gas.
        IArrakisLPModule _module = module;

        if (address(_module) == module_) revert SameModule();
        if (!_whitelistedModules.contains(module_)) {
            revert NotWhitelistedModule(module_);
        }

        module = IArrakisLPModule(module_);

        // #region withdraw manager fees balances.

        _withdrawManagerBalance(_module); // @audit-info module -> transfer fees to manager
...SKIP!....
        _module.withdraw(module_, BASE); // @audit-info transfer 100% of assets to new module
...SKIP!....
        uint256 len = payloads_.length;
        for (uint256 i = 0; i < len; i++) {
>>          (bool success,) = module_.call(payloads_[i]);
            if (!success) revert CallFailed();
        }
        emit LogSetModule(module_, payloads_);
    }

After setting the new module for the vault, the new vault is initialised by directly calling it using a low-level call. ValantisHOTModule::initialize()

    function initialize(
        address pool_,
        uint256 init0_,
        uint256 init1_,
        uint24 maxSlippage_,
        address metaVault_
    ) external initializer {
        if (metaVault_ == address(0)) revert AddressZero();
        if (pool_ == address(0)) revert AddressZero();
>>      if (init0_ == 0 && init1_ == 0) revert InitsAreZeros();
        if (maxSlippage_ > PIPS / 10) { // @audit-info 10% price slippage max
            revert MaxSlippageGtTenPercent();
        }

        metaVault = IArrakisMetaVault(metaVault_);
        pool = ISovereignPool(pool_);

        token0 = IERC20Metadata(metaVault.token0());
        token1 = IERC20Metadata(metaVault.token1());

>>      _init0 = init0_;
>>      _init1 = init1_;

        maxSlippage = maxSlippage_;
    }

When Initlilizing the Module, it is ensures that BOTH init values are not equal to 0, if both are 0 then the call revert. However one Init can be set to 0.

ValantisHOTModulePublic:deposit()

    function deposit(
        address depositor_,
        uint256 proportion_
    )
        external
        payable
        onlyMetaVault
        whenNotPaused
        nonReentrant
        returns (uint256 amount0, uint256 amount1)
    {
...SKIP!...
>>          if (!notFirstDeposit) {
                if (_amt0 > 0 || _amt1 > 0) {
                    address manager = metaVault.manager();

                    alm.withdrawLiquidity(_amt0, _amt1, manager, 0, 0); 
                }

>>              _amt0 = _init0; // @audit-info these are amts to be used on first deposit
>>              _amt1 = _init1;
                notFirstDeposit = true;
            }

>>          amount0 =
>>              FullMath.mulDivRoundingUp(proportion_, _amt0, BASE);
            amount1 =
                FullMath.mulDivRoundingUp(proportion_, _amt1, BASE);
        }
...SKIP!...
        alm.depositLiquidity(amount0, amount1, 0, 0); 

        // #endregion interactions.

        emit LogDeposit(depositor_, proportion_, amount0, amount1);
    }

When the first deposit() is called on a Module, the 2 Init values are used from the Initialization. If either is set to 0, then one amount passed to alm.depositLiquidity() will be 0.

HOT::depositLiquidity()

       (amount0Deposited, amount1Deposited) = ISovereignPool(_pool).depositLiquidity(
            _amount0,
            _amount1,
            _liquidityProvider,
            '',
            ''
        );

0 amounts easily pass through the depositLiquidity() function and are passed to the SovereignPool into depositLiquidity() SovereignPool::depositLiquidity()

    function depositLiquidity(
        uint256 _amount0,
        uint256 _amount1,
        address _sender,
        bytes calldata _verificationContext,
        bytes calldata _depositData
    ) external override onlyALM nonReentrant returns (uint256 amount0Deposited, uint256 amount1Deposited) {
        // We disable deposits,
        // since reserves are not meant to be stored in the pool
        if (sovereignVault != address(this)) revert SovereignPool__depositLiquidity_depositDisabled();

        // At least one token amount must be positive
>>      if (_amount0 | _amount1 == 0) {
            revert SovereignPool__depositLiquidity_zeroTotalDepositAmount();
        }

However at this point the SovereignPool will revert as if either passed amount is 0, then it will revert. This means that if an Executor passes one 0 init, then this will cause the module to be bricked as each first deposit will break.

Impact

Any module Initialized with one init value being 0, will have it's core functionality of depositing liquidity to be broken.

sherlock-audit / 2024-03-arrakis-judging

BiasedMerc - When Vault Executors set one _init to 0, deposit() will be broken #2

When Vault Executors set one _init to 0, deposit() will be broken

Summary

Vulnerability Detail

Impact

Code Snippet

Tool used

Recommendation