Reward tokens can be stolen from the MerklGaugeMiddleman contract

[hats-bug-reporter[bot]]

Github username: -- Twitter username: -- Submission hash (on-chain): 0x15dab49f2a5ab2c12db6e9aba0d6ff13d29d4513544b65c290d8df7933521c10 Severity: high

Description: Description\ The notifyRewards function notifies the DistributionCreator about the reward for a specific gauge. if the reward amount is less than the set minimum threshold. It sends the amount to the caller. The problem is that snyone can call the function to get a refund without actually having any funds in the contract and steal funds.  This bug can also be used to drain the contract by removing small amount of rewards untill the entire contract is completely drained of it's reward tokens. 

            if (amount_ > creatorCache.rewardTokenMinAmounts(address(tokenChache)) * params.numEpoch) {
                uint256 distributionAmount = creatorCache.createDistribution(params);
                emit CreateDistribution(msg.sender, gauge_, amount_, distributionAmount);
            } else {
                tokenChache.safeTransfer(msg.sender, amount_);
            }

https://github.com/Satsyxbt/Fenix/blob/7b81d318fd9ef6107a528b6bd49bb9383e1b52ab/contracts/integration/MerklGaugeMiddleman.sol#L125

Sponsor note: As stated above, the notifyRewards function is permissionless.  Anyone can call the function to execute the attack and steal rewards.  

Recommendation

Let the function check if the user has any funds in the contract before sending him a refund.

[BohdanHrytsak]

Thank you for the submission.

This behaviour is expected. It is not expected that the funds will be sent and distributed in different transactions, which would provide a window for the following.

This option is also indicated in the natspec: https://github.com/Satsyxbt/Fenix/blob/7b81d318fd9ef6107a528b6bd49bb9383e1b52ab/contracts/integration/MerklGaugeMiddleman.sol#L102-L103

For secure distribution, the user can use notifyRewardWithTransfer for example

[0xisaacc]

Hi @BohdanHrytsak,

It seems you misunderstood what I meant. The main issue is that anyone can steal funds from the contract without having any rewards in the contract as long as the stolen amount is below the minimum threshold. This can be done is one transaction and this is the main bug.

The second part is that this theft of small amounts can be carried out repeatedly for maximum damage until the entire contract is drained. This can be done across multiple transactions.

With regards to function NotifyRewardsWithTransfer. Notice that both function NotifyRewards and NotifyRewardsWithTransfer use the same internal function _notifyReward If a user calls NotifyRewardsWithTransfer with an amount that is below the minimum threshold, it gets refunded back to the user. An attacker that understands this will call the NotifyRewards function with an amount that is below the minimum threshold so that the function will send him a refund even though he didn't transfer anything to the contract. This allows him to steal rewards without transferring anything to the contract.

Thanks.

[kakarottosama]

If user (dumbly for no reason) send the fund to this contract and let it sit idle, then yes for sure, anyone can stole it, this is what your submission explain about. But, this contract is not expected for anyone to send the fund to it, there is no benefit of doing that.

but, what @BohdanHrytsak says is, this contract is intended to be use as one transaction in send and distribution.

maybe you can check in GaugeUpgradable.sol of what the meaning about it will be send in one transaction.

    function notifyRewardAmount(
        address token,
        uint256 reward
    ) external nonReentrant isNotEmergency onlyDistribution updateReward(address(0)) {
        require(token == address(rewardToken), "not rew token");
        rewardToken.safeTransferFrom(DISTRIBUTION, address(this), reward);
        if (isDistributeEmissionToMerkle) {
@>          rewardToken.safeTransfer(merklGaugeMiddleman, reward);
@>          IMerklGaugeMiddleman(merklGaugeMiddleman).notifyReward(address(this), 0);
        } else {

if this notifyRewardAmount is called, then it will either distribute, or refund, which is done in one transaction, attacker can't sandwich it.

---

... anyone can steal funds from the contract without having any rewards in the contract ...

a bit confusing here.

If a user calls NotifyRewardsWithTransfer with an amount that is below the minimum threshold, it gets refunded back to the user. An attacker that understands this will call the NotifyRewards function with an amount that is below the minimum threshold so that the function will send him a refund even though he didn't transfer anything to the contract. This allows him to steal rewards without transferring anything to the contract.

In this case, it will not work. The user get the refund back first before attacker transaction executed. The (NotifyRewardsWithTransfer -> send token -> notify reward -> refund) is done one transaction, attacker can't put their tx in between.

[BohdanHrytsak]

The user above described it correctly.

Remains unchanged