Mansa11 - `distribute` can be DOSed by an attacker

[sherlock-admin4]

Mansa11

High

distribute can be DOSed by an attacker

Summary

The VestedRewardsDistribution::distribute can be bricked leading to users not able to earn rewards. https://github.com/sherlock-audit/2024-06-makerdao-endgame/blob/main/endgame-toolkit/src/VestedRewardsDistribution.sol#L152-L166 https://github.com/makerdao/dss-vest/blob/19a9d663bb3a2737f1f0c763365f1dfc6788aad2/src/DssVest.sol#L232-L241

Description

Anyone can call VestedRewardsDistribution.distribute which calls StakingReward.notifyRewardAmount and distributes the accumulated rewards to the users and sets a new periodFinish.

When a user calls distribute, the unpaid returns the amount of vested which is claimable and it makes a call to the dssVest.vest with the said amount in order to mint the required gem tokens needed. The dssVest.vest also has an implementation that gets the current unpaid amount and compares it with the unpaid amount from the distribute function in order to get the minimum value.

The vulnerability lies in the fact that a malicious user can advantage of this by DOSing the distribute function making it difficult or impossible for legit users to earn rewards.

The unpaid gotten in distribute might be different from the unpaid from the vest, and in this case the minimum is being selected according to this line from the dssVest.vest function ->

• amt = min(amt, _maxAmt);

     function _vest(uint256 _id, uint256 _maxAmt) internal lock {
        Award memory _award = awards[_id];
        require(_award.usr != address(0), "DssVest/invalid-award");
        require(_award.res == 0 || _award.usr == msg.sender, "DssVest/only-user-can-claim");
        uint256 amt = unpaid(block.timestamp, _award.bgn, _award.clf, _award.fin, _award.tot, _award.rxd);
        amt = min(amt, _maxAmt);
        awards[_id].rxd = toUint128(add(_award.rxd, amt));
        pay(_award.usr, amt);
        emit Vest(_id, amt);
    }

From the above, the amount to be minted to the the VestedRewardsDistribution contract is based on the amt = min(amt, _maxAmt); which selects the min between the amt and _maxAmt. In the case whereby the selected amount is the amt , it mints a lesser amount to the VestedRewardsDistribution when this happens, the distribute() tries to the initial amount to the stakingRewards contract. However, the transaction is going to fail because the specified amount will be insufficient.

Attack scenario

• Malicious user stake some amounts
• Malicious user monitors the memepool for potential distribute calls.
• Legit User calls distribute
• distribute calculates amount as -> amount = dssVest.unpaid(vestId);
• Malicious front runs the tx and withdraw his staked amount
• dssVest.vest function is being called
• amount returned from dssVest.vest => amt = min(amt, _maxAmt); which is now lesser(minus malicious actors amount)
• dssVest.vest mints amt tokens to the VestedRewardsDistribution contract
• transfer in distribution fails due to contract not having enough tokens as the amount minted from dssVest.vest is lesser than what he is trying to send

Impact

distribute can be DOSed making it very difficult or impossible for legit users to earn rewards

POC

    function distribute() external returns (uint256 amount) {
        require(vestId != INVALID_VEST_ID, "VestedRewardsDistribution/invalid-vest-id");

        amount = dssVest.unpaid(vestId);
        require(amount > 0, "VestedRewardsDistribution/no-pending-amount");

        lastDistributedAt = block.timestamp;
        dssVest.vest(vestId, amount);

        require(gem.transfer(address(stakingRewards), amount), "VestedRewardsDistribution/transfer-failed"); //@audit the transfer here will fail due to the fact the amount of gem minted to this contract is  smaller than the amount of gem its trying to send to the stakingRewards due to the fact that it is getting the unpaid twice, both here and in the vest function
        stakingRewards.notifyRewardAmount(amount);

        emit Distribute(amount);
    }

Recommendation

Remove the unpaid logic from the distribute() function and only use the one from the dssVest.vest

[sunbreak1211]

This issue doesn't make any sense, there are not two unpaid functions. It is just the dssvest one that is called from distribute to save the amount that the dssvest will pay. The reason to have to do this is because the vest call doesn't return the vested value.