Migrator Vulnerability: Festive Orange Scallop DOS Risk

by Luna Greco 56 views

Hey everyone, let's dive into a critical vulnerability we've uncovered in the Migrator contract related to the "Festive Orange Scallop" project. This issue has the potential to cause a Denial of Service (DOS) or, at the very least, break core functionality. It stems from the way the migrateAllPositions function is designed, specifically the order of operations and how tokens are minted and borrowed during the migration process. We're going to break down the root cause, the impact, and potential mitigation strategies.

Summary

This article discusses a critical vulnerability within the migrateAllPositions function of the Migrator contract. The flaw lies in the incorrect order of operations during the migration process, potentially leading to reverts, denial of service (DOS), or incorrect migration of positions from Mendi to Malda. The vulnerability stems from minting tokens in V2 markets without first securing the underlying assets from Mendi, resulting in undercollateralization and failed transfers.

Root Cause

The primary function of the migrator is to seamlessly move positions from Mendi to Malda, which essentially acts as the new Mendi. The migrateAllPositions function is intended to handle this migration process, but a flaw in its logic creates a serious problem. Let's walk through the intended steps and pinpoint where things go wrong. First, let's examine the code snippet of the vulnerable function:

 function migrateAllPositions() external {
 // 1. Collect all positions from Mendi
 Position[] memory positions = _collectMendiPositions(msg.sender);

 uint256 posLength = positions.length;
 require(posLength > 0, "[Migrator] No Mendi positions");

 // 2. Mint mTokens in all v2 markets
 for (uint256 i; i < posLength; ++i) {
 Position memory position = positions[i];
 if (position.collateralUnderlyingAmount > 0) {
 uint256 minCollateral =
 position.collateralUnderlyingAmount - (position.collateralUnderlyingAmount * 1e4 / 1e5);
 ImErc20Host(position.maldaMarket).mintOrBorrowMigration(
 true, position.collateralUnderlyingAmount, msg.sender, address(0), minCollateral
 );
 }
 }

 // 3. Borrow from all necessary v2 markets
 for (uint256 i; i < posLength; ++i) {
 Position memory position = positions[i];
 if (position.borrowAmount > 0) {
 ImErc20Host(position.maldaMarket).mintOrBorrowMigration(
 false, position.borrowAmount, address(this), msg.sender, 0
 );
 }
 }

 // 4. Repay all debts in v1 markets
 for (uint256 i; i < posLength; ++i) {
 Position memory position = positions[i];
 if (position.borrowAmount > 0) {
 IERC20 underlying = IERC20(IMendiMarket(position.mendiMarket).underlying());
 underlying.approve(position.mendiMarket, position.borrowAmount);
 require(
 IMendiMarket(position.mendiMarket).repayBorrowBehalf(msg.sender, position.borrowAmount) == 0,
 "[Migrator] Mendi repay failed"
 );
 }
 }

 // 5. Withdraw and transfer all collateral from v1 to v2
 for (uint256 i; i < posLength; ++i) {
 Position memory position = positions[i];
 if (position.collateralUnderlyingAmount > 0) {
 uint256 v1CTokenBalance = IMendiMarket(position.mendiMarket).balanceOf(msg.sender);
 IERC20(position.mendiMarket).safeTransferFrom(msg.sender, address(this), v1CTokenBalance);

 IERC20 underlying = IERC20(IMendiMarket(position.mendiMarket).underlying());

 uint256 underlyingBalanceBefore = underlying.balanceOf(address(this));

 // Withdraw from v1
 // we use address(this) here as cTokens were transferred above
 uint256 v1Balance = IMendiMarket(position.mendiMarket).balanceOfUnderlying(address(this));
 require(
 IMendiMarket(position.mendiMarket).redeemUnderlying(v1Balance) == 0,
 "[Migrator] Mendi withdraw failed"
 );

 uint256 underlyingBalanceAfter = underlying.balanceOf(address(this));
 require(
 underlyingBalanceAfter - underlyingBalanceBefore >= v1Balance, "[Migrator] Redeem amount not valid"
 );

 // Transfer to v2
 underlying.safeTransfer(position.maldaMarket, position.collateralUnderlyingAmount);
 }
 }
 }

The function currently operates in the following sequence:

  1. Collects all positions from Mendi.
  2. Mints tokens on all available V2 markets.
  3. Borrows from all V2 markets.
  4. Repays all debt in V1 markets (Mendi).
  5. Withdraws and transfers collateral from Mendi V1 to V2.

The problem, guys, is the order of operations. Steps 4 and 5 are flipped. To make this work correctly, step 4 should actually be step 2, and step 5 should be step 3. Think about it: we need to repay all debt and withdraw collateral from Mendi before we start minting and borrowing in Malda. Otherwise, we're setting ourselves up for failure.

In the current sequence, we're minting in V2 without any backing assets from Mendi. We haven't withdrawn the assets from Mendi yet, so there's nothing to support the newly minted mERC20 tokens. This is like trying to build a house on thin air! Additionally, when we borrow (passing the migrator address as the receiver), we open up new debt on V2 while still carrying debt on V1 (Mendi). This temporarily puts us in an undercollateralized state, which isn't good.

The second step involves minting without any backing assets, which calls mERC20.mintOrBorrowMigration. Let's peek inside this function:

 function mintOrBorrowMigration(bool mint, uint256 amount, address receiver, address borrower, uint256 minAmount)
 external
 onlyMigrator
 {
 require(amount > 0, mErc20Host_AmountNotValid());
 if (mint) {
 // @audit why to pass false in doTransfer
 _mint(receiver, receiver, amount, minAmount, false); // @audit why both the user and receiver as receiver
 emit mErc20Host_MintMigration(receiver, amount);
 } else {
 _borrowWithReceiver(borrower, receiver, amount);
 emit mErc20Host_BorrowMigration(borrower, amount);
 }
 }

Notice that the doTransfer parameter is passed as false in the _mint function call. This is crucial because it leads to a critical issue. The _mint function looks like this:

 function _mint(address user, address receiver, uint256 mintAmount, uint256 minAmountOut, bool doTransfer)

Because doTransfer is false, this particular branch of code will never execute:

uint256 actualMintAmount = doTransfer ? _doTransferIn(minter, mintAmount) : mintAmount;

What does this mean? It means the user doesn't need to transfer any assets to the mToken yet. The balances will increase, sure, but without any actual value being transferred. This is a recipe for disaster.

Now, let's consider the borrowing step (the second step in the current flow). Borrowing does involve transferring tokens out (we enter the else branch because the mint boolean variable is false). This leads to a call to _borrowWithReceiver:

 _borrowWithReceiver(borrower, receiver, amount);

Which in turn calls the __borrow function and invokes this critical section:

 if (doTransfer) {
 /*
 * We invoke _doTransferOut for the borrower and the borrowAmount.
 * Note: The mToken must handle variations between ERC-20 and ETH underlying.
 * On success, the mToken borrowAmount less of cash.
 * _doTransferOut reverts if anything goes wrong, since we can't be sure if side effects occurred.
 */
 _doTransferOut(receiver, borrowAmount);
 }

Here's where the real trouble begins. We're trying to transfer underlying tokens out using _doTransferOut, but since no tokens were ever transferred in during the minting step (remember, doTransfer was false), this is highly likely to revert. We're either trying to transfer tokens that don't exist or tokens that belong to someone else. The _doTransferOut and _doTransferIn functions look like this:

 function _doTransferIn(address from, uint256 amount) internal virtual override returns (uint256) {
 // @audit donation attacks may happen
 uint256 balanceBefore = IERC20(underlying).balanceOf(address(this));
 IERC20(underlying).safeTransferFrom(from, address(this), amount);
 uint256 balanceAfter = IERC20(underlying).balanceOf(address(this));
 return balanceAfter - balanceBefore;
 }

 /**
 * @dev Performs a transfer out, ideally returning an explanatory error code upon failure rather than reverting.
 * If caller has not called checked protocol's balance, may revert due to insufficient cash held in the contract.
 * If caller has checked protocol's balance, and verified it is >= amount, this should not revert in normal conditions.
 */
 function _doTransferOut(address payable to, uint256 amount) internal virtual override {
 IERC20(underlying).safeTransfer(to, amount);
 }

The bottom line? Since no tokens were required to be transferred in the minting phase, borrowing will likely fail because we're trying to transfer out underlying assets that simply aren't there. This means the migration will almost certainly revert or, at the very least, produce incorrect results.

To recap, here's a breakdown of the core issues:

  1. Incorrect Order of Operations: The sequence of steps in migrateAllPositions is fundamentally flawed.
  2. Mismatched Receiver: The migrator is being passed as the receiver of the borrower, expecting it to have assets to repay debts on Mendi's behalf. However, the user didn't transfer any underlying assets during the minting step, so the migrator won't have the necessary tokens. Tokens might be minted incorrectly using other users' assets.
  3. No Backing Assets for Minting: We're minting tokens without transferring any underlying assets to support them.
  4. Attempting to Transfer Non-Existent Assets: We're trying to withdraw assets from the underlying balance that either don't exist or belong to someone else because no tokens were transferred during minting.
  5. Double Leveraging and Undercollateralization: If the function somehow manages to execute, we'll end up being double leveraged and undercollateralized.

Internal Pre-conditions

  • The Migrator contract must be deployed and properly initialized.
  • The Mendi and Malda markets must be active and functional.
  • Users must have existing positions in the Mendi market that need to be migrated.

External Pre-conditions

  • Users must have sufficient balances of the underlying assets to cover any potential borrowing or repayment requirements.
  • The underlying tokens must be approved for transfer by the Migrator contract.
  • Gas fees must be sufficient to execute the transactions.

Attack Path

  1. A user initiates the migrateAllPositions function.
  2. The function collects the user's positions from Mendi.
  3. The function attempts to mint mTokens in Malda without transferring underlying assets.
  4. The function attempts to borrow from Malda, triggering the _doTransferOut function.
  5. The _doTransferOut function reverts because there are insufficient underlying assets, causing the entire migration to fail.

Impact

This is a high-severity issue. It breaks the core functionality of the Migrator contract, making it impossible to migrate positions from Mendi to Malda. The incorrect order of operations and the flawed token transfer logic lead to a high probability of transaction reversion and a potential Denial of Service (DOS). The function is fundamentally flawed, and the borrow operation expects the migrator to receive underlying assets from the minting process. Since minting doesn't transfer anything (because we haven't withdrawn from Mendi yet and doTransfer is set to false, preventing the transfer branch from being reached), the tokens won't be transferred. This will lead to a DOS.

PoC

N/A (The vulnerability is evident from code analysis.)

Mitigation

No response

Mitigation

To fix this, guys, the order of operations in the migrateAllPositions function needs to be corrected. Here's the proposed sequence:

  1. Collect all positions from Mendi.
  2. Repay all debts in V1 markets (Mendi).
  3. Withdraw and transfer all collateral from V1 to V2.
  4. Mint mTokens in all V2 markets.
  5. Borrow from all V2 markets.

Furthermore, the logic within the mintOrBorrowMigration function needs to be revised to ensure that underlying assets are properly transferred during the minting process. The doTransfer parameter should likely be set to true in the _mint function call to ensure that tokens are actually transferred.

By addressing these issues, we can ensure the migrateAllPositions function works as intended and users can seamlessly migrate their positions from Mendi to Malda.

This analysis highlights the critical importance of careful planning and thorough review of smart contract logic, especially when dealing with complex operations like asset migration. A simple oversight in the order of operations can have significant consequences, leading to functionality failures and potential financial losses.