c4-bot-3 commented 6 months ago

Lines of code

https://github.com/code-423n4/2024-03-abracadabra-money/blob/1f4693fdbf33e9ad28132643e2d6f7635834c6c6/src/staking/LockingMultiRewards.sol#L361-L393

Vulnerability details

Proof of Concept

LockingMultiRewards is a fork of Curve MultiRewards which is also just a modified modified version of the Synthetix staking rewards contract, now common to all forks of Synthetix there is a vulnerability in regarding to the notifyRewardAmount() method.

Take a look at https://github.com/code-423n4/2024-03-abracadabra-money/blob/1f4693fdbf33e9ad28132643e2d6f7635834c6c6/src/staking/LockingMultiRewards.sol#L361-L393

    function notifyRewardAmount(address rewardToken, uint256 amount, uint minRemainingTime) public onlyOperators {
        if (!_rewardData[rewardToken].exists) {
            revert ErrInvalidTokenAddress();
        }

        _updateRewards();
        rewardToken.safeTransferFrom(msg.sender, address(this), amount);

        Reward storage reward = _rewardData[rewardToken];

        uint256 _nextEpoch = nextEpoch();
        uint256 _remainingRewardTime = _nextEpoch - block.timestamp;

        if (_remainingRewardTime < minRemainingTime) {
            revert ErrInsufficientRemainingTime();
        }

        // Take the remainder of the current rewards and add it to the amount for the next period
        if (block.timestamp < reward.periodFinish) {
            amount += _remainingRewardTime * reward.rewardRate;
        }

        // avoid `rewardRate` being 0
        if (amount < _remainingRewardTime) {
            revert ErrNotEnoughReward();
        }

        reward.rewardRate = amount / _remainingRewardTime;
        reward.lastUpdateTime = uint248(block.timestamp);
        reward.periodFinish = _nextEpoch;

        emit LogRewardAdded(amount);
    }

This function is used to distribute new rewards to the stakers, keep in mind that there is a current minimum rewardDuration period, so all rewards notified by the operators before any user starts staking is effectively lost, this is a pretty popular bug case for synthetix staking contract forks, and is sufficiently explained in this blog post

The issue in summary that whatever rate, rewards and distribution you set, if you initiate the process at time X, then when the first user stakes at Y and begins the accumulation process, the process would be from Y forward. Meaning that the rewards for the period Y - X would be left stuck in the contract. A bad mitigation we've discussed is that these stuck rewards would still be available for distribution the next time notifyRewardAmount() is invoked, but this does not guarantee anything. The rewards from Y - X would still be lost, no matter how many periods pass since the issue is in the time when the first staker stakes.

Impact

Stakers only have "rewardDuration" days minimum to stake their tokens before all rewards are lost.

A partial amount of these rewards will be lost anyway, that's to say if someone stakes before the full duration but after a while when it was called.

Recommended Mitigation Steps

One good recommendation would be to consider the method used here: https://github.com/PeggyJV/cellar-contracts/blob/afd970c36e9a520326afc888a11d40cdde75c6a7/src/CellarStaking.sol#L219 They introduce a function that checks when the first deposit occurs and then initiates the reward distribution process instead of doing it manually in notifyRewardAmount()

Assessed type

Timing

c4-pre-sort commented 6 months ago

141345 marked the issue as duplicate of #37

c4-judge commented 5 months ago

thereksfour changed the severity to QA (Quality Assurance)

code-423n4 / 2024-03-abracadabra-money-findings