Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wrong logic in _updateForAfterDistribution can lead to massive overdistribution of rewards #26

Open
hats-bug-reporter bot opened this issue Feb 27, 2024 · 2 comments
Labels
bug Something isn't working invalid This doesn't seem right

Comments

@hats-bug-reporter
Copy link

Github username: @deadrosesxyz
Twitter username: @deadrosesxyz
Submission hash (on-chain): 0x33ffd4191b604c94bac6b7574d66a833c80587ca8ef5e120a64cccf974d952e2
Severity: high

Description:
Description
Wrong logic in _updateForAfterDistribution can lead to massive overdistribution of rewards

Attack Scenario
Let's look at how _updateForAfterDistribution works.

    function _updateForAfterDistribution(address _gauge) private {
        address _pool = poolForGauge[_gauge];
        uint256 _time = _epochTimestamp() - 604800;
        uint256 _supplied = weightsPerEpoch[_time][_pool];

        if (_supplied > 0) {
            uint256 _supplyIndex = supplyIndex[_gauge];
            uint256 _index = index; // get global index0 for accumulated distro
            supplyIndex[_gauge] = _index; // update _gauge current position to global position
            uint256 _delta = _index - _supplyIndex; // see if there is any difference that need to be accrued
            if (_delta > 0) {
                uint256 _share = (_supplied * _delta) / 1e18; // add accrued difference for each supplied token
                if (isAlive[_gauge]) {
                    claimable[_gauge] += _share;
                }
            }
        } else {
            supplyIndex[_gauge] = index; // new users are set to the default global state
        }
    }

The way it works is the following:

  1. It gets the gauge's balance in the last epoch
  2. It gets the index delta
  3. It multiplies the gauge's balance in the last epoch by the delta.

However, this becomes problematic if the gauge has not been updated consistently and has not been updated for a whole epoch for example.

Let's look at a simple example. (assume 1e18 of tokens is distributed per epoch for simplicity).

  1. First epoch, index is 0. There are 2 gauges. One of the gauges ends the epoch with weight 1e18 and the other one with 0 (total weight is 1e18)
  2. New epoch rolls. 1e18 tokens have been distributed, index is now 1e18. The gauge which had weight is updated and now has claimable = 1e18. The other gauge is not updated (important).
  3. Now the 2nd gauge receives 1e18 weight. Total weight is now 2e18
  4. The next epoch rolls and 1e18 is distributed. Index increases by (1e18 * 1e18) / 2e18 = 0.5e18. Index is now 1.5e18
  5. Now after the first gauge's updated, it will be allocated the 0.5e18 tokens as supposed to.
  6. However, when updating the 2nd gauge, since it hasn't been updated since the beginning, the delta will be 1.5e18. The gauge will be allocated 1e18 * 1.5e18 / 1e18 = 1.5e18 tokens for the 2nd round.

The 2nd gauge unfairly received more tokens than it should. Note that this is highly scalable.
This can also work in the opposite direction if a gauge has previously had bigger parts of the pot and is updated after having little weight in an epoch.

This would become even more problematic after reviving a gauge which has been inactive for long period of time (e.g. reviving after 1 year of being killed) as the gauge will get rewards for all the time that has passed.

Attachments

  1. Proof of Concept (PoC) File
  1. Revised Code File (Optional)
@hats-bug-reporter hats-bug-reporter bot added the bug Something isn't working label Feb 27, 2024
@deadrosesxyz
Copy link

note: it can be checked how Velodrome fixes this problem by using their equivalent function any time the user votes/ resets for certain gauge

@BohdanHrytsak
Copy link
Collaborator

Thank you for the submission.

The view refers to the part of the code that is in the OOS. Therefore, to accept this problem as valid, it must be high.

Your submission says that if the index for gauge is not updated for a certain epoch, then the next epoch when it is updated, the delta will be too high, which will create a reward overdistribution. But this situation can only happen if the gauge has explicitly forgotten the distribution calls. https://github.com/Satsyxbt/Fenix/blob/7b81d318fd9ef6107a528b6bd49bb9383e1b52ab/contracts/core/VoterUpgradeable.sol#L735

Although this situation is unlikely since any user can do this, and the owners will also ensure the correctness of the work. What reduces the seriousness of it to a medium

since in case of absence of votes, it will be executed:

} else {
            supplyIndex[_gauge] = index; // new users are set to the default global state
        }

and update the index to the correct value

Since this case is possible only if an update to this gauge was not explicitly triggered, although this means that the protocol administration made a mistake, it reduces the severity to Medium, OOS

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working invalid This doesn't seem right
Projects
None yet
Development

No branches or pull requests

2 participants