Badger-Vested-Aura contest - GimelSec's results

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);
    }

Lines of code

https://github.com/Badger-Finance/vested-aura/blob/v0.0.2/contracts/MyStrategy.sol#L249 https://github.com/Badger-Finance/vested-aura/blob/v0.0.2/contracts/MyStrategy.sol#L257-L263 https://github.com/Badger-Finance/vested-aura/blob/v0.0.2/contracts/MyStrategy.sol#L275

Vulnerability details

Impact

In balancer document:

In the above example code, we set our token_BAL limit to 0, which means we are willing to accept 100% slippage on our trade. That is generally a very bad idea

It lacks slippage control when calling BALANCER_VAULT.swap, making it suffer from 100% slippage and front-running attack.

Proof of Concept

The third parameter of BALANCER_VAULT.swap is 0:

https://github.com/Badger-Finance/vested-aura/blob/v0.0.2/contracts/MyStrategy.sol#L249

            uint256 balEthBptEarned = BALANCER_VAULT.swap(singleSwap, fundManagement, 0, type(uint256).max);

https://github.com/Badger-Finance/vested-aura/blob/v0.0.2/contracts/MyStrategy.sol#L275

            harvested[0].amount = BALANCER_VAULT.swap(singleSwap, fundManagement, 0, type(uint256).max);

And the minAmountsOut is empty:

https://github.com/Badger-Finance/vested-aura/blob/v0.0.2/contracts/MyStrategy.sol#L257-L263

            IBalancerVault.ExitPoolRequest memory exitPoolRequest = IBalancerVault.ExitPoolRequest({
                assets: assets,
                minAmountsOut: new uint256[](2),
                userData: abi.encode(ExitKind.EXACT_BPT_IN_FOR_ONE_TOKEN_OUT, balEthBptEarned, BPT_WETH_INDEX),
                toInternalBalance: false
            });
            BALANCER_VAULT.exitPool(BAL_ETH_POOL_ID, address(this), payable(address(this)), exitPoolRequest);

Tools Used

None

Recommended Mitigation Steps

Consider setting a limit value for BALANCER_VAULT.swap and minAmountsOut.

minAmountsOut good practice:

A good practice would be to user queryExit in BalancerHelpers to find the current amounts of tokens you would get for your BPT, and then account for some possible slippage.

Lines of code

https://github.com/Badger-Finance/vested-aura/blob/v0.0.2/contracts/MyStrategy.sol#L428-L431 https://github.com/Badger-Finance/badger-vaults-1.5/blob/0.1.0/contracts/BaseStrategy.sol#L346-L353

Vulnerability details

Impact

_sendBadgerToTree() sends amount BADGER to BADGER_TREE. However _processExtraToken() in _sendBadgerToTree() also sends amount BADGER to the vault. it sends amount BADGER twice. It could result in revert if it doesn't have enough balance of BADGER.

Proof of Concept

_sendBadgerToTree() sends amount BADGER to BADGER_TREE https://github.com/Badger-Finance/vested-aura/blob/v0.0.2/contracts/MyStrategy.sol#L428-L431

    function _sendBadgerToTree(uint256 amount) internal {
        IERC20Upgradeable(BADGER).safeTransfer(BADGER_TREE, amount);
        _processExtraToken(address(BADGER), amount);
    }

_processExtraToken() in _sendBadgerToTree() also sends amount BADGER to the vault https://github.com/Badger-Finance/badger-vaults-1.5/blob/0.1.0/contracts/BaseStrategy.sol#L346-L353

    function _processExtraToken(address _token, uint256 _amount) internal {
        require(_token != want, "Not want, use _reportToVault");
        require(_token != address(0), "Address 0");
        require(_amount != 0, "Amount 0");

        IERC20Upgradeable(_token).safeTransfer(vault, _amount);
        IVault(vault).reportAdditionalToken(_token);
    }

Tools Used

None

Recommended Mitigation Steps

Removing _processExtraToken() in _sendBadgerToTree() can fix it. But I’m not sure if _processExtraToken() is necessary in _sendBadgerToTree(). If so, _processExtraToken() should be rewrite in MyStrategy.sol to prevent sending BADGER twice.

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.