Maple Finance A-1

Actors for which manual redeem mode is enabled may go against the purpose of the withdrawal queue mechanism by performing step 1 of the redeem flow (getting their request processed) but delaying step 2, exchanging shares for the available assets indefinitely until an appropriate time.

As a result, they may have an advantage in certain market conditions when an unrealized loss is about to increase, which they may front-run as they can perform exchange instantly while others would need to go through the withdrawal manager queue.

In the withdrawal-manager-cyclical module, MapleWithdrawalManagerInitializer._initialize() does not perform safe downcasting of provided inputs, such as startTime_, cycleDuration_, and windowDuration_. As a result, the provided system invariant may be broken.

The system is designed to reject any attempts to set a start time that falls in the past.

If provided startTime_, cycleDuration_, and windowDuration_ are greater than uint64, their values will pass initial checks such as startTime_ >= block.timestamp, but these values will get truncated on assignment, and incorrect values will be set in cycleConfigs[0]. Allowing even startTime to be 0.

function _initialize(address pool_, uint256 startTime_, uint256 cycleDuration_, uint256 windowDuration_) internal {
        require(pool_           != address(0),      "WMI:ZERO_POOL");
        require(startTime_      >= block.timestamp, "WMI:INVALID_START");
        require(windowDuration_ != 0,               "WMI:ZERO_WINDOW");
        require(windowDuration_ <= cycleDuration_,  "WMI:WINDOW_OOB");

        pool        = pool_;
        poolManager = IPoolLike(pool_).manager();

        cycleConfigs[0] = CycleConfig({
            initialCycleId:   1,
            initialCycleTime: uint64(startTime_),
            cycleDuration:    uint64(cycleDuration_),
            windowDuration:   uint64(windowDuration_)
        });

        emit Initialized(pool_, cycleDuration_, windowDuration_);
        emit ConfigurationUpdated({
            configId_:         0,
            initialCycleId_:   1,
            initialCycleTime_: uint64(startTime_),
            cycleDuration_:    uint64(cycleDuration_),
            windowDuration_:   uint64(windowDuration_)
        });
    }

Proof of concept:

function test_createInstance_success() external {
   // @audit - startTime set to large value 
   bytes memory calldata_ = abi.encode(address(pool), uint256(type(uint64).max) + 1, 7 days, 2 days);

    MapleWithdrawalManager withdrawalManager_ = MapleWithdrawalManager(factory.createInstance(calldata_, "SALT"));

    (
        uint64 initialCycleId_,
        uint64 initialCycleTime_,
        uint64 cycleDuration_,
        uint64 windowDuration_
    ) = withdrawalManager_.cycleConfigs(0);

    assertEq(withdrawalManager_.pool(),           address(pool));
    assertEq(withdrawalManager_.latestConfigId(), 0);

    assertEq(initialCycleId_,   1);
    // @audit - initialCycleTime should never be smaller than block.timestamp
    // but because of truncation of values larger than type(uint64).max it ends up being 0
    assertEq(initialCycleTime_, 0);
    assertEq(cycleDuration_,    7 days);
    assertEq(windowDuration_,   2 days);
}

Remediations to consider:

• Update implementation to perform safe downcasting and revert in case of out-of-range values.

MaplePoolPermissionManager's hasPermission function accepts an arbitrary length array for lenders_ and loops through all addresses passed to check the corresponding pool permission level and bitmap. However, if no lenders are passed to this function, the returned value will be true, potentially allowing access to gated functions without permission.

It is worth mentioning that this is not possible in the current codebase, as it’s impossible to provide arbitrary values to this function outside of external off-chain calls to the view function.

Consider returning false if no lenders are provided. Alternatively, document this behavior to avoid pitfalls.

Consider removing onlyGovernor modifier from the MaplePoolPermissionManager contract since it is not used.

For the following events, consider using an indexed attribute for one or more parameters:

• IMaplePoolPermissionManager
  • LenderAllowlistSet
  • PermissionAdminSet
  • PoolBitmapsSet
  • PoolPermissionLevelSet
• IMapleWithdrawalManagerInitializer
  • Initialized
• IMapleWithdrawalManager
  • RequestCreated (for the owner parameter)

In both withdrawal-manager-cyclical and withdrawal-manager-queue modules, the implementation of MapleWithdrawalManagerFactory.createInstance() function contains redundant and unnecessary assignment.

isInstance[instance_ = super.createInstance(arguments_, salt_)] = true;

Consider removing the mentioned assignment since super.createInstance() performs the same in the parent function implementation.

isInstance[instance_] = true;

Across the two withdrawal manager contract types (queue and cyclical) there are logic inconsistencies in implementing pausable functionality.

The whenProtocolNotPaused modifier checks for isFunctionPaused in the queue withdrawal manager.

modifier whenProtocolNotPaused() {
    require(!IGlobalsLike(globals()).isFunctionPaused(msg.sig), "WM:PAUSED");
    _;
}

However, the whenProtocolNotPaused modifier in the cyclical manager checks protocolPaused which is less flexible and does not allow fine-grained control.

modifier whenProtocolNotPaused() {
    require(!IGlobalsLike(globals()).protocolPaused(), "WM:PROTOCOL_PAUSED");
    _;
}

Consider updating implementation to provide consistent behavior between Withdrawal Managers of different types to reduce maintenance impact and prevent operational issues.

In the MapleWithdrawalManager, _processManualExit() has the following implementation.

function _processManualExit( uint256 shares_, address owner_)
    internal returns ( uint256 redeemableShares_, uint256 resultingAssets_)
{
    require(shares_ > 0,                              "WM:PE:NO_SHARES");
    require(shares_ <= manualSharesAvailable[owner_], "WM:PE:TOO_MANY_SHARES");

    ( redeemableShares_ , resultingAssets_ ) = _calculateRedemption(shares_);

    // @audit - case when shares_ < redeemableShares_ is not reachable
    require(shares_ <= redeemableShares_, "WM:PE:NOT_ENOUGH_LIQUIDITY");
    ...
}

The value of redeemableShares_ returned from the _calculateRedemption() is always represented by the following relationship redeemableShares_ <= shares_. Therefore, shares_ cannot ever be smaller than redeemableShares_.

The same logic applies to the similar guard condition in the processRedemptions() function.

require(maxSharesToProcess_ <= redeemableShares_, "WM:PR:LOW_LIQUIDITY");

Consider changing conditions in the implementation above to the following to indicate that only full redemption would be possible.

In _processManualExit()

require(shares_ == redeemableShares_, "WM:PE:NOT_ENOUGH_LIQUIDITY");

and in processRedemptions()

require(maxSharesToProcess_ == redeemableShares_, "WM:PR:LOW_LIQUIDITY");

Maple Finance now features two types of withdrawal managers: Cyclical and Queue withdrawal managers. Both of these withdrawal managers feature upgrade functionality, which is access-controlled. However, access control is inconsistent.

Upgrading the cyclical withdrawal manager is allowed for the pool delegate or governor, while in the case of the queue withdrawal manager, upgrades are allowed to be performed by the pool delegate or security admin.

require(msg.sender == poolDelegate_ || msg.sender == governor(), "WM:U:NOT_AUTHORIZED");

require(msg.sender == poolDelegate_ || msg.sender == securityAdmin(), "WM:U:NOT_AUTHORIZED");

Consider updating access control in the Cyclical withdrawal manager to allow securityAdmin() instead of the governor().

For the withdrawal-manager-cyclical module, MapleWithdrawalManager proxy functions do not feature pausable behavior since whenProtocolNotPaused modifier is missing. This contrasts the implementation of other similar contracts across modules in the Maple Finance codebase.

Due to the above, fine-grained control over paused functionality on a specific contract instance is unavailable. However, a fallback is a pauseable functionality on the factory level. Functions missing whenProtocolNotPaused modifier:

• MapleWithdrawalManager.migrate()
• MapleWithdrawalManager.setImplementation()
• MapleWithdrawalManager.upgrade()

The same functions have the corresponding modifier in withdrawal-manager-queue:MapleWithdrawalManager.

MapleWithdrawalManager.processExit() emits WithdrawalCanceled event in addition to WithdrawalProcessed when all lockedShares are processed, which is slightly misleading and could potentially cause errors in off-chain tracking and monitoring tools.

Consider not emitting this event when all lockedShares are processed.

In the fixed-term-loan module MapleLoan.skim() is accessible to anyone, while in the open-term-loan module MapleLoan.skim() is accessible only to the borrower and governor.

Consider updating access control for the MapleLoan.skim() in the fixed-term-loan module to prevent external parties from profiting from the mistakes of the protocol users.

In the fixed-term-loan module MapleLoan.skim() has no zero address check for the destination address, while in the open-term-loan module MapleLoan.skim() corresponding check is present.

Consider adding a zero address check to the MapleLoan.skim() in the fixed-term-loan module to prevent assets from being inadvertently locked.

In the open-term-loan module, MapleRefinancer contains unused import of IERC20.

Consider removing this unnecessary import.

In the withdrawal-manager-cyclical module, the MapleWithdrawalManager contract implements pool manager access control directly in functions addShares(), removeShares(), and processExit().

require(msg.sender == poolManager, "WM:AS:NOT_POOL_MANAGER");

However, in the Queue withdrawal manager this access control is implemented using onlyPoolManager() modifier.

Consider replacing repetitive access controls with the onlyPoolManager() modifier in the cyclical withdrawal manager.

In the pool-permission-manager module, the MaplePoolPermissionManager contract implements several modes for access control. Two modes that allow function level control and higher level but less fine-grained pool level control are represented by FUNCTION_LEVEL and POOL_LEVEL constant values.

These modes require corresponding bitmaps to be set in poolBitmaps mapping storage variable for each pool. When values in specific bitmap match bitmaps of lenders from respective lenderBitmaps mapping entries, access is allowed; otherwise is denied.

However, the default when corresponding bitmaps in these two modes are not set is to allow anyone access. This default comes with a high risk of misconfiguration, and actors responsible for configuring the permission manager (pool delegate and protocol admins) must take necessary precautions.

While this design choice is intentional, and it is meant to allow transfers to be allowed by default, Maple Finance will need to ensure that pool delegates are aware of misconfiguration risks.

In the cyclical withdrawal manager, the delay period when the new configuration starts to apply is +3 from the current cycle. This allows all users who have already locked their shares not to be affected during withdrawal (in case of enough liquidity during withdrawal).

When the pool delegate changes the configuration, it will take effect only on the start of the third cycle.
This way all users that have already locked their shares will not have their withdrawal time affected.

    C1       C2       C3             C4
|===--.--|===-----|===-----|==========----------|
      ^                     ^
configuration change     new configuration kicks in

Users that request a withdrawal during C1 will withdraw during WW3 using the old configuration.
Users that lock their shares during and after C2 will withdraw in windows that use the new configuration.

However, if a delay is meant to be used as a Timelock mechanism to allow those unhappy with the new configuration to exit, the defined delay may be too short. Depending at which moment the new config is set in the current cycle, those users who haven’t locked their shares yet may have time from 0 < time-to-decide < cycleDuration to decide on their withdrawal. This, in extreme cases, is a too short time period for the user to react.

Consider increasing the delay to +4, so that users who haven’t yet locked their shares have at least one full cycle to decide on their withdrawal.

As suggested by Chainlink documentation, Optimistic L2 oracle implementations should check their corresponding L2 Sequencer uptime feed to ensure that the sequencer is live before trusting the data feed returned by the oracle. Even with the Oracle staleness price check, if the sequencer is down, there could be multiple price changes provided by the Oracle that won’t go through and will get queued and applied once the sequencer is up; if price changes are high, it could cause unfair conditions for users and borrowers.

Maple Base Oracles are currently not being used by the protocol; if enabled checks should be added to verify that the sequencer is live and provide a grace period to react to potential high price changes (See suggested example in Chainlink docs).