M Extensions Updates

Whenever a function guarded with the isNotLocked modifier is called, Locker.set(caller_) is executed. If msg.sender is a trusted router, caller_ is set to whatever address the router returns. Otherwise, the real msg.sender is used.

However, SwapFacility uses msg.sender in all of its functions. For example, it will try transferring assets out of msg.sender, etc... This will result in reverts when the function is called by a trusted router since it's expected to only forward the call, not pay for the transactions. Furthermore, msg.sender is passed to the permit() calls as well, meaning it has to be the signer of the message, which is impossible for the router.

There is also asymmetry existing when calling JMI.wrap(), because it uses ISwapFacility(msg.sender).msgSender(), which will be the correct forwarded sender.

Replace the msg.sender usage in SwapFacility with msgSender().

The JMIExtension overwrites all the MYieldToOne hooks which reverts if a given account is frozen to also add the _requireNotPaused check, except for the _beforeApprove hook.

Consider to also overwrite and add the _requireNotPaused to the _beforeApprove hook.

The JMIExtension contract uses balanceOf to perform checks related to the asset amounts deposited instead of a internal mapping. On the other hand it uses the variable totalAssets to store the total amount of all deposited non-M-token assets.

This can lead to DoS and stuck funds if someone would transfer assets into the contract directly. For example:

• totalAssets = 0
• Two assets are whitelisted (Asset1 & Asset2)
• Bob deposits 100 Asset1 and 100 Asset2
  • totalAssets = 200
• Eve transfers 50 Asset1 into the contract
• Eve calls replaceAssetWithM to get 150 Asset1 out of the contract
  • totalAssets = 200 - 150 = 50
• Now it’s not possible to get the 100 Asset2 out of the contract as totalAssets would underflow in that case

Another point is that asset caps can be consumed without wrapping.

For a wrap to succeed, the asset cap of the asset being wrapped should not be exceeded.

assetCap(asset) >= (IERC20(asset).balanceOf(address(this)) + amount)

Because balanceOf() is used, anyone can transfer asset tokens to the contract and consume the cap. The tokens cannot be utilized since totalAssets won't be modified.

Consider to implement a internal mapping instead to track asset amounts.

The _fromExtensionToAssetAmount calculation in the _replaceAssetWithM function could round to 0 in the edge case that the given asset has less decimals than the $M token and a dust amount is given.

This would result in the user losing $M tokens and decreasing the totalAssets without receiving any assets in return.

The same behavior is also present in the _wrap() flow, where users can send assets, but have 0 extension tokens minted for them.

        uint256 jmiAmount_ = _fromAssetToExtensionAmount(asset, amount);

				...

        _mint(recipient, jmiAmount_);

Consider reverting in both cases if the resulting amount is 0.

Before interacting with the system, an extension has to be either approved by an admin or be an approved earner.

    function isApprovedExtension(address extension) public view returns (bool) {
        return _isApprovedEarner(extension) || isAdminApprovedExtension(extension);
    }

The _isApprovedEarner() function will return true for any address if the earners list is being ignored.

    function _isApprovedEarner(address extension) private view returns (bool) {
        return
            IRegistrarLike(registrar).get(EARNERS_LIST_IGNORED_KEY) != bytes32(0) ||
            IRegistrarLike(registrar).listContains(EARNERS_LIST_NAME, extension);
    }

Therefore, whenever the earners list is ignored, any arbitrary address can be used for an extension. This allows emitting Swapped, SwappedOutM and JMIAssetReplaced() events with arbitrary addresses and poses risks if the code changes in future.

Consider excluding the EARNERS_LIST_IGNORED_KEY part from _isApprovedEarner() and managing the swappers via approvals.

To wrap an mToken into JMIExtension users must call SwapFacility.swap() and specify mToken as tokenIn and JMIExtension as tokenOut. This will execute the first branch in _swap():

if (tokenIn == mToken) return _swapInM(tokenOut, amount, recipient);

The _swapInM() function is not adjusted to support JMIExtension, instead it used the generic MExtension interface to call wrap()

IMExtension(extensionOut).wrap(recipient, amount);

Because JMIExtension inherits from MExtension, the call will succeed, but MExtension.wrap() will be executed instead of JMIExtension.wrap() There the internal _wrap(address,address,uint256) function is executed

    function wrap(address recipient, uint256 amount) external onlySwapFacility {
        // NOTE: `msg.sender` is always SwapFacility contract.
        //       `ISwapFacility.msgSender()` is used to ensure that the original caller is passed to `_beforeWrap`.
        _wrap(ISwapFacility(msg.sender).msgSender(), recipient, amount);
    }

This function is different than the _wrap(address,address,address,uint256) function defined in JMIExtension . From there on, the whole wrap is executed by the logic defined in MExtension instead of JMIExtension, which means the empty _beforeWrap() function of MExtension is executed and the following validation, including the pause check, is not performed:

    function _beforeWrap(address asset, address account, address recipient, uint256 amount) internal view virtual {
        _requireNotPaused();

        if (!isAllowedAsset(asset)) revert AssetNotAllowed(asset);
        if (!isAllowedToWrap(asset, amount)) revert AssetCapReached(asset);

        super._beforeWrap(account, recipient, amount);
    }

Since asset here is mToken, skipping the two if statements is fine, but not calling _requireNotPaused() means the code doesn’t respect the paused flag at all.

In result, wrapping mTokens into the JMIExtension will be possible even when it’s in a paused state.

Call the correct function If tokenOut in SwapFacility.swapInM() is the JMIExtension .

If one of the allowed assets (stablecoins) in the JMIExtension would lose in value a arbitrage opportunity arises:

• Traders buys assets for $0.9
• Trader deposit the assets into the JMIExtension
• Trader swaps the JMIExtension into a safe one
• Repeat

Be aware of this risk and react as quickly as possible in that case by pausing the contracts and setting the cap of the given asset to zero.

The replaceAssetWithM flow does not check if the given asset was whitelisted (cap > 0).

This function can therefore be used to swap any M0 extension to any token in the JMIExtension.

If by accident a token with higher value would land inside the contract any user could claim it by for example performing a $1 M0 Extension against 1 WETH swap.

Be aware that this function can be used in that way and document this behavior.

Or consider to only allow the withdraw of whitelisted tokens to decrease potential attack surface. In this case however a new whitelist would make more sense than the current cap logic as otherwise it would no longer be possible to prevent further deposits while still allowing withdraws.

Whenever an extension is wrapping mTokens, it pulls amount of these mTokens from the sender and mints the exact same amount of extension tokens. However, due to the index tracking behavior of the MToken , transferring X amount can result in less than X tokens received.

        IMTokenLike(mToken).transferFrom(msg.sender, address(this), amount);
        _mint(recipient, amount);

In the case of wrap(), the system will mint more extension tokens that the received mTokens, which means some of the extension tokens will not be redeemable. The same is true for _replaceAssetWithM() .

The opposite rounding problem exists in _unwrap() , there the amount being subtracted is rounded up and because of that the system will be potentially losing value with each transfer.

There are already comments in the MExtension acknowledging the problem. While it’s hard to be solved for the _unwrap() case, you can add balance delta tracking for wrap() and _replaceAssetWithM() to avoid some of the losses.

The approve hook reverts if the spender is frozen, regardless of the allowance amount. This prevents users from setting allowance to zero for spenders that later became frozen, effectively trapping their prior approvals until the spender is unfrozen.

Allow approve(owner, spender, 0) to succeed even if the spender is frozen.

The JMIExtension enforces freeze checks for wrap/unwrap/transfer via MYieldToOne hooks, but _replaceAssetWithM() does not validate whether the recipient is frozen. A frozen account can therefore receive allowed assets directly through replaceAssetsWithM()

If this is not expected, revert if the recipient is frozen.

The setAssetCap saves the given assets decimals and it is not possible to update them in case the decimals of the given ERC20 token would ever change.

Be aware of that and consider to refetch the decimals every time the setAssetCap function is called to be able to react on such a change.

Now asset balances are tracked with a internal balance. Therefore if now non $M token assets are accidentally transferred into the JMIExtension contract they are lost.

Consider to add a sweep function to be able to withdraw these assets.

A try/catch was added to SwapFacility.canSwapViaPath() to fetch the paused status of the two tokens.

        // If contracts are paused, return false
        try Pausable(tokenIn).paused() returns (bool tokenInPaused) {
            isTokenInPaused = tokenInPaused;
        } catch {}
        try Pausable(tokenOut).paused() returns (bool tokenOutPaused) {
            isTokenOutPaused = tokenOutPaused;
        } catch {}

Because the addresses are arbitrary, any contract (or EOA with code) can be passed as a token, return a data that cannot be decoded to a bool and make the call to canSwapViaPath() to revert. This can lead to DOS for external integrators that expect the function to never revert.

Document the risk of reverting.

The following two variables in the SwapFacility.canSwapViaPath() are assigned their default values (false for bool), which is unnecessary.

        bool isTokenInPaused = false;
        bool isTokenOutPaused = false;

Consider removing the assignments.

The balance of the tokens in the Asset struct is stored as uint240. The comment states that M token's supply won't exceed uint240, so balance should be safe.

        // M token's supply can't exceed uint240, so uint240 is safe to use.
        uint240 balance;

However, this balance doesn't track M assets, but every other enabled asset. In the _wrap() function the amount of tokens transferred is unsafely cast to uint240. If the token is non-standard , i.e has very big precision, and the asset cap allows it, the amount used may exceed uint240 and result in wrong accounting.

1. Fix the comment to explain the right reason why using uint240 is safe
2. Make sure to not use assets which can have such big supplies
3. Configure asset caps with this in mind