Lines of code

https://github.com/Badger-Finance/vested-aura/blob/v0.0.2/contracts/MyStrategy.sol#L421-L425 https://github.com/Badger-Finance/vested-aura/blob/v0.0.2/contracts/MyStrategy.sol#L107-L113 https://github.com/Badger-Finance/vested-aura/blob/v0.0.2/contracts/MyStrategy.sol#L411 https://github.com/Badger-Finance/vested-aura/blob/v0.0.2/contracts/MyStrategy.sol#L290

Vulnerability details

Impact

In _sendTokenToBribesProcessor(), it sends tokens to bribesProcessor. It seems to be ok because claimBribesFromHiddenHand() will confirm that bribesProcessor is not address(0). sweepRewardToken() also triggers _sendTokenToBribesProcessor(). But it doesn’t check bribesProcessor’s address. Which could cause permanent loss of reward tokens if bribesProcessor hasn't been set.

Proof of Concept

In _sendTokenToBribesProcessor(), it sends tokens to bribesProcessor without any check. https://github.com/Badger-Finance/vested-aura/blob/v0.0.2/contracts/MyStrategy.sol#L421-L425

    function _sendTokenToBribesProcessor(address token, uint256 amount) internal {
        // TODO: Too many SLOADs
        IERC20Upgradeable(token).safeTransfer(address(bribesProcessor), amount);
        emit RewardsCollected(token, amount);
    }

claimBribesFromHiddenHand() will confirm that bribesProcessor is not address(0) https://github.com/Badger-Finance/vested-aura/blob/v0.0.2/contracts/MyStrategy.sol#L290

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

        …
    }

But sweepRewardToken() doesn’t check bribesProcessor’s address https://github.com/Badger-Finance/vested-aura/blob/v0.0.2/contracts/MyStrategy.sol#L107-L113

    function sweepRewardToken(address token) public nonReentrant {
        _onlyGovernanceOrStrategist();
        _onlyNotProtectedTokens(token);

        uint256 toSend = IERC20Upgradeable(token).balanceOf(address(this));
        _handleRewardTransfer(token, toSend);
    }

Tools Used

None

Recommended Mitigation Steps

Add check in _sendTokenToBribesProcessor()

    function _sendTokenToBribesProcessor(address token, uint256 amount) internal {
        // TODO: Too many SLOADs
        require(address(bribesProcessor) != address(0), "Bribes processor not set");
        IERC20Upgradeable(token).safeTransfer(address(bribesProcessor), amount);
        emit RewardsCollected(token, amount);
    }

or sweepRewardToken()

    function sweepRewardToken(address token) public nonReentrant {
        _onlyGovernanceOrStrategist();
        _onlyNotProtectedTokens(token);
        require(address(bribesProcessor) != address(0), "Bribes processor not set");

        uint256 toSend = IERC20Upgradeable(token).balanceOf(address(this));
        _handleRewardTransfer(token, toSend);
    }

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