search

code-423n4 / 2024-05-bakerfi-findings

0 stars 0 forks source link

BalancerFlashLender#receiveFlashLoan does not validate the originalCallData #2

Open c4-bot-7 opened 1 month ago

c4-bot-7 commented 1 month ago

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

stalinMacias commented 1 month ago

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 commented 4 weeks ago

0xleastwood marked the issue as primary issue

0xleastwood commented 3 weeks ago

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 commented 3 weeks ago

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

c4-judge commented 3 weeks ago

0xleastwood marked the issue as selected for report

stalinMacias commented 3 weeks ago

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.

zhaojio commented 3 weeks ago

The key issue is that an attacker can pass arbitrary FlashLoanData into onFlashLoan.

An attacker can initiate Flash Loan with x Amount and call the _supplyBorrow or _repayAndWithdraw function with y amount of data.originalAmount

// data.originalAmount can be any value , amount is the amount of FlashLoan initiated by the attacker
 _supplyBorrow(data.originalAmount, amount, fee);
stalinMacias commented 3 weeks ago

@zhaojio Yes, data.originalAmount can be crafted by an attacer that initiates a flashloan directly on Balancer, this is true, but the problem with this is that amount is the amount of funds that were flashloaned from balancer, and:

  1. If amount != 0, the call reverts when the BalancerFlashLender contract tries to pull amount + fees from the Strategy contract because it has 0 allowance, this allowance is granted only when the call goes through the functions of the Strategy contracts that initiates a flashloan, see this comment for the full details
  2. if amount is 0, the call reverts on Aave, because Aave reverts withdrawals of 0 amount, see this comment for full details.

Then, if any possible combination for the flashloaned amount ends up causing the tx to revert, the attack vector is not actually possible to be executed on these contracts, as such, the reported problem is invalid.

zhaojio commented 3 weeks ago

I checked it again. In most protocols, it is difficult to guarantee that all the tokens approved will be used up.

If the borrower(StrategyLeverage) has tokens that are not used up, the attacker can execute the attack. This is possible due to a division exception or an error in the oracle price

In short, allowing FlashLoads to be initiated from other contracts is wrong and dangerous and needs to be avoided.

stalinMacias commented 3 weeks ago

@zhaojio

If the borrower(StrategyLeverage) has tokens that are not used up, the attacker can execute the attack. Where exactly is the division that would leave unspent allowance?

All the allowance that the Strategy grants to the BalancerFlashLender contract is consumed at the end of the flashloan, when the BalancerFlashLender pulls the amount + fee to repay Balancer for the flashloan. Allowance is granted here:

And allowance is consumed here:

As we can see, the exact same allowance granted, is the exact same allowance consumed, no leftovers.

In short, allowing FlashLoads to be initiated from other contracts is wrong and dangerous and needs to be avoided.

Correct, there are different ways to prevent flashloans to be initiated from arbitrary accounts, in the case of this protocol, this is prevented by ensuring that the BalancerFlashLender contract has enough allowance to transfer from the Strategy, if it does not, it means that the flashloan was not initiated by the Strategy itself.

zhaojio commented 3 weeks ago 
    function flashFee(address, uint256 amount) external view override returns (uint256) {
        uint256 perc = _balancerVault.getProtocolFeesCollector().getFlashLoanFeePercentage();
        if (perc == 0 || amount == 0) {
            return 0;
        }

        return (amount * perc) / _BALANCER_MAX_FEE_PERCENTAGE;
    }
zhaojio commented 3 weeks ago

@stalinMacias As an Auditor, we should focus on finding as many bugs as possible rather than pointing out errors in other people's reports. It is the judge's job to find errors in the report. If there are errors in the report, but it makes sense to the project side, the judge can decide whether to pass the verification.

stalinMacias commented 3 weeks ago 
    function flashFee(address, uint256 amount) external view override returns (uint256) {
        uint256 perc = _balancerVault.getProtocolFeesCollector().getFlashLoanFeePercentage();
        if (perc == 0 || amount == 0) {
            return 0;
        }

        return (amount * perc) / _BALANCER_MAX_FEE_PERCENTAGE;
    }

This is a separate problem that has been reported on #11 . But this fact was never mentioned in this report, so, I doubt it would classify as a dupe of that report.

As an Auditor, we should focus on finding as many bugs as possible rather than pointing out errors in other people's reports.

As auditors, especially in contests, it is also our duty to help the judge by sharing all the information we can about the validity of a report, that's the purpose of the pjqa. More often than not, trying to fix something when there is not really a problem can end up introducing a real bug in the codebase

All in all, I've made my points and shared all the information I wanted to share with the judge. I'll let the judge take it from here.

zhaojio commented 3 weeks ago

More often than not, trying to fix something when there is not really a problem can end up introducing a real bug in the codebase

I doubt this claim, developers are not considering situations where flash loads can be originated from other contracts, and it is safer to prohibit flash loads from other contracts.

0xleastwood commented 2 weeks ago

Reading through all these comments, i agree that _repayAndWithdraw() would fail when a zero amount repayment is made, so the only action that is really possible is _supplyBorrow() which would require some funds to already be in the contract. Unlikely for this to ever be the case because the contract doesn't normally hold funds that aren't being put to use in someway, but do correct me if this assumption is incorrect.

So i'm not sure how this issue can be exploited even if i do agree that it is an issue. For the time being, i will downgrade this to medium severity because it is obvious this is not intended behavour even if it may not lead to funds being lost.

c4-judge commented 2 weeks ago

0xleastwood changed the severity to 2 (Med Risk)

stalinMacias commented 2 weeks ago

Hey @0xleastwood , thanks for reading all of our comments. As you mentioned, the Strategy contracts are not expected to hold funds, all funds are deposited into Aave.

If funds are sitting on the Strategy, then, that is caused due to a different bug.

And, assuming there are any funds left on the Strategy, by executing this attack vector and calling the action _supplyBorrow(), what will happen is that those leftover funds would be deposited to Aave, and the attacker would get nothing in return, all the opposite, the attacker would practically be gifting gas for putting to work those funds on behalf of the strategy.

While I do agree that this may not be intended behavior, I can't see any loss of funds, nor any disruption to the contract's functionalities, because of this, I'm more inclined to see this report as a QA due to the lack of impact.

Thanks for taking the time to walk through my comments on this report.

stalinMacias commented 2 weeks ago

@zhaojio You may want to double-check report #11

The rounding difference can cause the tx to revert, not to leave unused allowance.

Balancer rounds up the fees to charge for the flashloan, while, the BalancerFlashFee contract rounds down. This rounding difference can potentially compute the allowance to grant between the Strategy and the BalancerFlashFee contract to be lower than what is required, thus, causing the tx to revert.

ickas commented 2 weeks ago

Fixed → https://github.com/baker-fi/bakerfi-contracts/pull/50

0xleastwood commented 2 weeks ago

Thank you for all your comments, but this is clearly unintended behaviour and should be fixed even if it is not currently exploitable. I believe medium severity is still justified.