Lines of code

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

Vulnerability details

Impact

Why you still has a TODO in the final code?

TODO: Hardcode claim.account = address(this)?

It is not implemented yet. claim.account may be any value, which may break the claiming process or let user steal fund that intended to be used in MyStrategy to their own wallet by set claim.account to their wallet.

Proof of Concept

    // TODO: Hardcode claim.account = address(this)?
    /// @dev allows claiming of multiple bribes, badger is sent to tree
    /// @notice Hidden hand only allows to claim all tokens at once, not individually.
    ///         Allows claiming any token as it uses the difference in balance
    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");
    }

Obviously state that there is a TODO not implemented yet.

Tools Used

Manual

Recommended Mitigation Steps

Implement this TODO, or choose not to implement it and remove TODO comment.

code-423n4 / 2022-06-badger-findings