AkshaySrivastav - `PriceTierVesting.getVestedFraction` returns 0 when the oracle price is equal to the price of first tier

[sherlock-admin2]

AkshaySrivastav

high

PriceTierVesting.getVestedFraction returns 0 when the oracle price is equal to the price of first tier

Summary

PriceTierVesting.getVestedFraction function incorrectly returns 0 as the vested fraction when the reported oracle price is less than or equal to the price of first PriceTier.

Vulnerability Detail

As per the current implementation of getVestedFraction function

        uint256 price = _getOraclePrice();

        for (uint256 i = tiers.length; i != 0; ) {
            unchecked {
                --i;
            }
            if (price > tiers[i].price) {
                return tiers[i].vestedFraction;
            }
        }
        return 0;

For a PriceTier array, when the oracle price is less than or equal to the price of a tier n, the function returns the fraction of tier n - 1 or 0 if the loop is exhausted. This mechanism behaves incorrectly for the first element of PriceTier array.

Consider this scenario

• Price tiers are set as [{100, 2500}, {110, 5000}, {120, 10000}].
• When the oracle price is reported as 100, even though the config has 2500 as its respective fraction, the function returns 0.

Essentially the user cannot claim any tokens if the oracle price becomes 100 (or goes below that).

This implementation of getVestedFraction seems to be copy-pasted from TrancheVesting contract. The implementation fits correctly with the time based vesting logic of TrancheVesting but breaks the price based vesting of PriceTierVesting.

Impact

When the oracle price becomes equal to the price of first tier or goes below that (which is a very likely scenario), the vested fraction of a user is considered as 0.

Most notable when the oracle price is equal to price of first tier, even though the config has a fraction value for that price, the function returns 0 preventing users from claiming any tokens.

Code Snippet

https://github.com/sherlock-audit/2023-06-tokensoft/blob/main/contracts/contracts/claim/abstract/PriceTierVesting.sol#L84-L94

Tool used

Manual Review

Recommendation

Consider changing the > operator to >= to make the tiers[i].price inclusive in the tier range.

            if (price >= tiers[i].price) {
                return tiers[i].vestedFraction;
            }

[akshaysrivastav]

Hey, before submitting this issue I confirmed the bug from sponsors (with discord username: layne7534).

In the attached screenshot the sponsor confirms that the reported behaviour is not intended. I hope the judges can review this.

This report demonstrates that when the oracle price is equal to price of first tier, even though the config has a fraction value for that price, the function returns 0 preventing users from claiming any tokens. The ideal behaviour should be to return the admin configured fraction for that respective price tier.

I am aware that discord chat is not considered as source of truth in sherlock. I believe this issue somehow got slipped during the judging phase.

[akshaysrivastav]

Hey @cr-walker @LayneHaber can you please share your thoughts on this bug report and my comment.

[Oot2k]

Escalate Seems like a valid issue based on comment above. Referring to: "This report demonstrates that when the oracle price is equal to price of first tier, even though the config has a fraction value for that price, the function returns 0 preventing users from claiming any tokens. The ideal behavior should be to return the admin configured fraction for that respective price tier."

[sherlock-admin2]

Escalate Seems like a valid issue based on comment above. Referring to: "This report demonstrates that when the oracle price is equal to price of first tier, even though the config has a fraction value for that price, the function returns 0 preventing users from claiming any tokens. The ideal behavior should be to return the admin configured fraction for that respective price tier."

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.

[Shogoki]

Seems like a valid bug to me. However it affects users only when the returned price is exactly at the first price tier. Can be a valid medium, i think.

[akshaysrivastav]

More precisely, it affects users when the returned price is less than or equal to the price of first tier.

[Shogoki]

More precisely, it affects users when the returned price is less than or equal to the price of first tier.

That is right, but if it is less than the price it is intended behaviour to return 0, isn`t it?

[akshaysrivastav]

That is right, but if it is less than the price it is intended behaviour to return 0, isn`t it?

Likely, I can't find any docs around this. So maybe sponsors can confirm.

Also, simply returning 0 when price goes below the first tier does not seem ideal, and looks like an incomplete implementation.

Comment from my report:

This implementation of getVestedFraction seems to be copy-pasted from TrancheVesting contract. The implementation fits correctly with the time based vesting logic of TrancheVesting but breaks the price based vesting of PriceTierVesting.

[jkoppel]

What are the consequences of this that makes it medium? There's an infinitesimal band returned by the oracle in which this applies, and then the consequence is that the user needs to wait for the price to move. Which means they need to wait at most 24 hours for the price to update and budge by under 0.000000000001%. Most Chainlink data feeds have 18 decimals of precision.

In comparison, #10 was deemed too unlikely to count, when it only requires two airdrops for the same number of tokens, but has the consequences of one user stealing all of another user's tokens. This issue requires a volume-weighted average of prices over many different exchanges to hit an exact value to 18 decimals, and even then it only delays claim by at most 24 hours.

Like, we talk about defenses against price manipulation. But have there been any cases of someone manipulating a price not just to be higher or lower, but to be an exact value to 18 decimals?

[akshaysrivastav]

This issue is more about the incorrect implementation of the getVestedFraction function.

Returning 0 value when price is equal to the price of first tier is the most extreme/impactful situation. The bug works similarly for all tiers. For more context let's see this example:

The admin sets price tiers as [{100, 2500}, {110, 5000}, {120, 10000}]. Here, when

• price > 120, function returns 10000.
• price = 120, function returns 5000.
• 110 < price < 120, function returns 5000.
• price = 110, function returns 2500.
• 100 < price < 110, function returns 2500.
• price = 100, function returns 0.
• price < 100, function returns 0.

It can be seen that whenever the price is equal to price of tier n, the function returns the vested fraction of tier n - 1, which is incorrect and is the core reported bug.

Hence the report demonstrates an incorrect code implementation which leads to incorrect vested fraction calculation for users. The calculated fraction does not strictly honour the tier config set by admin.

[cr-walker]

As @jkoppel mentions, this will never be a real world issue given the noise in Chainlink price feeds. No plans to change the behavior.

[cr-walker]

But I am fine with categorizing this as a low severity bug since I understand that technically a user might be surprised by this behavior (if they somehow encountered it).

[akshaysrivastav]

So would you consider returning the vested fraction of tier n - 1 when the price is equal to price of tier n as the intended implementation? Due to which the vested fractions of users gets calculated incorrectly (as explained in scenario).

I agree that exact equality of oracle price and tier price will happen less frequently but that's why the issue was reported as medium (acc to the sherlock rules). Because whenever this in-frequent event occurs it impacts the vested fraction and claim amounts of users.

[hrishibhat]

Result: Low Unique This is not a valid medium. This seems to be an off-by-one error, while technically this may not be the intended design, given that in the unlikely event, as raised by the Sponsor this will never be a real-world issue given the noise in Chainlink price feeds. and given the points raised in this comment, especially about the decimals and the impact: https://github.com/sherlock-audit/2023-06-tokensoft-judging/issues/150#issuecomment-1673734795 don't see the impact as severe enough to consider it a medium

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• Oot2k: rejected