ctf_sec - In Redeemer.sol contract, allowance is not properly given to underlying contract before redeeming.

[sherlock-admin]

ctf_sec

high

In Redeemer.sol contract, allowance is not properly given to underlying contract before redeeming.

Summary

In Redeemer.sol contract, allowance is not properly given to underlying contract before redeeming.

Vulnerability Detail

Note that in Lender.sol, we have this function

    /// @notice bulk approves the usage of addresses at the given ERC20 addresses.
    /// @dev the lengths of the inputs must match because the arrays are paired by index
    /// @param u array of ERC20 token addresses that will be approved on
    /// @param a array of addresses that will be approved
    /// @return true if successful
    function approve(address[] calldata u, address[] calldata a)
        external
        authorized(admin)
        returns (bool)
    {
        for (uint256 i; i != u.length; ) {
            IERC20 uToken = IERC20(u[i]);
            if (address(0) != (address(uToken))) {
                Safe.approve(uToken, a[i], type(uint256).max);
            }
            unchecked {
                ++i;
            }
        }
        return true;
    }

this is important because this function needs to be properly called so the underlying smart contract has allowance.

The illuminate integrates with 9 protocol, the lending flow is that the user transfer fund to the lender.sol lender interact with external contract, external contract transfer the fund that user supply out and then mint someting to lender. This is the step where we need allowance.

We need to approve that an external contract can use the lender's fund.

I think the same case applies to the redeemer.sol

When redeeming, the we transfer the fund from lender.sol to the redeemer

      // Cache the lender to save gas on sload
        address cachedLender = lender;

        // Get the amount to be redeemed
        uint256 amount = IERC20(principal).balanceOf(cachedLender);

        // Receive the principal token from the lender contract
        Safe.transferFrom(
            IERC20(principal),
            cachedLender,
            address(this),
            amount
        );

then the external contract burn the token and return us other token. To let external contract burn our token, we need to give proper allowance.

However, the only approve function in Redeemer.sol is:

    /// @notice approves the converter to spend the compounding asset
    /// @param i an interest bearing token that must be approved for conversion
    function approve(address i) external authorized(marketPlace) {
        if (i != address(0)) {
            Safe.approve(IERC20(i), address(converter), type(uint256).max);
        }

I believe this is not sufficient, if the external contract has insufficient allowance to burn out token, redeem fails.

Impact

Let us go over an example using the Sense finance redeem.

This is what is called for Sense finance redeem.

// Redeem the tokens from the Sense contract
ISenseDivider(divider).redeem(a, s, amount);

which calls:

https://github.com/sense-finance/sense-v1/blob/7b37dec129dbf207a2d2ac2469ced7c75b157691/pkg/core/src/Divider.sol#L305

    /// @notice Burn PT of a Series once it's been settled
    /// @dev The balance of redeemable Target is a function of the change in Scale
    /// @param adapter Adapter address for the Series
    /// @param maturity Maturity date for the Series
    /// @param uBal Amount of PT to burn, which should be equivalent to the amount of Underlying owed to the caller
    function redeem(
        address adapter,
        uint256 maturity,
        uint256 uBal
    ) external nonReentrant whenNotPaused returns (uint256 tBal) {
        // If a Series is settled, we know that it must have existed as well, so that check is unnecessary
        if (!_settled(adapter, maturity)) revert Errors.NotSettled();

        uint256 level = adapterMeta[adapter].level;
        if (level.redeemRestricted() && msg.sender != adapter) revert Errors.RedeemRestricted();

        // Burn the caller's PT
        Token(series[adapter][maturity].pt).burn(msg.sender, uBal);

note the line:

Token(series[adapter][maturity].pt).burn(msg.sender, uBal);

We does not give the sufficient allowance for token to burn!!

I think the same situation applies to rest of the redeem function including Swivel, Yield, Element, Pendle, APWine, Tempus and Notional protocols if the underlying contract burn our token or transfer out fund!

Code Snippet

https://github.com/sherlock-audit/2022-10-illuminate/blob/main/src/Redeemer.sol#L276-L324

https://github.com/sherlock-audit/2022-10-illuminate/blob/main/src/Redeemer.sol#L368-L377

Tool used

Manual Review

Recommendation

We recommend the project add the function in redeemer as well so admin can give proper allowance for redeeming!

Or Please approve before redeeming for each redeem operation.

    function approve(address[] calldata u, address[] calldata a)
        external
        authorized(admin)
        returns (bool)
    {
        for (uint256 i; i != u.length; ) {
            IERC20 uToken = IERC20(u[i]);
            if (address(0) != (address(uToken))) {
                Safe.approve(uToken, a[i], type(uint256).max);
            }
            unchecked {
                ++i;
            }
        }
        return true;
    }

[sourabhmarathe]

The admin has a method specifically for approving the transfer of principal tokens. It is to be called by the admin:

    function approve(
        address u,
        uint256 m,
        address r
    ) external authorized(admin) returns (bool);