BalancerFlashLender#receiveFlashLoan does not validate the originalCallData

[c4-bot-7]

Lines of code

https://github.com/code-423n4/2024-05-bakerfi/blob/59b1f70cbf170871f9604e73e7fe70b70981ab43/contracts/core/flashloan/BalancerFlashLender.sol#L109

Vulnerability details

Impact

receiveFlashLoan does not validate the originalCallData,
The attacker can pass any parameters into the receiveFlashLoan function and execute any Strategy instruction :_supplyBorrow _repayAndWithdraw _payDeb.

Proof of Concept

The StrategyLeverage#receiveFlashLoan function only validates whether the msg.sender is _balancerVault, but does not validate the originalCallData:

   function receiveFlashLoan(address[] memory tokens,
        uint256[] memory amounts, uint256[] memory feeAmounts, bytes memory userData
    ) external {
 @>     if (msg.sender != address(_balancerVault)) revert InvalidFlashLoadLender();
        if (tokens.length != 1) revert InvalidTokenList();
        if (amounts.length != 1) revert InvalidAmountList();
        if (feeAmounts.length != 1) revert InvalidFeesAmount();

        //@audit originalCallData is not verified
        (address borrower, bytes memory originalCallData) = abi.decode(userData, (address, bytes));
        address asset = tokens[0];
        uint256 amount = amounts[0];
        uint256 fee = feeAmounts[0];
        // Transfer the loan received to borrower
        IERC20(asset).safeTransfer(borrower, amount);

@>      if (IERC3156FlashBorrowerUpgradeable(borrower).onFlashLoan(borrower,
                tokens[0], amounts[0], feeAmounts[0], originalCallData
            ) != CALLBACK_SUCCESS
        ) {
            revert BorrowerCallbackFailed();
        }
        ....
    }

_balancerVault.flashLoan can specify the recipient:

    _balancerVault.flashLoan(address(this), tokens, amounts, abi.encode(borrower, data));

An attacker can initiate flashLoan from another contract and specify the recipient as BalancerFlashLender. _balancerVault will call the balancerFlashlender#receiveFlashLoan function,
Since the caller of the receiveFlashLoan function is _balancerVault, this can be verified against msg.sender.

The StrategyLeverage#onFlashLoan function parses the instructions to be executed from originalCallData(FlashLoanData) and executes them.

    function onFlashLoan(address initiator,address token,uint256 amount, uint256 fee, bytes memory callData
    ) external returns (bytes32) {
        if (msg.sender != flashLenderA()) revert InvalidFlashLoanSender();
        if (initiator != address(this)) revert InvalidLoanInitiator();
        // Only Allow WETH Flash Loans
        if (token != wETHA()) revert InvalidFlashLoanAsset();
        //loanAmount = leverage - msg.value;
@>      FlashLoanData memory data = abi.decode(callData, (FlashLoanData));
        if (data.action == FlashLoanAction.SUPPLY_BOORROW) {
            _supplyBorrow(data.originalAmount, amount, fee);
            // Use the Borrowed to pay ETH and deleverage
        } else if (data.action == FlashLoanAction.PAY_DEBT_WITHDRAW) {
            // originalAmount = deltaCollateralInETH
            _repayAndWithdraw(data.originalAmount, amount, fee, payable(data.receiver));
        } else if (data.action == FlashLoanAction.PAY_DEBT) {
            _payDebt(amount, fee);
        }
        return _SUCCESS_MESSAGE;
    }

So the attacker by calling the _balancerVault flashLoan function, designated recipient for BalancerFlashLender, borrower for StrategyLeverage,
An attacker can invoke the _supplyBorrow _repayAndWithdraw _payDeb function in StrategyLeverage with any FlashLoanData parameter.

Tools Used

vscode, manual

Recommended Mitigation Steps

1. BalancerFlashLender#flashLoan function to record the parameters called via hash.
2. Verify the hash value in the receiveFlashLoan function.

Assessed type

Access Control

[0xStalin-eth]

The attack described on this report is impossible to execute on any Strategy, the tx would reverte when the BalancerFlashLender contract tries to pull the WETH to repay the flashloan from the borrower (the strategy)

  function receiveFlashLoan(
      ...
  ) external {
      ...

      //@audit => As per the report, the execution would transfer the flashloaned funds to the `borrower`, which is a Strategy
      // Transfer the loan received to borrower
      IERC20(asset).safeTransfer(borrower, amount);

      //@audit => The execution would be forwarded to the borrower (The Strategy)
      if (
          IERC3156FlashBorrowerUpgradeable(borrower).onFlashLoan(
              borrower,
              tokens[0],
              amounts[0],
              feeAmounts[0],
              originalCallData
          ) != CALLBACK_SUCCESS
      ) {
          revert BorrowerCallbackFailed();
      }

      //@audit => At this point the tx will revert because the Strategy did not granted any allowance to this contract.
      //@audit => The Strategy contracts grants the exact required allowance to this contract as part of the execution flow of the `deploy()`, `undeploy()` & `harvest()`, which all those functions are only callable by the owner (The Vault).
      //@audit => So, unless the Strategy contract granted allowance to the BalancerFlashLender contract, the two below lines will cause the tx to be reverted.

      // Verify that this contract is able to pay the debt
      if (IERC20(asset).allowance(address(borrower), address(this)) < fee + amount) {
          revert NoAllowanceToPayDebt();
      }
      // Return the loan + fee to the vault
      IERC20(asset).safeTransferFrom(address(borrower), address(_balancerVault), amount + fee);
  }

The only way calls to the BalancerFlashLender.receiveFlashLoan() won't revert are when the borrower (A Strategy) contract has granted allowance to the BalancerFlashLender contract to pull the required WETH to repay the flashloan, and the only place where a Strategy grants allowance to this contract is on the execution flow of any of the functions that initiates a flashloan (deploy(), undeploy() & harvest()).

deploy() => wETH().approve(flashLenderA(), deltaDebt + fee)
undeploy() => wETH().approve(flashLenderA(), deltaDebt + fee)
harvest() => _adjustDebt() => wETH().approve(flashLenderA(), deltaDebt + fee)

For the above reasons, the described attack on the report doesn't cause any harm to the Strategies, thus it should be an invalid report.

[c4-judge]

0xleastwood marked the issue as primary issue

[0xleastwood]

I think the strategy to pull this off would look a little differently. You would be flash-loaning a zero amount so then both the amount and fee parameters would be zero in StrategyLeverage.onFlashLoan(). The target action would be FlashLoanAction.PAY_DEBT_WITHDRAW where data.originalAmount and data.receiver are controlled by the attacker. Hence, they could skim off excess collateral, or alternatively repay on behalf of the borrower (because Aave allows this) and then take all the collateral.

When the BalancerFlashLender contracts requests to transfer funds back to the flash loan contract and there is nothing to transfer, then it will not revert because it needs no approval to make a zero amount transfer. It doesn't appear either that balancer pools would reject zero amount flash loans.

This seems feasible even if the correct attack was not outlined by the wardens.

[0xleastwood]

I'm considering giving partial credit for not outlining this correctly but will think on this.

[c4-judge]

0xleastwood marked the issue as selected for report

[0xStalin-eth]

Hello Judge @0xleastwood
I see what you are saying, that's an interesting attack vector, unfortunately, because the Strategy contracts interact with the repay() on Aave, this attack vector can't be executed.

If we trace the execution of the target action FlashLoanAction.PAY_DEBT_WITHDRAW we have: StrategyLeverage.onFlashLoan() => StrategyLeverage._repayAndWithdraw() => StrategyAAVEv3._repay() => aaveV3.repay(), and, checking the aave contracts, we have: Pool.repay() => BorrowLogic.executeRepay() => ValidationLogic.validateRepay(), where we have a validation that prevents repayments of 0 amount.

The same is true for the target action FlashLoanAction.PAY_DEBT, its execution flow is: StrategyLeverage.onFlashLoan() => StrategyLeverage._payDebt() => StrategyAAVEv3._repay() => aaveV3.repay()

  function repay(
    address asset,
    uint256 amount,
    uint256 interestRateMode,
    address onBehalfOf
  ) public virtual override returns (uint256) {
    return
      BorrowLogic.executeRepay(
        _reserves,
        _reservesList,
        _usersConfig[onBehalfOf],
        DataTypes.ExecuteRepayParams({
          asset: asset,
          //@audit => Specified amount to be repaid
          amount: amount,
          interestRateMode: DataTypes.InterestRateMode(interestRateMode),
          onBehalfOf: onBehalfOf,
          useATokens: false
        })
      );
  }

  function executeRepay(
    mapping(address => DataTypes.ReserveData) storage reservesData,
    mapping(uint256 => address) storage reservesList,
    DataTypes.UserConfigurationMap storage userConfig,
    DataTypes.ExecuteRepayParams memory params
  ) external returns (uint256) {
    ...

    ValidationLogic.validateRepay(
      reserveCache,
      //@audit => Specified amount to be repaid
      params.amount,
      params.interestRateMode,
      params.onBehalfOf,
      stableDebt,
      variableDebt
    );
    ...
  }

  function validateRepay(
    DataTypes.ReserveCache memory reserveCache,
    //@audit => Specified amount to be repaid
    uint256 amountSent,
    ...
  ) internal view {
    //@audit => Not possible to repay 0, tx is reverted
    require(amountSent != 0, Errors.INVALID_AMOUNT);
    ...
  }

Now, knowing that Aave doesn't allow repayments of 0 amounts, checking the contracts of the protocol we'll realize that the specified amount to be repaid on Aave is the amount of the flashloan, so, if the flashloan is for 0 tokens, the repayment on Aave will revert.

    function onFlashLoan(
        address initiator,
        address token,
        //@audit-info => `amount` is the amount that was borrowed on the flashloan!
        uint256 amount,
        uint256 fee,
        bytes memory callData
    ) external returns (bytes32) {
       ...
        if (data.action == FlashLoanAction.SUPPLY_BOORROW) {
            ...
            // Use the Borrowed to pay ETH and deleverage
        } else if (data.action == FlashLoanAction.PAY_DEBT_WITHDRAW) {
            ...
            //@audit-info => `amount` is the amount that was borrowed on the flashloan!
            _repayAndWithdraw(data.originalAmount, amount, fee, payable(data.receiver));
        
        } else if (data.action == FlashLoanAction.PAY_DEBT) {
            ...
            //@audit-info => `amount` is the amount that was borrowed on the flashloan!
            _payDebt(amount, fee);
        }
        return _SUCCESS_MESSAGE;
    }

    function _repayAndWithdraw(
        uint256 withdrawAmountInETh,
        //@audit-info => flashloaned amount
        uint256 repayAmount,
        uint256 fee,
        address payable receiver
    ) internal {
        ...
        //@audit-info => Uses all the flashloaned amount to repay debt (WETH) in aave
        _repay(wETHA(), repayAmount);
    }

    function _payDebt(uint256 debtAmount, uint256 fee) internal {
        //@audit-info => Uses all the flashloaned amount to repay debt (WETH) in aave
        _repay(wETHA(), debtAmount);
    }

    //@audit => If the flashloaned amount is 0, the execution on Aave will blow up the entire tx.
    function _repay(address assetIn, uint256 amount) internal override virtual {
        ...
        //@audit => Requests a withdraw for the amount of flashloaned tokens
        if (aaveV3().repay(assetIn, amount, 2, address(this)) != amount) revert FailedToRepayDebt();
    }

For the above reason, I still stand with my original comment about this report being invalid because the attack vector can't be executed on this protocol.