Malicious Governance can set malicious bribesProcessor to steal rewards that are not protected

[code423n4]

Lines of code

https://github.com/Badger-Finance/vested-aura/blob/v0.0.2/contracts/MyStrategy.sol#L98-L101
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#L421-L425

Vulnerability details

Impact

bribesProcessor is used to receive bribes and reward tokens. However there is almost no check for newBribesProcessor in setBribesProcessor. Malicious governance can set any address as bribesProcessor. Malicious governance could set an EOA as bribesProcessor.

Proof of Concept

There is almost no check for newBribesProcessor in setBribesProcessor.
https://github.com/Badger-Finance/vested-aura/blob/v0.0.2/contracts/MyStrategy.sol#L98-L101

     ///@dev Change the contract that handles bribes
    function setBribesProcessor(IBribesProcessor newBribesProcessor) external {
        _onlyGovernance();
        bribesProcessor = newBribesProcessor;
    }

After setting a malicious bribesProcessor. Malicious governance can call sweepRewardToken to steal rewards that are not protected.
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 a check in setBribesProcessor to make sure that newBribesProcessor is a contract.

[GalloDaSballo]

Because the bribesProcessor can be set, there are no guarantees in the code that the bribesProcessor will not be malicious, any check would be an illusion as malicious governance would be able to also fabricate the contract.

That said the bribesProcessor will have the vault address hardcoded to only work with it, guaranteeing that once bribes are received they will be properly processed.

Don't have a strong opinion on the validity of this finding as ultimately yet, the fact that governance can change the bribes processor address does open to the ability of stealing yield.

At the same time the finding is lazy and doesn't offer a solution that is actionable