Illuminate contest - shenwilly's results

Lines of code

https://github.com/code-423n4/2022-06-illuminate/blob/912be2a90ded4a557f121fe565d12ec48d0c4684/marketplace/ERC5095.sol#L100-L101 https://github.com/code-423n4/2022-06-illuminate/blob/912be2a90ded4a557f121fe565d12ec48d0c4684/marketplace/ERC5095.sol#L116-L117

Vulnerability details

In the current implementation of ERC5095, withdrawing or redeeming an allowance didn't reduce the allowance. An address that has been given allowance once can abuse it and keep redeeming the owner's iPT token.

withdraw and reduce only check the allowance, but didn't reduce it.

Proof of Concept

• Alice has 1000 iPT. Alice gives 100 iPT allowance to Bob.
• After the iPT expires, Bob calls redeem to redeem Alice's 100 iPT into the underlying 100 USDC token.
• As the approval is not decreased, Bob can keep calling redeem until Alice's fund is depleted.
• Alice lost all 1000 iPT, and Bob receives 1000 USDC instead of the approved 100.

Recommended Mitigation Steps

Reduce the approval in withdraw and redeem:

require(_allowance[holder][msg.sender] >= underlyingAmount, 'not enough approvals');
_approve(owner, spender, _allowance[holder][msg.sender] - underlyingAmount);
return IRedeemer(redeemer).authRedeem(underlying, maturity, holder, receiver, underlyingAmount);     

Lines of code

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

Vulnerability details

A malicious user can input malicious contract address y and mint infinite iPT token.

Lending via Yield or Swivel relies on swapping the underlying token via yieldspace pool y. However, the current implementation has two issues.

First, the user can input arbitrary y address. This allows an attacker to provide their own malicious contract. Second, the returned PT amount is not validated. As the value of returned is obtained from yield(), which calls y:

uint128 returned = IYield(y).sellBasePreview(Cast.u128(a));

A malicious y contract can return a large value without actually transferring back the PT, and the attacker will receive free iPT. In addition, there's no cost to do this attack as the underlying token is transferred back to the malicious contract.

Proof of Concept

• Malicious user Alice prepares a malicious y contract that implements IYield contract and mimic the behaviour of a correct Yield contract, except for two functions:
  • sellBasePreview() returns maximum amount of token.
  • sellBase() is a no-op.
• Alice calls Yield lend, using the proper parameters except for y which will use the malicious contract.
• The code will run properly until L219, where yield() will return a maximum amount (and not transferring back any PT).
• Alice receives a maximum amount of iPT and proceed to dump it on an AMM.

Recommended Mitigation Steps

Instead of relying on returned, use the difference of PT balance before and after yield() to ensure that Lender received the correct amount of PT:

address yieldPT = IMarketPlace(marketPlace).markets(u, m, 2);

uint256 preBalance = IERC20(yieldPT).balanceOf(address(this));
yield(u, y, a - calculateFee(a), address(this));
uint256 postBalance = IERC20(yieldPT).balanceOf(address(this));

IERC5095(principalToken(u, m)).mint(msg.sender, postBalance - preBalance);

In addition, consider being more strict by implementing a whitelist for y to prevent future vulnerabilities by malicious contract.

Lines of code

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

Vulnerability details

Lending via Swivel & Element lend didn't mint iPT token to the sender, causing loss of funds.

Unlike other lend functions which swap underlying token from msg.sender to the corresponding PT and mint iPT back to them, the current Swivel & Element lend implementations are missing the iPT mint function.

Proof of Concept

• Alice tries to mint iPT using Element lend with 1000 USDC as the underlying token.
• 1000 USDC is sent to Lender but Alice didn't receive any iPT.

Recommended Mitigation Steps

Add the iPT minting function.

Lines of code

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

Vulnerability details

Redeeming iPT via redeem will always fail. A griefer can burn token from any holder.

Calling Illuminate redeem is supposed to burn iPT and transfer back the underlying token to the user. However, there are several issues with the current implementation:

uint256 amount = token.balanceOf(msg.sender);

...

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

First, the amount is obtained by querying msg.sender's balance. However, in the next step, it's token from o address that is being burned instead of msg.sender.

The final line should be a transfer of underlying token from Redeemer contract to the user. Instead it tries to transfer underlying token from Lender to this contract, which will always fail as Lender is not supposed to hold any underlying token.

This issue also opens up the possibility of griefing by sending the underlying token to Lender and calling redeem to burn iPT from any holder.

Proof of Concept

• Bob has 1000 iPT. Alice has a grudge against Bob.
• On maturity, Alice buys 1000 iPT and sends 1000 USDC to Lender, then proceed to call Illuminate redeem with Bob's address as o.
• Redeemer reads Alice's iPT balance (1000), then proceed to burn that amount from Bob's wallet. 1000 USDC in Lender is sent to Redeemer. Alice redeem the 1000 iPT back to USDC.
• Both Alice and Bob are now down 1000 USDC. 1000 USDC is now stuck at Redeemer.

Recommended Mitigation Steps

Julian clarified that the function allows anyone to burn from any address, but the owner of the iPT themselves will receive the underlying tokens. Therefore, the code should be fixed so that it will query o (the token holder) and transfer the underlying token to them:

uint256 amount = token.balanceOf(o);

...

// Burn the prinicipal token from Illuminate
token.burn(o, amount);

// Transfer the original underlying token back to the user
Safe.transfer(IERC20(u), o, amount);

As a side note, allowing anyone to burn and redeem the iPT of any holder could be dangerous in context of composability. Contracts that aren't aware of this mechanism might end up having funds stuck in them. For example, suppose there is a USDC-iPT/ETH Uniswap pool. When the iPT has matured and someone redeemed the iPT in the pool into USDC, the USDC will be stuck forever and LPs of the pool would lose fund.

Consider limiting redeem to msg.sender instead of allowing arbitrary redemption of any iPT holder.

Lines of code

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

Vulnerability details

Redeeming APWine and Tempus PT will always fail, causing a portion of iPT to not be able to be redeemed for the underlying token.

The issue is caused by the incorrect implementation of redeem:

uint256 amount = IERC20(principal).balanceOf(lender);
Safe.transferFrom(IERC20(u), lender, address(this), amount);

The first line correctly calculates the balance of PT token available in Lender. However, the second line tries to transfer the underlying token u instead of principal from Lender to Redeemer. Therefore, the redeeming process will always fail as both APWine.withdraw and ITempus.redeemToBacking will try to redeem non-existent PT.

Recommended Mitigation Steps

Fix the transfer line:

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

Lines of code

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

Vulnerability details

A malicious or compromised governance can arbitrarily approve any tokens (especially PT tokens) inside the contract and steal them.

While the approve function is needed to give allowance to external protocols that integrate with Illuminate, it opens up the possibility for governance to rug users.

Recommended Mitigation Steps

Modify the approve function to use a delay mechanism like withdrawing. In addition, add a approval revoke function that can be executed instantly in case of emergency.

Low Risk Vulnerabilities

1. Use transferFrom instead of transfer when trading PT via MarketPlace.sol

sellPrincipalToken(), buyPrincipalToken(), sellUnderlying(), and buyUnderlying() transfer token from the contract to the pool, instead of transferring them from msg.sender to the pool directly. This means msg.sender must send the token to the contract first before calling the trade function. As the function is permissionless, any PT sent to the contract can be stolen by a frontrunner.

Recommended Mitigation Steps

If these functions are intended to be called atomically using external contract, the proper way to use it should be documented.

If not, considering updating transfer to transferFrom from msg.sender directly to the pool.

Related Codes

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

2. Missing event implementation

There is a natspec comment indicating an event should be emitted when changing the feenominator, but the event is missing.

Recommended Mitigation Steps

Implement the event or remove the comment.

Related Codes

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

3. Wrong natspec comments

The @notice comment of setSwivel is wrong as it describe setFee's behaviour instead.

Recommended Mitigation Steps

Update it so it correctly describes behaviour of setSwivel.

Related Codes

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

Non-critical Vulnerabilities

1. Typos

L631 remaing should be remaining. L125 prinicipal should be principal. L189 prinicipal should be principal.