ERC 4626 - Moonwell

The maxDeposit(address) function of an ERC-4626 vault is expected to return the maximum number of assets which can be deposited by the specified address, however in MoonwellERC4626, it underlying returns the value of maxMint(address) - the maximum number of shares which can be deposited:

/// @notice maximum amount of underlying tokens that can be deposited
///         into the underlying protocol
function maxDeposit(address) public view override returns (uint256) {
    return maxMint(address(0));
}

Concerning invocations of maxMint(address) which do not return type(uint256).max, an incorrect value will be returned to the caller, which will invalidate the contractual obligations of the ERC-4626 specification.

Scale values returned from calls to maxMint(address(0)) back into assets when not unlimited:

/// @notice maximum amount of underlying tokens that can be deposited into the
///         underlying protocol
function maxDeposit(address) public view override returns (uint256) {
  uint256 shares = maxMint(address(0));

  // unlimited  
  if (shares == type(uint256).max) return type(uint256).max;

  return convertToAssets(shares);
}

Remediation Plan

SOLVED: The Moonwell team solved the issue by reversing the inverted logic and added the required conversion from shares back into assets.

In the unlikely event a MarketConfig were to define an emissionToken of mToken (i.e. through a DAO vote or governance attack), the MoonwellERC4626 vault may inadvertently withdraw all the underlying asset liquidity to the rewardsRecipient during a call to claimRewards():

for (uint256 i = 0; i < configs.length; i++) {
    uint256 amount = ERC20(configs[i].emissionToken).balanceOf(
        address(this)
    );

    if (amount != 0) {
        /// gas opti, skip transfer and event emission if no rewards
        ERC20(configs[i].emissionToken).safeTransfer(
            rewardRecipient,
            amount
        );

        emit ClaimRewards(amount, configs[i].emissionToken);
    }
}

Here, the rewardsRecipient is able to claim the MoonwellERC4626's balanceOf the returned emissionTokens. If the underlying asset of the vault was configured as an emissionToken for a market, the entirety of underlying vault liquidity could be withdrawn by the rewardsRecipient.

If an emissionToken is address(0) or equal to address(mToken), continue iterating.

Additionally, consider using a try statement to wrap limited-gas calls to emissionTokens if they cannot be trusted.

Remediation Plan

RISK ACCEPTED: The Moonwell team accepted the risk of this issue, but never intended to configure an MToken as an emissionToken.

One of the unique properties of the MoonwellERC4626 vault is its dependence upon a dedicated factory contract to initialize vault deployments in a way that is resistant to inflation attacks. This in turn liberates the vault implementation from the burden of inflation logic encapsulation.

To prevent classical inflation attacks, Factory4626 ensures an initial quantity of vault shares are burned according to the following formula:

uint256 initialMintAmount = 10 ** ((ERC20(asset).decimals() * 2) / 3);

The resultant implication is that for tokens with zero decimals, an initialMintAmount of merely a single share is expected to be burned to the zero address upon new vault creation.

Burning a single share is approximate to using a decimalOffset of zero in a Virtual Asset Shares (VAS) implementation, which, while sufficient to negate the profitability of classical vault inflation attacks, leaves some griefing vulnerabilities remaining actionable. In these attacks, a depositor's collateral can be lost at some expense to the attacker.

Enforce a minimum amount of shares to be burned, i.e 1_000.

Remediation Plan

ACKNOWLEDGED: The Moonwell team has acknowledged the issue, and confirmed that tokens with zero decimals will never be listed on Moonwell.