samuraii77 - Incorrect require statement can cause user to be unable to claim

[sherlock-admin2]

samuraii77

high

Incorrect require statement can cause user to be unable to claim

Summary

Incorrect require statement can cause user to be unable to claim

Vulnerability Detail

The owner can call AdvancedDistributorInitializable#adjust() in order to adjust the amount that a user can claim:

    function adjust(address beneficiary, int256 amount) external onlyOwner {
        DistributionRecord memory distributionRecord = records[beneficiary];
        require(distributionRecord.initialized, "must initialize before adjusting");

        uint256 diff = uint256(amount > 0 ? amount : -amount);
        require(diff < type(uint120).max, "adjustment > max uint120");

        if (amount < 0) {
            // decreasing claimable tokens
            require(total >= diff, "decrease greater than distributor total");
            require(distributionRecord.total >= diff, "decrease greater than distributionRecord total");
            total -= diff;
            records[beneficiary].total -= uint120(diff);
            token.safeTransfer(owner(), diff);
        } else {
            // increasing claimable tokens
            total += diff;
            records[beneficiary].total += uint120(diff);
        }
        _reconcileVotingPower(beneficiary);
        emit Adjust(beneficiary, amount);
    }

The issue here is in this require statement:

      require(distributionRecord.total >= diff, "decrease greater than distributionRecord total");

The require statement should actually check for the unclaimed tokens and not the total tokens of a particular user.

Imagine the following scenario:

1. Bob and Alice have 100 tokens each that are already fully available to claim
2. The value of the total variable is equal to 200 tokens and the contract has the needed amount of tokens, 200 tokens
3. The owner decides to adjust the tokens Bob can claim to 0
4. He calls adjust() with the right input (address of Bob and -100 tokens) but Bob frontruns him and takes out his tokens
5. The function goes into the first if block:

   if (amount < 0) {
           // decreasing claimable tokens
           require(total >= diff, "decrease greater than distributor total");
           require(distributionRecord.total >= diff, "decrease greater than distributionRecord total");
           total -= diff;
           records[beneficiary].total -= uint120(diff);
           token.safeTransfer(owner(), diff);
       }

6. The first require statement passes as 200 is larger than 100
7. The second require statement passes as 100 is equal to 100
8. The value of the total variable is now 100 (200 - 100) and the total claimable tokens for Bob is 0 (100 - 100)
9. 100 tokens are transferred to the owner because of the last line

Now, the contract balance is 0 as Bob already took out his 100 tokens and the owner got the other 100 tokens because the require statement should have actually checked for the unclaimed tokens of Bob and reverted instead of checking for his total claimable tokens. Alice tries to withdraw her 100 tokens but since the balance of the contract is 0, she will not be able to.

Impact

Incorrect require statement can cause user to be unable to claim

Code Snippet

https://github.com/sherlock-audit/2024-05-tokensoft-distributor-contracts-update/blob/67edd899612619b0acefdcb0783ef7a8a75caeac/contracts/packages/hardhat/contracts/claim/factory/AdvancedDistributorInitializable.sol#L124-L145

Tool used

Manual Review

Recommendation

Make the following change:

- require(distributionRecord.total >= diff, "decrease greater than distributionRecord total");
+ require(distributionRecord.total - distributionRecord.claimed >= diff, "decrease greater than the unclaimed beneficiary tokens");

[samuraii77]

Escalate

Issue should be valid.

[sherlock-admin3]

Escalate

Issue should be valid.

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.

[Hash01011122]

Would be appreciated if you point out the reason on why you think this is valid @samuraii77

[samuraii77]

I think the report explains it very well, not sure what to add to it. It is a clear issue disallowing users to claim their rewards in the case explained above. If anything in particular is unclear, please let me know and I will answer.

[MxAxM]

In this explained scenario owner can return transferred amount to contract, so users can claim their tokens.

[samuraii77]

That's true and I am aware of this scenario but imagine what that would look like in a real scenario:

1. The issue explained in my report occurs
2. Alice tries to withdraw her tokens after waiting for them to vest for a long time (let's say a year, she might have even planned exactly what to do with that money as she has been expecting them for a while)
3. She tries to do that but the transaction reverts (it even costs her gas to do that, possibly very pricey knowing the gas prices that can be reached on L1)
4. She might try again and again as she is expecting it to work costing her even more gas
5. She contacts the owners/moderators about her issue
6. They start to investigate what happened
7. They notice the issue
8. They verify Alice actually should receive those tokens
9. They deposit those tokens (costing them gas as well)
10. Alice finally takes out her tokens

Being extremely conservative here, it is very unlikely that all of this will be fixed in less than a day. It might take a few days and especially if that was during the weekend, it will take even longer. Keep in mind that this can then occur for every single time someone takes out his tokens, that will create a lot of issues and disputes.

Also, imagine the case explained in the parenthesis in step 2 occurs. She has waited for her tranche to unlock for over a year and has timed an important purchase the day of the unlock, possibly something extremely important as an apartment purchase. The funds will unlock on Friday and she will immediately claim them and withdraw them to her bank in order to make that important purchase scheduled for Friday. The issue explained in my report occurs and now she can't do it. As explained above, it will probably take at least until Monday until the issue is taken care of and she can take out her tokens. As that was a super important purchase she timed particularly for that day, she is in an extremely bad situation and might not even be able to make her super important purchase.

At the very least, this issue constitutes to be of Medium severity, the impact is clearly there and the likelihood is extremely high.

[WangSecurity]

Firstly, if what you describe above is the case, then I believe it's DOS of Alice, but it fairly can be resolved in less than 1 week and I'm not sure it's a time-sensitive function (in general, not only for one user). Secondly, wouldn't this line revert in that case records[beneficiary].total -= uint120(diff);?

[jkoppel]

I reported the root cause of this as a Low. See github.com/sherlock-audit/2024-05-tokensoft-distributor-contracts-update-judging/issues/68, #3

I'll be a little more brief than usual because I'm about to go to bed, but, basically: this scenario boils down to "if the contract is underfunded, then the contract is underfunded, and people can't withdraw funds until it becomes more funded." Which seems, like, not an issue. The protocol is very intentionally designed to allow the admin to adjust a user's total below their already claimed amount.

[samuraii77]

@WangSecurity it doesn't revert, the issue explained is possible. If I am not mistaken due to not looking at the contract code right now, this does not revert as total never changes, just the claimed amount changes and from that, the contract computes the amount claimable.

Also, I believe that since the explained scenario in my last comment with Alice making an important purchase is actually possible, then this can be considered a time sensitive function. There is no mathematics we can do here to decide what % of people would need the money immediately but we can safely assume that some will, thus the function can be considered time sensitive.

Also, about the locking of funds, the DoS period is unknown. It can be a year for all we know, it lasts until someone sends funds into the contract.

[samuraii77]

And to answer jkoppel, with all due respect, I believe you did not notice this exact issue. Judging from your report, you use the word somehow implying that you don't actually know how that could happen (unless not funded in the first place which is a separate issue I have submitted). I believe that you would have submitted this issue as a Medium at the very least if you noticed that issue as that is clearly problematic.

[jkoppel]

Hi samuraii77! Nope! I use the word somehow because it's not a significant issue even if it does happen. If something else makes the contract insolvent, like just plain underfunding it, then that something else is the root cause. And I don't think this scenario is problematic at all. If the owner takes out more funds than are owed to users, they'll have to put some of those funds back in.

[WangSecurity]

Firstly, I agree that this function can time-sensitive for specific users in specific situations. But, I don't see it as time-sensitive in general (unlike managing trading positions, for example).

Secondly, Let's look at a couple of code comments:

1. The code comment for adjust function:

 * @dev Adjust the quantity claimable by a user, overriding the value in the distribution record.

2. Code comments for struct DistributionRecord:

   uint120 total; // total token quantity claimable uint120 claimed; // token quantity already claimed

So, as we see the function adjust has to change the quantity claimable by a user which is the definition of total. Hence, the function is doing what it is expected to.

But, still the situation in this report may still happen. But the admin can simply refund the contract with the user funds. I believe this issue is about DOSing the user (Alice in the example), but as I've explained at the beginning, it's not a time-sensitive function, and can be solved in less than 7 days.

Hence, planning to reject the escalation and invalidate the report.

[WangSecurity]

Result: Invalid Unique

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• samuraii77: rejected