IllIllI - Fee-on-transfer underlyings can be used to mint Illuminate PTs without fees

[sherlock-admin]

IllIllI

high

Fee-on-transfer underlyings can be used to mint Illuminate PTs without fees

Summary

Fee-on-transfer underlyings can be used to mint Illuminate PTs without fees

Vulnerability Detail

Illuminate's Lender does not confirm that the amount of underlying received is the amount provided in the transfer call. If the token is a fee-on-transfer token (e.g. USDT which is currently supported), then the amount may be less. As long as the fee is smaller than Illuminate's fee, Illuminate will incorrectly trust that the fee has properly been deducted from the contract's balance, and then will swap the funds and mint an Illuminate PT.

Impact

Theft of unclaimed yield

Attackers can mint free PT at the expense of Illuminate's fees.

Code Snippet

This is one example from one of the lend() functions, but they all have the same issue:

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

750        function lend(
751            uint8 p,
752            address u,
753            uint256 m,
754            uint256 a,
755            uint256 r
756        ) external unpaused(u, m, p) returns (uint256) {
757            // Instantiate Notional princpal token
758            address token = IMarketPlace(marketPlace).token(u, m, p);
759    
760            // Transfer funds from user to Illuminate
761  @>        Safe.transferFrom(IERC20(u), msg.sender, address(this), a);
762    
763            // Add the accumulated fees to the total
764            uint256 fee = a / feenominator;
765            fees[u] = fees[u] + fee;
766    
767            // Swap on the Notional Token wrapper
768  @>        uint256 received = INotional(token).deposit(a - fee, address(this));
769    
770            // Verify that we received the principal tokens
771            if (received < r) {
772                revert Exception(16, received, r, address(0), address(0));
773            }
774    
775            // Mint Illuminate zero coupons
776  @>        IERC5095(principalToken(u, m)).authMint(msg.sender, received);
777    
778            emit Lend(p, u, m, received, a, msg.sender);
779            return received;
780:       }

https://github.com/sherlock-audit/2022-10-illuminate/blob/main/src/Lender.sol#L750-L780

And separately, if any of the external PTs ever become fee-on-transfer (e.g. CTokens, which are upgradeable), users would be able to mint Illuminate PT directly without having to worry about the FOT fee being smaller than the illuminate one, and the difference would be made up by other PT holders' principal, rather than Illuminate's fees:

// File: src/Lender.sol : Lender.mint()   #2

270        function mint(
271            uint8 p,
272            address u,
273            uint256 m,
274            uint256 a
275        ) external unpaused(u, m, p) returns (bool) {
276            // Fetch the desired principal token
277            address principal = IMarketPlace(marketPlace).token(u, m, p);
278    
279            // Transfer the users principal tokens to the lender contract
280 @>         Safe.transferFrom(IERC20(principal), msg.sender, address(this), a);
281    
282            // Mint the tokens received from the user
283 @>         IERC5095(principalToken(u, m)).authMint(msg.sender, a);
284    
285            emit Mint(p, u, m, a);
286    
287            return true;
288:       }

https://github.com/sherlock-audit/2022-10-illuminate/blob/main/src/Lender.sol#L270-L288

POC

Imagine that the Illuminate fee is 1%, and the fee-on-transfer fee for USDT is also 1%

1. A random unaware user calls one of the lend() functions for 100 USDT
2. lend() does the transferFrom() for the user and gets 99 USDT due to the USDT 1% fee
3. lend() calculates its own fee as 1% of 100, resulting in 99 USDT remaining
4. lend() swaps the 99 USDT for a external PT
5. the user is given 99 IPT and only had to spend 100 USDT, and Illuminate got zero actual fee, and actually has to make up the difference itself in order to withdraw any fees (see other issue I've filed about this).

Tool used

Manual Review

Recommendation

Check the actual balance before and after the transfer, and ensure the amount is correct, or use the difference as the amount

[sourabhmarathe]

Set label to high because based on what the report indicated.

[IllIllI000]

@sourabhmarathe can you elaborate on what aspect of the report made this a high? https://github.com/sherlock-audit/2022-10-illuminate-judging/issues/104 describes a separate way of how to mint IPT using protocol fees

[sourabhmarathe]

I was just updating the issue to reflect what the Watson had put on the report. To me, it appeared mislabeled as the original report had a high level severity at the top of the report.

[sourabhmarathe]

Re #104: It should not be marked as a duplicate. It's a separate issue in it's own right. That said, it doesn't put user funds at risk, so I think it should remain at a Medium.

[JTraversa]

I dont quite think this should be valid all given we are not planning to accept any niche tokens that would include fee on transfers. (We are launching DAI, USDC, stETH)

The admin currently has complete control over market creation meaning suggested remediations increase gas costs for our users with very minimal or no benefit at the moment!

[IAm0x52]

Escalate for 1 USDC

Reminder @Evert0x

[sherlock-admin]

Escalate for 1 USDC

Reminder @Evert0x

You've created a valid escalation for 1 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.

[KenzoAgada]

Escalate for 50 USDC See sponsor's comments.

I dont quite think this should be valid all given we are not planning to accept any niche tokens that would include fee on transfers. (We are launching DAI, USDC, stETH)

The admin currently has complete control over market creation meaning suggested remediations increase gas costs for our users with very minimal or no benefit at the moment!

While USDT can be upgraded to have FoT, this is an external condition, therefore this issue might be more properly described as a medium at best.

[sherlock-admin]

Escalate for 50 USDC See sponsor's comments.

I dont quite think this should be valid all given we are not planning to accept any niche tokens that would include fee on transfers. (We are launching DAI, USDC, stETH)

The admin currently has complete control over market creation meaning suggested remediations increase gas costs for our users with very minimal or no benefit at the moment!

While USDT can be upgraded to have FoT, this is an external condition, therefore this issue might be more properly described as a medium at best.

You've created a valid escalation for 50 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.

[Evert0x]

Escalation accepted

[sherlock-admin]

Escalation accepted

This issue's escalations have been accepted!

Contestants' payouts and scores will be updated according to the changes made on this issue.

[sourabhmarathe]

https://github.com/Swivel-Finance/illumigrate/pull/280