Illuminate contest - datapunk's results

Lines of code

https://github.com/code-423n4/2022-06-illuminate/blob/912be2a90ded4a557f121fe565d12ec48d0c4684/lender/Lender.sol#L465 https://github.com/tempus-finance/tempus-protocol/blob/f431e821c81d8cdeae8ad433d160230563f121de/contracts/TempusController.sol#L59

Vulnerability details

Impact

Confusing at least, potentially wrong

Proof of concept

function depositAndFix(
    ITempusAMM tempusAMM,
    ITempusPool tempusPool,
    uint256 tokenAmount,
    bool isBackingToken,
    uint256 minTYSRate,
    uint256 deadline
) external payable override nonReentrant returns (uint256, uint256)

The function returns two values, while the caller expects only one

uint256 returned = ITempus(tempusAddr).depositAndFix(Any(x), Any(t), a - fee, true, r, d) -
        illuminateToken.balanceOf(address(this)); 

Also not sure if subtract illuminateToken.balanceOf(address(this)) is the correct logic here.

Recommendation

Maybe do the following?

uint256 returned = ITempus(tempusAddr).depositAndFix(Any(x), Any(t), a - fee, true, r, d)[0] -
        illuminateToken.balanceOf(address(this)); 

Lines of code

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

Vulnerability details

Impact

It may have unexpected rounding issues with dividing first

Proof of concept

The repo has changed since the contest started. The original repo has returned += amountLent * (order.premium / order.principal); instead of the currently correct code in https://github.com/code-423n4/2022-06-illuminate/blob/912be2a90ded4a557f121fe565d12ec48d0c4684/lender/Lender.sol#L285

Recommendation

fixed

Lines of code

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

Vulnerability details

Impact

Transferred token to wrong contract

Proof of concept

To redeem, IERC20(principal) should be transferred from lender to redeemer, not marketplace https://github.com/code-423n4/2022-06-illuminate/blob/912be2a90ded4a557f121fe565d12ec48d0c4684/redeemer/Redeemer.sol#L187

Recommendation

Do this instead:

    IElementToken(principal).withdrawPrincipal(amount, address(this));

Lines of code

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

Vulnerability details

Proof of concept

In https://github.com/code-423n4/2022-06-illuminate/blob/912be2a90ded4a557f121fe565d12ec48d0c4684/lender/Lender.sol#L205, y is checked against u and m, but not p. From the usage, it seems when p == uint8(MarketPlace.Principals.Yield, y is a pool for swapping underlying and yield PT, while when p == uint8(MarketPlace.Principals.Illuminate), y is a pool for swapping underlying and illuminate PTs.

Impact

Without proper checking, it is hard to see if the logic is correct. It might very well be the case, where y is used in wrong scenario.

Recommendation

Check the fyToken in the following fashion:

    IYield pool = IYield(y);
    if (p == uint8(MarketPlace.Principals.Yield && pool.fyToken!=yieldPT)
        revert WrongY();
    else if (p == uint8(MarketPlace.Principals.Yield && pool.fyToken!=yieldPT)
        revert WrongY();

Lines of code

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

Vulnerability details

Proof of concept

In https://github.com/code-423n4/2022-06-illuminate/blob/912be2a90ded4a557f121fe565d12ec48d0c4684/lender/Lender.sol#L301, yield PTs is purchased to lender.sol (address(this)), yet msg.sender does not have illuminate PTs Minted and distributed.

Impact

msg.sender loses fund since its underlying token is transferred into lender, yet it has no receipt to prove it.

Recommendation

Check the length in the following fashion:

    IERC5095(principalToken(u, m)).mint(msg.sender, returned);

Lines of code

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

Vulnerability details

Impact

Anyone can remove the principal tokens anytime from these protocols

Proof of concept

Tempus and Notional allows for early withdrawal, which is OK as protocol design, but since redeem() function can be called by anyone, this can lead to undesired behavior

Recommendation

Test maturity before calling redeem()

Lines of code

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

Vulnerability details

Impact

Transferred the wrong token, later withdraw will fail from APWine

Proof of concept

To redeem, IERC20(principal) should be transferred from lender to redeemer, not u

Recommendation

Do this instead:

    Safe.transferFrom(IERC20(principal), lender, address(this), amount);

Lines of code

https://github.com/code-423n4/2022-06-illuminate/blob/912be2a90ded4a557f121fe565d12ec48d0c4684/redeemer/Redeemer.sol#L193 https://github.com/notional-finance/wrapped-fcash/blob/019cfa20369d5e0d9e7a38fea936cc649704780d/contracts/wfCashERC4626.sol#L90

Vulnerability details

Impact

No redeem actually happens for Notional

Proof of concept

maxRedeem() function is a public view function that returns balance(owner);

Recommendation

Use the redeem function instead: https://github.com/notional-finance/wrapped-fcash/blob/019cfa20369d5e0d9e7a38fea936cc649704780d/contracts/wfCashERC4626.sol#L205

Lines of code

https://github.com/code-423n4/2022-06-illuminate/blob/912be2a90ded4a557f121fe565d12ec48d0c4684/marketplace/MarketPlace.sol#L142 https://github.com/code-423n4/2022-06-illuminate/blob/912be2a90ded4a557f121fe565d12ec48d0c4684/marketplace/MarketPlace.sol#L157 https://github.com/code-423n4/2022-06-illuminate/blob/912be2a90ded4a557f121fe565d12ec48d0c4684/marketplace/MarketPlace.sol#L172 https://github.com/code-423n4/2022-06-illuminate/blob/912be2a90ded4a557f121fe565d12ec48d0c4684/marketplace/MarketPlace.sol#L187

Vulnerability details

Proof of concept

In Marketplace.sol The pool.base()/fyToken() transfers out of its own balance.

1. It is unclear how these tokens get into Marketplace.
2. While it is likely the sender sends those tokens int Marketplace first then make the desired calls, it is more clear if transferFrom is used explicitly.

Impact

With transfer(), it is unclear what the workflow looks like to leverage Marketplace for swaps between the zcTokens and underlying assets. With transferFrom(), it would be much clearer. It is also more consistent with mint() and burn() functions.

Recommendation

Change transfer from transferFrom:

    Safe.transferFrom(ERC20(address(pool.base())), msg.sender, address(pool), a);  
    OR
    Safe.transferFrom(ERC20(address(pool.fyToken())), msg.sender, address(pool), a);

Lines of code

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

Vulnerability details

Impact

The lend function for tempus will fail with the right market.

Proof of concept

checks if (ITempus(principal).yieldBearingToken() != IERC20Metadata(u)), while it should check ITempus(principal).backingToken()

Recommendation

Do this instead:

    if (ITempus(principal).backingToken() != IERC20Metadata(u))

Lines of code

https://github.com/code-423n4/2022-06-illuminate/blob/912be2a90ded4a557f121fe565d12ec48d0c4684/marketplace/MarketPlace.sol#L143 https://github.com/code-423n4/2022-06-illuminate/blob/912be2a90ded4a557f121fe565d12ec48d0c4684/marketplace/MarketPlace.sol#L158 https://github.com/code-423n4/2022-06-illuminate/blob/912be2a90ded4a557f121fe565d12ec48d0c4684/marketplace/MarketPlace.sol#L173 https://github.com/code-423n4/2022-06-illuminate/blob/912be2a90ded4a557f121fe565d12ec48d0c4684/marketplace/MarketPlace.sol#L188 https://github.com/code-423n4/2022-06-illuminate/blob/912be2a90ded4a557f121fe565d12ec48d0c4684/lender/Lender.sol#L648

Vulnerability details

Impact

The preview functions return potential swap results, the caller should check these results first to avoid extreme slippages. Otherwise, the caller functions can be sandwich attacked since there is no slippage tolerance limit and the transaction won't revert even if there was 99% slippage when doing the swaps.

Recommendation

Check preview returns against an acceptable slippage limit, and revert otherwise. If it is too high it can lead to Denial of Service attacks but zero slippage tolerance limit is not much better.

Severity: QA

wrong comments

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

Severity: QA

remove extra code

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

should be

    address illuminateToken = principalToken(u, m);

Severity: QA

Check underlying and maturity of principal token when being set in Marketplace.sol instead of when being used in Lender.sol

Proof of Concept

There are multiple places in Lender.sol such as below, where principal token is checked for underlying token and maturity: https://github.com/code-423n4/2022-06-illuminate/blob/912be2a90ded4a557f121fe565d12ec48d0c4684/lender/Lender.sol#L331 The check could happen in Marketplace.sol when they are first set in: https://github.com/code-423n4/2022-06-illuminate/blob/912be2a90ded4a557f121fe565d12ec48d0c4684/marketplace/MarketPlace.sol#L67 https://github.com/code-423n4/2022-06-illuminate/blob/912be2a90ded4a557f121fe565d12ec48d0c4684/marketplace/MarketPlace.sol#L98

Impact

Checking Principal address early can:

1. make sure no mistakes are made when Principal markets are set. As once set, they are immutable.
2. save gas, when Lender.sol is called to lend, as there is no more need to check validity multiple times.

Recommended Mitigation Steps

Move u/m checking to Marketplace.sol from Lender.sol

Severity: Gas

use uint256 max = type(uint256).max in place of uint256 max = 2**256 - 1;

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

Severity: Gas

use ++i in place of i++

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

Severity: Gas

Check underlying and maturity of principal token when being set in Marketplace.sol instead of when being used in Lender.sol

Proof of Concept

There are multiple places in Lender.sol such as below, there principal token is checked for underlying token and maturity: https://github.com/code-423n4/2022-06-illuminate/blob/912be2a90ded4a557f121fe565d12ec48d0c4684/lender/Lender.sol#L331 The check could happen in Marketplace.sol when they are first set in: https://github.com/code-423n4/2022-06-illuminate/blob/912be2a90ded4a557f121fe565d12ec48d0c4684/marketplace/MarketPlace.sol#L67 https://github.com/code-423n4/2022-06-illuminate/blob/912be2a90ded4a557f121fe565d12ec48d0c4684/marketplace/MarketPlace.sol#L98

Impact

Checking Principal address ealy can:

1. make sure no mistakes are made when Principal markets are set. As once set, they are immutable.
2. save gas, when Lender.sol is called to lend, as there is no more need to check validity multiple times.

Recommended Mitigation Steps

Move u/m checking to Marketplace.sol from Lender.sol