DEX Security Review: Mangrove Protocol Audit

Description

The MorphoVaultRouter is an extension of the ERC4626Router designed specifically for Morpho Vaults. Morpho vault depositors not only earn interest from underlying borrowers but also accrue MORPHO token rewards, as Morpho incentivizes lending. However, since the router contract is the entity supplying funds to Morpho, it accrues all MORPHO rewards on behalf of its depositors.

To claim these rewards, the admin of the MorphoVaultRouter must call claimRewardsForToken.

function claimRewardsForToken(IERC20 token, uint amount, bytes32[] calldata proof, address receiver)
    external
    onlyAdmin
{
    MORPHO_REWARD_DISTRIBUTOR.claim(msg.sender, address(token), amount, proof);
    token.transfer(receiver, amount);
    emit RewardsClaimed(token, amount, receiver);
}

When a router is deployed for a specific Kandel, the router’s admin is transferred to ERC4626Kandel. After the rewards are collected, the Kandel must call adminWithdrawTokens to withdraw them.

The issue is that ERC4626Kandel has no way to call claimRewardsForToken on the router, which results in lost Morpho rewards.

Recommendation

Consider creating an ERC4626Kandel tailored for Morpho that can call claimRewardsForToken.

Description

The claimRewardsForToken function of the MorphoVaultRouter contract is intended for the admin to claim token rewards from Morpho's rewards distributor. To do this, the function first intends to call IMorphoRewardDistributor::claim and then transfer the received tokens to a receiver account passed as parameter by the admin.

However, the first argument passed to claim is msg.sender - that is, the admin's account. As a consequence, the MorphoVaultRouter never receives the tokens from the distributor, and the token transfer after the call to claim will inevitably fail. Thus breaking the whole claiming process.

Recommendation

Modify the claimRewardsForToken function so that it correctly integrates with Morpho's rewards distributor contract, ensuring that the MorphoVaultRouter contract is the one receiving the rewards before transferring them to the intended receiver account. Also, consider improving the integration tests to ensure this integration with Morpho's contract works as intended.

Description

The _tokenBalance in the ERC4626Router is used to return the balance of a token hold by the router, this is computed by getting the balance of the router and the number of assets hold by the router in the the token's vault by calling convertToAssets.

  function _tokenBalance(IERC20 token) public view returns (uint balance) {
    balance = token.balanceOf(address(this));
    IERC4626 vault = vaults[token];
    if (address(vault) != address(0)) {
      balance += vault.convertToAssets(vault.balanceOf(address(this)));
    }
  }

The problem with this is that if the vault has fees on withdraw, the convertToAssets will not take that into consideration according to the EIP, this will result in untruthful balance returned by this function.

convertToAssets

Must NOT include any fees that are charged against assets in the vault.

Recommendation

Consider using previewRedeem as this should return the closes number of assets that would be received if redeem would have been called.

previewRedeem

MUST return as close to and no more than the exact amount of assets that would be withdrawn in a redeem call in the same transaction. I.e. redeem should return the same or more assets as previewRedeem if called in the same transaction.

Description

It is a known issue that ERC4626 deposit and withdrawal operations lack slippage checks. The EIP does not enforce them, leaving it to integrators to ensure slippage checks are implemented whenever a deposit or withdrawal is performed.

Every deposit or withdrawal creates an opportunity for MEV by manipulating the share price, which can result in fewer shares or assets being received by the vault.

A good example where slippage can cause significant damage is the setVaultForToken function. The router's admin can set or change a vault for a specific token by calling this function.

The flow is as follows:

1. Suppose the router currently holds an oldVault for a token T and has "shares" of that vault.
2. When an admin calls setVaultForToken(T, newVault), the code:
   1. Redeems 100% of the shares from oldVault into the underlying token T.
   2. Sets vaults[T] = newVault.
   3. Deposits all token T holdings into the newly assigned vault.

Since both _deposit and redeem lack slippage checks, they can be easily exploited through MEV. The above flow can be summarized as:

• Redeem up to oldVault.maxRedeem(...) shares.
• Deposit all resulting token T holdings into newVault.

If the share prices of the old and new vaults can be manipulated, an attacker could sandwich the change, causing the vault to withdraw fewer assets and deposit even less. This is possible by decreasing the share price during redemption from the old vault and increasing it during the deposit into the new vault.

This issue currently has a medium impact because the planned deployment is only for Arbitrum and Base, which do not have a public mempool, making exploitation less frequent. However, accidental MEV is still possible.

function setVaultForToken(IERC20 token, IERC4626 vault) public virtual onlyAdmin {
    // Verify that the token is not the zero address
    require(address(token) != address(0), "ERC4626Router/zeroToken");

    // If a previous vault exists, withdraw all assets
    IERC4626 oldVault = vaults[token];
    if (address(oldVault) != address(0)) {
        uint shares = oldVault.balanceOf(address(this));
        if (shares > 0) {
            uint maxRedeemable = oldVault.maxRedeem(address(this));
            require(maxRedeemable >= shares, "ERC4626Router/maxRedeemExceeded");
            oldVault.redeem(maxRedeemable < shares ? maxRedeemable : shares, address(this), address(this));
        }
    }

    // Set the new vault
    vaults[token] = vault;
    
    // Re-deposit the token
    _deposit(token);
}

Recommendation

Consider adding slippage checks throughout the router for both deposit and withdrawal operations from the vault.

Description

The ERC4626Router deposits funds into various vaults associated with different tokens. The shares of these vaults are held in the router for later redemption.

function _deposit(IERC20 token) internal {
    IERC4626 vault = vaults[token];
    if (address(vault) != address(0)) {
        uint balance = token.balanceOf(address(this));
        if (balance > 0) {
            uint maxDeposit = vault.maxDeposit(address(this));
            uint toDeposit = maxDeposit < balance ? maxDeposit : balance; 
            token.approve(address(vault), toDeposit);
            vault.deposit(toDeposit, address(this)); // <== As seen here
        }
    }
    // If no vault is found, do nothing
}

The issue arises when the token itself is a vault for another ERC20 token. The router lacks awareness of nested vaults and will misbehave in such cases.

Proof of Concept (PoC)

Consider the following setup:

• USDC (base token)
• Yearn USDC Vault (a Yearn vault that accepts USDC and returns yvUSDC shares)
• Yearn yvUSDC Vault (a hypothetical meta-vault that accepts yvUSDC tokens as assets and returns yvyvUSDC shares)

Now, assume the router is configured with the following token-to-vault pairs:

• USDC → Yearn USDC Vault
• yvUSDC → Yearn yvUSDC Vault

At this point, the Yearn USDC Vault yields yvUSDC shares, and the Yearn yvUSDC Vault yields yvyvUSDC shares.

Since yvUSDC exists both as an asset (for the meta-vault) and as a share (from the first vault), this dual role causes issues. When withdrawing or switching vaults for the yvUSDC token, the router aggregates both balances, potentially preventing it from fulfilling USDC withdrawals because the associated shares were already deposited or withdrawn.

Recommendation

Consider documenting this limitation so users are aware that nested vaults are not supported.

Description

During the ERC4626Kandel create offers flow we will end up in the __posthookSuccess__ call. This function attempts to push the BASE/QUOTE tokens to the router, which then deposits them into the respective ERC4626 vault associated with each token.

function __posthookSuccess__(MgvLib.SingleOrder calldata order, bytes32 makerData)
    internal
    override
    returns (bytes32 repostStatus)
{
    // Handle dual offer posting
    transportSuccessfulOrder(order);

    // If first puller, the router should deposit liquidity into the vault
    uint baseBalance = BASE.balanceOf(address(this));
    uint quoteBalance = QUOTE.balanceOf(address(this));

    erc4626Router().pushAndDeposit(BASE, baseBalance, QUOTE, quoteBalance);
    // Reposting offer residuals if any – but do not call super, since Direct will flush tokens unnecessarily
    repostStatus = MangroveOffer.__posthookSuccess__(order, makerData);
}

If the ERC4626 vault itself has a deposit hook (such as Euler’s Vaults, which support deposit hooks), there is a possibility that the call could reenter ERC4626Kandel from the vault’s hook.

Attached is a callgraph of the make offer flow with two highlights:

1. Green: where it reenters
2. Red: affected components from state changes after reentrancy

This issue is considered low risk for the following reason:
The ERC4626Router is responsible for depositing into the vault, and most hook implementations allow the msg.sender to perform additional logic. If a hook is enabled in the vault, only the router could act maliciously. However, this is not a concern, as the router has no malicious intent.

Recommendation

Consider further analyzing whether this reentrancy could affect the offer-making flow, as time constraints prevented a definitive conclusion. If no changes are made, consider documenting this behavior.

Description

The withdrawFundsForToken function withdraws a specified amount of assets from the router, which in turn attempts to withdraw from both the local balance and the ERC4626 vault.

An issue arises because ERC4626 rounds up shares slightly to avoid rounding errors. However, this can cause the call to revert if the router is unable to withdraw the exact amount specified as the withdraw will require slightly more shares than what we have available in the router:

if (amount_ != 0) {
    erc4626Router().withdraw(token, amount_);
}

Due to time constraints, we were unable to fully validate all flows where this issue might occur.

Recommendation

Consider using maxWithdrawal to determine the maximum amount that can be withdrawn, and instead of attempting to withdraw a fixed amount, withdraw as much as possible. This also provides additional protection for the Morpho or other ERC4626 vaults implementation, as they might enforces various withdrawal caps.

Description

Throughout the contracts we can see various typos or minor issues that we aggregated into one issue:

• In MorphoVaultRouter.sol#L6, the IMangrove import is not used
• In MorphoKandelSeeder.so#L1, there's a missing SPDX-License-Identifier at the top of the file.
• In ERC4626KandelSeeder.sol#L62, the NewERC4626Kandel params are wrongfully emitted, because the owner and address(kandel) parameters are inverted in the current version. It should be:

 emit NewERC4626Kandel(
      owner, olKeyBaseQuote.hash(), olKeyBaseQuote.flipped().hash(), address(kandel), address(kandel)
 );

• In ERC4626Kandel.sol#107-109, the makerData is not returned in the __lastLook__, this function just calls the parent function. So it'd be better if you just remove it.

• In ERC4626Kandel.sol#L44, IOfferLogic and AbstractRouter imports aren't used.

• In ERC4626Router.sol#L87, there's an unnecessary condition maxRedeemable < shares, because this is already checked on L86 and the maxRedeemable cannot be lower than shares.

• In ERC4626Router.sol#L76, the setVaultForToken function has some weird behaviors that might not be fully understood:

  1. If a new vault is set to address(0), the whole balance from the old vault remains in the router.
  2. If the new vault's maximum deposit is lower than before, then part of the past balance remains in the router as well. This should be properly documented.

• In ERC4626Router.sol#L134, the docstrings for the pushedAmount return value should clarify that if the token handled takes fees on transfer, the returned amount might be larger than the actual amount pushed.

• In ERC4626Router.sol#L97, the docstrings for the tokenBalanceOf function should state that the balance returned accounts for tokens in the contract plus the ones in the corresponding vault. Also, should specify whether potential fees are already accounted or not in the returned amount.

• The docstrings for the MorphoVaultRouter contract are duplicated.

Recommendation

Consider fixing the minor issues mentioned above.

Description

Some functions throughout the codebase may benefit from emitting events to log sensitive actions and changes:

• ERC4626Router.sol#L62 - adminWithdrawTokens and adminWithdrawNative.
• ERC4626Router.sol#L92 - setVaultForToken. Note that in MorphoVaultRouter there's an event VaultSet declared, although it's never used.

Recommendation

Consider emitting relevant events in the functions mentioned above.

Description

The ERC4626Router deposits assets into the vault using the _deposit function. In this function, an approval is performed beforehand. If, for any reason, the vault does not consume the full approval and the token is one like USDT, which reverts if there are any previous approvals, the entire deposit will revert.

While the chances of this happening are very low, as a safeguard, it is better to use OpenZeppelin's forceApprove, which resets approvals before setting them to the new value.

function _deposit(IERC20 token) internal {
    IERC4626 vault = vaults[token];
    if (address(vault) != address(0)) {
      uint balance = token.balanceOf(address(this));
      if (balance > 0) {
        uint maxDeposit = vault.maxDeposit(address(this));
        uint toDeposit = maxDeposit < balance ? maxDeposit : balance;
        token.approve(address(vault), toDeposit);
        vault.deposit(toDeposit, address(this));
      }
    }
    // if no vault is found, do nothing
  }

Recommendation

Consider using OpenZeppelin's forceApprove instead of approve.

Description

The setVaultForToken function of the ERC4626Router contract does not check whether the passed token is indeed the asset supported by the passed vault contract. Although only a trusted role in the system is expected to call this function, adding a validation should help avoid unexpected errors when setting tokens for vaults.

Recommendation

Consider including a require statement to check that address(token) == vault.asset() in the setVaultForToken function.

Description

All low-level function calls in the MangroveERC4626KandelVault contract use Solidity's built-in abi.encodeWithSignature function, which does not perform full type checks of the parameters.

Recommendation

To avoid potential errors when creating the calldata for low-level calls, it's advisable to rely on abi.encodeCall instead, which performs full type checks. Also consider documenting why low-level calls are needed in this contract, instead of using regular high-level calls.

Description

When a MangroveERC4626KandelVault is deployed, the vault itself becomes the admin of the Kandel. This means that only the vault can call onlyAdmin functions.

The following functions are restricted to onlyAdmin in the Kandel:

• adminWithdrawTokens
• adminWithdrawNative
• setVaultForToken
• setBaseQuoteTickOffset
• populateFromOffset
• populateChunkFromOffset
• setStepSize
• setGasprice
• setGasreq
• populate
• populateChunk
• withdrawFunds
• retractAndWithdraw
• retractOffers
• approve
• withdrawFromMangrove
• setAdmin

However, only the following functions are actually called within the vault or can be called by the vault:

• setVaultForToken
• adminWithdrawTokens
• adminWithdrawNative
• withdrawFunds
• withdrawFromMangrove
• setBaseQuoteTickOffset
• populate
• populateChunk
• retractOffers
• retractAndWithdraw

Recommendation

Consider double-checking whether any additional functions should be callable by the MangroveERC4626KandelVault.