Illuminate contest - WatchPug's results

Lines of code

https://github.com/code-423n4/2022-06-illuminate/blob/912be2a90ded4a557f121fe565d12ec48d0c4684/lender/Lender.sol#L400-L413

Vulnerability details

uint256 returned;
{
    // Add the accumulated fees to the total
    uint256 fee = calculateFee(a);
    fees[u] += fee;

    address[] memory path = new address[](2);
    path[0] = u;
    path[1] = principal;

    // Swap on the Pendle Router using the provided market and params
    returned = IPendle(pendleAddr).swapExactTokensForTokens(a - fee, r, path, address(this), d)[0];

}

The first item ([0]) of the returned amounts is used as returned.

That's the amountIn of the swap, which will always == a, "amount of principal tokens to lend", while the amountOut is the one that we should be using for minting zcTokens.

Recommendation

Change to:

uint256 returned;
{
    // Add the accumulated fees to the total
    uint256 fee = calculateFee(a);
    fees[u] += fee;

    address[] memory path = new address[](2);
    path[0] = u;
    path[1] = principal;

    // Swap on the Pendle Router using the provided market and params
    returned = IPendle(pendleAddr).swapExactTokensForTokens(a - fee, r, path, address(this), d)[1];

}

Lines of code

https://github.com/code-423n4/2022-06-illuminate/blob/912be2a90ded4a557f121fe565d12ec48d0c4684/lender/Lender.sol#L433-L473

Vulnerability details

https://github.com/code-423n4/2022-06-illuminate/blob/912be2a90ded4a557f121fe565d12ec48d0c4684/lender/Lender.sol#L433-L473

function lend(
    uint8 p,
    address u,
    uint256 m,
    uint256 a,
    uint256 r,
    uint256 d,
    address t,
    address x
) public unpaused(p) returns (uint256) {
    {
        // Instantiate market and tokens
        address principal = IMarketPlace(marketPlace).markets(u, m, p);
        if (ITempus(principal).yieldBearingToken() != IERC20Metadata(u)) {
            revert NotEqual('underlying');
        } else if (ITempus(principal).maturityTime() > m) {
            revert NotEqual('maturity');
        }

        // Get the underlying token
        IERC20 underlyingToken = IERC20(u);

        // Transfer funds from user to Illuminate, Scope to avoid stack limit
        Safe.transferFrom(underlyingToken, msg.sender, address(this), a);
    }

    // Add the accumulated fees to the total
    uint256 fee = calculateFee(a);
    fees[u] += fee;

    // Swap on the Tempus Router using the provided market and params
    IERC5095 illuminateToken = IERC5095(principalToken(u, m));
    uint256 returned = ITempus(tempusAddr).depositAndFix(Any(x), Any(t), a - fee, true, r, d) -
        illuminateToken.balanceOf(address(this));

    // Mint Illuminate zero coupons
    illuminateToken.mint(msg.sender, returned);

    emit Lend(p, u, m, returned);
    return returned;
}

At L465, returned should not include - illuminateToken.balanceOf(address(this)).

With the current implementation, the attacker can send some illuminateToken to the Lender.sol contract and make every users to receive less.

Some lend() transactions may also revert if ITempus(tempusAddr).depositAndFix(Any(x), Any(t), a - fee, true, r, d) returns less than illuminateToken.balanceOf(address(this)).

Lines of code

https://github.com/code-423n4/2022-06-illuminate/blob/912be2a90ded4a557f121fe565d12ec48d0c4684/lender/Lender.sol#L317-L367

Vulnerability details

function lend(
    uint8 p,
    address u,
    uint256 m,
    uint256 a,
    uint256 r,
    uint256 d,
    address e,
    bytes32 i
) public unpaused(p) returns (uint256) {
    // Get the principal token for this market for element
    address principal = IMarketPlace(marketPlace).markets(u, m, p);

    // the element token must match the market pair
    if (IElementToken(principal).underlying() != u) {
        revert NotEqual('underlying');
    } else if (IElementToken(principal).unlockTimestamp() > m) {
        revert NotEqual('maturity');
    }
    // Transfer underlying token from user to illuminate
    Safe.transferFrom(IERC20(u), msg.sender, address(this), a);

    // Track the accumulated fees
    fees[u] += calculateFee(a);

    uint256 purchased;
    {
        // Create the variables needed to execute an element swap
        Element.FundManagement memory fund = Element.FundManagement({
            sender: address(this),
            recipient: payable(address(this)),
            fromInternalBalance: false,
            toInternalBalance: false
        });

        Element.SingleSwap memory swap = Element.SingleSwap({
            userData: '0x00000000000000000000000000000000000000000000000000000000000000',
            poolId: i,
            amount: a - calculateFee(a),
            kind: Element.SwapKind.In,
            assetIn: Any(u),
            assetOut: Any(principal)
        });

        // Conduct the swap on element
        purchased = IElement(e).swap(swap, fund, r, d);
    }

    emit Lend(p, u, m, purchased);
    return purchased;
}

Based on the context, it's pretty clear that the current implementation forgot to call IERC5095(illuminateToken).mint(msg.sender, purchased);.

Lines of code

https://github.com/code-423n4/2022-06-illuminate/blob/912be2a90ded4a557f121fe565d12ec48d0c4684/redeemer/Redeemer.sol#L158-L198

Vulnerability details

function redeem(
    uint8 p,
    address u,
    uint256 m
) public returns (bool) {
    // Get the principal token that is being redeemed by the user
    address principal = IMarketPlace(marketPlace).markets(u, m, p);

    // Make sure we have the correct principal
    if (
        p != uint8(MarketPlace.Principals.Swivel) &&
        p != uint8(MarketPlace.Principals.Element) &&
        p != uint8(MarketPlace.Principals.Yield) &&
        p != uint8(MarketPlace.Principals.Notional)
    ) {
        revert Invalid('principal');
    }

    // The amount redeemed should be the balance of the principal token held by the Illuminate contract
    uint256 amount = IERC20(principal).balanceOf(lender);

    // Transfer the principal token from the lender contract to here
    Safe.transferFrom(IERC20(principal), lender, address(this), amount);

    if (p == uint8(MarketPlace.Principals.Swivel)) {
        // Redeems zc tokens to the sender's address
        ISwivel(swivelAddr).redeemZcToken(u, m, amount);
    } else if (p == uint8(MarketPlace.Principals.Element)) {
        // Redeems principal tokens from element
        IElementToken(principal).withdrawPrincipal(amount, marketPlace);
    } else if (p == uint8(MarketPlace.Principals.Yield)) {
        // Redeems prinicipal tokens from yield
        IYieldToken(principal).redeem(address(this), address(this), amount);
    } else if (p == uint8(MarketPlace.Principals.Notional)) {
        // Redeems the principal token from notional
        amount = INotional(principal).maxRedeem(address(this));
    }

    emit Redeem(p, u, m, amount);
    return true;
}

There are some protocols (eg Notional) that allows redeem before maturity, when doing so, they will actually make a market sell, usually means a discounted sale.

Since redeem() is a public function, anyone can call it before maturity, and force the whole protocol to sell it's holdings at a discounted price, causing fund loss to the stake holders.

https://github.com/notional-finance/wrapped-fcash/blob/8f76be58dda648ea58eef863432c14c940e13900/contracts/wfCashERC4626.sol#L155-L169

function previewRedeem(uint256 shares) public view override returns (uint256 assets) {
    if (hasMatured()) {
        assets = convertToAssets(shares);
    } else {
        // If withdrawing non-matured assets, we sell them on the market (i.e. borrow)
        (uint16 currencyId, uint40 maturity) = getDecodedID();
        (assets, /* */, /* */, /* */) = NotionalV2.getPrincipalFromfCashBorrow(
            currencyId,
            shares,
            maturity,
            0,
            block.timestamp
        );
    }
}

Recommendation

Consider only allow unauthenticated call after maturity.

Lines of code

https://github.com/code-423n4/2022-06-illuminate/blob/912be2a90ded4a557f121fe565d12ec48d0c4684/redeemer/Redeemer.sol#L177-L194

Vulnerability details

    int256 amount = IERC20(principal).balanceOf(lender);

    // Transfer the principal token from the lender contract to here
    Safe.transferFrom(IERC20(principal), lender, address(this), amount);

    if (p == uint8(MarketPlace.Principals.Swivel)) {
        // Redeems zc tokens to the sender's address
        ISwivel(swivelAddr).redeemZcToken(u, m, amount);
    } else if (p == uint8(MarketPlace.Principals.Element)) {
        // Redeems principal tokens from element
        IElementToken(principal).withdrawPrincipal(amount, marketPlace);
    } else if (p == uint8(MarketPlace.Principals.Yield)) {
        // Redeems prinicipal tokens from yield
        IYieldToken(principal).redeem(address(this), address(this), amount);
    } else if (p == uint8(MarketPlace.Principals.Notional)) {
        // Redeems the principal token from notional
        amount = INotional(principal).maxRedeem(address(this));
    }

INotional.maxRedeem() is a view function, we should use redeem() instead to redeem principal token.

https://github.com/notional-finance/wrapped-fcash/blob/8f76be58dda648ea58eef863432c14c940e13900/contracts/wfCashERC4626.sol#L120-L122

function maxRedeem(address owner) public view override returns (uint256) {
    return balanceOf(owner);
}

In the current implementation, the principal token from Notional can not be redeemed.

Lines of code

https://github.com/code-423n4/2022-06-illuminate/blob/912be2a90ded4a557f121fe565d12ec48d0c4684/lender/Lender.sol#L247-L305

Vulnerability details

function lend(
    uint8 p,
    address u,
    uint256 m,
    uint256[] memory a,
    address y,
    Swivel.Order[] calldata o,
    Swivel.Components[] calldata s
) public unpaused(p) returns (uint256) {

    // lent represents the number of underlying tokens lent
    uint256 lent;
    {
        // returned represents the number of underlying tokens to lend to yield
        uint256 returned;

        uint256 totalFee;
        // iterate through each order a calculate the total lent and returned
        for (uint256 i = 0; i < o.length; ) {
            Swivel.Order memory order = o[i];
            // Require the Swivel order provided matches the underlying and maturity market provided
            if (order.underlying != u) {
                revert NotEqual('underlying');
            } else if (order.maturity > m) {
                revert NotEqual('maturity');
            }

            {
                uint256 amount = a[i];
                // Determine the fee
                uint256 fee = calculateFee(amount);
                // Track accumulated fees
                totalFee += fee;
                // Amount lent for this order
                uint256 amountLent = amount - fee;
                // Sum the total amount lent to Swivel (amount of ERC5095 tokens to mint) minus fees
                lent += amountLent;
                // Sum the total amount of premium paid from Swivel (amount of underlying to lend to yield)
                returned += amountLent * order.premium / order.principal;
            }

            unchecked {
                i++;
            }
        }
    
        // Track accumulated fee
        fees[u] += totalFee;

        // transfer underlying tokens from user to illuminate
        Safe.transferFrom(IERC20(u), msg.sender, address(this), lent);
        // fill the orders on swivel protocol
        ISwivel(swivelAddr).initiate(o, a, s);

        yield(u, y, returned, address(this));
    }
    emit Lend(p, u, m, lent);
    return lent;
}

L297, lent is the amount of underlyingToken transferFrom user's wallet.

However, lent is the sum of amounts without the fees, see L275-583.

This means the user wont be paying for the fees when they lend() to Swivel.

Recommendation

Change to:

fees[u] += totalFee;

// transfer underlying tokens from user to illuminate
Safe.transferFrom(IERC20(u), msg.sender, address(this), lent + totalFee);
// fill the orders on swivel protocol
ISwivel(swivelAddr).initiate(o, a, s);

Lines of code

https://github.com/code-423n4/2022-06-illuminate/blob/912be2a90ded4a557f121fe565d12ec48d0c4684/marketplace/ERC5095.sol#L108-L119

Vulnerability details

https://github.com/code-423n4/2022-06-illuminate/blob/912be2a90ded4a557f121fe565d12ec48d0c4684/marketplace/ERC5095.sol#L108-L119

function redeem(uint256 principalAmount, address receiver, address holder) external override returns (uint256 underlyingAmount){
    if (block.timestamp < maturity) {
        revert Maturity(maturity);
    }
    if (holder == msg.sender) {
        return IRedeemer(redeemer).authRedeem(underlying, maturity, msg.sender, receiver, principalAmount);
    }
    else {
        require(_allowance[holder][msg.sender] >= underlyingAmount, 'not enough approvals');
        return IRedeemer(redeemer).authRedeem(underlying, maturity, holder, receiver, principalAmount);     
    }
}

https://github.com/code-423n4/2022-06-illuminate/blob/912be2a90ded4a557f121fe565d12ec48d0c4684/redeemer/Redeemer.sol#L275-L296

function authRedeem(
    address u,
    uint256 m,
    address f,
    address t,
    uint256 a
) public authorized(IMarketPlace(marketPlace).markets(u, m, 0)) returns (bool) {
    // Get the principal token for the given market
    IERC5095 pt = IERC5095(IMarketPlace(marketPlace).markets(u, m, 0));

    // Make sure the market has matured
    if (block.timestamp < pt.maturity()) {
        revert Invalid('not matured');
    }

    // Burn the user's principal tokens
    pt.burn(f, a);

    // Transfer the original underlying token back to the user
    Safe.transfer(IERC20(u), t, a);
    return true;
}

In the current implementation, at or after maturity, user can call redeem() and get back the underlying tokens always at a 1:1 ratio.

At or after maturity, burns exactly principalAmount of Principal Tokens from owner and sends underlyingAmount of underlying tokens to receiver.

However, some fixed-rate lending protocols use strategies that may allow a certain amount of loss, as a result, by the time the product matures, it may not be able to redeem 100% of the face value.

See the comment in Element's redeem function (called withdrawPrincipal()).

https://github.com/element-fi/elf-contracts/blob/885666433894c598223ea6e32f8cf38236efc2f1/contracts/Tranche.sol#L222-L224

This method will return 1 underlying for 1 principal except when interest is negative, in which case the principal tokens is redeemable pro rata for the assets controlled by this vault.

This means that when some of the PTs from certain protocols can only be redeemed at a lower rate, users that withdraw earlier will be able to get back 100% of the face value, and the late users may not be able to redeem due to insufficient balance.

Recommendation

Consider requiring the protocol PTs to be redeemed fully before the users can redeem their ERC5095 tokens, and once the protocol PTs are all redeemed, ERC5095 tokens can be redeemed proportionally based on the total supply and balance of underlyingTokens.