DOS in all 4 token removal functions of Rebalancing contract due to mismatched modifier on `snapshot()` and `setTokenAndSupplyRecord()`

[hats-bug-reporter[bot]]

Github username: @burhankhaja Twitter username: imaybeghost Submission hash (on-chain): 0xf221a2ad4ecb625bddce705994bb9a8326030b58a854c8c951325750da85a9c3 Severity: high

Description: Description\ There are 4 types of token removal functions in Rebalancing contract:

• removePortfolioToken(address)
• removeNonPortfolioToken(address)
• removePortfolioTokenPartially(address,uint)
• removeNonPortfolioTokenPartially(address,uint)

All of them internally call _tokenRemoval() which itself calls below two functions on TokenExclusionManager contract:

• snapshot()

• setTokenAndSupplyRecord()

  function _tokenRemoval(address _token, uint256 _tokenBalance) internal {
      ......Skipped.......
  
  // Snapshot for record-keeping before removing the token
  uint256 currentId = tokenExclusionManager.snapshot();
  
  // Record the removal details in the token exclusion manager
  tokenExclusionManager.setTokenAndSupplyRecord(
    currentId - 1,
    _tokenBalance,
    _token,
    portfolio.totalSupply()
  );
  
      ......Skipped.......
      ......Skipped.......
  
  }

Unfortunately, both these above functions are defined with onlyPortfolioManager modifier which only allows calls from Portfolio contract but not from the rebalancing contract

  function snapshot() external override onlyPortfolioManager returns (uint256) {
          ......Skipped....... 
  }

    function setTokenAndSupplyRecord(
    uint256 _snapShotId,
    uint256 _balanceAtRemoval,
    address _tokenRemoved,
    uint256 _totalSupply
  ) external override onlyPortfolioManager {
          ......Skipped.......
  }

As a result All the token removal functions will result in revert and the Major functionality of the protocol is broken.

Impact\ Loss of Portfolio Investor funds:

As the portfolio managers won't be able to remove neither non-portfolio token nor the portfolio token.

Imagine if portfolio managers had bad investment and they want to get rid of it to prevent further losses, they won't be able to do that, in return their portfolio will keep making losses for its investors.

Attack Scenario\ Asset Manager will not be able to remove neither the portfolio tokens nor the non-portfolio tokens.

Recommendation\ Use onlyRebalancerContract() modifier instead of onlyPortfolioManager() on snapshot() and setTokenAndSupplyRecord() functions:

-  function snapshot() external override onlyPortfolioManager returns (uint256) {
+ function snapshot() external override onlyRebalancerContract returns (uint256) {
          ......Skipped....... 
  }

-  function setTokenAndSupplyRecord(uint256 _snapShotId, uint256 _balanceAtRemoval, address _tokenRemoved,uint256 _totalSupply) external override onlyPortfolioManager {
+ function setTokenAndSupplyRecord(uint256 _snapShotId, uint256 _balanceAtRemoval, address _tokenRemoved,uint256 _totalSupply) external override onlyRebalancerContract {
          ......Skipped.......
  }

**Attachments**

1. **Proof of Concept (PoC) File**
<!-- You must provide a file containing a proof of concept (PoC) that demonstrates the vulnerability you have discovered. -->

2. **Revised Code File (Optional)**
<!-- If possible, please provide a second file containing the revised code that offers a potential fix for the vulnerability. This file should include the following information:
- Comment with a clear explanation of the proposed fix.
- The revised code with your suggested changes.
- Any additional comments or explanations that clarify how the fix addresses the vulnerability. -->

[kakarottosama]

Invalid. when creating portfolio, accessController.setUpRoles is correctly set rebalancing contract, and set it with PORTFOLIO_MANAGER_ROLE

[burhankhaja]

sorry, my bad