When harvesting a strategy and adjusting the debt, all the leftover collateral that is not used to swap the withdrawn collateral from Aave for WETH to repay the flashloan will be locked and lost in the Strategy contract

[c4-bot-4]

Lines of code

https://github.com/code-423n4/2024-05-bakerfi/blob/main/contracts/core/strategies/StrategyLeverage.sol#L545-L547 https://github.com/code-423n4/2024-05-bakerfi/blob/main/contracts/core/hooks/UseSwapper.sol#L95-L97

Vulnerability details

Impact

Collateral can be locked and lost in the Strategy contract.

Proof of Concept

When harvesting a strategy and adjusting the debt to maintain the loan to value of the strategy, the strategy does the following steps:

1. Computes the deltaDebt required to readjust the loan to value within the accepted boundaries.
2. Takes a WETH flashloan on Balancer for the exact deltaDebt amount
3. Repays WETH on Aave for the exact amount that was flashloaned borrowed on Balancer
4. Uses the UniQuoterV2 to compute the amount of collateral needed to repay the flashloan (including the flashloan fees)
5. Withdraws collateral from Aave for exact amount computed by the UniQuoterV2
6. Does an EXACT_OUTPUT swap on Uniswap. It requests to receive the exact debtAmount + fees (to repay the flashloan) in exchange for at most the withdrawn amount of collateral from Aave
7. Does a couple of extra checks and finally the flashloan is repaid.

The problem identified on this report is caused due to some issues in the steps 4 & 6. Let's dive into it.

The first part of the problem is caused due to the way how the UniQuoter is invoked. The fee of the pool that is sent to the UniQuoter is hardcoded to be 500, which represents a pool of (0.05% fee). This can cause two problems:

1. The execution can be reverted if there is not an existing pool for the COLLATERAL/WETH at a 0.05% fee. The UniQuoter will receive the call and will derive the address of the pool based on the tokenIn, tokenOut and fee. If there is not a pool for the 0.05% fee (500), the call will be reverted and the whole harvesting execution will blown up.

2. The second problem is when the swapFeeTier is different than 500, or in other words, that the fee of the UniPool that is configured for the strategy is different than 500 (0.05%), for example, if the strategy is configured to work with a pool with a fee of 0.01% (100).

   • In this case, the execution won't revert, but the computed amount will be bigger than what is really required. For example
   • The debtAmount + fee to repay the flashloan is 100WETH.
   • The UniQuoter will compute how much collateral is required to get 100WETH by swapping the collateral on a UniPool with a 0.05% fee. to make calculations easier, assume collateral and weth have a 1:1 conversion.
   • 100 collateral + 0.05% fee charged by the pool ===> 100 + 0.5 ===> 100.5 Collateral.
   • Then, the execution will withdraw from Aave the computed amount by the UniQutoer (100.5).
   • Now, once the Strategy has the 100.5 collateral on its balance, the execution will do a swap requesting 100 WETH to repay the flashloan. When requesting the swap, the fees of the pool where the swap will be actually executed is set by using the swapFeeTier. Assume the Strategy is configured to work with the UniPool with the lowest fee available (0.01%).
   • To do an EXACT_OUTPUT swap on a pool with 0.01% fee for 100 WETH, the required amount of tokenIn (collateral) will be:
     • 100 WETH + 0.01% fee charged by the pool ===> 100 + 0.1 ===> 100.1 Collateral.
   • This means, after doing the swap for WETH to repay the flashloan, the Strategy will have on its balance a total of 0.4 leftover collateral that was not used during the swap.

StrategyLeverage._payDebt() function

function _payDebt(uint256 debtAmount, uint256 fee) internal {
  ...

  // Get a Quote to know how much collateral i require to pay debt
  (uint256 amountIn, , , ) = uniQuoter().quoteExactOutputSingle(
      //@audit-issue => The computed `amountIn` is based on a pool with fees of 0.05%!
      IQuoterV2.QuoteExactOutputSingleParams(ierc20A(), wETHA(), debtAmount + fee, 500, 0)
  );    

  //@audit-info => Withdraws the exact computed `amountIn` by the UniQuoter
  _withdraw(ierc20A(), amountIn, address(this) );

  uint256 output = _swap(
      ISwapHandler.SwapParams(
          ierc20A(),
          wETHA(),
          ISwapHandler.SwapType.EXACT_OUTPUT,
          amountIn,
          debtAmount + fee,
          //@audit-info => The swap is performed on a pool with this fees
          //@audit-issue => When this value is lower than 500 (Using a pool with a lower fee), not all the withdrawn collateral will be used for the swap!
          _swapFeeTier,
          bytes("")
      )
  );
  ...
}

Now it comes the second part of the problem, the Strategy checks if there is any leftover collateral after the swap, and if there is any, it does a self transfer for the leftover amount. This can cause one of these two problems:

1. The most problematic is that the leftover collateral will simply be left in the Strategy, it won't be re-supplied to Aave, neither pull out of the Strategy, it will be simply left in the Strategy's balance, from where it will be irrecoverable, meaning, the leftover collateral will be locked in the Strategy contract.

2. Depending on the Collateral's contract, there are some ERC20s that reverts the execution if they receive a self-transfer of tokens.

UseSwapper._swap() function

function _swap(
    ISwapHandler.SwapParams memory params
) internal override returns (uint256 amountOut) {
    ...

    // Exact Input
    if (params.mode == ISwapHandler.SwapType.EXACT_INPUT) {
        ...
        // Exact Output
    } else if (params.mode == ISwapHandler.SwapType.EXACT_OUTPUT) {
      //@audit-info => Does an EXACT_OUTPUT swap
      //@audit-info => `amountIn` represents the exact amount of collateral that was required to swap the requested amount of WETH to repay the flashloan!
        uint256 amountIn = _uniRouter.exactOutputSingle(
            IV3SwapRouter.ExactOutputSingleParams({
                tokenIn: params.underlyingIn,
                tokenOut: params.underlyingOut,
                fee: fee,
                recipient: address(this),
                amountOut: params.amountOut,
                amountInMaximum: params.amountIn,
                sqrtPriceLimitX96: 0
            })
        );

      //@audit-issue => Self transfering the leftover collateral after the swap. This leftover collateral will be left in the Strategy's balance, causing it to be unnusable.
        if (amountIn < params.amountIn) {
            IERC20(params.underlyingIn).safeTransfer(address(this), params.amountIn - amountIn);
        }

        ...
    }
}

To recapitulate the most important points, the biggest impact because of the two problems on steps 4 & 6 is when the UniPool configured for the strategy uses a lower fee than 0.05% (500). In this case, the leftover collateral after doing the EXACT_OUTPUT swap for the required amount of WETH to repay the flashloan will be left and locked in the Strategy.

Tools Used

Manual Audit, Uniswap Pool's Explorer & UniV2Quoter contract

Recommended Mitigation Steps

To address this problem, I'd recommend to apply the two below suggestions.

1. Do not set a hardcoded value for the pool fee when calling the UniQuoter, instead, send the same value of the configured pool (swapFeeTier)
2. Instead of doing the self transfer of the leftover collateral after the swap, opt to re-supply it to Aave. In this way, that leftover collateral can still be managed by the Strategy.

Assessed type

Context

[c4-judge]

0xleastwood marked the issue as duplicate of #34

[c4-judge]

0xleastwood marked the issue as selected for report

[c4-judge]

0xleastwood marked the issue as not a duplicate

[c4-judge]

0xleastwood changed the severity to 2 (Med Risk)

[0xleastwood]

This seems to predominantly impact yield in two ways:

• Harvest function fails to be callable, but users can still withdraw collateral.
• Harvest does not fail but there is some value leakage that happens over time.

Neither of these impact user's funds directly so medium severity seems right.

[stalinMacias]

Hello Judge @0xleastwood

I'd like to clarify the second point raised in your comment to downgrade the severity of this report to medium

Harvest does not fail but there is some value leakage that happens over time. Neither of these impact user's funds directly so medium severity seems right.

Actually, when harvest does not fail, and, causes the leftover collateral to be left sitting on the protocol, those funds are actually the funds deposited by the users. While it is true that the leakage happens over time, those funds are user funds, not only yield.

I'd like to ask if you could take a second look at your verdict for the severity of this report and if you would consider re-assigning the original severity based on this clarification

[ickas]

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

[0xleastwood]

Hello Judge @0xleastwood

I'd like to clarify the second point raised in your comment to downgrade the severity of this report to medium

Harvest does not fail but there is some value leakage that happens over time. Neither of these impact user's funds directly so medium severity seems right.

Actually, when harvest does not fail, and, causes the leftover collateral to be left sitting on the protocol, those funds are actually the funds deposited by the users. While it is true that the leakage happens over time, those funds are user funds, not only yield.

I'd like to ask if you could take a second look at your verdict for the severity of this report and if you would consider re-assigning the original severity based on this clarification

I see what you mean, even though the amount is somewhat on the smaller side, a debt adjustment will leave some excess collateral stuck as it rebalances to maintain a target LTV.

[c4-judge]

0xleastwood changed the severity to 3 (High Risk)