scammed - Tokens with high decimals will DoS the BribeRewarder contract

[sherlock-admin4]

scammed

Medium

Tokens with high decimals will DoS the BribeRewarder contract

Summary

In case tokens with high decimals or cheap tokens are used in BribeRewarder rewarder updates will fail most of the time, rendering the contract unable to distribute rewards to anyone.

Vulnerability Detail

Token with 24 [decimals](https://github.com/d-xo/weird-erc20?tab=readme-ov-file#high-decimals) used in BribeRewarder will DoS the voting mechanism and reward distribution for the pools and period for which are registered:

  function onRegister() external override {
      IBribeRewarder rewarder = IBribeRewarder(msg.sender);

      _checkRegisterCaller(rewarder);

      uint256 currentPeriodId = _currentVotingPeriodId;
      (address pool, uint256[] memory periods) = rewarder.getBribePeriods();
      for (uint256 i = 0; i < periods.length; ++i) {

          require(periods[i] >= currentPeriodId, "wrong period");
          require(_bribesPerPriod[periods[i]][pool].length + 1 <= Constants.MAX_BRIBES_PER_POOL, "too much bribes");
          _bribesPerPriod[periods[i]][pool].push(rewarder);
      }
  }

The issue lies in the way that the debtPerShare is calculated in the Rewarder library:

function getDebtPerShare(uint256 totalDeposit, uint256 totalRewards) internal pure returns (uint256) {
      return totalDeposit == 0 ? 0 : (totalRewards << Constants.ACC_PRECISION_BITS) / totalDeposit;
  }

Due to the bitwise operation performed if totalRewards are 10000e24 for the current period (low priced token) all transactions invoking the _modify function will revert and will effectively brick the voting for this pool.

Impact

BribeRewarder with high decimal token or cheap token with 18 decimals will brick the voting for this pool also disabling the claiming.

Code Snippet

https://github.com/sherlock-audit/2024-06-magicsea/blob/main/magicsea-staking/src/libraries/Rewarder.sol#L37-L39

Tool used

Manual Review

Recommendation

Use different math to handle tokens with big decimals.

[YordanVuchev]

Escalate on @Slavchew's behalf

This issue is a duplicate of https://github.com/sherlock-audit/2024-06-magicsea-judging/issues/237. Based on the duplication rules of Sherlock issue must cover 4 points in order to be assigned as duplicate:

Identify the root cause Both issues identify the root cause that the bitwise operations with high numbers of tokens will cause reverts and block the voting for the given pools, they both identifygetDebtPerShare which is the function where the DOS is caused:

237:

When that period starts, the attacker first uses 1 wei to vote for the selected pool, which will initialize the value of accDebtPerShare to a huge value.

When other legitimate users go to vote to that same pool, the transaction will revert due to overflow.

In step 4, the attacker votes using 1 wei to initialize accDebtPerShare to a huge value, which will happen here:

650:

Due to the bitwise operation performed if totalRewards are 10000e24 for the current period (low-priced token) all transactions invoking the _modify function will revert (due to overflow) and will effectively brick the voting for this pool.

Identify at least a Medium impact

Both issues cover the scenario where voting for these pools will be blocked due to the overflow caused by the operations performed (#237: custom token with no value that increases debt per share, #650: cheap token that increases debt per share)

Identify a valid attack path or vulnerability path

Both issues have valid attack paths, while #237 performs the attack by first minting uint256.max and then sending 1 token, #650 uses the high number of total rewards and shows that any amount added to this number will lead to overflow that will also block the voting.

Fulfills other submission quality requirements (e.g. provides a PoC for categories that require one)

Note that point 4 is only for specific issues and this one is not from this category - https://docs.sherlock.xyz/audits/judging/judging#vi.-requirements

[sherlock-admin3]

Escalate on @Slavchew's behalf

This issue is a duplicate of https://github.com/sherlock-audit/2024-06-magicsea-judging/issues/237. Based on the duplication rules of Sherlock issue must cover 4 points in order to be assigned as duplicate:

Identify the root cause Both issues identify the root cause that the bitwise operations with high numbers of tokens will cause reverts and block the voting for the given pools, they both identifygetDebtPerShare which is the function where the DOS is caused:

237:

When that period starts, the attacker first uses 1 wei to vote for the selected pool, which will initialize the value of accDebtPerShare to a huge value.

When other legitimate users go to vote to that same pool, the transaction will revert due to overflow.

In step 4, the attacker votes using 1 wei to initialize accDebtPerShare to a huge value, which will happen here:

650:

Due to the bitwise operation performed if totalRewards are 10000e24 for the current period (low-priced token) all transactions invoking the _modify function will revert (due to overflow) and will effectively brick the voting for this pool.

Identify at least a Medium impact

Both issues cover the scenario where voting for these pools will be blocked due to the overflow caused by the operations performed (#237: custom token with no value that increases debt per share, #650: cheap token that increases debt per share)

Identify a valid attack path or vulnerability path

Both issues have valid attack paths, while #237 performs the attack by first minting uint256.max and then sending 1 token, #650 uses the high number of total rewards and shows that any amount added to this number will lead to overflow that will also block the voting.

Fulfills other submission quality requirements (e.g. provides a PoC for categories that require one)

Note that point 4 is only for specific issues and this one is not from this category - https://docs.sherlock.xyz/audits/judging/judging#vi.-requirements

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.

[chinmay-farkya]

I think that #237 and #650 should both be clubbed with the combined issue of weird erc20 tokens ie. #545.

All have the same root cause of allowing weird tokens and the same fix of either documenting that non-standard tokens are not allowed or implementing a whitelist of allowed tokens.

From sherlock's rules for Root cause grouping :

Issues with the same conceptual mistake. Example: different untrusted external admins can steal funds.

Similarly, this is a single mistake of allowing random tokens which gives rise to many many different impacts.

[0xSmartContract]

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.

You are overlooking the fact that the entire project has a Weird problem. These are listed in the "Weird Token" list in Sherlock Criteria, and if we classify them one by one, there will be more than 545 copies and will not contribute to the audit.

My opinion is clear here

[0xSmartContract]

I think that #237 and #650 should both be clubbed with the combined issue of weird erc20 tokens ie. #545.

All have the same root cause of allowing weird tokens and the same fix of either documenting that non-standard tokens are not allowed or implementing a whitelist of allowed tokens.

From sherlock's rules for Root cause grouping :

Issues with the same conceptual mistake. Example: different untrusted external admins can steal funds.

Similarly, this is a single mistake of allowing random tokens which gives rise to many many different impacts.

There is already an escalade in #237 and #657,

It is not right to evaluate them together with #545, I think they should be evaluated separately

Note: In duplicates, you should go with a single perspective in the project, if you go very micro in some issues and make duplicates in the big picture in some issues, there will be no reliability

In this project, Duplicate went with a single perspective, if we change this, 25 issues will be affected, which we will not do

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

[WangSecurity]

Issue #545 is escalated to re-consider duplicates, so more information on this escalation will be provided when the new families are decided.

[WangSecurity]

The decision on #545 is to keep all the issues as duplicates based on the following:

Root cause groupings 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.

Hence, this issue will also remain a duplicate of #545 since it involves weird tokens. It won't be a duplicate of #237, since the latter can happen with a standard token (I believe almost 0 price is not a weird trait). Hence, planning to reject the escalation and leave the issue as it is.

[chinmay-farkya]

Hey @WangSecurity I'd like to ask you why do you think so :

since #237 can happen with a standard token

The description of #237 says this :

"An attacker creates a custom token and mints the maximum amount of tokens" : This statement means that the attacker needs to completely control the token, deploy it themselves and use it to mint unlimited amounts.

"The attacker transfers the total supply of the token to the rewarder contracts and sets it up to distribute the whole supply only in one period to a specific pool." : For this to happen the attacker also needs to hold control over the complete totalsupply of that token.

I don't think this qualifies to be called a standard token. Both these statements are requirements for the bug to manifest, and directly point to a user-controlled token which comes under the umbrella of "weird tokens" coz there are many possible permutations of this kind of token.

IMO a "standard" token is USDC which is a normal market traded token. A random token deployed by attacker for the attack, who can mint, transfer whole totalsupply at their will cannot be called a "standard" token.

Hence, I'd argue that #237 is also a duplicate of #545.

[Slavchew]

The decision on #545 is to keep all the issues as duplicates based on the following:

Root cause groupings 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.

Hence, this issue will also remain a duplicate of #545 since it involves weird tokens. It won't be a duplicate of #237, since the latter can happen with a standard token (I believe almost 0 price is not a weird trait). Hence, planning to reject the escalation and leave the issue as it is.

@WangSecurity #237 talks about a user being able to create a token and mint a lot of it to create an overflow, this report talks about the same problem in the same line of code, just described differently. The selected shows PoC with 18 decimal token which is cheap and any user can get very large amount to create overflow and this report talks about the same - BribeRewarder with high decimal token or cheap 18 decimal token will block voting for this pool also disable request.

We never limited the issue to high decimal tokens only, we just explain it like this because they add 5-6 more numbers but aren't the only ones, I know it's not the best but it was written in the last 10-15 minutes of the contest, but stand by our words, it shows the same issue! - Either if you create a token and mint a very large amount, or go buy an extremely cheap token, it's the same. The general problem can be created with a very large token balance, no matter how the attacker gets it, indeed creating one yourself is cheaper than buying one, but there are many tokens where $1 can buy billions of tokens.

So I think this is #237 in a nutshell - an extremely large token balance and indicating where and how to exploit it, so this report is a valid duplicate. There is no such thing as partial in Sherlock, but I also agree that #237 is better explained.

And I also want to mention that the above comment from @chinmay-farkya isn't for here - https://github.com/sherlock-audit/2024-06-magicsea-judging/issues/650#issuecomment-2286822657. The discussion here is about this issue and that it is a duplicate of #237. If anyone thinks #237 should be treated differently, they should comment under #237 not here.

[WangSecurity]

"An attacker creates a custom token and mints the maximum amount of tokens" : This statement means that the attacker needs to completely control the token, deploy it themselves and use it to mint unlimited amounts. "The attacker transfers the total supply of the token to the rewarder contracts and sets it up to distribute the whole supply only in one period to a specific pool." : For this to happen the attacker also needs to hold control over the complete totalsupply of that token. I don't think this qualifies to be called a standard token. Both these statements are requirements for the bug to manifest, and directly point to a user-controlled token which comes under the umbrella of "weird tokens" coz there are many possible permutations of this kind of token. IMO a "standard" token is USDC which is a normal market traded token. A random token deployed by attacker for the attack, who can mint, transfer whole totalsupply at their will cannot be called a "standard" token.

@chinmay-farkya fair point, but the weird tokens only are the ones that have weird traits mentioned here as linked in the rules. The user controlling the token is not a weird trait. That's why I don't see #237 as a duplicate of #545, hope you see my point.

talks about a user being able to create a token and mint a lot of it to create an overflow, this report talks about the same problem in the same line of code, just described differently. The selected shows PoC with 18 decimal token which is cheap and any user can get very large amount to create overflow and this report talks about the same - BribeRewarder with high decimal token or cheap 18 decimal token will block voting for this pool also disable request. We never limited the issue to high decimal tokens only, we just explain it like this because they add 5-6 more numbers but aren't the only ones, I know it's not the best but it was written in the last 10-15 minutes of the contest, but stand by our words, it shows the same issue! - Either if you create a token and mint a very large amount, or go buy an extremely cheap token, it's the same. The general problem can be created with a very large token balance, no matter how the attacker gets it, indeed creating one yourself is cheaper than buying one, but there are many tokens where $1 can buy billions of tokens

@Slavchew Yeah, I see your point and I indeed overlooked it. The report does mention standard but low-price tokens and the revert in the very same line. In that case, I would agree it's fair to duplicate with #237 because this report identified the very line where the issue happens and indeed mentions standard tokens that can lead to such behaviour. Excuse me for misunderstadingin here.

Planning to accept the escalation and duplicate with #237

[chinmay-farkya]

@WangSecurity I see your point that this is not specifically listed in the weird tokens, but I still believe a custom token controlled by user falls into the same category as it can be anything (with or without any of the listed traits).

The root cause of the complete family of issues is that random tokens are allowed to be used, and the weird-tokens repository can not be a full representation of all kinds of custom tokens existing in the forest.

Even if a custom token without any special traits can be used maliciously, it does fall into the root cause of "allowing custom tokens to be linked to the briberewarder".

Moreover, the recommendation in #237 is :

To mitigate this issue is recommended to create a whitelist of tokens that limits the tokens that can be used as rewards for bribing.

which is the same as all other duplicates of #545 (which itself contains a lot of different categories). So I think #237 and this #650 both have the same root cause and fix as the family of #545, then why separate this one and group all others ?

[Slavchew]

First of all I by you are still writing under that issue since the discussion here is only for duplication. And second it is not a weirdness of token to be cheap at price and an attacker to be able to get a big amount of it.

@WangSecurity please do not allow this kind of conversation under issues that aren't the main one.

[midori-fuse]

This issue isn't even correct. Bit shifting in Solidity is non-arithmetic, and does not revert on overflow.

Run this code in Remix and you'll see. This left-shifts an integer 300 times, but the tx still goes through.

// SPDX-License-Identifier: GPL-3.0

pragma solidity >=0.8.2 <0.9.0;

contract Test {
    function testOverflow() public pure {
        uint256 val = 1;
        for (uint i = 0; i < 300; i++) {
            val = (val << 1);    
        }
    }
}

Here's the coded PoC for the described scenario in this submission. Run with forge test --match-test testDepositOverflow -vv in BribeRewarder.t.sol

function testDepositOverflow() public {
    ERC20Mock(address(rewardToken)).mint(address(this), type(uint256).max);
    ERC20Mock(address(rewardToken)).approve(address(rewarder), type(uint256).max);

    uint maxamt = 10000 * (10 ** 24);
    rewarder.fundAndBribe(1, 1, maxamt);

    _voterMock.setCurrentPeriod(1);
    _voterMock.setStartAndEndTime(block.timestamp, block.timestamp + 1000);
    (uint256 startTime, uint256 endTime) = _voterMock.getPeriodStartEndtime(1);

    skip(999);
    vm.prank(address(_voterMock));
    rewarder.deposit(1, 1, 1e18);
}

You can change maxamt to even type(uint256).max and it still doesn't revert. You can also change skip(999) to skip(anything) or even delete it. Sure it overflows, but where is the DoS? Where is the bricking of voting?

The issue #237 relies on L28's multiplication, which indeed reverts on overflow. However this submission identified "due to the bitwise operation performed", which is not the cause of overflow. In other words this submission never even correctly identified the overflow.

The attack path here is also incorrect. The other issue relies on (1) bribing a large count of tokens, then (2) the attacker has to vote 1 wei to cause accDebtPerShare to be super large first, and then subsequent votes overflow. The only attack this submission mentioned was just creating a large token to cause overflow in the bit shifting, which was proven incorrect up over the PoC I provided.

This submission's root cause claim was false, thus invalidating the attack path and the impact altogether. By the rules this cannot be a dupe.

[Slavchew]

As I already mentioned, the problem is not described well, I know that. But it shows the same problem where and with what type of tokens it can be achieved, it also doesn't say that the function will revert on the bitwise operation, but that the bitwise operation will cause a big number which then causes _modify() to revert, which is true.

The report is kind of partial, but since Sherlock does not have that idea, it can't be marked as partial. Let @WangSecurity solve it, but the problem is still the same, that's why there are selected problems and duplicates, not all should be exactly the same.

[midori-fuse]

Yes, I'm just setting the facts straight and duplication rules, nothing much. This report is indeed missing a lot of information (for example, the root cause of which calculation/value overflows, and the attack path of how the overflow is triggered).

[WangSecurity]

The question is why I allow for 2 conversations here. It's simple, both started here and I don't think it would be comfortable to move it anywhere now. Also, I believe both questions are connected:

1. This issue wants to be a duplicate of #237.
2. The second question is to duplicate #237 with #545.

Hence, answering @chinmay-farkya may resolve the other question as well!

Answering @chinmay-farkya concern. I actually see where it comes from and why it's completely viable. Indeed, the current conceptual mistake of #545 is that it's about weird tokens, but it can be clubbed with #237 as the conceptual mistake of allowing any token to be used for Bribe Rewards. As correctly noted, indeed both this and #545 and #237 can be fixed by implementing a whitelist for example (I understand there are other viable fixes).

About @midori-fuse concern of this issue not being a sufficient duplicate. The report says Due to the bitwise operation performed if totalRewards are 10000e24 for the current period (low priced token) all transactions invoking the _modify function will revert and will effectively brick the voting for this pool. This is partially correct, indeed the bitwise operation makes the value very large, but it's not the very reason why the revert happens, as noted by @midori-fuse. And as shown in #237 the revert happens here. The bitwise operation helps the revert happen, but it's not the root cause of it I believe.

Hence, I would agree with @chinmay-farkya here and plan to make #237 the main report of that family, the family will have high severity, since I believe #237 is high severity. But, I also agree with @midori-fuse that this report is not sufficient since it doesn't correctly identify the root cause. Hence, the escalation will be rejected.

[WangSecurity]

Result: Invalid Unique

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• DPS: rejected