c4-submissions commented 1 year ago

Lines of code

https://github.com/reserve-protocol/protocol/blob/9ee60f142f9f5c1fe8bc50eef915cf33124a534f/contracts/plugins/assets/erc20/RewardableERC20.sol#L86

Vulnerability details

Impact

The previously identified vulnerability of potential rounding issues during reward calculations has not been fully mitigated. The current strategy to keep remainders and use them in subsequent _claimAndSyncRewards() calls does not adequately address the issue when the rewardToken has a decimal smaller than 6 and/or the total reward tokens entailed is much smaller. This could lead to significant truncation losses as the remainder rolls over until it's large enough to overcome truncation, unfairly disadvantaging users, particularly those exiting the investment earlier, as they would miss out on a sizable amount of reward. This rounding issue, if left unresolved, can erode trust and potentially open up the system to arbitrage opportunities, further exacerbating the loss of rewards for regular users.

PoC (Proof of Concept)

Scenario

Let's assume rewardToken still has 6 decimals but there are only 0.5 million rewardToken to be distributed for the year and _claimAndSyncRewards() is called every minute. And, totalSupply = 10^6 with 18 decimals.

The expected rewards for 1 min are 500000 / 365 / 24 / 60 = 0.95 rewardToken = 950000 wei.

Initially, assume balanceAfterClaimingRewards = 1950000 (wei), and _previousBalance = 1000000 (wei), making delta = 950000 (wei).

deltaPerShare will be calculated as:

(950000 10^18) / (10^6 10^18) = 0

Now, balanceAfterClaimingRewards is updated to:

previous balance + (deltaPerShare totalSupply / one) = 1000000 + (0 (10^6 * 10^18) / 10^18) = 1000000 + 0 = 1000000 (wei)

As illustrated, the truncation issue causes deltaPerShare to equal 0. This will lead to a scenario where the rewards aren't distributed accurately among users, particularly affecting those who exit earlier before the remainder becomes large enough to surpass truncation.

In a high-frequency scenario where _claimAndSyncRewards is invoked often, users could miss out on a significant portion of rewards, showcasing the inadequacy of the proposed mitigation in handling the rounding loss effectively.

Mitigation

Using a bigger multiplier as the original report suggested seems viable, but finding a suitably discrete factor could be tricky.

While keeping the current change per PR #896, I suggest adding another step by normalizing both delta and _totalSupply to PRICE_DECIMALS, i.e. 18, that will greatly minimize the prolonged remainder rollover. The intended decimals may be obtained by undoing the normalization when needed. Here are the two useful functions (assuming decimals is between 1 to 18) that could help handle the issue but it will require further code refactoring on _claimAndSyncRewards() and _syncAccount().

    /// @dev    Convert decimals of the value to price decimals
    function _toPriceDecimals(uint128 _value, uint8 decimals, address liquidityPool)
        internal
        view
        returns (uint256 value)
    {
        if (PRICE_DECIMALS == decimals) return uint256(_value);
        value = uint256(_value) * 10 ** (PRICE_DECIMALS - decimals);
    }

    /// @dev    Convert decimals of the value from the price decimals back to the intended decimals
    function _fromPriceDecimals(uint256 _value, uint8 decimals, address liquidityPool)
        internal
        view
        returns (uint128 value)
    {
        if (PRICE_DECIMALS == decimals) return _toUint128(_value);
        value = _toUint128(_value / 10 ** (PRICE_DECIMALS - decimals));
    }

Assessed type

Decimal

5z1punch commented 1 year ago

deltaPerShare will be calculated as:

(950000 * 10^18) / (10^6 * 10^18) = 0

It's based on uint256 deltaPerShare = (delta * one) / _totalSupply; and uint256 delta = balanceAfterClaimingRewards - _previousBalance;.

But the delta is not always 950000. It will accumulate over time because the lastRewardBalance = balanceAfterClaimingRewards = _previousBalance + (deltaPerShare * _totalSupply) / one.

So the max loss will be less than 10^6 wei. Is my understanding correct? Have I missed anything?

bin2chen66 commented 1 year ago

My personal understanding is that this is an acceptable simple solution.

See mitigation description for details https://github.com/code-423n4/2023-09-reserve-mitigation-findings/issues/9

This is a simple way to fix Some rewards might be locked inside the contract.
But it's not fair to the user that the rewards should be allocated to the next time. The next totalSupply would be different.
A more reasonable approach would be to increase the precision, e.g. by using decimals = 27.
but the implementation requires more modifications

raymondfam commented 1 year ago

The rollover issue could be quite pronounced/prolonged if all of the factors below were to kick in together:

A much smaller numerator than anticipated due to:

e.g. 10k rewardToken to be distributed for the year
A much smaller decimal (1 - 5) associated with rewardToken

A much bigger denominator than anticipated due to a bigger totalSupply entailed say in tens of millions or even larger.

thereksfour commented 1 year ago

According to https://code4rena.notion.site/Guidelines-for-Code4rena-mitigation-reviews-ed10fc5cfbf640bd8dcec66f38b343c4, I'm inclined to consider this a new issue.

❓ What belongs in the 70% HM pool? The distinction between "mitigation not confirmed" and "mitigation errors and new issues" can be nuanced. Some suggestions from our judges as to what belongs in each category:

Mitigation not confirmed (30% pool): sponsor tried to apply a fix, but the same issue remains; mitigation is not sufficient to address the issue cited.

New issues and mitigation errors (70% pool): Mitigation created another High/Medium issue; any other High or Medium-risk issues discovered in the codebase.

The original issue causes rewards to be locked in contract, and the mitigation solves it, but introduces the new issue of rewards being incorrectly distributed.

The potential loss caused by this issue is related to the supply and price of deposit token and the decimals and price of the reward token.

In the extreme case where the deposit token is SHIB and the reward token is WBTC (8 decimals), the loss may be unacceptable (1 WBTC reward for per 700 USD SHIB loss).

Also #9 also points out this issue, I'll consider it as a duplicate of this one.

And please let me know if there are any disagreements.