Dandling approvals in `FlashLoanProvider` contract with Aave pools as spenders

[hats-bug-reporter[bot]]

Github username: @JacoboLansac Twitter username: jacolansac Submission hash (on-chain): 0xcb3eb220e001344633da7e393119832ae4512bd11117ffe8b3cb3b42c67df090 Severity: low

Description: When the FlashLoanProvider handles a flashloan callback with the executeOperation() function, there is a forceApprove() statement to approve the Aave pool to spend _flAmount+flFees from the FlashLoanProvider contract:

    /**
    * @notice Callback from Aave/Spark after receiving the flash-borrowed assets
    * @dev After validation, flashLoanCallback() is called on the caller of flashLoan().
    * @param assets The addresses of the flash-borrowed assets
    * @param amounts The amounts of the flash-borrowed assets
    * @param fees The fee of each flash-borrowed asset
    * @param initiator The address of the flashloan initiator
    * @param params The byte-encoded params passed when initiating the flashloan
    */
    function executeOperation(
        address[] calldata assets,
        uint256[] calldata amounts,
        uint256[] calldata fees,
        address initiator,
        bytes calldata params
    ) external override returns (bool) {
        // Can only be called by the Aave/Spark pool, and the FL can only ever be initiated by this contract.
        if (msg.sender != address(POOL)) revert CommonEventsAndErrors.InvalidAccess();
        if (initiator != address(this)) revert CommonEventsAndErrors.InvalidAddress(initiator);
        if (assets.length != 1 || amounts.length != 1 || fees.length != 1) revert CommonEventsAndErrors.InvalidParam();

        (IOrigamiFlashLoanReceiver flReceiver, bytes memory _params) = abi.decode(params, (IOrigamiFlashLoanReceiver, bytes));
        IERC20 _asset = IERC20(assets[0]);
        uint256 _flAmount = amounts[0];
        uint256 _flFees = fees[0];

        // Transfer the asset to the Origami FL receiver, and approve the repayment to Aave/Spark in full
        {
            _asset.safeTransfer(address(flReceiver), _flAmount);
@>          _asset.forceApprove(address(POOL), _flAmount + _flFees);
        }

        // Finally have the receiver handle the callback
        return flReceiver.flashLoanCallback(_asset, _flAmount, _flFees, _params);
    }
}

However, this approval is unnecessary, because the funds are transferred to the flReceiver just in the previous line (so the flash-loaned tokens are not in this contract anymore). Moreover, the flashloan + fees are repaid by the flReceiver, as can be seen at the end of OrigamiLovTokenFlashAndBorrowManager:flashLoanCallback():

     /**
     * @notice Invoked from IOrigamiFlashLoanProvider once a flash loan is successfully
     * received, to the msg.sender of `flashLoan()`
     * @param token The ERC20 token which has been borrowed
     * @param amount The amount which has been borrowed
     * @param fee The flashloan fee amount (in the same token)
     * @param params Client specific abi encoded params which are passed through from the original `flashLoan()` call
     */
    function flashLoanCallback(
        IERC20 token,
        uint256 amount,
        uint256 fee,
        bytes calldata params
    ) external override returns (bool) {
        if (msg.sender != address(flashLoanProvider)) revert CommonEventsAndErrors.InvalidAccess();
        if (address(token) != address(_debtToken)) revert CommonEventsAndErrors.InvalidToken(address(token));

        // Decode the type & params and call the relevant callback function.
        // Each function must result in the `amount + fee` sitting in this contract such that it can be
        // transferred back to the flash loan provider.
        (RebalanceCallbackType _rebalanceType, bool force, bytes memory _rebalanceParams) = abi.decode(
            params, 
            (RebalanceCallbackType, bool, bytes)
        );

        if (_rebalanceType == RebalanceCallbackType.REBALANCE_DOWN) {
            (RebalanceDownParams memory _rdParams) = abi.decode(_rebalanceParams, (RebalanceDownParams));
            _rebalanceDownFlashLoanCallback(
                amount, 
                fee, 
                _rdParams,
                force
            );
        } else {
            (RebalanceUpParams memory _ruParams) = abi.decode(_rebalanceParams, (RebalanceUpParams));
            _rebalanceUpFlashLoanCallback(
                amount, 
                fee, 
                _ruParams,
                force
            );
        }

        // Transfer the total flashloan amount + fee back to the `flashLoanProvider` for repayment
@>      _debtToken.safeTransfer(msg.sender, amount+fee);
        return true;
    }

This means that at the end of each executeOperation() call, there will be dandling approvals to the Aave pool contract to spend the flashloan+fees tokens on behalf of the FlashLoanProvider contract.

Impact: low

The impact is low for two reasons:

• There are no functions in the Aave pool that can leverage this dandling approval
• No funds are sitting in the FlashLoanProvider for more than one transaction, so there is nothing to steal

Recommendation

I would still recommend removing this approval, in case some other changes made to the code could suddenly make this dandling approval not-harmless

    function executeOperation(
        address[] calldata assets,
        uint256[] calldata amounts,
        uint256[] calldata fees,
        address initiator,
        bytes calldata params
    ) external override returns (bool) {
        // Can only be called by the Aave/Spark pool, and the FL can only ever be initiated by this contract.
        if (msg.sender != address(POOL)) revert CommonEventsAndErrors.InvalidAccess();
        if (initiator != address(this)) revert CommonEventsAndErrors.InvalidAddress(initiator);
        if (assets.length != 1 || amounts.length != 1 || fees.length != 1) revert CommonEventsAndErrors.InvalidParam();

        (IOrigamiFlashLoanReceiver flReceiver, bytes memory _params) = abi.decode(params, (IOrigamiFlashLoanReceiver, bytes));
        IERC20 _asset = IERC20(assets[0]);
        uint256 _flAmount = amounts[0];
        uint256 _flFees = fees[0];

        // Transfer the asset to the Origami FL receiver, and approve the repayment to Aave/Spark in full
        {
            _asset.safeTransfer(address(flReceiver), _flAmount);
-           _asset.forceApprove(address(POOL), _flAmount + _flFees);
        }

[frontier159]

I don't think this one is true - we need to approve the aave POOL to pull tokens, as it does at the end of the flashloan.

If you remove the line as you suggest then re-running tests shows failures: forge test --mc StEth

[JacoboLansac]

You are totally right, my bad. It is invalid.

In the following line, I misunderstood that msg.sender (flashLoanProvider) was the actual pool providing the flashloan (instead of the origami flashloan provider), and that the loan was already repaid after that line.

// Transfer the total flashloan amount + fee back to the `flashLoanProvider` for repayment
@>      _debtToken.safeTransfer(msg.sender, amount+fee);

I should have changed the line and run the tests myself, sorry.