search

hats-finance / Velvet-Capital-0x0bb0c08fd9eeaf190064f4c66f11d18182961f77

Core smart contracts of Velvet Capital
Other
0 stars 1 forks source link

VaultConfig.sol - adding new tokens would lead to users being unable to withdraw their underlying collateral #44

Open hats-bug-reporter[bot] opened 1 week ago

hats-bug-reporter[bot] commented 1 week ago

Github username: @PlamenTSV Twitter username: @p_tsanev Submission hash (on-chain): 0x0b5749153b21246bd35dacad82ac7bae98fa3632fd1e945335bc8ae312b262d6 Severity: medium

Description: Description\ The vault manager operates based on it's vault config in the aspect of what tokens are accepted, since every deposit is done proportianally to current supplies and is done in all tokens. When depositing, you are forced to deposit all of the tokens in the same ratio and similiarly, when withdrawing you get back from all tokens with the same ratio, depending on the burned portfolioToken. This functionality is flawed for when new tokens get added, since existing portfolioTokens before the addition, would allow you to receive from the new tokens you did not deposit into the contract.

Attack Scenario\ Picture the simple scenario: The portfolio vault initially only accepts USDC and USDT. Alice deposits 1000 of both and gets the initial portofolio minted, let's say another 1000 A new token, doesnt matter which one, gets added, lets say BNB Bob deposits 1000 of all tokens, USDC USDT and BNB, and receives his respective portfolio tokens.

Now when Alice attempts to burn her portfolio shares and get her tokens back, the _multiTokenWithdrawal will return to her proportianlly 1000 of every token for her portfolio shares, meaning that now the supplies of USDC and USDT would hold Bob's funds, but BNB's supplies would be emptied out.

Bob is unable to retrieve his BNB tokens, since Alice got them unintentionally

Attachments

  1. Proof of Concept (PoC) File

  2. Revised Code File (Optional)

Recommendation\ Use some kind of mapping for users to keep track of the tokens they have deposited, instead of the global tokens that are at all times interchangable. For e.g when Alice deposits and we have 2 tokens, those 2 tokens should be cached for her and on withdraw, iterate over the tokens that her shares are backing instead of all the tokens, where she has no deposit.

langnavina97 commented 1 week ago

If the portfolio gets rebalanced and a new user deposits, the new user will provide the current tokens in the ratio accepted by the protocol. The shares are minted based on the added value. For example, if there are 1000 tokens representing the current token balances and a new user doubles the token balances with their deposit, they will receive tokens that represent 50% of the portfolio value. If the new user adds the same value as already existing in the portfolio, they will receive 1000 portfolio tokens. The total supply will be 2000 tokens, making the new user's share 1000/2000, which is 50%.

Additionally, when a user withdraws from a rebalanced portfolio, they should receive the current underlying tokens of the portfolio rather than the exact tokens they initially invested. This ensures that the value of their withdrawal reflects the current composition of the portfolio.

@PlamenTSV