Anyone can bypass whitelist restrictions with batch contracts

[hats-bug-reporter[bot]]

Github username: @0xfuje Twitter username: 0xfuje Submission hash (on-chain): 0x683539dba401843200192eb8ca7bd4a584fd75aa33808bc441ff41021b7102d5 Severity: medium

Description:

Impact

Access control bypass: anyone can bypass the whitelist when depositing or withdrawing when the batch contracts are enabled to interact with the vaults

Description

The problem is this that if the DepositBatch and WithdrawBatch contracts are allowed to interact with a vault via whitelisting them, it removes the barrier to restrict only whitelisted users to interact with the contracts, since they use a direct call with multiTokenDepositFor() / multiTokenWithdrawalFor() for interacting with the vaults.

DepositBatch.sol - multiTokenSwapAndTransfer()

  function multiTokenSwapAndTransfer(
    FunctionParameters.BatchHandler memory data 
  ) external payable nonReentrant{
    address[] memory tokens = IPortfolio(data._target).getTokens();
    uint256 tokenLength = tokens.length;
    uint256[] memory depositAmounts = new uint256[](tokenLength);
    address user = msg.sender;
    address _depositToken = data._depositToken;

    if (data._callData.length != tokenLength)
      revert ErrorLibrary.InvalidLength();

    // Transfer tokens from user if no ETH is sent
    if (msg.value == 0) { 
      TransferHelper.safeTransferFrom(
        _depositToken,
        user,
        address(this),
        data._depositAmount
      );
    }

    // Perform swaps and calculate deposit amounts for each token
    for (uint256 i; i < tokenLength; i++) {
      address _token = tokens[i];
      uint256 balance;
      if (_token == _depositToken) {
        //Sending encoded balance instead of swap calldata
        balance = abi.decode(data._callData[i], (uint256));
      } else {
        (bool success, ) = SWAP_TARGET.delegatecall(data._callData[i]); 
        if (!success) revert ErrorLibrary.CallFailed();
        balance = _getTokenBalance(_token, address(this));
      }
      if (balance == 0) revert ErrorLibrary.InvalidBalanceDiff();
      IERC20(_token).approve(data._target, balance); 
      depositAmounts[i] = balance;
    }

    IPortfolio(data._target).multiTokenDepositFor( // @audit m - if depositBatch contract is allowed -> anyone can bypass whitelist restrictions
      user,
      depositAmounts,
      data._minMintAmount
    );

    //Return any leftover vault token dust to the user
    for (uint256 i; i < tokenLength; i++) {
      address _token = tokens[i];
      TransferHelper.safeTransfer(
        _token,
        user,
        _getTokenBalance(_token, address(this))
      );
    }

    // Return any leftover invested token dust to the user
    if (msg.value == 0) {
      TransferHelper.safeTransfer(
        _depositToken,
        user,
        _getTokenBalance(_depositToken, address(this))
      );
    } else {
      (bool sent, ) = user.call{value: address(this).balance}("");
      if (!sent) revert ErrorLibrary.TransferFailed();
    }
  }

Recommendation

Consider to use a solution that makes the user the msg.sender (e.g. delegatecall, but be wary of it's dangers) for multiTokenDepositFor and multiTokenWithdrawalFor calls. This will ensure the batch contracts can't be used to bypass the whitelist.

[langnavina97]

The function multiTokenDepositFor includes a check to verify if the user for whom the deposit is being made is whitelisted. For withdrawals, no whitelist check is required, as anyone holding a token should be able to withdraw.

@0xfuje