Don't Allow users to claim non-portfolio tokens without decreasing their share balances

[hats-bug-reporter]

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

Description:
Description
As Per the natspec, The Purpose of claimRemovedTokens() is to Allows users to claim their share of tokens that have been removed from the portfolio due to certain events (e.g., lack of liquidity).

When the AssetManager removes portfolio tokens, they are sent to the TokenExclusiveManager to be claimed by the user.

Everything about the calculation is correct and works as expected in case of portfolio token removals. The user is able to claim removed portfolio token as per his shares (portfoliotoken balances) and no unexpected fund loss occurs.

But in case of non-portfolio token removals,

User shouldn't be able to claim them, or his shares should get subtracted, since he didn't contribute anything to the non-portfolio tokens of the vault contract. (Portfolio.sol)

Attack Scenario
Imagine this case:

• the current portfolio tokens are: {{ USDT, DAI and TAO }} & ALICE owns 25% of the portfolio
• Besides the portfolio tokens, let's say other tokens like 100 ETH have been sent to the vault contract.(Portfolio.sol)
• Since alice didn't contribute anything to the ETH deposits, he shouldn't be receiving even a WEI.
• Now imagine AssetManager removes ETH from vault via removeNonPortfolioToken(), it will be sent to the tokenExclusiveManager since the same _tokenRemoval() logic is implemented for both portfolio and non-portfolio token removals in Rebalancing contract.
• Unfortunately, ALICE will be able to claim 25 ETH via claimRemovedTokens as the same logic is applied for both portfolio and non-portfolio token removals (where the user's percentage of portfoliotokens is calculated, since alice owns 25% of portfolio tokens, therefore he will receive 25 ETH)
• Even worse, his share balance will be constant i.e he will still be owning the 25% of the portfolio tokens {{ USDT, DAI, TAO }}
• Therefore, alice was able to steal portion of non-portfolio tokens from the vault.

Recommendation
Either separate the logic for Portfolio and Non-Portfolio token removals or decrease the user's share balances while claiming non-portfolio tokens.

Attachments

1. Proof of Concept (PoC) File

2. Revised Code File (Optional)

[langnavina97]

The share could not be decreased fairly without the use of any oracle. Additionally, the share divided by the total supply represents the user's share in the portfolio. It's the asset manager's responsibility to claim the tokens. We'll take snapshots of the current balances when a user receives or burns tokens to ensure fair distribution of the removed tokens. @burhankhaja

[burhankhaja]

At this point, i believe there is something wrong with my english, why are u not getting my point.
i am talking about the tokens held by portfolio.sol contract outside of the portfolio tokens can be claimed by users which should be considered bug,

There is no way asset manager can claim them, until he himself owns 100% of the portfolio(as an investor) which is impossible for the portfolio management business.

Common, give it a thought it is visible bug
Have u properly understood the attack scenario part?

[kakarottosama]

I think we need to understand how and why the non-portfolio token exist in the vault. It is not just a random person send the non-portfolio token to the vault, there is no reason to do that.

IMO, this non-portfolio token came from claimRewardTokens so the _tokenToBeClaimed might be the non-portfolio token. It's clear if this reward token is the non-portfolio asset manager can remove them from vault to TokenExclusiveManager for equal share distribution. maybe @langnavina97 can confirm this.

@burhankhaja reading your comments on your submissions, you seems frustrated, please calm a bit brother. Recalling you 100% sure a high issue and eagerly want to create POC on #93 , but IMO, you are not understand the protocol well on that issue.

[langnavina97]

Correct @kakarottosama !

A non-portfolio token can be the result from claiming reward tokens. Asset managers will have the choice to either rebalance the tokens (reinvest) or remove the tokens so the portfolio token holders can claim them separately. Taking the snapshot data ensures a fair distribution of the claimed tokens.

@burhankhaja