MigratorV2 Audit

1

Summary

Timeline: From 2025-03-28 To 2025-04-11

Total Issues: 37 (34 resolved, 2 partially resolved)

High Severity Issues: 1 (1 resolved)

Medium Severity Issues: 4 (4 resolved)

Low Severity Issues: 18 (16 resolved, 1 partially resolved)

Notes & Additional Information: 14 (13 resolved, 1 partially resolved)

Scope

OpenZeppelin audited the woof-software/migrator-v2 repository at commit f5385e7.

The fix review was conducted at commit 4f57d33. All fixes were submitted as one commit.

In scope were the following files:

contracts
├── MigratorV2.sol
├── adapters
│   ├── AaveV3Adapter.sol
│   ├── AaveV3UsdsAdapter.sol
│   ├── MorphoAdapter.sol
│   ├── MorphoUsdsAdapter.sol
│   ├── SparkAdapter.sol
│   └── SparkUsdsAdapter.sol
├── errors
│   └── CommonErrors.sol
├── modules
│   ├── ConvertModule.sol
│   └── SwapModule.sol
└── utils
    ├── DelegateReentrancyGuard.sol
    └── UniswapV3PathFinder.sol

System Overview

MigratorV2 is a flexible and modular smart contract designed to facilitate the migration of user positions from multiple lending protocols (Aave, Spark, and Morpho) to Compound III (Comet). The contract leverages Uniswap V3 flash loans to streamline the migration process, minimizing gas costs, reducing manual interventions, and ensuring smooth transitions without disrupting user positions. With its adaptable architecture, MigratorV2 is designed to handle future protocol extensions without requiring redeployment.

Migration

The migration process involves several key steps to facilitate the transfer of assets between protocols.

  1. Initially, the contract may initiate a flash loan from Uniswap V3, using either the base token of the target market or DAI for USDS markets. The loaned funds are then used to repay the user’s existing debt in the source protocol, which could be Aave, Spark, or Morpho.
  2. Following debt repayment, the contract withdraws the user’s collateral from the source protocol. If necessary, the contract swaps assets via Uniswap V3 or converts DAI to USDS at a 1:1 ratio to enable smooth migration. Users have the option to select specific collaterals to migrate or migrate all available assets, either fully or partially.
  3. Finally, the migrated collateral is deposited into the selected Compound III market on behalf of the user, with the option to deposit in the market’s base token (such as USDC or USDS) or in one of the supported collateral tokens.

Flash Loan Repayment

The flash loan repayment logic involves different strategies depending on the type of collateral migrated. In the case of full collateral migration using the base token, part of the migrated funds will be used to repay the flash loan along with its associated fees. When collateral is migrated as non-base tokens, the contract attempts to repay the loan by withdrawing the necessary amount of the base token from the user’s Compound III balance. For mixed collateral migrations, where some funds are in base tokens and others are in collateral tokens, a debt is created if the base token balance is insufficient to cover the loan repayment. This debt can either be deducted from the base token or withdrawn from the collateral in Compound III.

Security Model and Trust Assumptions

During the audit, the following trust assumptions were made:

  • For the adapter contracts, the provided swap paths are correctly formulated and appropriate for the type of swap requested.
  • The deployer supplies accurate contract addresses for integrations with Uniswap V3 Pools, the lending protocols, and Comet Markets.
  • The USDS-to-DAI and DAI-to-USDS conversion pathways will remain unpaused and fees cannot be enabled for these conversions.

Privileged Roles

The only privileged role in this deployment is the owner of the MigratorV2 contract. This role has the ability to:

  • Manage adapter contracts for integrating with various protocols,
  • Configure the liquidity pools used to provide flash loans for specific Comet Markets, and
  • Pause or unpause designated functions.

It is assumed that the Compound Timelock will be assigned as the owner of the MigratorV2 contract.

Notes on UniswapV3PathFinder

The review identified multiple issues with the UniswapV3PathFinder contract. In response to the issues raised, the Woof! team has indicated that the aforementioned contract has been deprecated and will not be used within the Compound ecosystem. The team has moved the contract to a non-production folder in commit 4f57d33.

It is noted that the UniswapV3PathFinder contract should be re-audited before it is deployed and/or used in conjunction with any elements interacting with the Compound ecosystem (e.g., if it is to be used as a utility to provide information for a related UI).

High Severity

Multi-Swap Paths Cannot Be Constructed

The getBestMultiSwapPath function of the UniswapV3PathFinder contract finds the best multi-hop swap path for a given token pair. The MultiSwapParams input allows the user to specify an array of intermediate tokens as part of the multi-hop swap. The issue arises when the function attempts to construct the best connector paths. The path is always constructed with either the tokenIn or the tokenOut set as the tokenIn input variable for the _getBestSingleSwapPath. The _connectorPaths constructed in this way will always have either the params.tokenIn or params.tokenOut set as the inbound tokenIn.

The issue is that the _connectorPaths constructed above can now only be a single hop in length: from the tokenIn to the intermediary token represented by connectors[i]. These single-leg swaps are provided to the _getBestMultiSwapPath function inside a for loop which can only ever evaluate the single path it was provided and will, based on the estimatedAmount returned, override the value of the path named return variable. Consequently, the function is unable to evaluate and construct multi-swap paths and will simply return the path of the single path swap that returned the highest amount of tokens.

Consider correcting the path construction logic such that the outbound token for each hop of a multi-swap path becomes the inbound token for the next swap in the connector path.

Update: Resolved in commit 4f57d33. As per the Woof! team, the UniswapV3PathFinder contract has been deprecated and will not be used within the Compound ecosystem.

Medium Severity

Pausable Functionality Not Integrated

The MigratorV2 contract inherits from OpenZeppelin’s Pausable contract, enabling the owner to pause specific contract functionalities. However, the whenNotPaused and whenPaused modifiers are not applied to any of the contract functions. As a result, pausing the contract will have no impact, and all operations will continue, including the sensitive migrate function. Additionally, the constructor documentation states that “the contract is paused if any of the input arrays are empty”. However, neither is any check made against the length of the input arrays nor is an internal call made to _pause. This, the contract will effectively be unpaused even if this condition is met.

Consider applying the whenNotPaused or whenPaused modifiers to all relevant functions in the contract that are intended to be pausable (e.g., migrate). Also, consider pausing the contract as specified if the constructor is passed empty input arrays, or updating the documentation to reflect the current logic.

Update: Resolved in commit 4f57d33.

USDS/DAI Connector Tokens Incorrectly Handled

The UniswapV3PathFinder contract is used to identify the optimal path for single-hop and multi-hop token swaps. In the case of a single-hop swap between USDS and DAI, the UniswapV3 pool is not utilized as for all other generic token swaps, and instead a 1-1 conversion is directly facilitated.

The getBestMultiSwapPath function provides no similar special consideration for swaps between USDS and DAI. Instead, the _getBestSingleSwapPath function is called internally as with all other token types. As a result, instead of utilizing a 1-1 conversion between the tokens, the UniswapV3 pool for these tokens will be used.

Consider applying the same 1-1 conversion rate for swaps between USDS and DAI within the multi-hop swap path.

Update: Resolved in commit 4f57d33. As per the Woof! team, the UniswapV3PathFinder contract has been deprecated and will not be used within the Compound ecosystem.

Incorrect Flashloan Token Repaid

The AaveV3UsdsAdapter, SparkUsdsAdapter, and MorphoUsdsAdapter contracts enable the migration of collateral and borrow positions from their respective lending protocols to the target Comet Market. These adapters extend the functionality of the standard adapters by supporting conversion between USDS and DAI across multiple entry points, including flashloan repayment. For any other tokens, the flashloan is repaid by withdrawing from the user’s Comet position (as in the standard adapter).

However, across the three USDS-enabled adapters, the wrong token is withdrawn from the Comet Market token when repaying the flashloan. Although the flashBaseToken is repaid, the cometBaseToken is withdrawn from the user’s position. As a result, the flashloan repayment may fail, and any migrations involving flashloans with tokens other than DAI or USDS will not be guaranteed to succeed.

Consider updating the adapters to ensure that the flashBaseToken is withdrawn from the Comet Market rather than the cometBaseToken.

Update: Resolved in commit 4f57d33. The fix has been applied across the SparkUsdsAdapter, AaveV3UsdsAdapter, and MorphoUsdsAdapter contracts.

Migrations Using Flashloans Can Be Replayed

The MigratorV2 contract exposes a uniswapV3FlashCallBack function which is intended to be called as part of the migrate function flow when a user requires a flashloan as part of their migration. The uniswapV3FlashCallback function is protected from unauthorized external calls by a _storedCallbackHash check which ensures that the callback data supplied matches that which was crafted as part of the migrate call. However, when a user requires a flashloan in order to execute the migration, _storedCallbackHash is never reset. This means that an attacker can call uniswapV3FlashCallBack with the same data as the previous user and potentially replay the last user’s migration (if the user has granted enough allowance).

Consider resetting the _storedCallbackHash at the end of the uniswapV3FlashCallBack implementation to prevent the re-use of the same migration data by an attacker.

Update: Resolved in commit 4f57d33.

2

Low Severity

Native Tokens Are Not Supported as Collateral Assets

The Spark, Aave, and Morpho adapters include logic to handle cases where the withdrawn collateral corresponds to the chain’s native token. However, none of these protocols supports using native tokens as collateral, only their wrapped ERC-20 equivalents are supported. Specifically:

  • Aave allows users to deposit a network’s native token through a wrapped token gateway. When a user deposits a native token (e.g., ETH), the gateway wraps it (e.g., into WETH) before supplying it to the lending pool. For instance, to deposit ETH into Aave, a user would call the depositETH function on the gateway and receive aWETH in return.
  • Spark, as a fork of Aave, follows the same pattern. When spTokens (implemented as ATokens) are burned during withdrawal, the protocol performs an ERC-20 transfer instead of a native token transfer to return the underlying asset.
  • Morpho’s withdrawCollateral function similarly only supports ERC-20 tokens and does not return native tokens.

To avoid confusion and improve code readability, consider removing the unreachable code branch in the _migrateCollateral function of the adapters that attempts to handle native token cases.

Update: Resolved in commit 4f57d33. The fix has been applied across the SparkUsdsAdapter, AaveV3UsdsAdapter, and MorphoUsdsAdapter contracts by removing the logic pertaining to native token payments.

Fee-on-Transfer Tokens Are Not Fully Supported

Compound III is designed to support fee-on-transfer tokens. However, the current Migrator implementation may not support such tokens for the following reasons:

Consider documenting this limitation and ensuring that fee-on-transfer tokens are neither flashloaned nor used in swap operations.

Update: Resolved in commit 4f57d33. It is now indicated that fee-on-transfer tokens are not supported.

Lack of Deadlines for Uniswap Swaps

Uniswap v3 swaps accept a deadline parameter that defines the Unix timestamp after which the transaction will revert. This mechanism helps protect users from significant price swings due to delayed execution. However, throughout the codebase, block.timestamp is used as the swap deadline. This approach defeats the purpose of the deadline parameter, as block.timestamp reflects the time at which the transaction is included on-chain, causing the deadline check to always pass—regardless of how long the transaction remained pending in the mempool.

To provide users with enhanced protection against execution delays and price volatility, consider adding a deadline field to the SwapInputLimitParams and SwapOutputLimitParams structs, allowing users to explicitly set a deadline for their migration transactions.

Update: Resolved in commit 4f57d33. deadline fields have been added to the SwapInputLimitParams and SwapOutputLimitParams structs which are used for additional checks during swaps.

Flashloaned Token May Not to Be Available for Withdrawal

In cases where the balance of the flashloaned token is less than flashAmountWithFee, the _repayFlashloan function attempts to withdraw the shortfall from the Comet market. However, there are a few caveats to this approach:

  • If the flashloaned token is not Comet’s base token, the user must migrate a sufficient amount of the flashloaned token as collateral into Comet. Otherwise, the transaction will revert. This could be optimized by avoiding the supply of repayment tokens to Comet in the first place, since they are immediately needed to repay the flashloan.
  • Each Comet Market defines a baseBorrowMin parameter, which specifies the minimum amount that can be borrowed. As a result, if the flash-loaned token is the Comet’s base asset and a borrow is required for repayment, the withdrawal must exceed this minimum threshold for the transaction to succeed.
  • The MigratorV2 contract requires user allowance to both withdraw from and supply to the Comet. This requirement should be included in the documentation of the _repayFlashloan function.

To enhance user flexibility and simplify repayment logic, consider restricting flashloans to the target Comet’s base asset. This would allow the withdrawFrom call to create a borrow position (which must exceed the minimum threshold) against all migrated collateral, if necessary, instead of relying on sufficient token balances. In addition, consider updating the documentation to clearly state the need for user allowances to both withdraw from and supply to Comet on their behalf.

Update: Resolved in commit 4f57d33.

Missing Docstrings

Throughout the codebase, multiple instances of missing docstrings were identified:

Consider thoroughly documenting all functions (and their parameters) that are part of any contract’s public API. Functions implementing sensitive functionality, even if not public, should be clearly documented as well. When writing docstrings, consider following the Ethereum Natural Specification Format (NatSpec).

Update: Resolved in commit 4f57d33.

Missing Zero-Address Checks

When performing operations involving address parameters, it is crucial to ensure that the address is not set to zero, as this could disrupt the contract’s logic and result in undefined behavior.

Throughout the codebase, multiple instances of missing zero-address checks were identified:

Consider adding a zero-address check before assigning a state variable.

Update: Resolved in commit 4f57d33.

Unsafe ABI Encoding

It is not uncommon to use abi.encodeWithSignature or abi.encodeWithSelector to generate calldata for a low-level call. However, the first option is not typo-safe and the second option is not type-safe. The result is that both of these methods are error-prone and should be considered unsafe.

Within MigratorV2.sol, multiple instances of unsafe ABI encoding were identified:

Consider replacing all the occurrences of unsafe ABI encodings with abi.encodeCall which checks whether the supplied values actually match the types expected by the called function and also avoids errors caused by typos.

Update: Resolved in commit 4f57d33. abi.encodeCall is now used throughout the MigratorV2 contract.

Incorrect Error May Be Returned

The _swapFlashloanToBorrowToken and _swapCollateralToCompoundToken functions of the SwapModule contract handle reverts with a try-catch pattern. The issue is that if the call to the SwapRouter fails for any reason, it will always revert with a message stating that the SwapRouter does not support the interfaces used. There are several reasons swaps may revert, such as malformed path parameters or slippage protections, and the returned error may obfuscate the actual reason a migration failed.

Consider reworking the returned errors in the aforementioned functions to allow the actual error to be returned.

_Update: Resolved in commit 4f57d33.

Redundant Flashloan Repayment

Throughout the adapters in the codebase, even when a flashloan is not required for a collateral migration, the flashloanData passed to the adapter still contains the encoded liquidity pool and base token for the provided Comet Market.

As a result, flashloanData will never have a length of zero, and the migration logic will always attempt to repay a flashloan, even when none was taken. Consequently, the _repayFlashloan function in the AaveV3Adapter contract will always attempt to transfer 0 of the flashloanData.baseToken to the Uniswap V3 pool. Notably, some tokens revert on zero-value transfers. This behavior can cause the migration to fail, even when no flashloan is involved.

To avoid the above, consider checking whether the flashloan includes a non-zero borrow amount instead of relying on the length of flashloanData,to decide whether to invoke _repayFlashloan.

Update: Resolved in commit 4f57d33.

Misleading Documentation

Throughout the codebase, multiple instances of misleading documentation were identified:

Consider correcting the aforementioned comments to improve the overall clarity and readability of the codebase.

Update: Resolved in commit 4f57d33.

Using EnumerableSet Will Decrease Code Complexity

The MigratorV2 contract has two state variables related to adapters: _adapters which is a private address array, and allowedAdapters which is a public mapping of address to bool values. _adapters is only used to provide a convenient getter function for users to enumerate all added adapters. The allowedAdapters mapping is used in-contract to check if an adapter may be used. However, the same functionality can be obtained by using OpenZeppelin’s EnumerableSet library, which will reduce code complexity while providing the same functionality.

Consider using OpenZeppelin’s EnumerableSet library instead of recreating its functionality. This will help improve the clarity and maintainability of the codebase.

Update: Resolved in commit 4f57d33.

Redundant try-catch Block

Within the SwapModule contract, the _swapFlashloanToBorrowToken and _swapCollateralToCompoundToken functions will be utilized by child contracts to facilitate any swaps necessary to repay existing user debt. Both functions attempt to first make a call to UNISWAP_ROUTER using the ISwapRouter interface, and in the event that the call fails, attempt a call through the ISwapRouter02 interface.

However, given that the SwapRouter and SwapRouter02 contracts are deployed at different addresses, the success of each call is mutually exclusive. For any adapters which set UNISWAP_ROUTER to the SwapRouter02 contract address, the initial call to SwapRouter will always fail, needlessly consuming gas in the process. Likewise, the usage of a try-catch block will prove redundant in cases where the SwapRouter contract deployment is being used instead.

Consider taking a different approach to allow for adapter modularity with respect to the SwapRouter deployment used (e.g., including a boolean flag signifying which deployment address was set upon construction).

Update: Resolved in commit 4f57d33.

DAI/USDS Can Be Uninitialized

The UniswapV3PathFinder contract identifies the optimal path for swaps between an input and output token. Additionally, it allows for swaps between USDS and DAI at a 1-1 rate, bypassing the Uniswap v3 pool that is used for all other generic token types. These token addresses are set upon construction, and remain immutable for the duration of the contract deployment.

The USDS and DAI addresses are checked to ensure that if one of them is set, the other must also be set. However, there is currently no check that both values are set. In this case, the intended functionality will not perform as expected. Notably, within getBestSingleSwapPath, it will be possible to pass in address(0) as tokenIn and tokenOut which will be treated as a normal DAI-USDS swap regardless of the user-provided input/output token amounts.

Consider enforcing that USDS and DAI are both set to non-zero addresses upon construction.

Update: Resolved in commit 4f57d33. As per the Woof! team, the UniswapV3PathFinder contract has been deprecated and will not be used within the Compound ecosystem.

Unenforced DAI/USDS Swap Amounts

Within the UniswapV3PathFinder contract, any single-hop or multi-hop swaps must have a non-zero input or output amount. Otherwise, _quoteSwap will revert with MustBeSetAmountInOrAmountOut. In the case of a swap between USDS/DAI, the quoter is bypassed, and a special case value is immediately returned from getBestSingleSwapPath. As a result, no such validation will be performed that one of the provided values is non-zero, which is inconsistent with the rest of the codebase.

Consider enforcing that one of the input/output token amounts is non-zero directly within getBestSingleSwapPath instead of performing this check within _quoteSwap.

Update: Resolved in commit 4f57d33. As per the Woof! team, the UniswapV3PathFinder contract has been deprecated and will not be used within the Compound ecosystem.

Redundant else-if Branch

The UniswapV3PathFinder contract iterates over a number of pool fee tiers for a given input/output token pair to determine the tier that yields the greatest output.

However, within the getBestMultiSwapPath, _getBestSingleSwapPath, and _getBestMultiSwapPath functions, there is a redundant if-else if statement where both branches produce the same result. This redundancy does not affect functionality but may lead to confusion and hinder readability.

To improve clarity and maintainability, consider merging the conditions of the if and else-if branches into a single conditional statement.

Update: Fixed in commit 4f57d33. As per the Woof! team, the UniswapV3PathFinder contract has been deprecated and will not be used within the Compound ecosystem.

_isDebtCleared Checks Stable Debt Despite Unsupported Repayment

The Aave and Spark adapter contracts allow for the repayment of variable debt borrow positions within various markets. Only variable interest rate mode borrowings are supported, with a hardcoded INTEREST_RATE_MODE = 2. Additionally, the IS_FULL_MIGRATION variable, if set to true, will enforce that the user has no remaining debt within the specified market after repaying the borrow position.

However, in that case, the _isDebtCleared function checks not only the variable but also the stable debt balances to determine whether the debt is cleared. According to the Spark docs, the stable debt tokens are not currently used and will be deprecated in the future, whereas, in Aave V3, the use of stable debt has already been deprecated. Therefore, working directly with the variable debt tokens would be recommended.

To improve consistency, consider removing the stable debt balance check and only using the variable debt token balance check when determining whether user debt is cleared.

Update: Resolved in commit 4f57d33. The fix has been implemented in the SparkUsdsAdapter and AaveV3UsdsAdapter contracts.

Incorrect Documentation of ReentrancyGuardStorageLocation Slot Calculation

The DelegateReentrancyGuard contract provides protocol adapters with protection against reentrancy. The contract adapts OpenZeppelin’s ReentrancyGuard and has been optimized for use with delegatecall. It uses a custom storage slot to track the reentrancy status, stored as a constant to ensure consistent behavior with delegatecall.

However, the value of ReentrancyGuardStorageLocation does not match the documented computation. The comment states that it is calculated as keccak256("openzeppelin.storage.ReentrancyGuard") - 1 & ~0xff, which would yield a value of 0x2f5dfb728f951184cd9c29f0647c32c90a691d88bcdf960380c6bfa7b526aa00. However, the actual value used is 0x9b779b17422d0df92223018b32b4d1fa46e071723d6817e2486d003becc55f00, which aligns with the ERC-7201 specification. In reality, the correct computation should be keccak256(abi.encode(uint256(keccak256("openzeppelin.storage.ReentrancyGuard")) - 1)) & ~bytes32(uint256(0xff)), which includes an additional keccak256 and proper parenthesis, as specified in ERC-7201.

Consider updating the documentation to accurately reflect this computation, which will help avoid confusion for developers and auditors referencing the standard.

Update: Resolved in commit 4f57d33. As per the Woof! team, the UniswapV3PathFinder contract has been deprecated and will not be used within the Compound ecosystem.

The Excessive Amount Used for Repaying the Flashloan Is Not Refunded to the User

The _repayFlashloan function withdraws funds from the user if the contract’s balance is insufficient to repay the flash loan. However, if the contract holds more than the required amount, the excess remains locked in the contract, which could potentially be exploited by another user. In addition, the swaps from the flash loan token to the collateral token use the exactOutput method. This means that the amountIn for each swap may vary, leading to the accumulation of small leftover amounts (“dust”) over time.

To improve security and efficiency, consider returning any excess tokens to the user if the contract holds more than the required amount to repay the flash loan.

Update: Partially resolved in commit 4f57d33. Logic to deposit excess cometBaseToken funds into the Comet Market has been added to each adapter. However, an edge case remains, where dust may accrue from intermediate tokens during multi-hop swaps in which the exactOut functionality is used.

3

Notes & Additional Information

Overlap Between Standard Adapters and Their USDS Versions

All adapter contracts currently have two variants: a standard version and another tailored for Comets that use USDS as the base asset. However, there is significant overlap between these versions. For example, the SparkUsdsAdapter behaves almost identically to the SparkAdapter, with only a few additional execution branches to handle special cases involving DAI or USDS during migration. This makes SparkAdapter redundant, as SparkUsdsAdapter can fully handle both scenarios.

To simplify the codebase and reduce duplication, consider unifying the standard and USDS variants where possible. When unification is not feasible, consider using the standard variant as the base contract for the USDS-specific version.

Update: Resolved in commit 4f57d33. The standard and USDS variants have been unified.

Unused Named Return Variables

Named return variables are a way to declare variables that are meant to be used within a function’s body for the purpose of being returned as that function’s output. They are an alternative to explicit in-line return statements.

In all the adapters, the isCleared return variable of the _isDebtCleared function is unused.

Consider either using or removing any unused named return variables.

Update: Resolved in commit 4f57d33. The fix has been implemented across all adapters.

Unused Import

The IWrappedTokenGatewayV3 interface imported in AaveV3UsdsAdapter.sol is unused and should be removed.

Consider removing unused imports to improve the overall clarity and readability of the codebase.

Update: Resolved in commit 4f57d33.

Use calldata Instead of memory

When dealing with the parameters of external functions, it is more gas-efficient to read their arguments directly from calldata instead of storing them to memory. calldata is a read-only region of memory that contains the arguments of incoming external function calls. This makes using calldata as the data location for such parameters cheaper and more efficient compared to memory. Thus, using calldata in such situations will generally save gas and improve the performance of a smart contract.

Throughout the codebase, multiple instances where function parameters should use calldata instead of memory were identified:

  • In MigratorV2.sol, the flashData parameter
  • In UniswapV3PathFinder.sol, the params parameter
  • In UniswapV3PathFinder.sol, the params parameter

Consider using calldata as the data location for the parameters of external functions to optimize gas usage.

Update: Resolved in commit 4f57d33. As per the Woof! team, the UniswapV3PathFinder contract has been deprecated and will not be used within the Compound ecosystem.

Constant Not Using UPPER_CASE Format

In DelegateReentrancyGuard.sol, the ReentrancyGuardStorageLocation constant is not declared using the UPPER_CASE format.

According to the Solidity Style Guide, constants should be named with all capital letters with underscores separating words. For better readability, consider following this convention.

Update: Resolved in commit 4f57d33. As per the Woof! team, the UniswapV3PathFinder contract has been deprecated and will not be used within the Compound ecosystem.

Missing Named Parameters in Mappings

Since Solidity 0.8.18, developers can utilize named parameters in mappings. This means mappings can take the form of mapping(KeyType KeyName? => ValueType ValueName?). This updated syntax provides a more transparent representation of a mapping’s purpose.

Within MigratorV2.sol, multiple instances of mappings without named parameters were identified:

Consider adding named parameters to mappings in order to improve the readability and maintainability of the codebase.

Update: Resolved in commit 4f57d33.

Use Custom Errors

Since Solidity version 0.8.4, custom errors provide a cleaner and more cost-efficient way to explain to users why an operation failed.

Throughout the codebase, multiple instances of revert and/or require messages were identified:

For conciseness and gas savings, consider replacing require and revert messages with custom errors.

Update: Resolved in commit 4f57d33.

Multiple Storage Variables Could Be Cached on Stack

In the EVM, it is more gas-efficient to read values from the stack than from storage. When a storage variable, is accessed multiple times (e.g., in a loop), it is more efficient to cache its value on the stack and reuse the cached variable.

Throughout the codebase, multiple instances where this optimization could be applied were identified:

Consider caching the storage variable on the stack before using it multiple times, this optimization enhances gas efficiency by reducing repeated storage access, especially within loops.

Update: Resolved in commit 4f57d33. The UniswapV3PathFinder contract has been deprecated and the EnumerableSet library is now being used in the MigratorV2 contract.

Unused Errors

Throughout the codebase, multiple instances of unused errors were identified:

To improve the overall clarity, intentionality, and readability of the codebase, consider either using or removing any currently unused errors.

Update: Partially resolved in commit 4f57d33. However, the InvalidSlippageBps error remains unused.

Integrated Contract Not Intended to Be Called On-Chain

The UniswapV3PathFinder contract is closely integrated with the Uniswap V3 QuoterV2 contract. The QuoterV2 contract is not intended to be called on-chain and, by extension, the UniswapV3PathFinder should not be called on-chain either. However, the codebase does not communicate this fact to the users or integrators.

Consider adding a warning to the UniswapV3PathFinder contract’s documentation to make potential integrators aware of this limitation.

Update: Resolved in commit 4f57d33. As per the Woof! team, the UniswapV3PathFinder contract has been deprecated and will not be used within the Compound ecosystem.

maxGasEstimate Input Parameter Unnecessary

The maxGasEstimate input parameters used in the SingleSwapParams and MultiSwapParams serve no other purpose than to introduce a potential point of failure during the _quoteSwap function call in the UniswapV3PathFinder contract. When interacting with a contract that should not be called on-chain there is no purpose to limiting the gas as it can only serve to cause an unexpected revert. The current implementation simply returns the gasEstimate provided by the QuoterV2 contract.

Consider removing the maxGasEstimate limit imposed on calls to the QuoterV2 contract inside the _quoteSwap function.

Update: Resolved in commit 4f57d33. As per the Woof! team, the UniswapV3PathFinder contract has been deprecated and will not be used within the Compound ecosystem.

Zero Slippage Breaks Calculation

The _calculateSlippageAmount function within the SwapModule contract requires the slippage setting to be provided as a value in basis points. Should a user provide a slippage value of zero, the calculation will allow the entire swap amount to be lost to slippage.

Although zero slippage is not a realistic expectation, consider protecting the integrators of this contract by not allowing a 0 value when setting the slippage.

Update: Resolved in commit 4f57d33. The _calculateSlippageAmount internal function has been removed from the SwapModule contract.

Repeated Casting to IERC20

Within the _convertDaiToUsds and _convertUsdsToDai functions of the ConvertModule contract the addresses of the DAI and USDS tokens are cast to IERC20 types multiple times.

Consider using IERC20 types to store these variables instead of the current address type in order to avoid repeated casting, save gas, and increase code clarity.

Update: Resolved in commit 4f57d33.

Reentrancy Functionality Not Integrated

In the MigratorV2 contract, OpenZeppelin’s ReentrancyGuard is inherited to provide protection against reentrancy attacks. Separately, adapter contracts such as SparkAdapter rely on the DelegateReentrancyGuard to protect the executeMigration function. However, the nonReentrant modifier provided by ReentrancyGuard is currently unused throughout the MigratorV2 contract.

While this does not present an immediate security concern—since reentrancy protection is handled at the adapter level—consider removing the unused ReentrancyGuard inheritance from MigratorV2. Alternatively, consider applying the nonReentrant modifier to the relevant functions if additional protection is warranted.

Update: Resolved in commit 4f57d33.

Conclusion

MigratorV2 provides a robust and modular solution for users looking to migrate their positions from various decentralized lending protocols to Compound III. By utilizing Uniswap V3 flash loans, offering flexible migration options, and allowing for customization, it delivers a seamless and cost-efficient migration experience. The contract is designed with scalability in mind, enabling future protocol integrations and feature expansions without the need for redeployment.

The audit uncovered several high- and medium-severity issues, along with a number of minor findings. As a result, recommendations aimed at improving code consistency and readability were made. After delivery of the initial report, the development team indicated that two in-scope contracts, UniswapV3PathFinder and DelegateReentrancyGuard, have been deprecated and will no longer be used.

The fixes for identified issues were provided in a single commit with no way to isolate code changes to specific issues. Although the issues have been resolved, we are unable to confirm that no additional changes were made to the in-scope contracts. Consequently, we recommend a re-audit at the latest commit.

The development team is appreciated for being cooperative and providing clear explanations throughout the audit process.

1 Like