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

4 stars 4 forks source link

_repayAndWithdraw() _pendingAmount Incorrect calculation #35

Closed c4-bot-7 closed 3 months ago

c4-bot-7 commented 4 months ago

Lines of code

https://github.com/code-423n4/2024-05-bakerfi/blob/59b1f70cbf170871f9604e73e7fe70b70981ab43/contracts/core/strategies/StrategyLeverage.sol#L712

Vulnerability details

Vulnerability details

when user call vault.withdraw() will execute _repayAndWithdraw()

    function _repayAndWithdraw(
        uint256 withdrawAmountInETh,
        uint256 repayAmount,
        uint256 fee,
        address payable receiver
    ) internal {

        (uint256 collateralBalance,) = _getMMPosition();
        uint256 convertedAmount = _fromWETH(withdrawAmountInETh);
        uint256 withdrawAmount = collateralBalance > convertedAmount ? convertedAmount : collateralBalance;

@>      _repay(wETHA(), repayAmount);                        

@>      _withdraw(ierc20A(), withdrawAmount, address(this));

        // Convert Collateral to WETH
        uint256 wETHAmount = _convertToWETH(withdrawAmount);
        // Calculate how much ETH i am able to withdraw
@>      uint256 ethToWithdraw = wETHAmount - repayAmount - fee;
        // Unwrap wETH
        _unwrapWETH(ethToWithdraw);
        // Withdraw ETh to Receiver
        payable(receiver).sendValue(ethToWithdraw);
        emit StrategyUndeploy(msg.sender, ethToWithdraw);
@>      _pendingAmount = ethToWithdraw;
    }

This method calculates _pendingAmount, which represents the number of assets that AAVE has removed this time. Current formula: _pendingAmount = ethToWithdraw = wETHAmount - repayAmount - fee.

The correct formula is _pendingAmount = wETHAmount - repayAmount.

Because, during execution, funds are deposited and withdrawn as follows:

  1. _repay(wETHA(), repayAmount).
  2. _withdraw(ierc20A(), withdrawAmount, address(this)).

The above AAVE flow of funds has nothing to do with fee.

The fee is split from the wETHAmount and given to the Balancer to pay the fees for the flash loan, which shouldn't be counted in this formula.

Impact

_pendingAmount will eventually be used in undeploy() : _deployedAmount -= _pendingAmount And _deployedAmount is used in harvest() to calculate balanceChange, which affects the fee calculation of rebalance(). Wrong _pendingAmount leads to wrong fee calculation

Recommended Mitigation

    function _repayAndWithdraw(
        uint256 withdrawAmountInETh,
        uint256 repayAmount,
        uint256 fee,
        address payable receiver
    ) internal {

        (uint256 collateralBalance,) = _getMMPosition();
        uint256 convertedAmount = _fromWETH(withdrawAmountInETh);
        uint256 withdrawAmount = collateralBalance > convertedAmount ? convertedAmount : collateralBalance;

        _repay(wETHA(), repayAmount);                        

        _withdraw(ierc20A(), withdrawAmount, address(this));

        // Convert Collateral to WETH
        uint256 wETHAmount = _convertToWETH(withdrawAmount);
        // Calculate how much ETH i am able to withdraw
        uint256 ethToWithdraw = wETHAmount - repayAmount - fee;
        // Unwrap wETH
        _unwrapWETH(ethToWithdraw);
        // Withdraw ETh to Receiver
        payable(receiver).sendValue(ethToWithdraw);
        emit StrategyUndeploy(msg.sender, ethToWithdraw);
-       _pendingAmount = ethToWithdraw;
+       _pendingAmount = wETHAmount - repayAmount;
    }

Assessed type

Context

c4-judge commented 3 months ago

0xleastwood marked the issue as primary issue

hvasconcelos commented 3 months ago

The fee must be accommodated from the amount we withdraw from the vault. We keep the fee on the vault to pay the flash loan.

0xleastwood commented 3 months ago

Need to reserve the fee because otherwise the flash loan cannot be repaid.

c4-judge commented 3 months ago

0xleastwood marked the issue as unsatisfactory: Invalid