Multiple Address ERC20 tokens could be drained from vaults containing them

[hats-bug-reporter[bot]]

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

Description: Description\ The project specified all ERC20 and ERC777 tokens, except for fee-on-transfer and rebasing ones, to be usable as assets in any vaults. That said, even though there are mitigations set to prevent exploiting arbitrary tokens that are not a vault's asset, tokens with multiple addresses(proxied tokens) are at risk at being stolen this way.

Attack Scenario\ The function at hand is calcLocalSwap() from the Volatile version of the vault. Unlike the Amplified version, this one has a specific check for if the weights of both provided tokens are equal and uses a uniswap-like calculation that does not involve the price curve. This means that a user can pass any 2 arbitrary tokens and the check would pass, but in a regular scenario an attacker cannot drain any real value, apart from one he sends himself. But if one of the tokens provided is an alternate address for a proxied token, that address' weight would too be 0, but it's balance would reflect the real balance of the token in the vault. Thus a malicious actor can:

1. Send any value of a random token he wants and pass it as token0
2. Pass a secondary address for the proxy token as token1, so both weights would be unregistered - 0
3. Entering the if-clause, weights do not play a part in calculating the amount out, thus the attacker can drain out any amount of token1 out of the vault.

Attachments

1. Proof of Concept (PoC) File

A verbal PoC would be like: token0 is random, so the contract balance is A=0, weight is 0 token1 is the alternate address, so the balance is B, but the weight is 0 We enter the if-clause and using the formula: (B * amount) / (A + amount) we are able to drain the entire balance of B

Examples of the issue(containing similiar context to this repo): https://solodit.xyz/issues/m-5-swaphandlersol-check-that-collateral-token-cannot-be-swapped-is-insufficient-for-tokens-with-multiple-addresses-sherlock-taurus-taurus-git

https://github.com/d-xo/weird-erc20?tab=readme-ov-file#multiple-token-addresses

2. Revised Code File (Optional)

   There is no easy way to bypass this issue as you maybe want to allow users to have a broader choice of tokens but an easy fix would be just checking if the weights of the provided tokens are 0 and reverting or returning 0 if so.

[reednaa]

I don't agree that there is an issue (yet).

Convince me otherwise: Is USDC a case of this? (proxied) If not, can you provide me with a real world example of such a token?

[PlamenTSV]

Synthetix SNX tokens, sBTC, TUSD/TrueUSD(quite large market volume) stable coin Upgradable tokens like USDC and USDT could be assumed to be able to be made into such tokens in the future Another proof this vulnerability is real and quite exploitable (the article is older but it affected over 30 protocols): https://blog.openzeppelin.com/compound-tusd-integration-issue-retrospective I could look for more but the mentioned tokens, with the current TUSD with its 400 mill supply and 2 billion market cap, seem plenty of proof Also a historical medium

[reednaa]

I need to explore these weird coins more before I come to a conclusion. Thanks for the article.

[PlamenTSV]

I was skeptical of myself too, but the existence of TUSD really shifted the weights for me. Take all the time you need!

[reednaa]

I have granted the issue low or medium until we determine severity. Fix: Do a single check for weight equal to 0 on the comparision.

[PlamenTSV]

Fix is good Another fix is to track token balances internally instead of relying on balance off. This way you may also avoid some calculation issue that might get missed ;p

[reednaa]

We have decided to classify this issue as Low. Our decision is based on the following arguments:

• This issue applies to very few tokens, however, these tokens are rather unique. There are a lot of unique tokens but we can’t possible implement sufficient protections or logic to contain them all.
• There is undocumented and strange behavior with these tokens.

According to these arguments, the issue is low. We have determined the proposed fix to be too strong and will instead add a weight 0 check to the line circumventing the complex math that would fail on 0 weight.

[PlamenTSV]

I respect the provided arguments, but the tokens are not difficult to understand, they are simply tokens running through a proxy, giving them 2 different addresses. The likelihood is low yes, even though TUSD is a widely adopted stable coin, but the impact, per the medium provided to us can classify it even as a high: https://hatsfinance.medium.com/catalyst-rewards-up-to-64k-in-usdc-858dab016972

I classified it as a medium only because of the token variety, low likelihood + high impact => medium severity. You are right that you cannot account for every weird token, but stable coins shouldn't be a matter of discussion, especially widely adopted ones. The fix look good. I will respect any further decision you make.

[reednaa]

TUSD is not a widely adopted stable coin. It represents less than 0,3% of Curve's daily volume today. That is while it has a market cap that 1/27 of USDC and a volume roughly 1/100 of USDC. So by all measures, even though it is a stable coin some people use, other stable coins are used more and more often. We don't believe our average user would create a vault with TUSD considering that in every metric USDC and USDT would be better alternatives. Arguably in a multichain context, TUSD is going to be used even more infrequently considering they are only on 7 chains where USDC is on 27 and USDT is on 25. Furthermore, you could argue that TUSD belong in an amplified vault where the issue does not apply.

External contract interactions, including the ERC20 implementations are generally outside of our control and you can easily design ERC20 compatible tokens such that the logic is broken. It is not our intention to support every ERC20 but to support the ones that our users use. Given the above arguments, I would argue that TUSD is an example of an exoticly designed ERC20 token that Catalyst does not support (nor mentions supporting) rather than a common design.