Mamo PR - Moonwell

The StrategyMulticall contract does not have any protection against reentrancy risks. For the genericMulticall() function, an attacker (or a mistaken owner transaction) can supply the StrategyMulticall contract's own address as a call target, causing it to reenter itself before the first invocation completes. Because genericMulticall() uses a low-level .call{value:…} without any nonReentrant guard, the nested invocation would execute the same loop logic recursively.

Although the function is restricted by onlyOwner and requires sufficient msg.value for all calls (including the nested ones), a malicious or misconfigured batch could still trigger unexpected behavior or resource exhaustion under edge conditions.

Consider inheriting and applying OpenZeppelin’s ReentrancyGuard to StrategyMulticall and mark genericMulticall() with the nonReentrant modifier. This will block any attempt to call genericMulticall() (directly or indirectly) while it is already executing, fully eliminating the self-targeted reentrancy vector.

SOLVED: The Moonwell team fixed this finding in commit 24bb086 by adding a reentrancy protection as recommended.

It was identified that the StrategyMulticall and SlippagePriceChecker contracts in scope inherit from OpenZeppelin's OwnableUpgradeable library. Ownership of the contracts that are inherited from the OwnableUpgradeable module can be lost, as the ownership is transferred in a single-step process.

The address that the ownership is changed to should be verified to be active or willing to act as the owner. Ownable2StepUpgradeable is safer than OwnableUpgradeable for smart contracts because the owner cannot accidentally transfer smart contract ownership to a mistyped address. Rather than directly transferring to the new owner, the transfer only completes when the new owner accepts ownership.

To mitigate the risks associated with single-step ownership transitions and enhance contract security, it is recommended to adopt a two-step ownership transition mechanism, such as OpenZeppelin's Ownable2StepUpgradeable. This approach introduces an additional step in the ownership transfer process, requiring the new owner to accept ownership before the transition is finalized. The process typically involves the current owner calling a function to nominate a new owner, and the nominee then calling another function to accept ownership.

Implementing Ownable2StepUpgradeable provides several benefits:

1. Reduces Risk of Accidental Loss of Ownership: By requiring explicit acceptance of ownership, the risk of accidentally transferring ownership to an incorrect or zero address is significantly reduced.

2. Enhanced Security: It adds another layer of security by ensuring that the new owner is prepared and willing to take over the responsibilities associated with contract ownership.

3. Flexibility in Ownership Transitions: Allows for a smoother transition of ownership, as the nominee has the opportunity to prepare for the acceptance of their new role.

By adopting Ownable2StepUpgradeable, contract administrators can ensure a more secure and controlled process for transferring ownership, safeguarding against the risks associated with accidental or unauthorized ownership changes.

ACKNOWLEDGED: The Moonwell team acknowledged this finding.

It was identified that the StrategyMulticall and SlippagePriceChecker contracts in scope inherit from OpenZeppelin's OwnableUpgradeable library. In the OwnableUpgradeable contracts, the renounceOwnership() function is used to renounce the Owner permission. Renouncing ownership before transferring would result in the contract having no owner, eliminating the ability to call privileged functions.

/**
 * @dev Leaves the contract without owner. It will not be possible to call
 * `onlyOwner` functions. Can only be called by the current owner.
 *
 * NOTE: Renouncing ownership will leave the contract without an owner,
 * thereby disabling any functionality that is only available to the owner.
 */
function renounceOwnership() public virtual onlyOwner {
    _transferOwnership(address(0));
}

It is recommended that the Owner cannot call renounceOwnership() without first transferring ownership to another address. In addition, if a multi-signature wallet is used, the call to the renounceOwnership() function should be confirmed for two or more users.

ACKNOWLEDGED: The Moonwell team acknowledged this finding.

In the StrategyFactory contract, the @dev comment says Only callable by accounts with the BACKEND_ROLE in the MamoStrategyRegistry, but it is actually allowing the user to call createStrategyForUser() for itself:

/**
  * @notice Creates a new strategy for a specified user
  * @dev Only callable by accounts with the BACKEND_ROLE in the MamoStrategyRegistry
  * @param user The address of the user to create the strategy for
  * @return strategy The address of the newly created strategy
  */
function createStrategyForUser(address user) external returns (address strategy) {
  require(user != address(0), "Invalid user address");
  require(msg.sender == mamoBackend || msg.sender == user, "Only backend or user can create strategy");

Also, in the SlippagePriceChecker.sol file, the @title says PriceChecker, but the actual name of the contract is SlippagePriceChecker:

/**
 * @title PriceChecker
 * @notice Checks swap prices using Chainlink price feeds and applies slippage tolerance
 * @dev Implements the ISlippagePriceChecker interface with UUPS upgradeability
 */
contract SlippagePriceChecker is ISlippagePriceChecker, Initializable, UUPSUpgradeable, OwnableUpgradeable {

It is recommended to update the NatSpec comments to accurately reflect the actual implementation.

ACKNOWLEDGED: The Moonwell team acknowledged this finding.

The following imports are not used and could be removed:

• In StrategyMulticall.sol:

import {IStrategy} from "./interfaces/IStrategy.sol";

• In SlippagePriceChecker.sol:

import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol";

Consider removing any redundant import to simplify the codebase.

ACKNOWLEDGED: The Moonwell team acknowledged this finding.

In Solidity smart contract development, replacing hard-coded revert message strings with the Error() syntax is an optimization strategy that can significantly reduce gas costs. Hard-coded strings, stored on the blockchain, increase the size and cost of deploying and executing contracts.

The Error() syntax allows for the definition of reusable, parameterized custom errors, leading to a more efficient use of storage and reduced gas consumption. This approach not only optimizes gas usage during deployment and interaction with the contract but also enhances code maintainability and readability by providing clearer, context-specific error information.

It is recommended to replace hard-coded revert strings in require statements for custom errors, which can be done following the logic below:

1. Standard require statement (to be replaced):

require(condition, "Condition not met");

2. Declare the error definition to state:

error ConditionNotMet();

3. As currently is not possible to use custom errors in combination with require statements, the standard syntax is:

if (!condition) revert ConditionNotMet();

More information about this topic in the official Solidity documentation.

ACKNOWLEDGED: The Moonwell team acknowledged this finding.

In the function createStrategyForUser(), a named return variable strategy is declared in the function signature and then it is explicitly (and redundantly) returned:

function createStrategyForUser(address user) external returns (address strategy) {
  ...
  return strategy;
}

Consider simplifying the code so it does not explicitly return named returns to avoid redundancies.

ACKNOWLEDGED: The Moonwell team acknowledged this finding.

The project codebase contains some stylistic inconsistencies and deviations from Solidity best practices, which, while not directly impacting functionality, reduce code readability, maintainability, and adherence to standard conventions. Addressing these inconsistencies can enhance the clarity and professionalism of the code.

• Use Underscores or Scientific Notation for Number Literals: Instead of using 10_000 or 1_000, or 1e4 and 1e3, the StrategyFactory contract is using 10000 and 1000 as shown below.

uint256 public constant SPLIT_TOTAL = 10000; // 100% in basis points

uint256 public constant MAX_SLIPPAGE_IN_BPS = 1000; // 10% in basis points

uint256 public constant MAX_COMPOUND_FEE = 1000; // 10% in basis points

require(_splitMToken + _splitVault == SPLIT_TOTAL, "Split parameters must add up to 10000");

• Internal variables, constants and functions should start with an underscore (_): Notice the internal constant MAX_BPS does not start with an underscore (_) contrary to the style guide.

uint256 internal constant MAX_BPS = 10_000;

• Interface declaration inside a contract file: The SlippagePriceChecker.sol file is not only declaring the SlippagePriceChecker contract as the style guide recommends, but also an interface named IERC20MetaData. One file per declaration is preferred.

interface IERC20MetaData {
    function decimals() external view returns (uint8);
}

/**
 * @title PriceChecker
 * @notice Checks swap prices using Chainlink price feeds and applies slippage tolerance
 * @dev Implements the ISlippagePriceChecker interface with UUPS upgradeability
 */
contract SlippagePriceChecker is ISlippagePriceChecker, Initializable, UUPSUpgradeable, OwnableUpgradeable {
...

Consider implementing the style improvements highlighted above to enhance the readability and consistency of the project.

ACKNOWLEDGED: The Moonwell team acknowledged this finding.