chaduke - Loss of rewards due to frequent call of modifier ```updateReward()```.

[sherlock-admin2]

chaduke

High

Loss of rewards due to frequent call of modifier updateReward().

Summary

The code will be deployed on Ethereum. As a result, the average block time is 12 seconds, resulting in five blocks per minute. Given the huge number of users in Makerdao and five functions in StakingRewards that has the modifier updateReward(), the chance that there is someone who will call a function with the updateReward() per block is pretty high (only need five users per minute). Therefore, the tokens to be claimed might be a small value (accumulated in 12 seconds), due to rounding down error, this might lead to zero increase of rewardPerTokenStored, loss of rewards for all users. This will occur more frequently when _supply becomes larger, as the number of users increases.

The loss of rewards has high probability to occur by itself (zero cost) and affect each participant (might near 100% loss). Therefore, I mark this finding as high.

Root Cause

The accumulation of rewards is performed by updateReward(), a modifier for the five reward-related functions.

https://github.com/sherlock-audit/2024-06-makerdao-endgame/blob/main/endgame-toolkit/src/synthetix/StakingRewards.sol#L191C2-L199C6

updateReward() calls rewardPerToken() to calculate the new rewardPerTokenStored:

https://github.com/sherlock-audit/2024-06-makerdao-endgame/blob/main/endgame-toolkit/src/synthetix/StakingRewards.sol#L84-L90

However, if updateReward() is called too often, then due to round down error, (((lastTimeRewardApplicable() - lastUpdateTime) * rewardRate * 1e18) / _totalSupply) might be zero, leading to no increase for rewardPerTokenStored and the loss of rewards for participants (rewardPerTokenStored is updated at L192).

Internal pre-conditions

_totalSupply is a large number, will contribute easier loss of rewards due to easier rounding down to zero error.

External pre-conditions

Frequent call of any function with the modifier updateReward(), which is expected due to the large number of MakerDao users and only five blocks per minute for Ethereum and the five functions in the RewardsStaking contract.

Attack Path

No malicious code needs to be deployed. It will occur by itself when the user base increases and the _totalSupply increases, leading to HIGH probability of more frequent call of a function (five of such functions) with the modifier updateReward().

Impact

Loss of rewards due to frequent call of any function with the modifier updateReward().

PoC

consider the case of using NGT/NST as rewards/stake tokens, both of them have18 decimals.

Suppose_totalSupply = 100, 000, 000 ether and the amount of NGT rewards claimed (((lastTimeRewardApplicable() - lastUpdateTime) * rewardRate is smaller than 100,000, 000, say 99,999,999.

Then, we have 99,999,999 * e18 / 100, 000, 000 * e18 = 0. Therefore, no increase for rewardPerTokenStored, the rewards accumulated are lost for all.

In the following, we show that:

1) we set the total amount of reward tokens to be result.tot = 1.5 ether / 100000. 2) Bob stakes 100,000,000 ether staking tokens, as a result we have _totalSupply = 100000000000000000000000000; 3) rewardRate: 8267195 4) Each 12 seconds, we only accumulate 99206340 reward tokens; 5) After 12 seconds, when Bob calls getReward(), he gets no rewards at all! This is due the the rounding down error, and the accumulated reward of 99206340 are lost due to no increase of rewardPerTokenStored. 6) run forge test --match-test testReward1 -vv. 7) Install the Dsstest package under lib from https://github.com/makerdao/dss-vest. 8) change result.tot = 1.5 ether / 100000.

// SPDX-FileCopyrightText: © 2023 Dai Foundation <www.daifoundation.org>
// SPDX-License-Identifier: AGPL-3.0-or-later
//
// This program is free software: you can redistribute it and/or modify
// it under the terms of the GNU Affero General Public License as published by
// the Free Software Foundation, either version 3 of the License, or
// (at your option) any later version.
//
// This program is distributed in the hope that it will be useful,
// but WITHOUT ANY WARRANTY; without even the implied warranty of
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
// GNU Affero General Public License for more details.
//
// You should have received a copy of the GNU Affero General Public License
// along with this program.  If not, see <https://www.gnu.org/licenses/>.
pragma solidity ^0.8.16;

import {DssTest, stdStorage, StdStorage} from "dss-test/DssTest.sol";
import {IERC20} from "openzeppelin-contracts/token/ERC20/IERC20.sol";
import {DssVestWithGemLike} from "./interfaces/DssVestWithGemLike.sol";
import {IStakingRewards} from "./synthetix/interfaces/IStakingRewards.sol";
import {StakingRewards} from "./synthetix/StakingRewards.sol";
import {SDAO} from "./SDAO.sol";
import {VestedRewardsDistribution} from "./VestedRewardsDistribution.sol";
import "forge-std/Test.sol";
import "forge-std/console2.sol";
import {DssVestMintable} from "lib/dss-vest/src/DssVest.sol";
import {ERC20Mock} from "openzeppelin-contracts/mocks/ERC20Mock.sol";

contract VestedRewardsDistributionTest is DssTest {
    using stdStorage for StdStorage;

    struct VestParams {
        address usr;
        uint256 tot;
        uint256 bgn;
        uint256 tau;
        uint256 eta;
    }

    struct DistributionParams {
        VestedRewardsDistribution dist;
        DssVestWithGemLike vest;
        IStakingRewards rewards;
        IERC20Mintable rewardsToken;
        uint256 vestId;
        VestParams vestParams;
    }

    DistributionParams k;
    IERC20Mintable rewardsToken;
    ERC20Mock stakingToken;

    uint256 constant DEFAULT_DURATION = 365 days;
    uint256 constant DEFAULT_CLIFF = 0;
    uint256 constant DEFAULT_STARTING_RATE = uint256(200_000 * WAD) / DEFAULT_DURATION;
    uint256 constant DEFAULT_FINAL_RATE = uint256(2_000_000 * WAD) / DEFAULT_DURATION;
    uint256 constant DEFAULT_TOTAL_REWARDS = ((DEFAULT_STARTING_RATE + DEFAULT_FINAL_RATE) * DEFAULT_DURATION) / 2;

    function setUp() public {
        // DssVest checks if params are not too far away in the future or in the past relative to `block.timestamp`.
        // It has a 20 years interval check hardcoded, so we need to be at a time that is at least 20 years ahead of
        // the Unix epoch.  We are setting the current date of the chain to 2000-01-01 to comply with that requirement.
        vm.warp(946692000);

        rewardsToken = IERC20Mintable(address(new SDAO("K Token", "K")));
        console2.log("inital rewardsToken: ", address(rewardsToken));
        stakingToken = new ERC20Mock("NST", "NST", address(111), 1);
        console2.log("initial stakingToken: ", address(stakingToken));

        k = _setUpDistributionParams(
            DistributionParams({
                dist: VestedRewardsDistribution(address(0)),
                vest: DssVestWithGemLike(address(0)),
                rewards: IStakingRewards(address(0)),
                rewardsToken: rewardsToken,
                vestId: 0,
                vestParams: _makeVestParams()
            })
        );
    }

    function testReward1() public{
        address Bob = makeAddr("Bob");

        console2.log("\n testDistribution...........................");
        printk();
        // 1st distribution
        skip(k.vestParams.tau / 3);
        assertEq(k.rewardsToken.balanceOf(address(k.rewards)), 0, "Bad initial balance");

        printVestAward(1);

        vm.expectEmit(false, false, false, true, address(k.dist));
        emit Distribute(k.vestParams.tot / 3);
        console2.log("distribute 1........................");
        k.dist.distribute();

        console2.log("Bob stakes rewards...");

        deal(address(stakingToken), Bob, 100000000 ether); // 100M ether

        vm.startPrank(Bob);
        stakingToken.approve(address(k.rewards), 100000000 ether);
        k.rewards.stake(100000000 ether);
        vm.stopPrank();

        console2.log("Bob gets rewards...");

        skip(12 seconds);
        vm.startPrank(Bob);
        k.rewards.getReward();
        vm.stopPrank();

        assertEq(k.rewardsToken.balanceOf(Bob), 0);
    }

    function printk() public{
        console2.log("\n k values: $$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$");
        console2.log("dist: ", address(k.dist));
        console2.log("vest: ", address(k.vest));
        console2.log("rewards (staking contract): ", address(k.rewards));
        console2.log("stakingToken: ", address(stakingToken));
        console2.log("rewardsToken:", address(k.rewardsToken));
        console2.log("vesatId: ", k.vestId);
        console2.log("vestParams.usr:", k.vestParams.usr);
        console2.log("vestParams.tot:", k.vestParams.tot);
        console2.log("vestParams.bgn:", k.vestParams.bgn);
        console2.log("vestParams.tau:", k.vestParams.tau);
        console2.log("vestParams.eta:", k.vestParams.eta);
        console2.log("$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$$\n ");
    }

    function printVestAward(uint256 i) public {
       DssVestWithGemLike.Award  memory a = k.vest.awards(i);

       console2.log("The vest award information for ============", i);
       console2.log("usr: ", a.usr);
       console2.log("bgn: ", a.bgn);
       console2.log("clf: ", a.clf); // The cliff date  
       console2.log("fin: ", a.fin);
       console2.log("mgr: ", a.mgr);
       console2.log("res: ", a.res);
       console2.log("tot: ", a.tot);
       console2.log("rxd: ", a.rxd);
       console2.log("==========================================");
    }

    function printBalances(address a, string memory name) public
    {
        console2.log("\n =======================================");
        console2.log("balances for : ", name);
        console2.log("rewards token balance: ", k.rewardsToken.balanceOf(a));
        console2.log("=======================================\n");
    }

    function testDistribute1() public {   
        console2.log("\n testDistribution...........................");
        printk();
        // 1st distribution
        skip(k.vestParams.tau / 3);
        assertEq(k.rewardsToken.balanceOf(address(k.rewards)), 0, "Bad initial balance");

        printVestAward(1);

        vm.expectEmit(false, false, false, true, address(k.dist));
        emit Distribute(k.vestParams.tot / 3);
        console2.log("distribute 1........................");
        console2.log("distribute time: ", block.timestamp);
        k.dist.distribute();

       printBalances(address(k.rewards), "RewardsStaking");
       printBalances(address(k.dist), "Distribution");

        // 2nd distribution
        skip(k.vestParams.tau / 3);

        vm.expectEmit(false, false, false, true, address(k.dist));
        emit Distribute(k.vestParams.tot / 3);
        console2.log("distribute 2........................");
        k.dist.distribute();

        printBalances(address(k.rewards), "RewardsStaking");
         printBalances(address(k.dist), "Distribution");

        // 3rd distribution
        skip(k.vestParams.tau / 3);

        vm.expectEmit(false, false, false, true, address(k.dist));
        emit Distribute(k.vestParams.tot / 3);
        console2.log("distribute 3........................");
        k.dist.distribute();

       printBalances(address(k.rewards), "RewardsStaking");
        printBalances(address(k.dist), "Distribution");
    }

    function _setUpDistributionParams(
        DistributionParams memory _distParams
    ) internal returns (DistributionParams memory result) {
        result = _distParams;

        if (address(result.rewardsToken) == address(0)) {
            result.rewardsToken = IERC20Mintable(address(new SDAO("Token", "TKN")));
        }

        if (address(result.vest) == address(0)) {
            //result.vest = DssVestWithGemLike(      ?????
            //    deployCode("DssVest.sol:DssVestMintable", abi.encode(address(result.rewardsToken)))
            //);
            result.vest = DssVestWithGemLike(address(new DssVestMintable(address(result.rewardsToken))));
            result.vest.file("cap", type(uint256).max);
        }

        if (address(result.rewards) == address(0)) { // stakingTokens is zero?
            result.rewards = new StakingRewards(address(this), address(0), address(result.rewardsToken), address(stakingToken));
        }

        if (address(result.dist) == address(0)) {
            result.dist = new VestedRewardsDistribution(address(result.vest), address(result.rewards));
        }

        result.rewards.setRewardsDistribution(address(result.dist));
        _distParams.vestParams.usr = address(result.dist);            // distribution is the usr

        (result.vestId, result.vestParams) = _setUpVest(result.vest, _distParams.vestParams);
        result.dist.file("vestId", result.vestId);
        // Allow DssVest to mint tokens
        WardsLike(address(result.rewardsToken)).rely(address(result.vest));  // vest can call authorized fucntions in rewardsToken
    }

    function _setUpVest(
        DssVestWithGemLike _vest,
        address _usr
    ) internal returns (uint256 _vestId, VestParams memory result) {
        return _setUpVest(_vest, _usr, true);
    }

    function _setUpVest(
        DssVestWithGemLike _vest,
        address _usr,
        bool restrict
    ) internal returns (uint256 _vestId, VestParams memory result) {
        return _setUpVest(_vest, VestParams({usr: _usr, tot: 0, bgn: 0, tau: 0, eta: 0}), restrict);
    }

    function _setUpVest(
        DssVestWithGemLike _vest,
        VestParams memory _v
    ) internal returns (uint256 _vestId, VestParams memory result) {
        return _setUpVest(_vest, _v, true);
    }

    function _setUpVest(
        DssVestWithGemLike _vest,
        VestParams memory _v,
        bool restrict
    ) internal returns (uint256 _vestId, VestParams memory result) {
        result = _v;

        if (result.usr == address(0)) {
            revert("_setUpVest: usr not set");
        }
        if (result.tot == 0) {
            result.tot = 1.5 ether / 100000;      // change this ????
        }
        if (result.bgn == 0) {
            result.bgn = block.timestamp;
        }
        if (result.tau == 0) {
            result.tau = DEFAULT_DURATION;
        }
        if (result.eta == 0) {
            result.eta = DEFAULT_CLIFF;
        }

        _vestId = _vest.create(result.usr, result.tot, result.bgn, result.tau, result.eta, address(0));
        if (restrict) {
            _vest.restrict(_vestId);
        }
    }

    function _makeVestParams() internal pure returns (VestParams memory) {
        return VestParams({usr: address(0), tot: 0, bgn: 0, tau: 0, eta: 0});
    }

    bytes16 private constant _SYMBOLS = "0123456789abcdef";

    /**
     * @dev Converts a `uint256` to its ASCII `string` decimal representation.
     */
    function toString(uint256 value) internal pure returns (string memory) {
        unchecked {
            uint256 length = log10(value) + 1;
            string memory buffer = new string(length);
            uint256 ptr;
            /// @solidity memory-safe-assembly
            assembly {
                ptr := add(buffer, add(32, length))
            }
            while (true) {
                ptr--;
                /// @solidity memory-safe-assembly
                assembly {
                    mstore8(ptr, byte(mod(value, 10), _SYMBOLS))
                }
                value /= 10;
                if (value == 0) break;
            }
            return buffer;
        }
    }

    /**
     * @dev Return the log in base 10, rounded down, of a positive value.
     * Returns 0 if given 0.
     */
    function log10(uint256 value) internal pure returns (uint256) {
        uint256 result = 0;
        unchecked {
            if (value >= 10 ** 64) {
                value /= 10 ** 64;
                result += 64;
            }
            if (value >= 10 ** 32) {
                value /= 10 ** 32;
                result += 32;
            }
            if (value >= 10 ** 16) {
                value /= 10 ** 16;
                result += 16;
            }
            if (value >= 10 ** 8) {
                value /= 10 ** 8;
                result += 8;
            }
            if (value >= 10 ** 4) {
                value /= 10 ** 4;
                result += 4;
            }
            if (value >= 10 ** 2) {
                value /= 10 ** 2;
                result += 2;
            }
            if (value >= 10 ** 1) {
                result += 1;
            }
        }
        return result;
    }

    event Distribute(uint256 amount);
}

interface IERC20Mintable is IERC20 {
    function mint(address, uint256) external;
}

interface WardsLike {
    function rely(address) external;
}

Mitigation

Manage dustRewards as those accumulated rewards that have not been accounted for in rewardPerTokenStored. In this way, no accumulated rewards will be lost even they are small.

[telome]

This is a limitation of the Synthetix staking contract. As per the rules, "Any issue that exists in the original non-Maker Syntetix staking rewards contract is out of scope." In practice, the total supply of any of the tokens used as staking token is going to be less than 10^12 10^18 and the reward rate (a value that is adjustable by governance) is going to be (much) greater than 10^18/(243600), hence even if the update occurs every block (12 s), the rewardPerTokenStored will not stop increasing.