Illuminate contest - kirk-baird's results

Lines of code

https://github.com/code-423n4/2022-06-illuminate/blob/92cbb0724e594ce025d6b6ed050d3548a38c264b/lender/Lender.sol#L280

Vulnerability details

Impact

There is a division before multiplication bug that exists in lend() for the Swivel case.

If order.premium is less than order.principal then returned will round to zero due to the integer rounding.

When this occurs the user's funds are essentially lost. That is because they transfer in the underlying tokens but the amount sent to yield(u, y, returned, address(this)) will be zero.

Proof of Concept

    function lend(
        uint8 p,
        address u,
        uint256 m,
        uint256[] calldata 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');
                }
                // Determine the fee
                uint256 fee = calculateFee(a[i]);
                // Track accumulated fees
                totalFee += fee;
                // Sum the total amount lent to Swivel (amount of ERC5095 tokens to mint) minus fees
                lent += a[i] - fee;
                // Sum the total amount of premium paid from Swivel (amount of underlying to lend to yield)
                returned += (a[i] - fee) * (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;
    }

Specifically the function returned += (a[i] - fee) * (order.premium / order.principal);

Recommended Mitigation Steps

The multiplication should occur before division, that is ((a[i] - fee) * order.premium) / order.principal);.

Lines of code

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

Vulnerability details

Impact

The function swapExactTokensForTokens() will return and array with the 0 index being the input amount follow by each output amount. The 0 index is incorrectly used in Pendle lend() function as the output amount. As a result the value of returned will be the invalid (i.e. the input rather than the output).

Since this impacts how many PTs will be minted to the msg.sender, the value will very likely be significantly over or under stated depending on the exchange rate. Hence the msg.sender will receive an invalid number of PT tokens.

Proof of Concept

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

            returned = IPendle(pendleAddr).swapExactTokensForTokens(a - fee, r, path, address(this), d)[0];

Recommended Mitigation Steps

The amount of principal returned should be index 1 of the array returned by swapExactTokensForTokens().

Lines of code

https://github.com/code-423n4/2022-06-illuminate/blob/92cbb0724e594ce025d6b6ed050d3548a38c264b/marketplace/ERC5095.sol#L116

Vulnerability details

Impact

The _allowance check in ERC5095 is ineffective as it checks the allowance against underlyingAmount. Since underlyingAmount is only declared as the return variable and has not yet been set it is always zero.

The impact of this is that require(_allowance[holder][msg.sender] >= underlyingAmount, 'not enough approvals'); can never fail since _allowance[holder[msg.sender] >= 0 will always be true.

Thus, any users can redeem() tokens on another user's behalf so long as they have sufficient balance. The attacker would be transferred the allocated underlying tokens and the holder's balance will be deducted.

Proof of Concept

redeem() can will always succeed if holder != msg.sender.

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

As seen in the code above underlyingAmount is declared as the return value and therefore initialised to zero. Hence the require on line #116 require(_allowance[holder][msg.sender] >= underlyingAmount, 'not enough approvals'); will always succeed as x >= 0 is always true for unsigned integers.

Recommended Mitigation Steps

The require statement on line #116 should be updated to check against principalAmount rather than underlyingAmount.

require(_allowance[holder][msg.sender] >= principalAmount, 'not enough approvals');

Lines of code

https://github.com/code-423n4/2022-06-illuminate/blob/92cbb0724e594ce025d6b6ed050d3548a38c264b/marketplace/ERC5095.sol#L108-L119 https://github.com/code-423n4/2022-06-illuminate/blob/92cbb0724e594ce025d6b6ed050d3548a38c264b/marketplace/ERC5095.sol#L92-L103

Vulnerability details

Impact

The functions withdraw() and redeem() do not deduct a spender's allowance after spending the tokens (i.e. burning them for underlying tokens).

The impact of this is that an attacker may repeatedly spend the allowance. This allows anyone with a small allowance to redeem all the holder's tokens.

Proof of Concept

Neither of the functions withdraw() or redeem() deduct the _allowance[holder][msg.sender].

withdraw()

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

redeem()

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

Example. Alice gives Bob an allowance of say 1e18 and Alice's balance is 20e18. Bob may now exploit Alice by doing the following, to receive all 20e18 worth of underlying tokens.

for (i = 0; i < 20; i++) {
   principalToken.redeem(1e18, bob, alice);
}

Recommended Mitigation Steps

This issue may be remidiated by adding the following calls if holder != msg.sender

• _decreaseAllowance(holder, underlyingAmount) in withdraw()
• _decreaseAllowance(holder, principalAmount) in redeem()

Lines of code

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

Vulnerability details

Impact

The functions lend() Illuminate / Yield and lend() Swivel do not sufficiently validate y (Yield Pool) input parameter.

An attacker may set y to a malicious contract which passes the required checks for base() and maturity().

As a result when the underlying tokens are transferred to y they will be transferred to a contract in the attackers control. Additionally the attacker will also be minted Illumintate PT tokens which may be redeemed after maturity.

Proof of Concept

Consider the Illuminate case.

lend() Illumintae / Yield

    function lend(
        uint8 p,
        address u,
        uint256 m,
        uint256 a,
        address y
    ) public unpaused(p) returns (uint256) {
        // check the principal is illuminate or yield
        if (p != uint8(MarketPlace.Principals.Illuminate) && p != uint8(MarketPlace.Principals.Yield)) {
            revert Invalid('principal');
        }


        // uses yield token interface...
        IYield pool = IYield(y);


        // the yield token must match the market pair
        if (address(pool.base()) != u) {
            revert NotEqual('underlying');
        } else if (pool.maturity() > m) {
            revert NotEqual('maturity');
        }


        // transfer from user to illuminate
        Safe.transferFrom(IERC20(u), msg.sender, address(this), a);


        if (p == uint8(MarketPlace.Principals.Yield)) {
            // Purchase yield PTs to lender.sol (address(this))
            uint256 returned = yield(u, y, a - calculateFee(a), address(this));
            // Mint and distribute equivalent illuminate PTs
            IERC5095(principalToken(u, m)).mint(msg.sender, returned);
            
            emit Lend(p, u, m, returned);


            return returned;
        }
        else {
            // Purchase illuminate PTs directly to msg.sender
            uint256 returned = yield(u, y, a - calculateFee(a), msg.sender);


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


            return returned;
        }
    }

yield()

    function yield(
        address u,
        address y,
        uint256 a,
        address r
    ) internal returns (uint256) {
        // preview exact swap slippage on yield
        uint128 returned = IYield(y).sellBasePreview(Cast.u128(a));


        // send the remaing amount to the given yield pool
        Safe.transfer(IERC20(u), y, a);


        // lend out the remaining tokens in the yield pool
        IYield(y).sellBase(r, returned);


        return returned;
    }

If an attacker makes a false yield contract like the one below it will pass all the required checks. Then the function yield() will transfer the underlying tokens to FakeYield. Note the attacker also controls returnAmount which allows them to determine how many PTs that will be minted to the attacker.

contract FakeYield {
	IERC20 public base; // IYield.base()
	uint32 public maturity; // IYield.maturity()
	uint128 returnAmount;

	constructor (IERC20 _base, uint32 _maturity, uint128 _returnAmount) {
		base = _base;
		maturity = _maturity;
		returnAmount = _returnAmount;
	}

	// IYeild.sellBasePreview()
	function sellBasePreview(uint128 amount) returns (uint128) {
		return returnAmount;
	}

	// IYield.sellBase()
	function sellBase(uint128 amount) returns (uint128) {
		return returnAmount;
	}

	// Claim stolen tokens
	function transferTokens(IERC20 token) {
		token.transfer(token.balanceOf(address(this)), msg.sender);
	}
}

By calling lend() with this FakeYield the attacker will receive the underlying tokens transferred in (less the fees) in addition to the Illuminate PTs minted to them.

Recommended Mitigation Steps

y needs to be validated in the two vulnerable lend() functions. This can be done by fetching it from MarketPlace.pools(u, m) and having the owner add the pool in createMarket() (or setPool() but since it's now necessary for functionality createMarket() makes more sense).

Lines of code

https://github.com/code-423n4/2022-06-illuminate/blob/3ca41a9f529980b17fdc67baf8cbee5a8035afab/lender/Lender.sol#L247-L305 https://github.com/code-423n4/2022-06-illuminate/blob/3ca41a9f529980b17fdc67baf8cbee5a8035afab/lender/Lender.sol#L317-L367

Vulnerability details

Impact

Two of the lend functions will spend the lender's underlying tokens without minting them Illumintae Principal Tokens. The two functions are `Swivel lend() and Element lend().

The impact of this issue is that the user's spend underlying tokens for the swapping/minting of Element or Swivel PTs but will not receive any benefit.

The specifications state:

Within each lend operation, the Lender contract will extract a fee and lend the rest of the capital on the appropriate principal. The Lender contract will maintain custody of the principal token. To track the user's outstanding position, an ERC-5095 token for that market will be minted and sent to the user.

This is rated high risk as it breaks the core function of the protocol and results in lenders losing their deposits to the protocol.

Proof of Concept

Swivel lend() calls yield() which will mint the PTs to Lender. Note the lack of IERC5095(illuminateToken).mint(msg.sender, amount);.

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

Element lend() will swap the underlying into PTs with the recipient recipient: payable(address(this)), however there is no call to IERC5095(illuminateToken).mint(msg.sender, someAmount);.

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

Recommended Mitigation Steps

For each of the two functions listed above there should be call to IERC5095(illuminateToken).mint() which mints the Illumintae PTs to the msg.sender.

Lines of code

https://github.com/code-423n4/2022-06-illuminate/blob/92cbb0724e594ce025d6b6ed050d3548a38c264b/redeemer/Redeemer.sol#L127-L128

Vulnerability details

Impact

The function redeem() will transfer the underlying tokens to address(this) rather than msg.sender (or a recipient address).

The impact is that a depositor will not receive any funds from redeeming their Illuminate Principal tokens which are burnt. The underlying tokens will instead be locked in Redeemer since they cannot be transferred out.

Proof of Concept

In the function redeem() the following line #128 Safe.transferFrom(IERC20(u), lender, address(this), amount); will transfer the funds from the lender to address(this). However as stated in the comments the funds should be transferred "back to the user".

Since there is no way to retrieve the funds transferred to the Redeemer they are locked in the contract.

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

        if (p == uint8(MarketPlace.Principals.Illuminate)) {
            // Get Illuminate's principal token
            IERC5095 token = IERC5095(principal);
            // Get the amount of tokens to be redeemed from the sender
            uint256 amount = token.balanceOf(msg.sender);
            // Make sure the market has matured
            if (block.timestamp < token.maturity()) {
                revert Invalid('not matured');
            }
            // Burn the prinicipal token from Illuminate
            token.burn(o, amount);
            // Transfer the original underlying token back to the user
            Safe.transferFrom(IERC20(u), lender, address(this), amount); // @audit invalid from parameter

            emit Redeem(0, u, m, amount);
        } else { ... }

        return true;

Recommended Mitigation Steps

The transfer on line #128 should be updated to have a from parameter set to msg.sender.

// Transfer the original underlying token back to the user
Safe.transferFrom(IERC20(u), lender, msg.sender, amount);

Lines of code

https://github.com/code-423n4/2022-06-illuminate/blob/912be2a90ded4a557f121fe565d12ec48d0c4684/marketplace/MarketPlace.sol#L136-L144 https://github.com/code-423n4/2022-06-illuminate/blob/912be2a90ded4a557f121fe565d12ec48d0c4684/marketplace/MarketPlace.sol#L151-L159 https://github.com/code-423n4/2022-06-illuminate/blob/912be2a90ded4a557f121fe565d12ec48d0c4684/marketplace/MarketPlace.sol#L166-L174 https://github.com/code-423n4/2022-06-illuminate/blob/912be2a90ded4a557f121fe565d12ec48d0c4684/marketplace/MarketPlace.sol#L181-L189

Vulnerability details

Impact

Any user is able to spend pool.fyToken() and pool.baseToken() that are in the MarketPlace.

That is because each of the following functions requires that the tokens to be sold / transferred are already in MarketPlace.

• sellUnderlying()
• buyUnderlying()
• sellPrincipalToken()
• buyPrincipalToken()

These functions are all front-runnable if users do transfer() into MarketPlace and sellUnderlying() (or similar) in separate transactions, the attacker could call sellUnderlying() in between and spend the contracts balance.

There is also add the risk under the current design that if the user transfer() the too many funds to the MarketPlace that they will be left in the contract for any other user to spend.

Proof of Concept

Each of the functions follows the same logic so we'll use sellUnderlying() as an example.

    function sellUnderlying(
        address u,
        uint256 m,
        uint128 a
    ) external returns (uint128) {
        IPool pool = IPool(pools[u][m]);
        Safe.transfer(IERC20(address(pool.base())), address(pool), a);
        return pool.sellBase(msg.sender, pool.sellBasePreview(a));
    }

The function performs the following transfer Safe.transfer(IERC20(address(pool.base())), address(pool), a); to ensure the funds are now held in the pool. This functions simple transfer base tokens from MarketPlace to pool.

An attacker who did not transfer any tokens to MarketPlace could first call this function and since the msg.sender would be different the received PTs from pool.sellBase() would then be transferred to the attacker.

Recommended Mitigation Steps

To prevent front-running or any tokens needing to be stored in the market place to use transferFrom() where from is the msg.sender and that will ensure tokens are transferred straight from the user to pool.

For the sellUnderlying() case that would be to do Safe.transferFrom(IERC20(address(pool.base())), msg.sender, address(pool), a);.

Lines of code

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

Vulnerability details

Impact

Swivel lend() does not validate the o.exit and o.vault for each order before making the external call to Swivel. These values determine which internal functions is called in Swivel.

The intended code path is initiateZcTokenFillingVaultInitiate() which takes the underlying tokens and mints zcTokens to the Lender. If one of the other functions is called the accounting in lend(). Swivel may transfer more tokens from Lender to Swivel than paid for by the caller of lend().

The impact is that underlying tokens may be stolen from Lender.

Proof of Concept

Consider the example where initiateZcTokenFillingZcTokenExit() is called. This will transfer a - premiumFilled + fee from Lender to Swivel rather than the expected a + fee.

Recommended Mitigation Steps

In lend() restrict the values of o.exit and o.vault so only one case can be triggered in Swivel.initiate().

Lines of code

https://github.com/code-423n4/2022-06-illuminate/blob/912be2a90ded4a557f121fe565d12ec48d0c4684/lender/Lender.sol#L78 https://github.com/code-423n4/2022-06-illuminate/blob/912be2a90ded4a557f121fe565d12ec48d0c4684/lender/Lender.sol#L107 https://github.com/code-423n4/2022-06-illuminate/blob/912be2a90ded4a557f121fe565d12ec48d0c4684/lender/Lender.sol#L137 https://github.com/code-423n4/2022-06-illuminate/blob/912be2a90ded4a557f121fe565d12ec48d0c4684/lender/Lender.sol#L145 https://github.com/code-423n4/2022-06-illuminate/blob/912be2a90ded4a557f121fe565d12ec48d0c4684/lender/Lender.sol#L156 https://github.com/code-423n4/2022-06-illuminate/blob/912be2a90ded4a557f121fe565d12ec48d0c4684/lender/Lender.sol#L708

Vulnerability details

Impact

There are numerous methods that the admin could apply to rug pull the protocol and take all user funds.

• Lender.approve()

  • Both the functions on lines #78 and #107.
  • Admin can approve any token for an arbitrary address and transfer tokens out.

• Lender.setFee()

  • Does not have an lower limit.
  • feeNominator = 1 implies 100% of amount is taken as fees.

• Lender.withdraw()

  • Allows withdrawing any arbitrary ERC20 token
  • 3 Days is insufficient time for users to withdraw funds in the case of a rugpull.

• MarketPlace.setPrincipal()

  • Use (u, m, 0) -> to be an existing Illuminate PT from another market
  • Then set (u, m, 1) -> to be some malcious admin created ERC20 token to which they have infinite supply
  • Then call Lender.mint() for `(u, m, 1) and later redeem these tokens on the original market

Recommended Mitigation Steps

Without significant redesign it is not possible to avoid the admin being able to rug pull the protocol.

As a result the recommendation is to set all admin functions behind either a timelocked DAO or at least a timelocked multisig contract.

Lines of code

https://github.com/code-423n4/2022-06-illuminate/blob/92cbb0724e594ce025d6b6ed050d3548a38c264b/lender/Lender.sol#L266-L280

Vulnerability details

Impact

When calculating the protocol fee for lend() Swivel the fee is subtracted from amount to get lent. Since lent is the amount that is transferred to the Swivel to swivel and also the amount transferred from msg.sender. The protocol does not account for the admin fee. Although the admin fee is still added to fees[u].

Proof of Concept

lend() Swivel

                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');
                }
                // Determine the fee
                uint256 fee = calculateFee(a[i]);
                // Track accumulated fees
                totalFee += fee;
                // Sum the total amount lent to Swivel (amount of ERC5095 tokens to mint) minus fees
                lent += a[i] - fee;
                // Sum the total amount of premium paid from Swivel (amount of underlying to lend to yield)
                returned += (a[i] - fee) * (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);

Consider the case where the fee feeNominator = 10, a = [20], permium = principal = 1e18

Here we have

fee = 20 / 10 = 2
totalFee = 2
lent = 20 - 2 = 18
returned = (20 - 2) * 1 = 18

The amount transferred from msg.sender to Lender is lent = 18 which is the same amount sent to Swivel. Hence, the totalFee = 2 is not accounted for.

Recommended Mitigation Steps

The protocol should instead transfer the sum of a to the protocol. e.g.

            uint256 totalFee;
            uint256 totalAmount; // @audit
            // 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');
                }
                // Determine the fee
                uint256 fee = calculateFee(a[i]);
                // Track accumulated fees
                totalFee += fee;
                // Sum total amount owed by the lender
                totalAmount += a[i]; // @audit
                // Sum the total amount lent to Swivel (amount of ERC5095 tokens to mint) minus fees
                lent += a[i] - fee;
                // Sum the total amount of premium paid from Swivel (amount of underlying to lend to yield)
                returned += (a[i] - fee) * (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), totalAmount); // @audit
            // fill the orders on swivel protocol
            ISwivel(swivelAddr).initiate(o, a, s);

            yield(u, y, returned, address(this));

Lines of code

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

Vulnerability details

Impact

The Swivel lend() function should call the initiateZcTokenFillingVaultInitiate() case in Swivel.initiate(), this is the case that takes underlying tokens from msg.sender and mints zcTokens (the zcTokens are held by Lender and later redeemed in Redeemer).

initiateZcTokenFillingVaultInitiate() calculates a Swivel Protocol Fee (different to the Illuminate protocol fee ) which is transferred from Lender to Swivel.

Lender does not account for this fee when transferring funds from the depositor to Lender.

The impact is that the fee will be paid for by Lender rather than the user who calls lend() hence the protocol loses Swivel fee amount underlying tokens.

Note this is a separate bug to the miscalculation of the Lender protocol fee in the same function. This bug is the lack of inclusion of the Swivel protocol fee.

Proof of Concept

Consider the example where we call Swivel lend() with a = [100], Lender.feeNomiator = 0, Swivel.feenominators[0] = 10, o.premium = 1 and o.principal = 1.

In this example the amount transferred from the caller of lend() will be lent = a - calculateFee(a) = a - 0 = 1 from here.

Then the amount transferred to Swivel from Lender here will be as follows (Note: premiumFilled = (a * o.premium) / o.principal):

    uint256 fee = premiumFilled / feenominators[0];
    Safe.transferFrom(uToken, msg.sender, address(this), (a + fee));

Thus,

premiumFilled = a * 1 / 1 = 100

fee = 100 / 10 = 10

Hence, the amount sent from Lender to Swivel will be a + fee = 110. Since only 100 was transferred from the depositor to the Lender the protocol will be short 10 units.

Note the Lender also does not account for the amount transferred from o.maker to Lender here which offsets the fee but leaves excess tokens in Lender.

Recommended Mitigation Steps

The calculations for Swivel fee and the transfer from o.marker to Lender should be accounted for in lend().

Lines of code

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

Vulnerability details

Impact

Lender.mint() may use p = 0 which will mean principal is the same as principalToken(u, m) i.e. the Illuminate PT. The impact is we will transfer some principal to the Lender contract and it will mint us an equivalent amount of principal tokens.

This can be repeated indefinitely thereby minting infinite tokens. The extra balance will be store in Lender.

This is rated high as although the attacker does not receive the extra tokens stored within the Lender they may be consumed via any contract which has been approved via the approve() functions (e.g. Redeemer).

Proof of Concept

Lender.mint()

    function mint(
        uint8 p,
        address u,
        uint256 m,
        uint256 a
    ) public returns (bool) {
        //use market interface to fetch the market for the given market pair
        address principal = IMarketPlace(marketPlace).markets(u, m, p);
        //use safe transfer lib and ERC interface...
        Safe.transferFrom(IERC20(principal), msg.sender, address(this), a);
        //use ERC5095 interface...
        IERC5095(principalToken(u, m)).mint(msg.sender, a);


        emit Mint(p, u, m, a);


        return true;
    }

Steps:

• Lender.lend() with p = 0 to get some Illuminate principal tokens
• token.approve() gives Lender allowance to spend these tokens
• loop:
  • Lender.mint() with p = 0 minting more principal tokens

Recommended Mitigation Steps

In Lender.mint() ensure p != uint8(MarketPlace.Principals.Illuminate)) .

Lines of code

https://github.com/code-423n4/2022-06-illuminate/blob/912be2a90ded4a557f121fe565d12ec48d0c4684/lender/Safe.sol#L46-L70 https://github.com/code-423n4/2022-06-illuminate/blob/912be2a90ded4a557f121fe565d12ec48d0c4684/lender/Safe.sol#L18-L40 https://github.com/code-423n4/2022-06-illuminate/blob/912be2a90ded4a557f121fe565d12ec48d0c4684/marketplace/Safe.sol#L46-L70 https://github.com/code-423n4/2022-06-illuminate/blob/912be2a90ded4a557f121fe565d12ec48d0c4684/marketplace/Safe.sol#L18-L40 https://github.com/code-423n4/2022-06-illuminate/blob/912be2a90ded4a557f121fe565d12ec48d0c4684/redeemer/Safe.sol#L46-L70 https://github.com/code-423n4/2022-06-illuminate/blob/912be2a90ded4a557f121fe565d12ec48d0c4684/redeemer/Safe.sol#L18-L40

Vulnerability details

Impact

Safe.sol does not check that a contract exists before making the external call using the call Opcode. The impact of this is that calls to the zero address (or any address without bytecode) will not revert when Safe.transfer() or Safe.transferFrom() is called.

As a result it is possible to infinite mint tokens if the market place has any zero addresses in the array markets[u][m]. That is the final array that is in the map mapping(address => mapping(uint256 => address[9])) public markets;.

This will occur in createMarket() is called and one of the t values is address(0). The existence of MarketPlcae.setPrincipal() implies that some principal tokens may be added after a market is created. Also it is not expected that all 8 protocols will always have a market for the require pair (underlying token, maturity). Hence, createMarket() will likely be called with some zero addresses.

Note there are three instances of Safe.sol and this issue is present on all of them.

• lender/Safe.sol
• marketplace/Safe.sol
• redeemer/Safe.sol

Proof of Concept

If an assembly call() is made to an address which does not have any bytecode the return value is true and returndatasize() is zero. Hence, it will trigger success() case 0 and Safe.transferFrom() will execute successfully.

Lender.mint() fetches the principal token address from the market, IMarketPlace(marketPlace).markets(u, m, p). If principal is the zero address then Safe.transferFrom(IERC20(principal), msg.sender, address(this), a); will always succeed.

Now so long as principalToken(u, m) exists then the Lender.mint() will successfully mint a number of tokens to the msg.sender. Thereby earning the msg.sender free Illuminate PTs which can later be redeemed for underlying tokens.

    function mint(
        uint8 p,
        address u,
        uint256 m,
        uint256 a
    ) public returns (bool) {
        //use market interface to fetch the market for the given market pair
        address principal = IMarketPlace(marketPlace).markets(u, m, p);
        //use safe transfer lib and ERC interface...
        Safe.transferFrom(IERC20(principal), msg.sender, address(this), a);
        //use ERC5095 interface...
        IERC5095(principalToken(u, m)).mint(msg.sender, a);


        emit Mint(p, u, m, a);


        return true;
    }

Recommended Mitigation Steps

First a check should be added in Safe.transfer() and Safe.transferFrom() theat ensures the destination contract has bytecode. This can be done using extcodesize() and ensuring the return value is greater than 0.

Additionally, ensure that principal is not the zero address in Lender.mint(). Note that this should also be done in all occurrences where principal is fetched from the market.