ge6a - Possibility of breaking core invariant due to the USDC/USDT fee on transfer.

[sherlock-admin]

ge6a

medium

Possibility of breaking core invariant due to the USDC/USDT fee on transfer.

Summary

The USDT/USDC tokens have an implemented fee on transfer feature that is currently not in use. However, it may be activated in the future. If this happens, a user would receive more vault tokens than the collateral deposited, due to the fee imposed by USDT/USDC.

Vulnerability Detail

This would violate a core invariant defined in the README file:

When users deposit funds, the contract issues vault tokens in a 1:1 ratio with the deposited amount.

It would also result in a loss of funds for the protocol and a profit for the user, as the difference would be covered by the protocol.

        IERC20(collateralTokenAddress).safeTransferFrom(
            msg.sender,
            address(this),
            amount
        );
        uint256 amountInSolverVaultTokenDecimals = solverVaultTokenDecimals >=
            collateralTokenDecimals
            ? amount *
                (10 ** (solverVaultTokenDecimals - collateralTokenDecimals))
            : amount /
                (10 ** (collateralTokenDecimals - solverVaultTokenDecimals));

        SolverVaultToken(solverVaultTokenAddress).mint(
            msg.sender,
            amountInSolverVaultTokenDecimals
        );

In the above source code can be seen that the vault mints tokens based on the variable amount instead of the actual transferred amount.

Impact

Breaking core invariant and financial loss for the protocol.

Code Snippet

https://github.com/sherlock-audit/2023-12-symm-io/blob/60ee22a4c598220821385cfb5eee43f40aafd5f1/solver-vaults/contracts/SolverVaults.sol#L158-L181

Tool used

Manual Review

Recommendation

Adjust the deposit function to mint vault tokens based on the actual amount received instead of the amount variable. It will cost a little more gas but will prevent more serious problems.

Duplicate of #9

[gstoyanovbg]

Escalate

I do not agree with the reason for invalidating the report here and I believe that according to Sherlock's rules, this is a valid report.

1. The first highlighted reason is this question from the README file:

Are there any FEE-ON-TRANSFER tokens interacting with the smart contracts?
None

The answer is negative, but at the same time, it is indicated that the protocol is expected to interact with USDT and USDC, which have the possibility of a fee on transfer that is not active but can be activated at any time. The conclusion I made when reading this was that sponsors might not be aware of this possibility in the mentioned tokens. That's why I reported it. For me, this is a reason for this report to be valid, not the other way around.

2. The second highlighted reason is the rule for future issues from the Sherlock's rules:

Future issues: Issues that result from a future integration/implementation that was not intended (mentioned in the docs/README) or because of a future change in the code (as a fix to another issue) are not valid issues.

My opinion is that this rule is not applicable to the current report. It talks about future integrations or implementations as well as future changes in the source code. In the case of USDT/USDC, we are not talking about future changes that are still being developed but about functionality that is already present in the source code and can be activated through a setting. This does not fall into the category of a future issue. If we assume that this is a future issue, it would mean that we should invalidate every reported problem in Sherlock related to a change in settings after the initial deployment, which would not be correct.

In my opinion, the correct way to mention that sponsors are aware of this issue is to include it in the known issues/accepted risks section, and thus, Watsons would not submit it. However, this has not been done:

Please list any known issues/acceptable risks that should not result in a valid finding.
None

I understand that the fee-on-transfer case with USDT/USDC is well known, and sponsors are likely to be aware of it, which is why I hesitated before submitting the report and before escalating it. However, Watsons have no way to know whether sponsors are aware or not; for us, the primary source of truth is the README file. It is not fair for Watsons, who submitted this issue, to be penalized with worsened ratios because they followed the rules.

[sherlock-admin2]

Escalate

I do not agree with the reason for invalidating the report here and I believe that according to Sherlock's rules, this is a valid report.

1. The first highlighted reason is this question from the README file:

Are there any FEE-ON-TRANSFER tokens interacting with the smart contracts?
None

The answer is negative, but at the same time, it is indicated that the protocol is expected to interact with USDT and USDC, which have the possibility of a fee on transfer that is not active but can be activated at any time. The conclusion I made when reading this was that sponsors might not be aware of this possibility in the mentioned tokens. That's why I reported it. For me, this is a reason for this report to be valid, not the other way around.

2. The second highlighted reason is the rule for future issues from the Sherlock's rules:

Future issues: Issues that result from a future integration/implementation that was not intended (mentioned in the docs/README) or because of a future change in the code (as a fix to another issue) are not valid issues.

My opinion is that this rule is not applicable to the current report. It talks about future integrations or implementations as well as future changes in the source code. In the case of USDT/USDC, we are not talking about future changes that are still being developed but about functionality that is already present in the source code and can be activated through a setting. This does not fall into the category of a future issue. If we assume that this is a future issue, it would mean that we should invalidate every reported problem in Sherlock related to a change in settings after the initial deployment, which would not be correct.

In my opinion, the correct way to mention that sponsors are aware of this issue is to include it in the known issues/accepted risks section, and thus, Watsons would not submit it. However, this has not been done:

Please list any known issues/acceptable risks that should not result in a valid finding.
None

I understand that the fee-on-transfer case with USDT/USDC is well known, and sponsors are likely to be aware of it, which is why I hesitated before submitting the report and before escalating it. However, Watsons have no way to know whether sponsors are aware or not; for us, the primary source of truth is the README file. It is not fair for Watsons, who submitted this issue, to be penalized with worsened ratios because they followed the rules.

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.

[PlamenTSV]

The 2 reasons you have highlighted are exactly the reasons this is a LOW at best. The change of USDC/USDT to charge a fee would not happen unannounced and probably would never due to their wide integration across the ecosystem. And as of right now they do not charge a fee, thus 1. and 2. of your reasons are plainly obvious.

[gstoyanovbg]

The change of USDC/USDT to charge a fee would not happen unannounced and probably would never due to their wide integration across the ecosystem.

@PlamenTSV I searched for some public evidence for your claim, but I couldn't find any. I am not part of the USDT or USDC teams, and I cannot confirm your statements. I don't have a crystal ball to predict what changes may or may not occur in such a rapidly evolving ecosystem in the future. The fact is that two of the most popular stablecoins have such functionality. Everyone can assess for themselves whether someone implements a feature when they are 100% sure that it won't be used ever. Lastly, this is a quote from Sherlock's criteria for issue validity:

Medium: There is a viable scenario (even if unlikely) that could cause the protocol to enter a state where a material amount of funds can be lost.

[PlamenTSV]

My claim is purely logical, imagine how many hundreds of protocols would break if USDC or USDT enter a fee for their transactions. This is not something that would just happen unbeknownst to the public and it's a wild claim to base your finding around.
Its a "but what if they do" scenario. I wouldn't disagree with you if the tokens weren't so widespread in the ecosystem.

[gstoyanovbg]

@PlamenTSV I cannot know how many hundreds of protocols would break if USDC or USDT enter a fee for their transactions, but I know that 100% of the protocols I've worked on in the past would not break because they handle it correctly. In recent months, several reports in Sherlock have been invalidated due to speculative claims in the reports, so I do not expect speculative claims to be a reason for a report to be invalid. If we accept that your statement is a sufficient argument for invalidating this report, it means that no report related to a fee on transfer for USDT/USDC should be valid. Then, these types of issues would simply have been added to "VII. List of Issue categories that are not considered valid" in Sherlock's rules, and no one would have reported them. However, this has not been done, so for Sherlock, such types of issues can be valid. A quick search in the valid reports from the past supports this claim. I do not want to debate when/why USDT/USDC may introduce a fee on transfer, but I will also make a speculative statement - if economic conditions necessitate the introduction of a fee on transfer for USDC/USDT, they will not hesitate to turn that feature on regardless of the number of protocols that will be affected.

[PlamenTSV]

Point me to a valid issue from the past year for fee-on-transfer tokens that has the same context as this repo:

1. FEE-ON-TRANSFER tokens are NOT supported AT ALL
2. It uses ONLY USDC or USDT or both.

[gstoyanovbg]

@PlamenTSV I trust you that there isn't one. What does that prove? I assume this is directed towards a specific sentence from my previous comment taken out of context:

A quick search in the valid reports from the past supports this claim.

With it, I'm not claiming that there is a report completely identical to the current one, but that there are valid reports related to the fee on transfer for USDT, which supports my statement (supported by additional arguments above as well) that the following arguments are not enough to make this report invalid.

imagine how many hundreds of protocols would break if USDC or USDT enter a fee for their transactions

The change of USDC/USDT to charge a fee would not happen unannounced and probably would never due to their wide integration across the ecosystem

To be entirely precise, this is the most recent report that has validated the fee on transfer for USDT. To preempt any future comments, I have seen this sentence and realize that it has most likely been validated due to the ambiguous way it is written.

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

However, if your arguments about the probability of fee on transfer for USDC/USDT to be enabled were sufficient to invalidate a report, it would not have been considered valid.

We return again to this point from the README file:

Are there any FEE-ON-TRANSFER tokens interacting with the smart contracts?
None

I assume you expect the answer to this question to be positive for my report to be valid. If the sponsor is not aware that USDT/USDC has an option for a fee on transfer from their perspective, the protocol is not required to support the fee on transfer. In this situation, how is the sponsor expected to state that supports it, assuming they don't know that USDT/USDC has such an option? Isn't the paradox evident? And consequently, as Watson, how am I supposed to know whether the sponsor was aware that USDC/USDT has the option for a fee on transfer by reading this README file ?

I hope we don't dilute this discussion further before the lead judge and head of judging take a stance because it wouldn't be pleasant for them to read dozens of comments saying the same thing in different words.

[nevillehuang]

@PlamenTSV Thanks for doing my job for me, agree with all your points and stand by my previous comment here. @gstoyanovbg I hope you can understand it constitutes external admin (which is trusted mentioned in the read.me) future integration, so this is out of scope.

While some protocols have logic to cater to FOT, similar amounts of protocols do not. Since it is mentioned FOT is not supported in this protocol, to me your issue is purely speculation as to when a fee will be implemented by USDT/USDC admins as well.

[gstoyanovbg]

@nevillehuang No, unfortunately, I don't understand that exactly. I have some experience in developing and managing software projects, and for me, it is completely unclear how functionality that is already in the project's source code and can be activated through settings can be called 'future integration/implementation.' Let me give you a simple example - we have a protocol that accepts deposits. Admin decides to change the maximum amount of a specific token that a user can deposit through a setter that is already in the source code. Is this also called 'future integration/implementation' because the basis is the same? I don't deny that the administrators of USDT/USDC are trusted why do you think that activating FOT will make them not trusted?

Regarding the last part of your comment, I have already described the paradox due to which there is no way for the sponsor to state that they support FOT, assuming that they only work with USDT/USDC and are unaware that both tokens have a setting that can activate FOT.

[PlamenTSV]

Currently USDT/USDC are NOT fee on transfer. The contest repo is not wrong. You are basing your finding on the assumption that one day these tokens MIGHT activate their deprecated functionalities. It just does not affected the provided context, you are not entirely in the wrong, but we are playing "what if here".

[gstoyanovbg]

@PlamenTSV Before submitting this report, I was aware of the arguments that would likely invalidate it. That's precisely why I hadn't submitted it to other contests. When I delved deeper into the exact definitions of Sherlock's rules, I realized that the reasons for invalidating such reports are not justified. I believe I have described in sufficient detail, for each of the arguments presented by you and @nevillehuang , why it is not sufficient for invalidating this report, according to the current rules of Sherlock, of course.

Obviously, I'm not the only one who thinks this way, as similar reports continue to be submitted, and rightfully so. In this line of thought, I don't think it makes sense to continue this discussion unless some different arguments are presented. I expect a decision on the case from the head of judging at Sherlock. If this report is deemed invalid, I expect Sherlock to clearly define when this issue can be valid, and if it is not a valid problem for the platform, it should be clearly stated in the rules. It is not fair for Watsons who have read the rules of the platform and interpret them entirely literally, as one should interpret rules, to be penalized with worsened ratios for this.

[Czar102]

Hey, even if I was to agree with the escalation's reasoning, I think this should be invalid.

Are the admins of the protocols your contracts integrate with (if any) TRUSTED or RESTRICTED?

Trusted

Admins of USDT and USDC are trusted not to alter the parameters of the implementation that would break the protocol.

Planning to reject the escalation and leave the issue as is.

[gstoyanovbg]

@Czar102

External Admin trust assumptions:

1. When external-admin=trusted, issues related to these external admins being able to rug protocol users is not a valid issue. (Example: Aave governance has the intention of rugging Index Protocol)
2. When external-admin=restricted, issues related to these external admins affecting a protocol (being audited) by updating the external protocol parameters is a valid issue (Example: Aave governance has the intention to improve the Aave protocol ) as the bug can occur even when the external admin is well intended

I understand your argument, but this rule is so poorly defined that if we read it literally, it nowhere states that if trusted external admins change a parameter, it is not considered a bug. This conclusion is made indirectly based on point two, which says the same for restricted external admins. I am also not sure if the sponsors understand what Trusted external admin means because for most people it is an external admin that will not make malicious actions against the protocol intentionally.

This issue from previous contest also mentioned in the README that external admins are trusted. But the issue is valid regardless of that. However i know that rules > historical decisions.

I accept your reasoning but still think that some of Sherlock's rules have very ambiguous definitions, which is not in favor of any of the parties involved.

[PlamenTSV]

The issue you provide has a different README specifications.
You have a feedback channel for suggestions, you can shift there and provide your ideas. I am in support of your statements that the rules have a ton of holes and uncertainties but as it is right now, this is invalid.

[Czar102]

Result:
Invalid
Duplicate of #9

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• gstoyanovbg: rejected