roguereddwarf - SwapHandler.sol: Check that collateral token cannot be swapped is insufficient for tokens with multiple addresses

[sherlock-admin]

roguereddwarf

medium

SwapHandler.sol: Check that collateral token cannot be swapped is insufficient for tokens with multiple addresses

Summary

According to the contest page any non-rebasing ERC20 token is supposed to be supported.

The SwapHandler.swapForTau function checks that the collateralToken cannot be sent to the SwapAdapter for trading:

https://github.com/sherlock-audit/2023-03-taurus/blob/main/taurus-contracts/contracts/Vault/SwapHandler.sol#L54-L56

Vulnerability Detail

There exist however ERC20 tokens that have more than one address. In case of such a token, the above check is not sufficient. The token could be swapped anyway by using a different address.

Impact

The check that collateral cannot be swapped can be bypassed for tokens with multiple addresses.

Code Snippet

https://github.com/sherlock-audit/2023-03-taurus/blob/main/taurus-contracts/contracts/Vault/SwapHandler.sol#L45-L101

Tool used

Manual Review

Recommendation

Compare the balance of the collateral before and after sending tokens to the SwapAdapter and make sure it hasn't changed. Or implement a whitelist for tokens that can be swapped.

[Sierraescape]

Tokens with multiple addresses are pretty rare, so we're just going to note that the vault doesn't allow such tokens as collateral, and create wrappers for them if necessary.

https://github.com/protokol/taurus-contracts/pull/120

[spyrosonic10]

Escalate for 10 USDC

Token with different addresses is very very rare. Almost every protocols in Defi operating on assumption of token with single address.
This issue does not qualify as High/Medium.

[sherlock-admin]

Escalate for 10 USDC

Token with different addresses is very very rare. Almost every protocols in Defi operating on assumption of token with single address.
This issue does not qualify as High/Medium.

You've created a valid escalation for 10 USDC!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[roguereddwarf]

Escalate for 10 USDC

Disagree with previous escalation.
While these tokens are rare they do exist and as pointed out in my report any non-rebasing ERC20 is supposed to be supported which clearly includes tokens with multiple addresses.
So I think this is a valid medium.

[sherlock-admin]

Escalate for 10 USDC

Disagree with previous escalation.
While these tokens are rare they do exist and as pointed out in my report any non-rebasing ERC20 is supposed to be supported which clearly includes tokens with multiple addresses.
So I think this is a valid medium.

You've created a valid escalation for 10 USDC!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[hrishibhat]

Escalation accepted

Considering this a valid medium.
As pointed out in the second escalation, even though these tokens are rare the issue can still be considered valid medium.

Note: Going forward, Sherlock team will add additional clarity on such rare token cases in the README.

[sherlock-admin]

Escalation accepted

Considering this a valid medium.
As pointed out in the second escalation, even though these tokens are rare the issue can still be considered valid medium.

Note: Going forward, Sherlock team will add additional clarity on such rare token cases in the README.

This issue's escalations have been accepted!

Contestants' payouts and scores will be updated according to the changes made on this issue.

[MLON33]

Tokens with multiple addresses are pretty rare, so we're just going to note that the vault doesn't allow such tokens as collateral, and create wrappers for them if necessary.

https://github.com/protokol/taurus-contracts/pull/120

Fixed here

[IAm0x52]

No direct fix has been implemented but a note has been added explicitly stating that multi-address tokens are not supported

[jacksanford1]

Classifying this issue as "Acknowledged" since no direct fix was made.