IllIllI - No markets can be created since Illuminate PTs are not ERC-4626 tokens

[sherlock-admin]

IllIllI

medium

No markets can be created since Illuminate PTs are not ERC-4626 tokens

Summary

No markets can be created since Illuminate PTs are not ERC-4626 tokens, and will cause pool creation to fail

Vulnerability Detail

I checked with the sponsor and they confirmed that the plan was to use yieldspace-tv pools to swap Illuminate PTs for underlying, and that they planned to deploy the existing pool contract, rather than writing a new special module. The existing Pool contract relies on the ERC-4626 interface to accomplish some of its tasks (and tokens that do not comply with it need to create new modules in order to override those functions). One such task is the fetching of the price, which relies on IERC4626.convertToAssets() which does not exist in the EIP-5095 spec that the Illuminate PT follows. The fetching of the price is done in the pool constructor, and Illuminate PTs require the pool to already have been immutably set in the market before they're constructed, so therefore there is no way to create a market for any asset.

In addition to not being able to construct the pools, there are other functions such as asset(), and deposit() (note the flipped args), which do not exist in ERC5095 but are relied on by the Pool, so even if the constructor issue is addressed, things will fail later.

Impact

Smart contract unable to operate due to lack of token funds

MarketPlace.createMarket() can't be called with a valid pool, so nobody can use any feature of the Illuminate project.

Code Snippet

Market creation unconditionally constructs Illuminate PTs:

// File: src/MarketPlace.sol : MarketPlace.createMarket()   #1

150            // Create an Illuminate principal token for the new market
151            address illuminateToken = address(
152 @>             new ERC5095(
153                    u,
154                    m,
155                    redeemer,
156                    lender,
157                    address(this),
158                    n,
159                    s,
160                    IERC20(u).decimals()
161                )
162:           );

https://github.com/sherlock-audit/2022-10-illuminate/blob/main/src/MarketPlace.sol#L150-L162

Illuminate PTs are EIP-5095 contracts, not EIP-4626 ones, and do not implement the convertToAssets() function:

// File: src/tokens/ERC5095.sol   #2

13:@>  contract ERC5095 is ERC20Permit, IERC5095 {

https://github.com/sherlock-audit/2022-10-illuminate/blob/main/src/tokens/ERC5095.sol#L13

The immutable pool is set in the constructor, and comes from the MarketPlace:

// File: src/tokens/ERC5095.sol : ERC5095.constructor()   #3

37        constructor(
38            address _underlying,
39            uint256 _maturity,
40            address _redeemer,
41            address _lender,
42            address _marketplace,
43            string memory name_,
44            string memory symbol_,
45            uint8 decimals_
46        ) ERC20Permit(name_, symbol_, decimals_) {
47            underlying = _underlying;
48            maturity = _maturity;
49            redeemer = _redeemer;
50            lender = _lender;
51            marketplace = _marketplace;
52 @>         pool = IMarketPlace(marketplace).pools(underlying, maturity);
53:       }

https://github.com/sherlock-audit/2022-10-illuminate/blob/main/src/tokens/ERC5095.sol#L37-L53

Pools must be set ahead of time, and cannot change once set:

// File: src/MarketPlace.sol : MarketPlace.setPool()   #4

259        function setPool(
260            address u,
261            uint256 m,
262            address a
263        ) external authorized(admin) returns (bool) {
264            // Verify that the pool has not already been set
265            address pool = pools[u][m];
266    
267            // Revert if the pool already exists
268 @>         if (pool != address(0)) {
269 @>             revert Exception(10, 0, 0, pool, address(0));
270 @>         }
271    
272            // Set the pool
273            pools[u][m] = a;
274    
275            emit SetPool(u, m, a);
276            return true;
277:       }

https://github.com/sherlock-audit/2022-10-illuminate/blob/main/src/MarketPlace.sol#L259-L277

Yieldspace-tv Pools rely on the function that does not exist in the Illuminate PT:

    function _getCurrentSharePrice() internal view virtual returns (uint256) {
        uint256 scalar = 10**baseDecimals;
@>      return IERC4626(address(sharesToken)).convertToAssets(scalar);
    }

    /// Returns current price of 1 share in 64bit.
    /// Useful for external contracts that need to perform calculations related to pool.
    /// @return The current price (as determined by the token) scalled to 18 digits and converted to 64.64.
    function getC() external view returns (int128) {
@>      return _getC();
    }

    /// Returns the c based on the current price
    function _getC() internal view returns (int128) {
@>      return (_getCurrentSharePrice() * scaleFactor).divu(1e18);
    }

https://github.com/yieldprotocol/yieldspace-tv/blob/8685abc2f57c2f3130165404a77620a3220fb182/src/Pool/Pool.sol#L1400-L1415

getC() is called by the constructor, so pools cannot be constructed with Illuminate PTs:

@>        if ((mu = _getC()) == 0) {

https://github.com/yieldprotocol/yieldspace-tv/blob/8685abc2f57c2f3130165404a77620a3220fb182/src/Pool/Pool.sol#L193

The existing fork tests mostly use the Yield USDC pool rather than creating an actual new pool.

Tool used

Manual Review

Recommendation

Implement a new yieldspace-tv module for EIP-5095 contracts

[JTraversa]

Unless I've misunderstood hah, I think youve misunderstood yieldspace tv a little bit

The sharesToken is not the the illuminate PT, fyToken would be the illuminate PT.

sharesToken is the yield bearing vault that the baseToken is deposited into, meaning the variance would depend on your choice of external yield bearing protocol (a 4626 vault, euler, compound, lido, etc.,), not on your protocol's Principal Token implementation.

[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. As per them and the source code seems like the issue is invalid, the 4626 functions are not being run on the 5095iPT.

[sherlock-admin]

Escalate for 50 USDC See sponsor's comments. As per them and the source code seems like the issue is invalid, the 4626 functions are not being run on the 5095iPT.

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/239