IllIllI - The Notional version of lend() can be used to lock iPTs

[github-actions]

IllIllI

high

The Notional version of lend() can be used to lock iPTs

Summary

The Notional version of lend() can be used to lock extra iPTs in the Lender contract

Vulnerability Detail

The Notional version of lend() has no checks to ensure that the principal value, p, passed in is for Notional, and therefore the Illuminate principal value can be passed in and used, which will allow callers to buy iPTs to the Lender contract (rather than Notional PTs), and then mint a second one to themselves when the iPT is minted at the end of the function.

Impact

When the underlying is sold in the marketplace, the resulting iPT is given to the Lender contract, and there is no supported way to have those iPTs redeemed and their underlying released, which means when users try to redeem their own iPTs, there will be less underlying available than there should be, and they will have lost principal.

Code Snippet

In the code block below, there are no checks that p is for notional, and the market-provided token for that p is used directly for depositing, and at the end of the function, more iPTs are minted:

// File: src/Lender.sol : Lender.lend()   #1

875      function lend(
876          uint8 p,
877          address u,
878          uint256 m,
879          uint256 a,
880          uint256 r
881      ) external nonReentrant unpaused(u, m, p) matured(m) returns (uint256) {
882          // Instantiate Notional princpal token
883 @>       address token = IMarketPlace(marketPlace).markets(u, m, p);
884  
885          // Transfer funds from user to Illuminate
886          Safe.transferFrom(IERC20(u), msg.sender, address(this), a);
...  
894          // Swap on the Notional Token wrapper
895 @>       uint256 received = INotional(token).deposit(a - fee, address(this));
896  
897          // Convert decimals from principal token to underlying
898          received = convertDecimals(u, token, received); 
...
908          // Mint Illuminate zero coupons
909:@>       IERC5095(principalToken(u, m)).authMint(msg.sender, received);

https://github.com/sherlock-audit/2023-01-illuminate/blob/main/src/Lender.sol#L875-L909

The deposit() function sells the underlying for iPTs, using the marketplace:

// File: src/tokens/ERC5095.sol : ERC5095.deposit()   #2

191        // consider the hardcoded slippage limit, 4626 compliance requires no minimum param.
192        uint128 returned = IMarketPlace(marketplace).sellUnderlying(
193            underlying,
194            maturity,
195            Cast.u128(a),
196            shares
197:        );

https://github.com/sherlock-audit/2023-01-illuminate/blob/main/src/tokens/ERC5095.sol#L191-L197

NOTE: INotional is an ERC4626 token, and the deposit() function comes from that interface. While the Illuminate PT's deposit() function has a different signature (the argument order is flipped), it's clear that this is a mistake that will be corrected as a part of this audit, since comments within the deposit() function itself refer to the need to be compliant with the ERC4626 standard, and discussions in the prior audit extensively mention that compliance was in fact necessary, and the deposit() function is not a part of the EIP-5095 standard.

Tool used

Manual Review

Recommendation

Revert if p is not MarketPlace.Principals.Notional

[thereksfour]

Escalate for 5 USDC

One of the bases for this vulnerability is that ERC5095 and INotional use the same deposit function signature
And as mentioned in the NOTE, they were different at the time.
https://github.com/notional-finance/wrapped-fcash/blob/master/contracts/wfCashERC4626.sol#L178

    function deposit(uint256 assets, address receiver) external override returns (uint256) {

https://github.com/Swivel-Finance/illuminate/blob/main/src/tokens/ERC5095.sol#L197

    function deposit(address r, uint256 a) external override returns (uint256) {

This also means that the vulnerability cannot be exploited directly
Exploiting this vulnerability requires the project to modify the deposit function, which is not a low cost
Look at the Criteria for Issues:

Medium: There is a viable scenario (even if unlikely) that could cause the protocol to enter a state where a material amount of funds can be lost. The attack path is possible with assumptions that either mimic on-chain conditions or reflect conditions that have a reasonable chance of becoming true in the future. The more expensive the attack is for an attacker, the less likely it will be included as a Medium (holding all other factors constant). The vulnerability must be something that is not considered an acceptable risk by a reasonable protocol team.

High: This vulnerability would result in a material loss of funds and the cost of the attack is low (relative to the amount of funds lost). The attack path is possible with reasonable assumptions that mimic on-chain conditions. The vulnerability must be something that is not considered an acceptable risk by a reasonable protocol team.

There were assumptions in the report that were not valid at the time, so the report should be medium

[sherlock-admin]

Escalate for 5 USDC

One of the bases for this vulnerability is that ERC5095 and INotional use the same deposit function signature
And as mentioned in the NOTE, they were different at the time.
https://github.com/notional-finance/wrapped-fcash/blob/master/contracts/wfCashERC4626.sol#L178

    function deposit(uint256 assets, address receiver) external override returns (uint256) {

https://github.com/Swivel-Finance/illuminate/blob/main/src/tokens/ERC5095.sol#L197

    function deposit(address r, uint256 a) external override returns (uint256) {

This also means that the vulnerability cannot be exploited directly
Exploiting this vulnerability requires the project to modify the deposit function, which is not a low cost
Look at the Criteria for Issues:

Medium: There is a viable scenario (even if unlikely) that could cause the protocol to enter a state where a material amount of funds can be lost. The attack path is possible with assumptions that either mimic on-chain conditions or reflect conditions that have a reasonable chance of becoming true in the future. The more expensive the attack is for an attacker, the less likely it will be included as a Medium (holding all other factors constant). The vulnerability must be something that is not considered an acceptable risk by a reasonable protocol team.

High: This vulnerability would result in a material loss of funds and the cost of the attack is low (relative to the amount of funds lost). The attack path is possible with reasonable assumptions that mimic on-chain conditions. The vulnerability must be something that is not considered an acceptable risk by a reasonable protocol team.

There were assumptions in the report that were not valid at the time, so the report should be medium

You've created a valid escalation for 5 USDC!

To remove the escalation from consideration: Delete your comment.
To change the amount you've staked on this escalation: Edit your comment (do not create a new comment).

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[hrishibhat]

Escalation rejected

While Sherlock does not intend to award issues submitted based on anticipated code, this particular submission is considered unique for two reasons:

1. This contest is a fix review contest which required auditors to have context from the previous contest.
2. The deposit function that requires the code change has been mentioned previously
   IllIllI - No markets can be created since Illuminate PTs are not ERC-4626 tokens 2022-10-illuminate-judging#106 and a fix has been added
   Fix mint and desosit methods to erc4626 spec Swivel-Finance/illuminate#28

[sherlock-admin]

Escalation rejected

While Sherlock does not intend to award issues submitted based on anticipated code, this particular submission is considered unique for two reasons:

1. This contest is a fix review contest which required auditors to have context from the previous contest.
2. The deposit function that requires the code change has been mentioned previously
   IllIllI - No markets can be created since Illuminate PTs are not ERC-4626 tokens 2022-10-illuminate-judging#106 and a fix has been added
   Fix mint and desosit methods to erc4626 spec Swivel-Finance/illuminate#28

This issue's escalations have been rejected!

Watsons who escalated this issue will have their escalation amount deducted from their next payout.

[IllIllI000]

Swivel-Finance/illuminate#20
PR properly changes the code to revert() if the principal value passed in by the caller of the Notional version of lend() is not MarketPlace.Principals.Notional, as was outlined in the issue recommendation. The argument order is later properly fixed as PR#28, for ERC5095's mint() and deposit() functions.