Resolve security vulnerabilities from audit competition

README.md

@@ -30,8 +30,4 @@ To run the coverage of Bsc(set CHAIN_ID="56" in env), run the following command:
 
 ```
 $ npm run coverageBsc
-
-```
-
-### Note
-Issue found by other auditor and out of scope issue https://docs.google.com/spreadsheets/d/18K7O1Mms9261mnS9OfDFelg4nSkwC34uUoDC3OS-GHE/edit#gid=76608452
+```

contracts/FunctionParameters.sol

@@ -1,5 +1,5 @@
 // SPDX-License-Identifier: BUSL-1.1
-pragma solidity ^0.8.17;
+pragma solidity 0.8.17;
 
 /**
  * @title FunctionParameters
@@ -15,6 +15,7 @@ library FunctionParameters {
    * @param _baseRebalancingAddress Base Rebalancing module address for cloning
    * @param _baseAssetManagementConfigAddress Base AssetManagement Config address for cloning
    * @param _feeModuleImplementationAddress Fee Module implementation contract address
+   * @param  _baseTokenRemovalVaultImplementation Token Removal Vault implementation contract address
    * @param _baseVelvetGnosisSafeModuleAddress Base Gnosis-Safe module address for cloning
    * @param _gnosisSingleton Gnosis Singleton contract address
    * @param _gnosisFallbackLibrary Gnosis Fallback Library address
@@ -29,6 +30,7 @@ library FunctionParameters {
     address _baseRebalancingAddres;
     address _baseAssetManagementConfigAddress;
     address _feeModuleImplementationAddress;
+    address _baseTokenRemovalVaultImplementation;
     address _baseVelvetGnosisSafeModuleAddress;
     address _gnosisSingleton;
     address _gnosisFallbackLibrary;
@@ -123,6 +125,7 @@ library FunctionParameters {
     uint256 _minPortfolioTokenHoldingAmount;
     address _protocolConfig;
     address _accessController;
+    address _feeModule;
     address _assetManagerTreasury;
     address[] _whitelistedTokens;
     bool _publicPortfolio;

contracts/PortfolioFactory.sol

@@ -29,6 +29,8 @@ contract PortfolioFactory is
   address internal baseAssetManagementConfigAddress;
   address internal feeModuleImplementationAddress;
   address internal baseVelvetGnosisSafeModuleAddress;
+  address internal baseTokenRemovalVaultAddress;
+
   address public protocolConfig;
   bool internal portfolioCreationPause;
 
@@ -40,6 +42,9 @@ contract PortfolioFactory is
 
   uint256 public portfolioId;
 
+  // The mapping is used to track the deployed portfolio addresses.
+  mapping(address => bool) public whitelistedPortfolioAddress;
+
   struct PortfoliolInfo {
     address portfolio;
     address tokenExclusionManager;
@@ -56,19 +61,27 @@ contract PortfolioFactory is
   event PortfolioInfo(
     PortfoliolInfo portfolioData,
     uint256 indexed portfolioId,
-    address indexed _owner
+    string _name,
+    string _symbol,
+    address indexed _owner,
+    address indexed _accessController,
+    bool isPublicPortfolio
   );
   event PortfolioCreationState(bool indexed state);
   event UpgradePortfolio(address indexed newImplementation);
   event UpgradeAssetManagerConfig(address indexed newImplementation);
   event UpgradeFeeModule(address indexed newImplementation);
+  event UpdataTokenRemovalVaultBaseAddress(address indexed newImplementation);
   event UpgradeRebalance(address indexed newImplementation);
   event UpdateGnosisAddresses(
     address indexed newGnosisSingleton,
     address indexed newGnosisFallbackLibrary,
     address indexed newGnosisMultisendLibrary,
     address newGnosisSafeProxyFactory
   );
+  event UpgradeTokenExclusionManager(address indexed newImplementation);
+
+  event TransferSuperAdminOwnership(address indexed newOwner);
 
   /// @custom:oz-upgrades-unsafe-allow constructor
   constructor() {
@@ -95,7 +108,8 @@ contract PortfolioFactory is
       initData._gnosisFallbackLibrary == address(0) ||
       initData._gnosisMultisendLibrary == address(0) ||
       initData._gnosisSafeProxyFactory == address(0) ||
-      initData._protocolConfig == address(0)
+      initData._protocolConfig == address(0) ||
+      initData._baseTokenRemovalVaultImplementation == address(0)
     ) revert ErrorLibrary.InvalidAddress();
     _setBasePortfolioAddress(initData._basePortfolioAddress);
     _setBaseRebalancingAddress(initData._baseRebalancingAddres);
@@ -108,6 +122,10 @@ contract PortfolioFactory is
     _setFeeModuleImplementationAddress(
       initData._feeModuleImplementationAddress
     );
+
+    setTokenRemovalVaultImplementationAddress(
+      initData._baseTokenRemovalVaultImplementation
+    );
     baseVelvetGnosisSafeModuleAddress = initData
       ._baseVelvetGnosisSafeModuleAddress;
     protocolConfig = initData._protocolConfig;
@@ -173,9 +191,13 @@ contract PortfolioFactory is
       bytes("")
     );
 
+    ERC1967Proxy _feeModule = new ERC1967Proxy(
+      feeModuleImplementationAddress,
+      bytes("")
+    );
+
     // Access Controller
     AccessController accessController = new AccessController();
-
     ERC1967Proxy _assetManagementConfig = new ERC1967Proxy(
       baseAssetManagementConfigAddress,
       abi.encodeWithSelector(
@@ -190,6 +212,7 @@ contract PortfolioFactory is
             ._minPortfolioTokenHoldingAmount,
           _protocolConfig: protocolConfig,
           _accessController: address(accessController),
+          _feeModule: address(_feeModule),
           _assetManagerTreasury: initData._assetManagerTreasury,
           _whitelistedTokens: initData._whitelistedTokens,
           _publicPortfolio: initData._public,
@@ -200,13 +223,10 @@ contract PortfolioFactory is
       )
     );
 
-    ERC1967Proxy _feeModule = new ERC1967Proxy(
-      feeModuleImplementationAddress,
-      bytes("")
-    );
-
     ERC1967Proxy portfolio = new ERC1967Proxy(basePortfolioAddress, bytes(""));
 
+    whitelistedPortfolioAddress[address(portfolio)] = true;
+
     // Vault creation
     address vaultAddress;
     address module;
@@ -241,9 +261,14 @@ contract PortfolioFactory is
       })
     );
 
+    bool isPublic = initData._public;
+    string memory _name = initData._name;
+    string memory _symbol = initData._symbol;
+
     ITokenExclusionManager(address(_tokenExclusionManager)).init(
       address(accessController),
-      protocolConfig
+      protocolConfig,
+      baseTokenRemovalVaultAddress
     );
 
     IVelvetSafeModule(address(module)).setUp(
@@ -296,7 +321,11 @@ contract PortfolioFactory is
     emit PortfolioInfo(
       PortfolioInfolList[portfolioId],
       portfolioId,
-      msg.sender
+      _name,
+      _symbol,
+      msg.sender,
+      address(accessController),
+      isPublic
     );
     portfolioId = portfolioId + 1;
   }
@@ -313,7 +342,7 @@ contract PortfolioFactory is
   }
 
   /**
-   * @notice This function is used to upgrade the Portfolio contract
+   * @notice This function is used to upgrade the Token Exclusion Manager contract
    * @param _proxy Proxy address
    * @param _newImpl New implementation address
    */
@@ -323,7 +352,7 @@ contract PortfolioFactory is
   ) external virtual onlyOwner {
     _setBaseTokenExclusionManagerAddress(_newImpl);
     _upgrade(_proxy, _newImpl);
-    emit UpgradePortfolio(_newImpl);
+    emit UpgradeTokenExclusionManager(_newImpl);
   }
 
   /**
@@ -456,6 +485,27 @@ contract PortfolioFactory is
     baseRebalancingAddress = _rebalance;
   }
 
+  /**
+   * @notice This function is used to set the token removal vault implementation address
+   * @param _baseTokenRemovalVault Address of the token removal vault to set as base
+   */
+  function setTokenRemovalVaultImplementationAddress(
+    address _baseTokenRemovalVault
+  ) internal {
+    baseTokenRemovalVaultAddress = _baseTokenRemovalVault;
+  }
+
+  /**
+   * @notice This function is used to set the Token Removal Vault implementation address
+   * @param _newImpl New implementation address
+   */
+  function setTokenRemovalVaultModule(
+    address _newImpl
+  ) external virtual onlyOwner {
+    setTokenRemovalVaultImplementationAddress(_newImpl);
+    emit UpdataTokenRemovalVaultBaseAddress(_newImpl);
+  }
+
   /**
    * @notice This function allows us to update gnosis deployment addresses
    * @param _newGnosisSingleton New address of GnosisSingleton
@@ -504,6 +554,8 @@ contract PortfolioFactory is
     if (!accessController.hasRole(SUPER_ADMIN, msg.sender))
       revert ErrorLibrary.CallerNotSuperAdmin();
     accessController.transferSuperAdminOwnership(msg.sender, _account);
+
+    emit TransferSuperAdminOwnership(_account);
   }
 
   /**

contracts/bundle/DepositBatch.sol

@@ -9,13 +9,10 @@ import {IPortfolio} from "../core/interfaces/IPortfolio.sol";
 import {FunctionParameters} from "../FunctionParameters.sol";
 import {ReentrancyGuard} from "@openzeppelin/contracts/security/ReentrancyGuard.sol";
 
-
 /**
- * @title BatchHandler
- * @dev This contract facilitates batching multiple transactions for users, providing a seamless experience.
- * It swaps the user's single token (chosen by the user) to vault tokens of a portfolio using Enso and deposits
- * the tokens into the portfolio, issuing the user portfolio tokens. If the token to swap is the same as a portfolio token,
- * the user's amount is simply passed as calldata. Finally, any leftover dust is returned to the user.
+ * @title DepositBatch
+ * @notice A contract for performing multi-token swap and deposit operations.
+ * @dev This contract uses Enso's swap execution logic for delegating swaps.
  */
 contract DepositBatch is ReentrancyGuard {
   // The address of Enso's swap execution logic; swaps are delegated to this target.
@@ -25,28 +22,56 @@ contract DepositBatch is ReentrancyGuard {
    * @notice Performs a multi-token swap and deposit operation for the user.
    * @param data Struct containing parameters for the batch handler.
    */
-  function multiTokenSwapAndTransfer(
+  function multiTokenSwapETHAndTransfer(
     FunctionParameters.BatchHandler memory data
-  ) external payable nonReentrant{
+  ) external payable nonReentrant {
+    if (msg.value == 0) {
+      revert ErrorLibrary.InvalidBalance();
+    }
+
+    address user = msg.sender;
+
+    _multiTokenSwapAndDeposit(data, user);
+
+    (bool sent, ) = user.call{value: address(this).balance}("");
+    if (!sent) revert ErrorLibrary.TransferFailed();
+  }
+
+  /**
+   * @notice Performs a multi-token swap and deposit operation for the user.
+   * @param data Struct containing parameters for the batch handler.
+   */
+  function multiTokenSwapAndDeposit(
+    FunctionParameters.BatchHandler memory data,
+    address user
+  ) external payable nonReentrant {
+    address _depositToken = data._depositToken;
+
+    _multiTokenSwapAndDeposit(data, user);
+
+    // Return any leftover invested token dust to the user
+    uint256 depositTokenBalance = _getTokenBalance(
+      _depositToken,
+      address(this)
+    );
+    if (depositTokenBalance > 0) {
+      TransferHelper.safeTransfer(_depositToken, user, depositTokenBalance);
+    }
+  }
+
+  function _multiTokenSwapAndDeposit(
+    FunctionParameters.BatchHandler memory data,
+    address user
+  ) internal {
     address[] memory tokens = IPortfolio(data._target).getTokens();
+    address _depositToken = data._depositToken;
+    address target = data._target;
     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];
@@ -55,16 +80,20 @@ contract DepositBatch is ReentrancyGuard {
         //Sending encoded balance instead of swap calldata
         balance = abi.decode(data._callData[i], (uint256));
       } else {
+        uint256 balanceBefore = _getTokenBalance(_token, address(this));
         (bool success, ) = SWAP_TARGET.delegatecall(data._callData[i]);
         if (!success) revert ErrorLibrary.CallFailed();
-        balance = _getTokenBalance(_token, address(this));
+        uint256 balanceAfter = _getTokenBalance(_token, address(this));
+        balance = balanceAfter - balanceBefore;
       }
       if (balance == 0) revert ErrorLibrary.InvalidBalanceDiff();
-      IERC20(_token).approve(data._target, balance);
+
+      IERC20(_token).approve(target, 0);
+      IERC20(_token).approve(target, balance);
       depositAmounts[i] = balance;
     }
 
-    IPortfolio(data._target).multiTokenDepositFor(
+    IPortfolio(target).multiTokenDepositFor(
       user,
       depositAmounts,
       data._minMintAmount
@@ -73,23 +102,10 @@ contract DepositBatch is ReentrancyGuard {
     //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();
+      uint256 portfoliodustReturn = _getTokenBalance(_token, address(this));
+      if (portfoliodustReturn > 0) {
+        TransferHelper.safeTransfer(_token, user, portfoliodustReturn);
+      }
     }
   }