Badger-Vested-Aura contest - berndartmueller's results

Lines of code

https://github.com/Badger-Finance/vested-aura/blob/d504684e4f9b56660a9e6c6dfb839dcebac3c174/contracts/MyStrategy.sol#L249 https://github.com/Badger-Finance/vested-aura/blob/d504684e4f9b56660a9e6c6dfb839dcebac3c174/contracts/MyStrategy.sol#L259 https://github.com/Badger-Finance/vested-aura/blob/d504684e4f9b56660a9e6c6dfb839dcebac3c174/contracts/MyStrategy.sol#L275

Vulnerability details

Impact

Autocompounding auraBAL rewards into AURA requires multiple swaps (auraBAL -> BAL/ETH BPT -> WETH -> AURA) within MyStrategy._harvest.

The swaps are at risk of being front-run / sandwiched, resulting in a loss of funds.

Since MEV is very prominent I think the chance of that happening is pretty high.

Proof of Concept

MyStrategy.sol#L249

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

MyStrategy.sol#L259

IBalancerVault.ExitPoolRequest memory exitPoolRequest = IBalancerVault.ExitPoolRequest({
    assets: assets,
    minAmountsOut: new uint256[](2), // @audit-info missing slippage protection
    userData: abi.encode(ExitKind.EXACT_BPT_IN_FOR_ONE_TOKEN_OUT, balEthBptEarned, BPT_WETH_INDEX),
    toInternalBalance: false
});

MyStrategy.sol#L275

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

Tools Used

Manual review

Recommended mitigation steps

Consider adding slippage checks during swapping.

See https://dev.balancer.fi/resources/joins-and-exits/pool-exits#minamountsout for slippage protection info.

Lines of code

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

Vulnerability details

Impact

Twice the amount of BADGER tokens are sent to badgerTree within the MyStrategy._sendBadgerToTree function.

Due to insufficient BADGER tokens, this will break the following functionalities:

• MyStrategy.sweepRewardToken (L107-L113)

  function sweepRewardToken(address token) public nonReentrant {
      _onlyGovernanceOrStrategist();
      _onlyNotProtectedTokens(token);
  
      uint256 toSend = IERC20Upgradeable(token).balanceOf(address(this));
      _handleRewardTransfer(token, toSend); // @audit-info reverts for BADGER tokens due to sending twice the amount of tokens which are available
  }

• MyStrategy.claimBribesFromHiddenHand (L328-L334)

  ...
  else {
      uint256 difference = IERC20Upgradeable(token).balanceOf(address(this)).sub(beforeBalance[i]);
      if (difference > 0) {
          nonZeroDiff = true;
          _handleRewardTransfer(token, difference); // @audit-info reverts for BADGER tokens due to sending twice the amount of tokens which are available
      }
  }
  ...

Proof of Concept

MyStrategy.sol#L428-L431

function _sendBadgerToTree(uint256 amount) internal {
    IERC20Upgradeable(BADGER).safeTransfer(BADGER_TREE, amount);
    _processExtraToken(address(BADGER), amount); // @audit-info BADGER tokens are sent to vault
}

Tools Used

Manual review

Recommended mitigation steps

The intended behavior is to send BADGER tokens to the badger tree. This is done within BaseStrategy._processExtraToken, therefore it is not necessary to transfer BADGER tokens to BADGER_TREE.

BaseStrategy.sol#L338

///         Calls `Vault.reportAdditionalToken` to process fees and forward amount to badgerTree to be emitted.

Remove IERC20Upgradeable(BADGER).safeTransfer(BADGER_TREE, amount); from the MyStrategy._sendBadgerToTree function on L429.

Lines of code

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

Vulnerability details

Impact

All funds can be migrated (withdrawn) at once to the caller vault by using the BaseStrategy.withdrawToVault function which internally calls MyStrategy._withdrawAll.

The latter function has the following check in place:

MyStrategy.sol#L184-L187

require(
    balanceOfPool() == 0 && LOCKER.balanceOf(address(this)) == 0,
    "You have to wait for unlock or have to manually rebalance out of it"
);

Funds can only be withdrawn (migrated) if the balance in LOCKER is fully unlocked.

By locking a small amount of want tokens via AuraLocker.lock with the strategy address, a malicious individual can cause DoS and prevent withdrawing and migrating funds to the vault.

Proof of Concept

The following test case will replicate the DoS attack by locking "dust" want tokens for the strategy address. This causes vault.withdrawToVault to revert.

def test_frontrun_migration(locker, deployer, vault, strategy, want, governance, keeper):
    # Setup
    randomUser = accounts[6]
    snap = SnapshotManager(vault, strategy, "StrategySnapshot")

    startingBalance = want.balanceOf(deployer)
    depositAmount = startingBalance // 2
    assert startingBalance >= depositAmount
    # End Setup

    # Deposit
    want.approve(vault, MaxUint256, {"from": deployer})
    snap.settDeposit(depositAmount, {"from": deployer})

    chain.sleep(15)
    chain.mine()

    vault.earn({"from": keeper})

    chain.snapshot()

    # Test no harvests
    chain.sleep(86400 * 250)  ## Wait 250 days so we can withdraw later
    chain.mine()

    before = {"settWant": want.balanceOf(vault), "stratWant": strategy.balanceOf()}

    strategy.prepareWithdrawAll({"from": governance})

    want.approve(locker, 1, {"from": deployer})
    locker.lock(strategy, 1, { "from": deployer }) # Donate "dust" want tokens to strategy

    vault.withdrawToVault({"from": governance}) # @audit-info reverts with "You have to wait for unlock or have to manually rebalance"

    after = {"settWant": want.balanceOf(vault), "stratWant": strategy.balanceOf()}

    assert after["settWant"] > before["settWant"]
    assert after["stratWant"] < before["stratWant"]
    assert after["stratWant"] == 0

Tools Used

Manual review

Recommended mitigation steps

Call LOCKER.processExpiredLocks(false); in MyStrategy._withdrawAll directly and remove the check which enforces unlocking all want tokens on L184-L187.