Duplicate Tokens and Corresponding Weights Can Cause Unintended Rebalancing and Break Invariants

[hats-bug-reporter[bot]]

Github username: @MatinR1 Twitter username: MatinRezaii1 Submission hash (on-chain): 0xe72af1c1de81642047e5c2e4df422cf38e574689087ace1ce01d477a136d65b9 Severity: medium

Description: Description\ The function updateWeights() is responsible for updating the tokens and their respective weights inside a portfolio which can be called by the asset manager. At its core, this function invokes _updateWeights(), ensuring the lengths of tokens and weights match. It then transfers each token and its sell amount from the vault to the Enso swap handler for swapping. After that, the handler does an Enso swapping. After that, it ensures that the handler had sold the tokens successfully or not. Next steps are ensuring that the solver bought those tokens are inside the portfolio or not.

The problem is that the _updateWeights() does not check for a possible duplicate which asset manager wrongly passes to the function. In this case, a tokens weight is double considered and this would lead to a double transferring from the vault to the handler. What this function does is ensuring that the handler had sold these tokens or not. However, there is not a check for a possible double spending of a token.

https://github.com/hats-finance/Velvet-Capital-0x0bb0c08fd9eeaf190064f4c66f11d18182961f77/blob/main/contracts/rebalance/Rebalancing.sol#L91

Attack Scenario\

Consider the following scenario:

1. The asset manager intends to update the weights for tokens A, B, and C.
2. By mistake, he incorrectly enters token A's address twice, resulting in the tokens being set as A, A, and C.
3. The vault has a sufficient amount of token A, meeting the minimum criteria.
4. He then calls the updateWeights() function.
5. Consequently, instead of pulling and selling token B, token A is mistakenly sold twice.

This misconfiguration leads to an improper asset allocation, disrupting the intended portfolio balance.

As it just changes the balance of a token and not a direct theft, I think this issue should be considered as medium.

Attachments

2. Revised Code File (Optional)

Consider checking for a possible duplicate token address inside the _updateWeights() function.

[kakarottosama]

user input wrong value at best is just a low sanitize input issue, also Velvet's front-end can prevented such kind of issue easily, in other audit contest platform, wrong user input is not even valid as a low

[MatinR1]

If needed, I can provide examples where similar issues were classified as medium severity on other platforms. Checking for duplicate array members is a standard practice in smart contract audits due to its significant impact. The potential consequences of such an oversight, even beyond admin mistakes, underscore its importance. Ensuring robust input validation at the contract level is crucial to maintain the integrity and security of the system, regardless of front-end safeguards.

[langnavina97]

A check for duplicates is not required as we're not updating the token list. Sell tokens can be duplicates. If the portfolio, for example, has 50 USDT - 10 USDT is swapped to BTC and 10 USDT is swapped to ETH. In that case the sellToken would be a duplicate. @MatinR1