xiaoming90 - Vault secondary currencies can be fee-on-transfer tokens

[sherlock-admin]

xiaoming90

medium

Vault secondary currencies can be fee-on-transfer tokens

Summary

When creating or configuring a vault, the vault does not check that the secondary currencies are not fee-on-transfer tokens.

Vulnerability Detail

https://github.com/sherlock-audit/2023-03-notional-0xleastwood/blob/main/contracts-v2/contracts/internal/vaults/VaultConfiguration.sol#L197

File: VaultConfiguration.sol
194:         // Tokens with transfer fees create lots of issues with vault mechanics, we prevent them
195:         // from being listed here.
196:         Token memory underlyingToken = TokenHandler.getUnderlyingToken(vaultConfig.borrowCurrencyId);
197:         require(!underlyingToken.hasTransferFee); 

Tokens with transfer fees create lots of issues with vault mechanics. When creating or configuring a vault, Notional explicitly performs validation against the primary currency to ensure that its underlying token does not have a transfer fee at Line 197 above.

However, a vault does not only supports the primary currency. It also supports one or more secondary currencies. When configuring the vault, it should also check that the underlying token of secondary currencies does not have a transfer fee.

Impact

Tokens with transfer fees create lots of issues with vault mechanics. If a fee-on-transfer token is added as the secondary currency on a vault, the vault might not work or might cause accounting issues within the vault.

Code Snippet

https://github.com/sherlock-audit/2023-03-notional-0xleastwood/blob/main/contracts-v2/contracts/internal/vaults/VaultConfiguration.sol#L197

Tool used

Manual Review

Recommendation

Consider checking that the underlying token of secondary currencies does not have a transfer fee.

// Tokens with transfer fees create lots of issues with vault mechanics, we prevent them
// from being listed here.
Token memory underlyingToken = TokenHandler.getUnderlyingToken(vaultConfig.borrowCurrencyId);
require(!underlyingToken.hasTransferFee); 

+ if (vaultConfig.secondaryBorrowCurrencies[0] != 0) {
+   Token memory secondaryUnderlyingTokenOne = TokenHandler.getUnderlyingToken(vaultConfig.secondaryBorrowCurrencies[0]);
+   require(!secondaryUnderlyingTokenOne.hasTransferFee); 
+ }

+ if (vaultConfig.secondaryBorrowCurrencies[1] != 0) {
+   Token memory secondaryUnderlyingTokenTwo = TokenHandler.getUnderlyingToken(vaultConfig.secondaryBorrowCurrencies[1]);
+   require(!secondaryUnderlyingTokenTwo.hasTransferFee); 
+ }

[Jiaren-tang]

Escalate for 10 USDC. fee on transfer is not in scope and this issue should not be a seperate medium

the onchain context is:

DEPLOYMENT: Currently Mainnet, considering Arbitrum and Optimisim in the near future. ERC20: Any Non-Rebasing token. ex. USDC, DAI, USDT (future), wstETH, WETH, WBTC, FRAX, CRV, etc. ERC721: None ERC777: None FEE-ON-TRANSFER: None planned, some support for fee on transfer

clearly none of the supported ERC20 token is fee-on-transfer token

and the protocol clearly indicate

FEE-ON-TRANSFER: None planned, some support for fee on transfer

[sherlock-admin]

Escalate for 10 USDC. fee on transfer is not in scope and this issue should not be a seperate medium

the onchain context is:

DEPLOYMENT: Currently Mainnet, considering Arbitrum and Optimisim in the near future. ERC20: Any Non-Rebasing token. ex. USDC, DAI, USDT (future), wstETH, WETH, WBTC, FRAX, CRV, etc. ERC721: None ERC777: None FEE-ON-TRANSFER: None planned, some support for fee on transfer

clearly none of the supported ERC20 token is fee-on-transfer token

and the protocol clearly indicate

FEE-ON-TRANSFER: None planned, some support for fee on transfer

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.

[0xleastwood]

Escalate for 10 USDC. fee on transfer is not in scope and this issue should not be a seperate medium

the onchain context is:

DEPLOYMENT: Currently Mainnet, considering Arbitrum and Optimisim in the near future. ERC20: Any Non-Rebasing token. ex. USDC, DAI, USDT (future), wstETH, WETH, WBTC, FRAX, CRV, etc. ERC721: None ERC777: None FEE-ON-TRANSFER: None planned, some support for fee on transfer

clearly none of the supported ERC20 token is fee-on-transfer token

and the protocol clearly indicate

FEE-ON-TRANSFER: None planned, some support for fee on transfer

It clearly says, None planned, some support for fee on transfer. The codebase has logic to support fee-on-transfer tokens and should be designed to correctly handle this. Leveraged vaults fail to work with these types of tokens, so adding this check ensures correct vault configuration.

[Jiaren-tang]

This is very open to interpretation, but the sponsor said no fee-on-transfer in leverage vault

https://discord.com/channels/812037309376495636/1089926820247375872/1113532032732119181

https://docs.sherlock.xyz/audits/judging/judging

the sherlock judging guideline said:

Admin Input/call validation: Protocol admin is considered to be trusted in most cases, hence issues where Admin incorrectly enters an input parameter. Example: Make sure interestPerMin > 1 ether as it is an important parameter. This is not a valid issue. Admin could have an incorrect call order. Example: If an Admin forgets to setWithdrawAddress() before calling withdrawAll() This is not a valid issue.

the report

When creating or configuring a vault, the vault does not check that the secondary currencies are not fee-on-transfer tokens.

is admin configuration, which is not valid

don't want to write a paper about the issue, unlikely to change the payout significantly anyway.

I do admire senior watson's skill though, medium does not determine payout, high severity issue does.

[0xleastwood]

This is very open to interpretation, but the sponsor said no fee-on-transfer in leverage vault

https://discord.com/channels/812037309376495636/1089926820247375872/1113532032732119181

https://docs.sherlock.xyz/audits/judging/judging

the sherlock judging guideline said:

Admin Input/call validation: Protocol admin is considered to be trusted in most cases, hence issues where Admin incorrectly enters an input parameter. Example: Make sure interestPerMin > 1 ether as it is an important parameter. This is not a valid issue. Admin could have an incorrect call order. Example: If an Admin forgets to setWithdrawAddress() before calling withdrawAll() This is not a valid issue.

the report

When creating or configuring a vault, the vault does not check that the secondary currencies are not fee-on-transfer tokens.

is admin configuration, which is not valid

don't want to write a paper about the issue, unlikely to change the payout significantly anyway.

I do admire senior watson's skill though, medium does not determine payout, high severity issue does.

To clarify on the sponsor's comment, he is saying fee-on-transfer is explicitly not allowed in the leveraged vault framework, which is enforced by the VaultConfiguration.setVaultConfig() function. However, this does not also enforce that secondary currencies are NOT fee-on-transfer tokens, meaning it isn't being explicitly allowed.

[jeffywu]

Fee on transfer is explicitly not allowed in vaults. To me, this is a valid issue.

[hrishibhat]

Result: Low Unique Considering this issue as a low since setVaultConfig is an admin function and this can be categorized as admin input validation to make sure FOT tokens are not added to the vault.

[sherlock-admin]

Escalations have been resolved successfully!

Escalation status:

• ShadowForce: accepted