`claimBribesFromHiddenHand()` Can Be Front-run In Calling `hiddenHandDistributor.claim()`

[code423n4]

Lines of code

https://github.com/Badger-Finance/vested-aura/blob/d504684e4f9b56660a9e6c6dfb839dcebac3c174/contracts/MyStrategy.sol#L311

Vulnerability details

Impact

The function hiddenHandDistributor.claim(_claims); is callable by anyone so long as they have the correct merkle proof, which is publicly available. If called by an external user the rewards will be transferred to the contract before claimBribesFromHiddenHand().

The impact of this is that the balance difference for the assets will be zero since the rewards have already been transferred to MyStrategy. Hence, _notifyBribesProcessor() may never be called.

The tokens are still recoverable by calling the sweepRewardToken(). Hence, this is rated as medium severity rather than high.

Note also that native ETH cannot be included in this attack since the receive() function reverts if we are not in claimBribesFromHiddenHand() because isClaimingBribes is false.

Proof of Concept

claimBribesFromHiddenHand() will only account for the balance increase before and after calling hiddenHandDistributor.claim(_claims);. Which may be zero if an attacker has already claimed the rewards.

    function claimBribesFromHiddenHand(IRewardDistributor hiddenHandDistributor, IRewardDistributor.Claim[] calldata _claims) external nonReentrant {
        _onlyGovernanceOrStrategist();
        require(address(bribesProcessor) != address(0), "Bribes processor not set");

        uint256 beforeVaultBalance = _getBalance();
        uint256 beforePricePerFullShare = _getPricePerFullShare();

        // Hidden hand uses BRIBE_VAULT address as a substitute for ETH
        address hhBribeVault = hiddenHandDistributor.BRIBE_VAULT();

        // Track token balances before bribes claim
        uint256[] memory beforeBalance = new uint256[](_claims.length);
        for (uint256 i = 0; i < _claims.length; i++) {
            (address token, , , ) = hiddenHandDistributor.rewards(_claims[i].identifier);
            if (token == hhBribeVault) {
                beforeBalance[i] = address(this).balance;
            } else {
                beforeBalance[i] = IERC20Upgradeable(token).balanceOf(address(this));
            }
        }

        // Claim bribes
        isClaimingBribes = true;
        hiddenHandDistributor.claim(_claims);
        isClaimingBribes = false;

        bool nonZeroDiff; // Cached value but also to check if we need to notifyProcessor
        // Ultimately it's proof of non-zero which is good enough

        for (uint256 i = 0; i < _claims.length; i++) {
            (address token, , , ) = hiddenHandDistributor.rewards(_claims[i].identifier);

            if (token == hhBribeVault) {
                // ETH
                uint256 difference = address(this).balance.sub(beforeBalance[i]);
                if (difference > 0) {
                    IWeth(address(WETH)).deposit{value: difference}();
                    nonZeroDiff = true;
                    _handleRewardTransfer(address(WETH), difference);
                }
            } else {
                uint256 difference = IERC20Upgradeable(token).balanceOf(address(this)).sub(beforeBalance[i]);
                if (difference > 0) {
                    nonZeroDiff = true;
                    _handleRewardTransfer(token, difference);
                }
            }
        }

        if (nonZeroDiff) {
            _notifyBribesProcessor();
        }

        require(beforeVaultBalance == _getBalance(), "Balance can't change");
        require(beforePricePerFullShare == _getPricePerFullShare(), "Ppfs can't change");
    }

Recommended Mitigation Steps

Consider allowing claimBribesFromHiddenHand() to claim all tokens in the contract as bribes and transfer the entire token balance to the bribe processor. This would require ignoring all AURA or AURABAL bribes and instead treating these as harvestable rewards.

[GalloDaSballo]

Disagree with the finding, the warden confirms there's no risk of loss as The tokens are still recoverable by calling the sweepRewardToken(), my conclusion is there's no vulnerability here as such I must dispute