sherlock-audit / 2024-06-magicsea-judging

8 stars 5 forks source link

HonorLt - Dishonest reward calculation when lum rate changes #631

Closed sherlock-admin2 closed 3 months ago

sherlock-admin2 commented 3 months ago

HonorLt

Medium

Dishonest reward calculation when lum rate changes

Summary

setLumPerSecond immediately takes effect leading to a stepwise jump vulnerability.

Vulnerability Detail

When changing lum per second rate, it sets the value and skips the update function (commented out):

    function setLumPerSecond(uint96 lumPerSecond) external override onlyOwner {
        if (lumPerSecond > Constants.MAX_LUM_PER_SECOND) revert MasterChef__InvalidLumPerSecond();

        // _updateAll(_voter.getTopPoolIds()); // todo remove this

        _lumPerSecond = lumPerSecond;

        emit LumPerSecondSet(lumPerSecond);
    }

This means the old rate will not be accounted for at this timestamp for existing pools. On the next update, it will take the new rate and apply for the elapsed time as if these pools were operating with this new rate since the last update.

Malicious actors can make deposits, wait for an increase in lum per second, then claim the rewards as if they were depositing for the entire period with this increased value.

Claiming the rewards invokes _modify:

    function claim(uint256[] calldata pids) external override {
        for (uint256 i; i < pids.length; ++i) {
            _modify(pids[i], msg.sender, 0, true);
        }
    }

It then calculates rewards in the following way:

        uint256 totalLumRewardForPid = _getRewardForPid(rewarder, pid, oldTotalSupply);

Using new _lumPerSecond:

    function _getRewardForPid(Rewarder.Parameter storage rewarder, uint256 pid, uint256 totalSupply)
        private
        view
        returns (uint256)
    {
        return _getRewardForPid(pid, rewarder.getTotalRewards(_lumPerSecond, totalSupply), _voter.getTotalWeight());
    }

And outdated last update timestamp:

    function getTotalRewards(
        Parameter storage rewarder,
        uint256 rewardPerSecond,
        uint256 endTimestamp,
        uint256 totalSupply
    ) internal view returns (uint256) {
        if (totalSupply == 0) return 0;

        uint256 lastUpdateTimestamp = rewarder.lastUpdateTimestamp;
        uint256 timestamp = block.timestamp > endTimestamp ? endTimestamp : block.timestamp;

        return timestamp > lastUpdateTimestamp ? (timestamp - lastUpdateTimestamp) * rewardPerSecond : 0;
    }

Impact

Rate change affects previous deposits until synced with new values. Pools are operating with outdated reward rates. A positive stepwise jump will accredit users more rewards than entitled.

Code Snippet

https://github.com/sherlock-audit/2024-06-magicsea/blob/main/magicsea-staking/src/MasterchefV2.sol#L355-L357

https://github.com/sherlock-audit/2024-06-magicsea/blob/main/magicsea-staking/src/MasterchefV2.sol#L318

https://github.com/sherlock-audit/2024-06-magicsea/blob/main/magicsea-staking/src/MasterchefV2.sol#L546

https://github.com/sherlock-audit/2024-06-magicsea/blob/main/magicsea-staking/src/MasterchefV2.sol#L465

https://github.com/sherlock-audit/2024-06-magicsea/blob/main/magicsea-staking/src/libraries/Rewarder2.sol#L41-L63

Tool used

Manual Review

Recommendation

Uncomment _updateAll. The new rate should be applied linearly.

Watch for more context: https://www.youtube.com/watch?v=-9VmITcdm3c

Duplicate of #177