Jaraxxus - Approve to zero not used for lendunderlyingtokens, will affect tokens like USDT about 2023-10-notional-judging

[VagnerAndrei26]

Escalate
I'm escalating this to be looked on

@gstoyanovbg

I understand there seem to be some conflict within the contest details, but ultimately I think this is definitely an issue worth bringing up given notional is definitely interested in setting USDT as a lendToken in such vaults and not allowing such approvals would be affecting potential functionality and even potential profits since deposits from notional will be bricked.

Which ERC20 tokens do you expect will interact with the smart contracts?

any

I think in this case we should consider any ERC20 tokens beside the ones with non-standard behavior, since the README specifies no tokens with non-standard behavior should be considered for this audit.

[Czar102]

I agree, this is not a valid issue. If "any" ERC20 token is allowed, there could me a token that reverts on all interactions so we couldn't use it anyway. As said in Sherlock docs, the weird trait needs to be explicitly mentioned in the README so that its existence implying a loss of funds would be valid.

This issue may provide value to the team, but is not a valid finding in Sherlock. Planning to accept the escalation and make this issue invalid.

[nevillehuang]

@Czar102 agree with your reasoning, would take that into consideration in future judging decisions. My suggestion is to include the weird erc-20 github repo link for further clarification within the question for further clarification, which I believe will benefit all judges, watsons and sponsors.

[jeffywu]

Acknowledged

[jeffywu]

notional-finance/leveraged-vaults#64

[gstoyanovbg]

Sherlock's rules:

23. Non-Standard tokens: Issues related to tokens with non-standard behaviors, such as weird-tokens are not considered valid by default unless these tokens are explicitly mentioned in the README.

Weird-erc20:

Some tokens (e.g. USDT, KNC) do not allow approving an amount M > 0 when an existing amount N > 0 is already approved. This is to protect from an ERC20 attack vector described here.

README:

Q: Do you expect to use any of the following tokens with non-standard behaviour with the smart contracts?
No

@nevillehuang From my perspective this issue should be invalid because USDT is not explicitly mentioned in the README. Do i miss something ?

[nevillehuang]

@gstoyanovbg

I understand there seem to be some conflict within the contest details, but ultimately I think this is definitely an issue worth bringing up given notional is definitely interested in setting USDT as a lendToken in such vaults and not allowing such approvals would be affecting potential functionality and even potential profits since deposits from notional will be bricked.

Which ERC20 tokens do you expect will interact with the smart contracts?

any

[0xRizwan]

I agree with @gstoyanovbg comment.

approve to zero in case of USDT is the non-standard behavior and it is not explicitly mentioned in readme.md.

My understanding is, this particular question in readme is used to handle such non-standard token issue and in this case its none. If it was USDT/USDC instead of No then i think the issue could be considered as valid per sherlock rules.

Do you expect to use any of the following tokens with non-standard behaviour with the smart contracts?
No

Also, i think below sherlock rule is referencing to this question,

Sherlock's rules:

Non-Standard tokens: Issues related to tokens with non-standard behaviors, such as weird-tokens are not considered valid by default unless these tokens are explicitly mentioned in the README.

[sherlock-admin2]

Escalate
I'm escalating this to be looked on

@gstoyanovbg

I understand there seem to be some conflict within the contest details, but ultimately I think this is definitely an issue worth bringing up given notional is definitely interested in setting USDT as a lendToken in such vaults and not allowing such approvals would be affecting potential functionality and even potential profits since deposits from notional will be bricked.

Which ERC20 tokens do you expect will interact with the smart contracts?

any

I think in this case we should consider any ERC20 tokens beside the ones with non-standard behavior, since the README specifies no tokens with non-standard behavior should be considered for this audit.

You've created a valid escalation!

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.

[jaraxxus775]

1. Protocol states that any ERC20 token will be interacted with, so all kinds of ERC20 issues should be accounted for. It will only be a non-issue if the protocol specifically has checks to make sure that the ERC20 cannot be used, eg if only 6-18 decimals should be used, then there must be a check in the function. Likewise, if revert on zero value transfer tokens should not be used, then protocol must make sure that function only transfers > 0 amount. If protocol does not have any specific checks for ERC20 tokens, then it is an issue.

Which ERC20 tokens do you expect will interact with the smart contracts?
any

2. (USDT)'s approve() function will revert if the current approval is not zero, to protect against front-running changes of approvals. USDT cannot be used at all, which breaks protocol functionality. USDT is not a random token, it has a pretty huge TVL and is used in most lending protocols. AAVE sees 200m USDT locked in the lending market.

3. Sponsor found value in the issue.

4. Historically, approve to zero has always been awarded Medium difficulty (DODO-06-2023), (Derby-01-2023)

Thanks.

[Czar102]

Hi @jaraxxus775, please check the Sherlock rule mentioned in @gstoyanovbg's comment.

It is clear that they want to work with any non-weird token. Since this approval revert is considered a weird trait, this finding should be invalid.

[Czar102]

Result:
Invalid
Has duplicates

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• VagnerAndrei26: accepted

[xiaoming9090]

Fixed in PR 64