search

sherlock-audit / 2024-06-magicsea-judging

8 stars 5 forks source link

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

Open sherlock-admin3 opened 4 months ago

sherlock-admin3 commented 4 months ago

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.

0xSmartContract commented 4 months ago

Upon thorough review of the findings categorized under the "Weird Token Issue" umbrella, it is evident that these issues root from the non-standard behaviors exhibited by certain ERC20 tokens.

These tokens, referred to as "weird tokens," include fee-on-transfer tokens, rebasing tokens (both positive and negative), tokens with low or high decimals, and tokens with other non-standard characteristics.

Here are the technical and practical reasons why this grouping is justified:

According to Sherlock rules, the "Weird Token" List is unique and all of these issues are in this single list and are not separated.

https://docs.sherlock.xyz/audits/judging/judging#vii.-list-of-issue-categories-that-are-not-considered-valid

Non-Standard tokens: Issues related to tokens with non-standard behaviors, such as [weird-tokens] (https://github.com/d-xo/weird-erc20) are not considered valid by default unless these tokens are explicitly mentioned in the README.

Unified Theme and Clarity:By categorizing these issues together, the report maintains a coherent theme, making it easier for auditers and Sponsor to understand the underlying problem of dealing with unconventional ERC20 tokens.

Specification Looseness: The ERC20 specification is known to be loosely defined, leading to diverse implementations. Many tokens deviate from standard behaviors, causing unexpected issues when integrated into smart contracts.

Common Problems: Issues like fee-on-transfer, rebasing (both positive and negative), low/high decimals, and missing return values are common among these tokens, causing similar types of problems across different functions and contracts.

Here are key issues grouped under the "Weird ERC20 Tokens" category:

novaman33 commented 3 months ago

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

sherlock-admin3 commented 3 months ago

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 commented 3 months ago

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

https://github.com/sherlock-audit/2024-06-magicsea-judging/issues/545#issuecomment-2251178050

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

https://github.com/sherlock-audit/2024-06-magicsea-judging/issues/545#issuecomment-2251178050

https://github.com/sherlock-audit/2024-06-magicsea-judging/issues/545#issuecomment-2266711126

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;

image
novaman33 commented 3 months ago

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 - https://github.com/sherlock-audit/2024-06-magicsea-judging/issues/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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

@0xSmartContract Please read and answer the above comment.

WangSecurity commented 3 months ago

@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 commented 3 months ago

Result: Medium Has duplicates

sherlock-admin4 commented 3 months ago

Escalations have been resolved successfully!

Escalation status:

WangSecurity commented 3 months ago

Based on https://github.com/sherlock-audit/2024-06-magicsea-judging/issues/650#issuecomment-2294043026 and https://github.com/sherlock-audit/2024-06-magicsea-judging/issues/650#issuecomment-2290595068 #545 will be duplicated with #237 with high severity.

WangSecurity commented 3 months ago

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