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.

contract would save balance of contract in those tokens.
contract would call hiddenHandDistributor.claim() to claim rewards and it would receive some auraBAL balances.
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.
contract will calculated difference balance for auraBAL token and it would be tokens received from AuraLOCKER and hiddenHand.
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()

The finding has merit and given the information the warden had I believe the Judge will have to have the final say.

From my more POV, the attack would require:

"malicious" (but not so malicious, more like playful as tokens are sent to the BribesProcessor, not lost) governance or strategist (or strategist that frontrun governance just to be a nuisance
HiddenHandDistributor to have a malicious token that allow reEntrancy AND allows caller (strategist) to be able to regain control while being mid-claim in HiddenHands. (See https://etherscan.io/address/0x0b139682d5c9df3e735063f46fb98c689540cf3a#code#L1146)
OR Strategist calling a malicious contract, with the sole purpose of griefing the rewards

So this is actually doable but requires those pre-conditions

Additionally, notice how claimBribesFromHiddenHand doesn't check for protectedTokens, even want, that's because any of those tokens could be a bribe and as such we want to be able to process them properly.

In terms of impact, what would happen is that instead of immediately selling the auraBAL, the tokens would be in the bribesProcessor (implementation was linked although out of scope) and we would ragequit the tokens back to the BadgerTree or alternatively the manager could process them and re-emit them (autocompound with extra steps)

I want to comment the warden for the inquisitive nature, but I believe that impact as well as setup makes this a harvest with extra steps.

Open to any type of judging as some of the information I shared in the reply was most likely not available to the warden at the time of submission

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