ZanyBonzy - Lack of support for fee on transfer, rebasing and tokens with balance modifications outside of transfers.

[sherlock-admin3]

ZanyBonzy

Medium

Lack of support for fee on transfer, rebasing and tokens with balance modifications outside of transfers.

Summary

The protocol wants to work with various ERC20 tokens, but in certain cases doesn't provide the needed support for tokens that charge a fee on transfer, tokens that rebase (negatively/positively) and overall, tokens with balance modifications outside of transfers.

Vulnerability Detail

The protocol wants to work with various ERC20 tokens, but still handles various transfers without querying the amount transferred and amount received, which can lead a host of accounting issues and the likes downstream.

For instance, In MasterchefV2.sol during withdrawals or particularly emergency withdrawals, the last user to withdraw all tokens will face issues as the amount registered to his name might be signifiacntly lesser than the token balance in the contract, which as a result will cause the withdrawal functions to fail. Or the protocol risks having to send extra funds from their pocket to coverup for these extra losses due to the fees.
This is because on deposit, the amount entered is deposited as is, without accounting for potential fees.

    function deposit(uint256 pid, uint256 amount) external override {
        _modify(pid, msg.sender, amount.toInt256(), false);

        if (amount > 0) _farms[pid].token.safeTransferFrom(msg.sender, address(this), amount);
    }

Some tokens like stETH have a 1 wei corner case in which during transfers the amount that actually gets sent is actually a bit less than what has been specified in the transaction.

Impact

On a QA severity level, tokens received by users will be less than emitted to them in the event.
On medium severity level, accounting issues, potential inability of last users to withdraw, potential loss of funds from tokens with airdrops, etc.

Code Snippet

https://github.com/sherlock-audit/2024-06-magicsea/blob/42e799446595c542eff9519353d3becc50cdba63/magicsea-staking/src/MasterchefV2.sol#L287

https://github.com/sherlock-audit/2024-06-magicsea/blob/42e799446595c542eff9519353d3becc50cdba63/magicsea-staking/src/MasterchefV2.sol#L298

https://github.com/sherlock-audit/2024-06-magicsea/blob/42e799446595c542eff9519353d3becc50cdba63/magicsea-staking/src/MasterchefV2.sol#L309

https://github.com/sherlock-audit/2024-06-magicsea/blob/42e799446595c542eff9519353d3becc50cdba63/magicsea-staking/src/MasterchefV2.sol#L334

https://github.com/sherlock-audit/2024-06-magicsea/blob/42e799446595c542eff9519353d3becc50cdba63/magicsea-staking/src/MlumStaking.sol#L559

https://github.com/sherlock-audit/2024-06-magicsea/blob/42e799446595c542eff9519353d3becc50cdba63/magicsea-staking/src/MlumStaking.sol#L649

https://github.com/sherlock-audit/2024-06-magicsea/blob/42e799446595c542eff9519353d3becc50cdba63/magicsea-staking/src/MlumStaking.sol#L744

https://github.com/sherlock-audit/2024-06-magicsea/blob/42e799446595c542eff9519353d3becc50cdba63/magicsea-staking/src/MlumStaking.sol#L747

https://github.com/sherlock-audit/2024-06-magicsea/blob/42e799446595c542eff9519353d3becc50cdba63/magicsea-staking/src/rewarders/BribeRewarder.sol#L120

Tool used

Manual Code Review

Recommendation

Recommend implementing a measure like that of the _transferSupportingFeeOnTransfer function that can correctly handle these transfers. A sweep function can also be created to help with positive rebases and airdrops.

[novaman33]

Escalate,
This grouping is too general. Issues have different root causes and also different severity.

[sherlock-admin3]

Escalate,
This grouping is too general. Issues have different root causes and also different severity.

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.

[0xSmartContract]

I guess you didn't see this comment before Escalade;

#545 (comment)

Grouping the category in this way allows auditors and sponsors to better understand the general problems caused by non-standard ERC20 tokens. This way, the report is presented within a consistent theme.

This perspective is resolutely maintained during the entire project audit, if we were to evaluate it in this way, issue number 545 would need to be divided into 33 separate parts.

[novaman33]

I do see your point. However, by placing all these issue under one dupe is unfair to watsons and also could deceive the developers. For example one of the dupes is that fee-on-transfer tokens will cause insolvency in the protocol. I saw some functions that track the balance delta between transfers so the developer gave it very high priority for them to be supported. And I see now they have placed a won't fix tag. I also do not agree that this exact issue about those tokens can be medium. Furthermore, I believe that such grouping creates a bad example for future contest. All those issues also have different fixes, which will make it more difficult for the developer to mitigate those issues as they would have to go through all the dupes. Once again I see your point and I do appreciate the good job you did in judging this contest.

[0xSmartContract]

These issues, grouped under the heading of “Weird ERC20 Tokens,” arise from different token behaviors that arise due to the flexibility of the ERC20 standard.

Addressing such issues separately may require a specific review of each function and contract. However, grouping under specific headings allows us to better understand the root of these issues and develop a general solution strategy. For example, the “fee-on-transfer” and “rebasing” issues can be resolved by dynamically monitoring and appropriately handling transfers and balance changes. This approach can be applied across the protocol and provides a more consistent solution.

The severity of these issues may vary depending on which functions of the protocol they affect and the potential impact of these functions on users. However, making a general grouping allows us to get to the root of these problems and develop a consistent solution strategy. This helps developers to handle such problems more easily and effectively.

If there were no weird token problems in the entire project (all contracts and all functions), it would be necessary to separate them, but here we are talking about all weird token problems in the entire project

I don't see any effective argument to change the duplicate in 545, my opinion is very clear

[jsmi0703]

Root cause of #545: non-supporting of fee-on-transfer or rebasing tokens.
Root cause of #5, #112, #188, #721: the mistake on supporting fee-on-transfer tokens at addToPosition().
If those(#5, #112, #188, #721) not exist, the protocol team couldn't fix the error at all, I think.
It is because the protocol team believes that addToPosition() already supports fee-on-transfer tokens.
Therefore if those not exist, the protocol team could fix fee-on-transfer token problems only in other functions not in addToPosition() function.
So, they should be grouped as another issue.

[0xSmartContract]

#545 (comment)

#545 (comment)

In the two detailed opinions above, I have technically detailed why it should be grouped this way

Your list is very weak, incomplete and based on few arguments. I expect you to analyze the entire list and present a detailed argument

Also, if we look at this issue from different effects and angles, we need to make 33 differens issues;

[novaman33]

Hello, I will not go through all the 80 different issues. I have stated my opinion that these duplications are too general and I still stand by it. I reviewed a few of the issue and it seems like this general approach has caused some of the duplicates to be overlooked. For example I was surprised to see issues like this one - #475 marked as valid dupes. I think that many issues here lack sufficient proof of concept and do not show a clear attack path. I will leave it up to the sherlock judge to decide.

[0xSmartContract]

https://gist.github.com/0xSmartContract/4e7e9fc500f7d6e6061d94ffde6c2206

General approaches for issue #545 and its duplicates do not lead us to a conclusion, if you share your analysis with a list like in the link above, it can be evaluated accordingly

There is a very detailed analysis above, this analysis - despite the listing and detail; "This grouping is too general" does not go beyond a small suggestion

[novaman33]

Hello, I will not review 80 issues to point out the obvious validations of insufficient reports and duplications with different root causes.If @WangSecurity thinks that my escalation is wrong or does not give enough information,then it can be rejected.

[jsmi0703]

@0xSmartContract
Please read and answer the above comment.

[WangSecurity]

@jsmi0703 I believe this comment from @0xSmartContract is the answer to your question.

About the issue and the escalation, I believe arguments from both side are completely valid, but this time I'll agree with the Lead Judge and will keep all the issues inside one family based on the following duplication rule:

If the following issues appear in multiple places, even in different contracts. In that case, they may be considered to have the same root cause:
Issues with the same conceptual mistake.

Example: different untrusted external admins can steal funds

Here, the conceptual mistake is Weird tokens. Hence, planning to reject the escalation and leave the issue as it is.

FYI, I'm asking for this decision to never be quoted or used as reference anywhere as it's an exceptional case and I believe the Lead Judge made a fair decision.

[WangSecurity]

Result:
Medium
Has duplicates

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• novaman33: rejected

[WangSecurity]

Based on #650 (comment) and #650 (comment) #545 will be duplicated with #237 with high severity.

[WangSecurity]

UPD: ignore the above comment, it's irrelevant. For details, take a look at the discussion under #237