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

4 stars 4 forks source link

Rounding-down of `flashFee` can result in calls to flash loan to revert #11

Open c4-bot-1 opened 5 months ago

c4-bot-1 commented 5 months ago

Lines of code

https://github.com/code-423n4/2024-05-bakerfi/blob/main/contracts/core/flashloan/BalancerFlashLender.sol#L63 https://github.com/code-423n4/2024-05-bakerfi/blob/main/contracts/core/strategies/StrategyLeverage.sol#L247-L249

Vulnerability details

Description

The BalancerFlashLender::flashFee() function returns a rounded-down fee. This fee is then later used across the protocol while providing spend approval to the flash loan provider. This can result in an approval less than that expected by the provider and hence cause the call to flash loan to revert. This is because flash loan providers calculate their fee by rounding up in their favour, instead of rounding down:

File: contracts/core/flashloan/BalancerFlashLender.sol

    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;
    }

and

File: contracts/core/strategies/StrategyLeverage.sol

    function deploy() external payable onlyOwner nonReentrant returns (uint256 deployedAmount) {
        if (msg.value == 0) revert InvalidDeployAmount();
        // 1. Wrap Ethereum
        address(wETHA()).functionCallWithValue(abi.encodeWithSignature("deposit()"), msg.value);
        // 2. Initiate a WETH Flash Loan
        uint256 leverage = calculateLeverageRatio(
            msg.value,
            getLoanToValue(),
            getNrLoops()
        );
        uint256 loanAmount = leverage - msg.value;
@--->   uint256 fee = flashLender().flashFee(wETHA(), loanAmount);
        //§uint256 allowance = wETH().allowance(address(this), flashLenderA());
@--->   if(!wETH().approve(flashLenderA(), loanAmount + fee)) revert FailedToApproveAllowance();
        if (
            !flashLender().flashLoan(
                IERC3156FlashBorrowerUpgradeable(this),
                wETHA(),
                loanAmount,
                abi.encode(msg.value, msg.sender, FlashLoanAction.SUPPLY_BOORROW)
            )
        ) {
            revert FailedToRunFlashLoan();
        }

        deployedAmount = _pendingAmount;
        _deployedAmount = _deployedAmount + deployedAmount;
        emit StrategyAmountUpdate(_deployedAmount);
        // Pending amount is not cleared to save gas
        // _pendingAmount = 0;
    }

Impact

Flash loan call reverts for many amount and fee percentage combinations.

Tools Used

Manual review

Recommended Mitigation Steps

Round up in favour of the protocol. A library like solmate can be used which has mulDivUp:

    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;
+       return amount.mulDivUp(perc, _BALANCER_MAX_FEE_PERCENTAGE);
    }

Assessed type

Math

c4-judge commented 5 months ago

0xleastwood marked the issue as primary issue

c4-judge commented 4 months ago

0xleastwood marked the issue as selected for report

ickas commented 4 months ago

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