Badger-Vested-Aura contest - unforgiven's results

Lines of code

https://github.com/Badger-Finance/vested-aura/blob/d504684e4f9b56660a9e6c6dfb839dcebac3c174/contracts/MyStrategy.sol#L107-L113 https://github.com/Badger-Finance/vested-aura/blob/d504684e4f9b56660a9e6c6dfb839dcebac3c174/contracts/MyStrategy.sol#L405-L413 https://github.com/Badger-Finance/vested-aura/blob/d504684e4f9b56660a9e6c6dfb839dcebac3c174/contracts/MyStrategy.sol#L421-L425

Vulnerability details

Impact

If the value of bribesProcessor was 0x0 (the default is 0x0 and governance() can set to 0x0) then attacker can call sweepRewardToken() make contract to send his total balance in attacker specified token to 0x0 address.

Proof of Concept

the default value of bribesProcessor is 0x0 and governance can set the value to 0x0 at any time. rewards are stacking in contract address and they are supposed to send to bribesProcessor. This is sweepRewardToken() and _handleRewardTransfer() and _sendTokenToBribesProcessor() code:

  /// @dev Function to move rewards that are not protected
  /// @notice Only not protected, moves the whole amount using _handleRewardTransfer
  /// @notice because token paths are hardcoded, this function is safe to be called by anyone
  /// @notice Will not notify the BRIBES_PROCESSOR as this could be triggered outside bribes
  function sweepRewardToken(address token) public nonReentrant {
      _onlyGovernanceOrStrategist();
      _onlyNotProtectedTokens(token);

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

  function _handleRewardTransfer(address token, uint256 amount) internal {
      // NOTE: BADGER is emitted through the tree
      if (token == BADGER) {
          _sendBadgerToTree(amount);
      } else {
          // NOTE: All other tokens are sent to bribes processor
          _sendTokenToBribesProcessor(token, amount);
      }
  }

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

As you can see calling sweepRewardToken() eventually (sweepRewardToken() -> _handleRewardTransfer() -> _sendTokenToBribesProcessor()) would transfer reward funds to bribesProcessor and there is no check that bribesProcessor!=0x0 in execution follow. so attacker can call sweepRewardToken() when bribesProcessor is 0x0 and contract will lose all reward tokens.

Tools Used

VIM

Recommended Mitigation Steps

check the value of bribesProcessor in _sendTokenToBribesProcessor()

Lines of code

https://github.com/Badger-Finance/vested-aura/blob/d504684e4f9b56660a9e6c6dfb839dcebac3c174/contracts/MyStrategy.sol#L215-L282 https://github.com/Badger-Finance/vested-aura/blob/d504684e4f9b56660a9e6c6dfb839dcebac3c174/contracts/MyStrategy.sol#L107-L113 https://github.com/Badger-Finance/vested-aura/blob/d504684e4f9b56660a9e6c6dfb839dcebac3c174/contracts/MyStrategy.sol#L161-L166

Vulnerability details

Impact

auraBAL token is in protected tokens list, so it can't be transferred to bribeProcessor by using sweepRewardToken(). function _harvest() is supposed to call LOCKER.getReward() and then swap received auraBAL rewards and deposit them in LOCKER, but it only can do this action for auraBalEarned. There is no logic in contract code that can transfer auraBAL absolute balance and they only work for differential earned balance. LOCKER.getReward(account) is callable by anyone. attacker can call LOCKER.getReward() for contract address then the auraBAL rewards would be transfer to contract address and they would stuck in contract address forever.

Proof of Concept

This is sweepRewardToken() code:

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

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

As you can see it only support transferring not protected tokens. This is getProtectedTokens() code:

    function getProtectedTokens() public view virtual override returns (address[] memory) {
        address[] memory protectedTokens = new address[](2);
        protectedTokens[0] = want; // AURA
        protectedTokens[1] = address(AURABAL);
        return protectedTokens;
    }

As you can see auraBAL is in protected tokens so it's not possible to send it to bribeProcessor with function sweepRewardToken(). This is _harvest() code:

    /// @notice Autocompound auraBAL rewards into AURA.
    /// @dev Anyone can claim bribes for this contract from hidden hands with 
    ///      the correct merkle proof. Therefore, only tokens that are gained
    ///      after claiming rewards or swapping are auto-compunded.
    function _harvest() internal override returns (TokenAmount[] memory harvested) {
        uint256 auraBalBalanceBefore = AURABAL.balanceOf(address(this));

        // Claim auraBAL from locker
        LOCKER.getReward(address(this));

        harvested = new TokenAmount[](1);
        harvested[0].token = address(AURA);

        uint256 auraBalEarned = AURABAL.balanceOf(address(this)).sub(auraBalBalanceBefore);
        // auraBAL -> BAL/ETH BPT -> WETH -> AURA
        if (auraBalEarned > 0) {
            // Common structs for swaps
            IBalancerVault.SingleSwap memory singleSwap;

As you can see it only supports handling earned auraBAL amount, and it can't use all the auraBAL balances of contract. there is no other logic in the code that can process all balance of protected tokens (claimBribesFromHiddenHand() can only process hiddenHand rewards and send them to bribeProcessor and can't use all the balance of token, it only use amount earned from hiddenHand) function LOCKER.getReward() in AuraLocker is callable by anyone, so attacker can call this function for contract address and it would transfer auraBAL rewards to contract address and those auraBAL tokens would stuck in contract forever. attacker can do this before any transaction which calls _harvest() by front-running. users funds would stuck in contract and they can't withdraw it and because the reinvest can't happen so the yield of strategy would be damaged too.

Tools Used

VIM

Recommended Mitigation Steps

add some mechanism to handle all auraBAL balance of contract.

Lines of code

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

Vulnerability details

Impact

When calling BALANCER_VAULT.swap() and BALANCER_VAULT.exitPool() the minAmountsOut and limit value set to 0 which means there the operation can happen with big slippage. This can open contract to be vulnerable to front-running or sandwich-attack and creates MEV. miner can manipulate the swap pool before the transaction with big swap then run transaction and the rate for swaps in transaction would be terrible and then miner can do the reverse of first swap. with this users funds can be lost because contract would lose fund in swap() and exitPool()

Proof of Concept

This is _harvest() code:

    function _harvest() internal override returns (TokenAmount[] memory harvested) {
        uint256 auraBalBalanceBefore = AURABAL.balanceOf(address(this));

        // Claim auraBAL from locker
        LOCKER.getReward(address(this));

        harvested = new TokenAmount[](1);
        harvested[0].token = address(AURA);

        uint256 auraBalEarned = AURABAL.balanceOf(address(this)).sub(auraBalBalanceBefore);
        // auraBAL -> BAL/ETH BPT -> WETH -> AURA
        if (auraBalEarned > 0) {
            // Common structs for swaps
            IBalancerVault.SingleSwap memory singleSwap;
            IBalancerVault.FundManagement memory fundManagement = IBalancerVault.FundManagement({
                sender: address(this),
                fromInternalBalance: false,
                recipient: payable(address(this)),
                toInternalBalance: false
            });

            // Swap auraBal -> BAL/ETH BPT
            singleSwap = IBalancerVault.SingleSwap({
                poolId: AURABAL_BALETH_BPT_POOL_ID,
                kind: IBalancerVault.SwapKind.GIVEN_IN,
                assetIn: IAsset(address(AURABAL)),
                assetOut: IAsset(address(BALETH_BPT)),
                amount: auraBalEarned,
                userData: new bytes(0)
            });
            uint256 balEthBptEarned = BALANCER_VAULT.swap(singleSwap, fundManagement, 0, type(uint256).max);

            // Withdraw BAL/ETH BPT -> WETH
            uint256 wethBalanceBefore = WETH.balanceOf(address(this));

            IAsset[] memory assets = new IAsset[](2);
            assets[0] = IAsset(address(BAL));
            assets[1] = IAsset(address(WETH));
            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);

            // Swap WETH -> AURA
            uint256 wethEarned = WETH.balanceOf(address(this)).sub(wethBalanceBefore);
            singleSwap = IBalancerVault.SingleSwap({
                poolId: AURA_ETH_POOL_ID,
                kind: IBalancerVault.SwapKind.GIVEN_IN,
                assetIn: IAsset(address(WETH)),
                assetOut: IAsset(address(AURA)),
                amount: wethEarned,
                userData: new bytes(0)
            });
            harvested[0].amount = BALANCER_VAULT.swap(singleSwap, fundManagement, 0, type(uint256).max);
        }

        _reportToVault(harvested[0].amount);
        if (harvested[0].amount > 0) {
            _deposit(harvested[0].amount);
        }
    }

As you can see when calling BALANCER_VAULT.swap() the limit argument (3rd one) set to 0 and when calling BALANCER_VAULT.exitPool() the minAmountsOut set to 0, so there is no slippage specified for those calls and any rate would be acceptable for them. a malicious miner can attack the contract when a call happens for _harvest(). miner can do a big swap() in balancer pool so the exchange rate would be very bad, then execute the _harvest() transaction and contract would swap the tokens with bad exchange rate and lose funds. then miner execute the reverse of the first big swap.

Tools Used

VIM

Recommended Mitigation Steps

specify slippage when calling balancer pools.

Lines of code

https://github.com/Badger-Finance/vested-aura/blob/d504684e4f9b56660a9e6c6dfb839dcebac3c174/contracts/MyStrategy.sol#L427-L431 https://github.com/Badger-Finance/vested-aura/blob/d504684e4f9b56660a9e6c6dfb839dcebac3c174/contracts/MyStrategy.sol#L404-L413

Vulnerability details

Impact

Any logic that uses _sendBadgerToTree() would revert or double spend happens, because it transfers tokens then calls _processExtraToken() which tries to transfer those tokens again. So functions sweepRewardToken() and sweepRewards() will reverts for BADGER token because their execution would reach _sendBadgerToTree() and the amount is all balance it's. Function claimBribesFromHiddenHand() will revert or double spends happens if one of reward tokens were BAGER token, because its execution would reach _sendBadgerToTree().

Proof of Concept

This is _sendBadgerToTree() code:

    /// @dev Send the BADGER token to the badgerTree
    function _sendBadgerToTree(uint256 amount) internal {
        IERC20Upgradeable(BADGER).safeTransfer(BADGER_TREE, amount);
        _processExtraToken(address(BADGER), amount);
    }

As you can see it transferes defined amount of BADGER token from contract address and then calls _processExtraToken() with the BADGER address and same amount. This is _processExtraToken() code in BaseStrategy:

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

As you can see it tries to send same amount of specified token (which here is BADGER) again. so if some logic calls _sendBadgerToTree(amount) then two transfers would happen with the specified amount. if amount was all the contract balance then for sure the transaction will revert (because of inefficient fund) and if amount was less than half of the contract balance, then double spend would happen. This is _handleRewardTransfer() code:

    /// *** Handling of rewards ***
    function _handleRewardTransfer(address token, uint256 amount) internal {
        // NOTE: BADGER is emitted through the tree
        if (token == BADGER) {
            _sendBadgerToTree(amount);
        } else {
            // NOTE: All other tokens are sent to bribes processor
            _sendTokenToBribesProcessor(token, amount);
        }
    }

as you can see for BADGER token it calls _sendBadgerToTree(). so the bug will happen for _handleRewardTransfer() too if it is called with BADGER token. functions sweepRewardToken() and claimBribesFromHiddenHand() are calling _handleRewardTransfer(). if they call it with BADGER token the bug would happen to them to. This is sweepRewardToken() code:

    /// @dev Function to move rewards that are not protected
    /// @notice Only not protected, moves the whole amount using _handleRewardTransfer
    /// @notice because token paths are hardcoded, this function is safe to be called by anyone
    /// @notice Will not notify the BRIBES_PROCESSOR as this could be triggered outside bribes
    function sweepRewardToken(address token) public nonReentrant {
        _onlyGovernanceOrStrategist();
        _onlyNotProtectedTokens(token);

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

As you can see it calls _handleRewardTransfer() with specified token and total balance of contract in that token. so if token was BADGER the call would definitely revert and sweepRewardToken() would always for BADGER. because sweepRewards() uses sweepRewardToken() so sweepRewardToken() would fail if BADGER was one of it's tokens. claimBribesFromHiddenHand() calls _handleRewardTransfer() with difference amount so if the token was BADGER and the amount was less than half of contract BADGER balance, double spend would happen otherwise the call would revert.

Tools Used

VIM

Recommended Mitigation Steps

fix the bug that tries to transfer same amount twice for BADGER token.

Lines of code

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

Vulnerability details

Impact

Function claimBribesFromHiddenHand makes some external calls to token lists (which fetches from hiddenHandDistributor.rewards) if auraBAL was on of those tokens and also one of those tokens were malicious or made some external call then it's possible to call AuraLOCKER.getReward(MyStrategy) and change auraBAL balance and that would cause the difference calculation of auraBAL to go wrong and auraBAL tokens recieved from LOCKER would be transfer to bribeProcessor even so they are not supposed to.

Proof of Concept

This is hiddenHandDistributor.rewards() code:

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

As you can see code saves the balance of contract in token lists and then make external calls to transfer rewards. if auraBAL was in the token list and one other token was malicious or made some external call to strategist controllable address then that person can call AuraLOCKER.getReward(MyStrategy) which would change the auraBAL balance of contract and when contract tries to find earned auraBAL tokens the value of difference would be received values from hiddenHand and AuraLcoker and it would be transferred to bribeProcessor even so auraBAL from AuraLOCKER should only supposed to be used in _harvest() to swap and reinvest. these are the steps: 1 strategist would call claimBribesFromHiddenHand() with auraBAL and some malicious token in the list of tokens. 2. contract would save balance of contract in those tokens. 3. contract would call hiddenHandDistributor.claim() to claim rewards and it would receive some auraBAL balances. 4. contract would make external call to malicious token address and then strategist call AuraLOCKER.getReward(MyStrategy) and AuraLocker would transfer auraBAL tokens to MyStrategy address. 5. contract will calculated difference balance for auraBAL token and it would be tokens received from AuraLOCKER and hiddenHand. 7. contract would transfer difference amount of auraBAL to bribeProcessor and AuraLOCKER tokens would transfer to bribeProcessor.

So even so auraBAL is in protected tokens and auraBAL tokens from AuraLOCKER is supposed to be used in _harvest() and create compounding APR, but the tokens will transfer to bribeProcessor

Tools Used

VIM

Recommended Mitigation Steps

check for all the balance change of protected tokens in claimBribesFromHiddenHand()